10553: Process to update contact info fails if DAWSON is also updating other contact info at the same time #5648
Conversation
… before starting update contact info;
… before starting update email;
…sion to change email;
…play new error message for when the request times out;
…dOnceAllUpdatesComplete" resolves with user information;
… is available to be modified;
web-api/src/business/useCases/user/queueEmailUpdateAssociatedCasesWorker.test.ts
Outdated
Show resolved
Hide resolved
| user.role === ROLES.privatePractitioner || | ||
| user.role === ROLES.irsPractitioner || | ||
| user.role === ROLES.inactivePractitioner |
There was a problem hiding this comment.
This comment isn't really related to your bugfix, but I've seen this check in a few places, and I feel nervous about continuing this pattern. (It is very easy for a dev to think, "Ah, let me create a User" without realizing they need to create a Practitioner instead, or it is easy for a dev to forget one of the disjuncts like ROLES.inactivePractitioner.)
My first thought is that it would be nicer if we could construct the appropriate user with a factory, but that's more complicated and outside your ticket's scope. However, I wonder about your thoughts pulling this check (here and wherever else it is used, which includes admin.ts, changePasswordInteractor, and updateAssociatedCaseWorker) into a function like "isPractitionerUser" or something, which is at least a little cleaner.
web-api/src/business/useCases/user/queueEmailUpdateAssociatedCasesWorker.ts
Show resolved
Hide resolved
web-api/src/business/useCases/user/queueEmailUpdateAssociatedCasesWorker.ts
Show resolved
Hide resolved
web-client/src/presenter/sequences/updateUserInformationSequence.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
It is difficult for me to give a great review of this--I am definitely relying on the tests that you and Tenille have done--but it looks reasonable? I don't want to hold up code.
My main concern is that I think a lot of this work will change substantially as we move off of Dynamo and mostly off of Open Search and onto Postgres. In other words, this is a good deal of somewhat complex code to solve a problem that will likely be easier to handle in a couple of months (when we can avoid doing updates via upserts that overwrite the entire entity).
| role: Role; | ||
| }; | ||
|
|
||
| export class UserFactory { |
There was a problem hiding this comment.
I love that you are tackling this! That said, I am a little wary given that we have Practitioner, PrivatePractitioner, and IrsPractitioner. I am not sure how these all play together, and if we only get the UserFactory halfway working, someone might call it expecting it to get the right thing in all cases when it doesn't.
There was a problem hiding this comment.
(And then depending on complexity this maybe involves more testing, etc.)
There was a problem hiding this comment.
It is true @Mwindo that once this is migrated to Postgres it will simplify this logic. That is why we decided to make sure these process do not overlap and they wait until one finishes before starting the next (instead of modifying process to work in parallel). In the worker we check that the expected count of cases are updated and then toggle the flag again. We also error handle and timeout handle to make sure user is not stuck in a update state.
flexion#10553