Skip to content
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

Take a first stab at household join #1022

Draft
wants to merge 3 commits into
base: deploy/6761-table-actions
Choose a base branch
from

Conversation

martha
Copy link
Contributor

@martha martha commented Jan 13, 2025

Description

GH issue: https://github.com/open-path/Green-River/issues/5752

Depends on hmis-warehouse PR: greenriver/hmis-warehouse#5047

Type of change

  • Bug fix
  • New feature (adds functionality)
  • Code clean-up / housekeeping
  • Dependency update

Checklist before requesting review

  • I have performed a self-review of my code
  • I have run the code that is being changed under ideal conditions, and it doesn't fail
  • My code includes comments and/or descriptive variable names to help other engineers understand the intent (or not applicable)
  • My code follows the style guidelines of this project (eslint)
  • I have updated the documentation (or not applicable)
  • If it's not obvious how to test this change, I have provided testing instructions in this PR or the related issue

@@ -56,6 +66,11 @@ const AddToHouseholdButton = ({
inputVariables: { projectId, clientId },
pickListArgs: { projectId, householdId },
localConstants: { householdId, projectCocCount: cocCount },
onFieldChange: (linkId?: string, value?: any) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gigxz as you can see, this is still a draft with many todos, but this is the key part I'd like early feedback about!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @martha I will take a closer look. I agree with you that RHF may make this a bit different, for example maybe we could hook into the RHF watch functionality.

I'll explain the backend alternative I was thinking of, too, just to give some more detail. This is an approach where the backend would be solely responsible for determining whether there are conflicts:

  • Similarly to how the Delete Client returns some structured JSON data related to a warning, the SubmitForm mutation could return structured data attached to the validation error Client has another enrollment in this project that conflicts with this entry date. that it already returns. That could be implemented here by adding something like data : { conflicting_enrollment_id: 5 } to the validation error.
  • If -- like with DeleteClient -- we included ALL the relevant information about conflicting enrollment(s), the validation error itself would have sufficient data on it for us to show the Conflicting Enrollment alert.
  • With this approach,
    • There wouldn't be a need to do additional background queries based on the currently selected Entry Date
    • The user wouldn't be made aware of the conflict (and the option to "join") until they have already tried to submit the form
    • The logic for whether enrollments conflict or not would remain in one place (the Enrollment Validator here, which relies on the somewhat complicated with_conflicting_dates scope.) Since the logic is only in one place, we don't have to worry about making sure that the Backend and Frontend equivalents of "with conflicting dates" have perfectly matched logic. (Mis-matched logic would result in the backend error being surfaced but the frontend not showing the workflow option, or vice versa).
    • Once the Conflicting Enrollment alert is shown, the user would have the option to initiate the join, or change the Entry Date to something valid and re-submit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the feedback @gigxz. I've made a start on your idea -- it's a bit rough, but I think something like this will be necessary to selectively override the FormDialog's default rendering of its error messages.

I also have a question about this part:

If -- like with DeleteClient -- we included ALL the relevant information about conflicting enrollment(s), the validation error itself would have sufficient data on it for us to show the Conflicting Enrollment alert.

Am I understanding the following correctly?

  • the data attr of an error returned from the backend is just json, but can't be easily typed using HmisSchema objects
  • you're suggesting the error's data attr should return everything needed to render the ConflictingClientAlert, but not return all the data needed for the whole JoinHouseholds workflow, right?

If I got that right, then, I think I'd propose removing (descoping) the sentence "There [is/are] also [n] other household member[s] associated: [Additional Clients]" from the mocks here. It seems preferable to me to rely on our existing logic that lives in the schema objects for determining whether the current user has access to those clients, and/or access to view their names. The user will see those clients' names on the next step anyway, if they initiate the join workflow, so including them here seems more complicated than it's worth to me. (But maybe I am missing an easy way to do it.)

If I got that wrong, and you really did intend for the conflicting enrollment error message to include all the data necessary for the JoinHouseholds workflow -- including client alerts of the associated household members, nested data that is expected in a certain format/type in the common column definitions we are using, etc. -- then I'll have some followup questions about how the typing should work (hand-pick the data we need in the enrollment validator, then cast on the frontend to the relevant gql typescript types? is there a better way?)

Thanks for your help!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@martha good question, I didn't mean typing the data field but rather just including enough information for the explanatory alert. If it makes sense to descope some content from the alert, I think that's OK.
It is a bit brittle to have this un-typed contract between the backend and the frontend regarding what the error data looks like. The frontend should be as gracious as possible of course, so if the data is missing or malformed it just shows the default message.

$id: ID!
$limit: Int = 1
$offset: Int = 0
$filters: EnrollmentsForClientFilterOptions
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(If we end up keeping this approach) I'd suggest making openOnDate and projectId required variables, since that's the only intended use-case for this query. This resolves so much, it could get really expensive if it was resolved on a larger set of enrollments. I think we are assuming that it shouldn't return a ton of enrollments, because a client shouldn't have multiple open enrollments at a given project on a given date-- but thats only true if those args are passed. (Even though I see this defaults to limit: 1, it could still be used in other ways)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants