bcgov / nr-compliance-enforcement-cm

Case Management Solution for Compliance and Enforcement
Apache License 2.0
0 stars 0 forks source link

feat: ce 356 update and save hwc complaint assessment information #32

Closed dmitri-korin-bcps closed 5 months ago

dmitri-korin-bcps commented 6 months ago

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 COSOFFICER 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 ```json mutation CreateAssessment($createAssessmentInput: CreateAssessmentInput!) { createAssessment(createAssessmentInput: $createAssessmentInput) { caseIdentifier leadIdentifier assessmentDetails { actionNotRequired actionJustificationCode actionJustificationShortDescription actionJustificationLongDescription actionJustificationActiveIndicator actions { actor date actionCode shortDescription longDescription activeIndicator } } } } { "createAssessmentInput": { "leadIdentifier": "24-000021", "createUserId": "officerbob", "assessmentDetails": { "actionNotRequired": false, "actionJustificationCode": "OTHOPRPRTY", "actions": [{ "actor": "61d6d74f-0442-4479-ae4d-ff814a4de15d", "date": "3/1/2019", "actionCode": "ASSESSHIST", "activeIndicator": true }, { "actor": "a126bd2b-fc99-4fd0-a8cc-29ef4f595c97", "date": 1708578077, "actionCode": "ASSESSRISK", "activeIndicator": true }, { "actor": "a126bd2b-fc99-4fd0-a8cc-29ef4f595c97", "date": 1708578078, "actionCode": "CNFRMIDENT", "activeIndicator": true }, { "actor": "a126bd2b-fc99-4fd0-a8cc-29ef4f595c97", "date": 1708578079, "actionCode": "ASSESSHLTH", "activeIndicator": true } ] }, "agencyCode": "COS", "caseCode": "HWCR" } } ``` ### Update Assessment ```json mutation UpdateAssessment($updateAssessmentInput: UpdateAssessmentInput!) { updateAssessment(updateAssessmentInput: $updateAssessmentInput) { caseIdentifier leadIdentifier assessmentDetails { actionNotRequired actionJustificationCode actionJustificationShortDescription actionJustificationLongDescription actionJustificationActiveIndicator actions { actor date actionCode shortDescription longDescription activeIndicator } } } } { "updateAssessmentInput": { "leadIdentifier": "24-000020", "caseIdentifier": "06cd489e-aa99-481f-b4dd-8506b0e4e64a", "updateUserId": "officerjohn", "assessmentDetails": { "actionNotRequired": true, "actionJustificationCode": "OTHOPRPRTY", "actions": [{ "actor": "61d6d74f-0442-4479-ae4d-ff814a4de15d", "date": "3/1/2019", "actionCode": "ASSESSHIST", "activeIndicator": false }, { "actor": "a126bd2b-fc99-4fd0-a8cc-29ef4f595c97", "date": 1708578077, "actionCode": "ASSESSRISK", "activeIndicator": true }, { "actor": "a126bd2b-fc99-4fd0-a8cc-29ef4f595c97", "date": 1708578078, "actionCode": "CNFRMIDENT", "activeIndicator": true }, { "actor": "a126bd2b-fc99-4fd0-a8cc-29ef4f595c97", "date": 1708578079, "actionCode": "ASSESSHLTH", "activeIndicator": true }] }, "agencyCode": "COS", "caseCode": "HWCR" } } ``` ### Get Case and Assessment ```json query { getCaseFile (caseIdentifier: "669f9bd7-e24f-4853-8158-9222a964d5f6") { caseIdentifier leadIdentifier assessmentDetails { actionNotRequired actionJustificationCode actionJustificationShortDescription actionJustificationLongDescription actionJustificationActiveIndicator actions { actor date actionCode shortDescription longDescription activeIndicator } } } } ``` ### Get Case and Assessment by Lead Id ```json query { getCaseFileByLeadId (leadIdentifier: "24-000016") { caseIdentifier leadIdentifier assessmentDetails { actionNotRequired actionJustificationCode actionJustificationShortDescription actionJustificationLongDescription actionJustificationActiveIndicator actions { actor date actionCode shortDescription longDescription activeIndicator } } } } ``` ### Get Inaction Justification Types ```json query { inactionJustificationTypes (agencyCode: "COS") { actionJustificationCode agencyCode shortDescription longDescription displayOrder activeIndicator } } ``` ### Get HWCR Assessment Actions ```json query { HWCRAssessmentActions (actionTypeCode: "COMPASSESS") { actionTypeCode actionCode displayOrder activeIndicator shortDescription longDescription } } ```

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:

dmitri-korin-bcps commented 6 months ago

@afwilcox

Action Codes will need to have Short and Long descriptions returned in order for drop downs to function.

+ I modified the Action Codes query to return both short and long descriptions.

I'm not sure what the action_type_code on the CreateCaseFileInput mutation is for. Is it required?

+ 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”

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.

+ 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 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?)

+ 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.

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.

+ 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.

CreateCase

😄

afwilcox commented 6 months ago

@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.

afwilcox commented 6 months ago

@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:

{ "createCaseFileInput": {
            "lead_identifier": "24-000001",
            "assessment_details": {
                "action_not_required_ind": true,
                "inaction_reason_code": "OTHOPRPRTY",
                "assessment_actions": 
                    [{
                        "actor_guid": "5c5283b3-6bdd-47ea-a647-e578249aa163",
                        "action_date": "3/1/2019",
                        "action_code": "ASSESSRISK",
                        "active_ind": true
                    }]
            },
            "create_user_id": "POSTMAN"
        }
}

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.

{ "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": true
                    }]
            },
            "update_user_id": "POSTMAN"
        }
}

I also expected the following to inactivate "ASSESSRISK" and it also did not

{ "updateCaseFileInput": {
            "case_file_guid": "7146da78-d64f-4112-8bf6-67c910ac4c10",
            "assessment_details": {
                "action_not_required_ind": true,
                "assessment_actions": []
            },
            "update_user_id": "POSTMAN"
        }
}
dmitri-korin-bcps commented 6 months ago

@afwilcox These were valid bugs and now all are fixed in the most resent commit.

The only point to note is in order to inactivate an action you need to specify it explicitly in the update query. For example, to deactivate ASSESSHIST while activating ASSESSRISK you need to put

{ "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"
        }
}
afwilcox commented 6 months ago

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):

marqueone-ps commented 6 months ago

@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 case_file to assessment.

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

type Mutation {
   upsertAssesment(input: AssesmentDto! | AssesmentWithEquipment | etc...): Assesment!
   deleteAssesment(id: Int)
}

What I feel like you're looking for is more along the lines of the following:

type Mutation { 
   createAssesment(input: AssementInput!): Assesment
   createAssesmentWithCase(input: AssementInput!): Assesment
   createAssesmentWithEquipment(input: AssementInput!): Assesment
   createAssesmentWithNotes(input: AssementInput!): Assesment
   createAssessmentAndMaybeCreateCaseIfItDoesntExist(input: AssesmentInput!): Assesment,
   updateAssesmentById(input: AssementInput!): int
   updateAssesmentByIdWithCase(input: AssementInput!): Int
   updateAssesmentByIdWithEquipment(input: AssementInput!): Int
   updateAssesmentByIdWithNotes(input: AssementInput!): Int
   deleteAssesment(id: int!)
}

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:

  @Mutation('createAssesmentWithEquipment')
  @Roles(Role.COS_OFFICER)
  createAssesmentWithEquipment(@Args('input') input: AssesmentInput) {
    return this.service.create(input).insertEquipment(input);
  }

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

Problem: Schema Duplication One of the first things you realize when coding a GraphQL back-end from scratch is that it involves a lot of similar-but-not-quite-identical code, especially when it comes to schemas.

Now where I think specificity is absolutely paramount is in the example thats provided here: apollo docs

For instance, say you were building a password reset feature into your app. To actually send that email you may have a mutation named: sendPasswordResetEmail. This mutation is more like an RPC call then a simple CRUD action on a data type.

This is the perfect example where specificity is going to be important instead of a simple:

type Mutation {
   sendEmail(email: String, type: String, body: String)
}

we would want to see

type Mutation {
   sendPasswordResetEmail(input: EmailInput)
   sendForgotUsernameEmail(input: EmailInput)
...
}

Ultimately the graphql docs are very clear about one thing around names: <verb><noun>(input: <object type>) and I think that Dmitri is doing that correctly.

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 case_file_guid or note_text when we start including the type in the var name, fine for database entities, but not something we want or need in the frontend.

marqueone-ps commented 6 months ago

@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.

afwilcox commented 5 months ago

@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

dmitri-korin-bcps commented 5 months ago

@afwilcox

Refactoring is complete. Like discussed in the call there are separate mutations for creating and updating assessments. Because of that there is no need for “case_file_guid” parameter in the createAssessment mutation. We already have updateAssessment mutation that accepts that. Currently the workflow that covers both existing and new cases is as follows.

  1. Client run getCaseFile query
  2. If the case exists getCaseFile returns case_file_guid
  3. Client runs updateAssessment mutation and pass that case_file_guid to create actions and link them to the case.
  4. If getCaseFile indicates that the case does not exist client runs createAssessment mutation which creates a new case and actions

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. I am going to update the test queries and mutations in the PR comments shortly. DONE

afwilcox commented 5 months ago

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)

dmitri-korin-bcps commented 5 months ago

@afwilcox and @marqueone-ps

I renamed the types to use camelCase like indicated in the ticket spec. The test queries in the PR comment have been updated as well to reflect the new naming