#1031 Submission GQL Small Data Fetch #1060
Conversation
…into feat/1031-submission-gql-data-fetch
src/dictionary/api.ts
Outdated
| @@ -80,16 +80,26 @@ export const get = async (req: Request, res: Response) => { | |||
| return res.status(200).send(schema); | |||
| }; | |||
|
|
|||
| export const getClinicalSchemas = async (withFields: boolean) => | |||
There was a problem hiding this comment.
This function has always returned either [String] or [{ }]; this does not play nicely w/ GraphQL TypeDefs
Open to suggestions + solutions
There was a problem hiding this comment.
Refactored for reusability (API vs GQL)
API is currently converting Clinical response to JSON, which masks difference btwn [String] or [{ }]
| @@ -339,6 +354,8 @@ const typeDefs = gql` | |||
| oldValue: String! | |||
| donorId: String! | |||
| } | |||
|
|
|||
| scalar SchemaList | |||
There was a problem hiding this comment.
This scalar is a workaround for the issue in /src/dictionary/api
clinicalSubmissionTypesList cannot return [String] | [SchemaWithFieldsObject]; unions cannot include primitives
There was a problem hiding this comment.
Can you link me to more on this please? I don't understand and my gut is telling me that unions should be ok with primitives.
There was a problem hiding this comment.
They aren't: https://www.apollographql.com/docs/apollo-server/schema/unions-interfaces/
Only allows Object types
Entire Github thread about it: graphql/graphql-spec#215
There was a problem hiding this comment.
Going to sleep on it and review in the morning
In Gateway, this is simply [String] because the response array is converted to JSON, so that may be what client expects and I should just stick to the same format
I was trying to format this to see the proper object for GQL Server purposes, which I think is ideal but might require client side changes
There was a problem hiding this comment.
sounds good - let me know where the union is happening tomorrow please - I read the docs + github - very interesting - looks like unions don't work on scalars either
All of a union's included types must be [object types](https://www.apollographql.com/docs/apollo-server/schema/schema/#object-types) (not scalars, input types, etc.). Included types do not need to share any fields.
There was a problem hiding this comment.
Oh so this Union is related to getSchemasWithFields: #1060 (comment)
Where were either returning a String[] of Schema names or an array of SchemaWithFields objects
demariadaniel
changed the title
demariadaniel
requested review from
ciaranschutte,
joneubank and
UmmulkiramR
| .instance() | ||
| .getSchemaNames() | ||
| .then(result => result.filter(s => s !== ClinicalEntitySchemaNames.REGISTRATION)); | ||
|
|
There was a problem hiding this comment.
is this logic the same apart from the instance method call?
could be little more DRY. not sure which is the async call so not sure where to put await..something roughly likee..
export const getClinicalSchemas = async (withFields: boolean) => {
const managerInstance = await manager.instance();
const schemaNameFilter = (schemaName) => schemaName !== ClinicalEntitySchemaNames.REGISTRATION;
const schemas = withFields ? managerInstance.getSchemasWithFields().map(schema => schema.name) : managerInstance.getSchemaNames();
return schemas.filter(schemaNameFilter)
}There was a problem hiding this comment.
Yes I had a feeling this could be improved. But, we don't want to map getSchemasWithFields(), we need to return the objects with both names + fields, so schemaNameFilter needs to be called differently depending on value of withFields: 67ceec9
src/dictionary/api.ts
Outdated
| const schemas = await manager.instance().getSchemaNames(); | ||
| return res.status(200).send(schemas.filter(s => s !== ClinicalEntitySchemaNames.REGISTRATION)); | ||
| const withFields = req?.query?.includeFields?.toLowerCase() === 'true'; | ||
| const schemas = await getClinicalSchemas(withFields).then(result => result); |
There was a problem hiding this comment.
I think there's no need for then.
there's two different Promise methods at work here.
the chaining then and the more modern await - they both (in a simple case) do the same thing
waiting until Promise resolves and doing something it.
There was a problem hiding this comment.
Yes this was unnecessary, removed
| }) | ||
| .catch(err => { | ||
| L.error(`Failed to delete registration`, err); | ||
| return false; |
There was a problem hiding this comment.
nice logging thumbs up emoticon
| programShortName, | ||
| fileType || 'all', | ||
| version, | ||
| (<GlobalGqlContext>contextValue).egoToken, |
There was a problem hiding this comment.
unclear why this <GlobalGqlContext> is needed? Casting just the contextValue?
There was a problem hiding this comment.
| contextValue: any, | ||
| ) => { | ||
| const { programShortName, version } = args; | ||
| const submissionData = <DeepReadonly<ActiveClinicalSubmission>>( |
There was a problem hiding this comment.
is there one single way we can work around all this DeapReadonly stuff and stick with that approach?
I think this works out the same as using as DeepReadonly....
A utility function or a type guard that can be the de facto way for reusability, single point of functionality?
There was a problem hiding this comment.
I think this one we can handle by casting the return type of commitActiveSubmissionData: {} as DeepReadonly<ActiveClinicalSubmission>
There was a problem hiding this comment.
@demariadaniel left some questions & suggestions - approving
Description of changes
Type of Change
Checklist before requesting review:
developnot master)validationDependencyin meta tag for Argo Dictionary fields used in code