-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: ce 356 update and save hwc complaint assessment information #32
feat: ce 356 update and save hwc complaint assessment information #32
Conversation
…-and-Save-HWC-Complaint-Assessment-Information
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Action Codes will need to have Short and Long descriptions returned in order for drop downs to function.
I'm not sure what the action_type_code on the CreateCaseFileInput mutation is for. Is it required?
CreateCaseFile Mutations requires an input of AssessmentDetails, however there is no guarantee what order the sections are going to be entered in so this field should be optional if we are going to continue with this route, however I actually don't think you want assessment_actions here at all in the mutation. The original design for both the queries and the mutation called for more specific queries and mutations. This was based on the best practice to make mutations as specific as possible. Right now the CreateCaseFile mutation is responsible for creating both the case and the Assessment, as we continue to add sections this is going to grow and it will eventually be quite unmaintainable. I recommend that you remove the AssessmentDetails from CreatCaseFile Mutation and expose CreateAssessmentDetails as a top level mutation that will also create a case if it doesn't exist.
CreateAssessmentDetailsInput has assessment_actions as mandatory but I don't think this is correct because if action_not_requied_ind = 'N' there might not be any assessment actions provided (TBD on some requirements here maybe?)
I was unable to call the CreateCaseFile Mutation - this is because it looks there is something odd in the prisma generation where both a case_guid and a case_file are required. Only the case_guid was provided.
+ I modified the Action Codes query to return both short and long descriptions.
+ Action Type input parameter is to distinguish complaint assessment actions
+ from other possible actions that we may introduce in future. It is not required.
+ If not passed it default to “COMPASSESS”
+ The Case and AssessmentDetails objects are currently at the same level in the database.
+ I can modify the app to merge Case and AssessmentDetails objects at the GraphQL level.
+ We may leave mutation name CreateCase or change it to CreateAssessment.
+ However, since there is no such object as Assessment at the data layer,
+ I think it is more logical to name mutations CreateCase.
+ The assessment details are just a subset of the case’s fields.
+ Let me know what you think.
+ CreateAssessmentDetailsInput has assessment_actions parameter which has an array type and is mandatory.
+ If assessment does not require any actions, we pass an empty array in "assessment_actions"
+ and the app will not associate any actions with the case then.
+ I have just successfully tested it in GraphQL Playground.
+ CreateCaseFile mutation does not require any id as an input parameter. It automatically generates a case id.
+ See the screenshot.
+ Please use the GraphQL queries that I included in the PR comment for testing. 😄 |
@dmitri-korin-bcps Thanks for the information regarding the changes to you made to the API design. I do not believe we should be coupling our API design with our data model - one of the advantages of an API is to abstract the data model and provide business language functions to future front-end developers that are clearly described and self documenting. Here are a couple of quotes from some reference articles that indicate why I had requested specialized queries and mutations over general ones. (e.g. createAssessment as it's own endpoint instead of one massive case endpoint, and separate queries for each code table instead of a generic 'Actions' query)
I will surface this discussion on teams to get some broader input from the other developers and Ministry Architecture team. |
@dmitri-korin-bcps - I also think I found a couple of bugs with the functionality in the API - let me know if you see a mistake in my inputs. Leads (links to complaints) are not being stored correctly. Here is a sample input variable:
Updates do not appear to be working. Based on the created complaint above I was expecting the following to inactivate "ASSESSRISK" wand to insert "ASSESSHLTH" however it did not appear to make any changes.
I also expected the following to inactivate "ASSESSRISK" and it also did not
|
@afwilcox The only point to note is in order to inactivate an action you need to specify it explicitly in the update query. { "updateCaseFileInput": {
"case_file_guid": "7146da78-d64f-4112-8bf6-67c910ac4c10",
"assessment_details": {
"action_not_required_ind": true,
"assessment_actions":
[{
"actor_guid": "5c5283b3-6bdd-47ea-a647-e578249aa163",
"action_date": "3/1/2019",
"action_code": "ASSESSHLTH",
"active_ind": false
},
{
"actor_guid": "5c5283b3-6bdd-47ea-a647-e578249aa163",
"action_date": "3/1/2019",
"action_code": "ASSESSRISK",
"active_ind": true
}]
},
"update_user_id": "POSTMAN"
}
} |
Thanks @dmitri-korin-bcps - I like being explicit with setting the active indicator to false for the updates. Regarding our above discussion about refactoring I think this is the minimal set of changes required to bring this PR into compliance with best practices and to set us up well for next sprint is around the creation of the case, and the fact that it may or may not exist when the createAssessment mutation is called. I've gone through and tried to list out each change along with an explanation of why I'm recommending it: Renames:
Changes to CreateAssessmentInput:
Changes to AssessmentDetailsInput:
Changes to UpdateAssessmentInput :
Resolver Changes for createAssessment :
Resolver Changes for UpdateAssessment
Optional Changes (If you agree go ahead and make these, but they aren't required to close this PR):
|
@afwilcox this is really throwing me off and is incredibly confusing, though I do agree with some of the refactored names, though if I may I'd also recommend changing the name of the base type from I've been going back and forth between the docs and the apollo article you've posted and I'm not super sold on the idea of absolute specificity. If I were creating this first pass I would have created the following mutations
What I feel like you're looking for is more along the lines of the following:
When I look at this there's a lot going on, but more importantly our api surface area is now ten times bigger because of these extra very specific mutations, this is in turn going to increase the amount of mutation functions in the resolver. I also feel like we're going to end up with a log more code like this where we just chain methods together, unless each mutation is backed by its own host of helpers, for example:
Additionally this seems like something where we would end up falling into schema duplication because we have so many edge cases we're trying to handle: Schema duplication
Now where I think specificity is absolutely paramount is in the example thats provided here: apollo docs
This is the perfect example where specificity is going to be important instead of a simple:
we would want to see
Ultimately the graphql docs are very clear about one thing around names: One thing that is like nails on a chalk board for me is the fact we're including types in everything, its like we've taken a step backwards and we're back in the 80's again by using var names like |
@dmitri-korin-bcps for your input and return types I would highly recommend not using snake_case, and instead use camelCase. I think that for entities this is fine especially because the frontend is mostly expecting camelCase. |
@marqueone-ps - No, that's exactly what I'm trying to avoid. I don't want to have an uber 'case' mutation that is called eleventy billion different ways. I agree with @dmitri-korin-bcps that it makes sense to put the Case and the Assessment together in a single mutation (same with prevention and education and case; equipment and case; notes and case etc. While we might eventually want to create a createCase mutation on it's own - it's not technically required right now |
Refactoring is complete.
If client does not know case_file_guid there is a query getCaseFileByLeadId to find the case associated with this lead Id. Again if the case found client runs UpdateAssessment mutation and pass case_file_guid, otherwise createAssessment mutation. When testing remember that agencyCode and caseCode are now required parameters. |
Thanks @dmitri-korin-bcps I'm going to try and do a bit more testing - but just by looking at the GQL definition it looks good. I'm going to leave this PR open until we've got it fully integrated with the complaint management side in case the integration finds any issues. One thing I thought of, and it relates to the comment by @marqueone-ps about using camelCase instead of snake_case it looks like you've just reused the database column names for the types. My preference is that we would use more business friendly names in our API definition instead of our database columns which can get a bit gross sometimes. I've updated the ticket specs to include what I was thinking we could use (as well as adding the short/long descriptions which you added, but I had missed in the original specification) |
I renamed the types to use camelCase like indicated in the ticket spec. |
Description
Please provide a summary of the change and the issue fixed. Please include relevant context. List dependency changes.
Case Management changes for CE-356.
Both queries and mutations require authentication and membership in COS_OFFICER role
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
GraphQL queries for testing
View GraphQL queries
Create Assessment
Update Assessment
Get Case and Assessment
Get Case and Assessment by Lead Id
Get Inaction Justification Types
Get HWCR Assessment Actions
Checklist
Further comments
Thanks for the PR!
Deployments, as required, will be available below:
Please create PRs in draft mode. Mark as ready to enable:
After merge, new images are deployed in:
Thanks for the PR!
Deployments, as required, will be available below:
Please create PRs in draft mode. Mark as ready to enable:
After merge, new images are deployed in:
Thanks for the PR!
Deployments, as required, will be available below:
Please create PRs in draft mode. Mark as ready to enable:
After merge, new images are deployed in:
Thanks for the PR!
Deployments, as required, will be available below:
Please create PRs in draft mode. Mark as ready to enable:
After merge, new images are deployed in:
Thanks for the PR!
Deployments, as required, will be available below:
Please create PRs in draft mode. Mark as ready to enable:
After merge, new images are deployed in: