-
Notifications
You must be signed in to change notification settings - Fork 3
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
MCR-2359 submit + unlock resolver #1903
Conversation
… this is supposed to handle
e4f1fe3
to
13c5c25
Compare
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.
First pass! Looking good. Thanks for the refactoring to address null and undefined in the Prisma queries.
@@ -5,6 +5,18 @@ import { | |||
includeRateFormData, | |||
} from './prismaSharedContractRateHelpers' | |||
|
|||
const includeFirstSubmittedRateRev = { |
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.
nit: Should this be includeLatestSubmittedRateRev
? From the first look I thought it was getting the first ever submitted revision, but looks like its looking at the most recent created revision that is submitted.
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.
I like it. more clear
@@ -7,6 +7,18 @@ import { | |||
|
|||
// Generated Types | |||
|
|||
const includeFirstSubmittedContractRev = { |
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.
Same as rate. Should this be includeLatestSubmittedContractRev
const foundBeforeContract = await findContractWithHistory( | ||
tx, | ||
contractID | ||
) |
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.
Curious if this is just to make sure the contract exists or was the another purpose? The resolver queries for the contractID before calling this handler.
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.
great catch, this was for some debugging. It's gone now.
) | ||
const unlockRatePromises = [] | ||
for (const rateRevisionID of currentRateRevIDs) { | ||
const resPromise = store.unlockRate({ |
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.
Do we want to abort all rate unlocks if one fails, then return an error to client? If so, I don't think this will do it. Each one of the unlocks would have to be in the same transaction.
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.
you are quite right. I am going to make a ticket for the cleanup to figure this out. Right now we are only doing transactions inside of postgres functions. We need to decide on a design here. Either we should copy your pattern for update where we have a combined "unlockContractAndRates" function that we call from the handler, or we need to make it so that a transaction can be created in a resolver and passed into multiple postgres functions. I'm not sure how to do the latter yet.
WE'll also need to clean up Submit in the same manner.
|
||
const draftResult = toDomain(currentRevision.formDataProto) | ||
// If there are rates, submit those first | ||
if (initialFormData.rateInfos.length > 0) { |
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.
Shouldn't this be something like (initialFormData.submissionType === 'CONTRACT_AND_RATES' && initialFormData.rateInfos.length > 0)
. Otherwise I think we could just be adding right back rates that were deleted for contract only.
// this function translates undefineds into nulls | ||
function nullify<T>(field: T | undefined): T | null { | ||
if (field === undefined) { | ||
return null |
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.
We should have a generic db helpers place to put this kind of stuff
rateRevision.addtlActuaryContacts && | ||
rateRevision.addtlActuaryContacts.map( | ||
(c, idx) => ({ | ||
position: idx, |
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.
@macrael I think we should add this handling to insertContract
and insertRate
also - feels like that could be a place that introduces inconsistency. Otherwise looks good!
* submitContract working * submitRate working - update change history docs * start unlock work --------- Co-authored-by: MacRae Linton <macrael@truss.works>
Summary
This PR includes getting submit and unlock to work.
I'd like to get all the tests passing so if you see some that aren't running against the new feature flag lmk and I'll enable them. We should be able to run all our API tests now that all the resolvers have been updated.
There's some noisy changes unfortunatley. Making sha256 required touched a lot of files, as well as getting rid of some of our .d.ts files b/c I hoped it would fix our build issue (and in fact revealed some weird typing issues that were hidden!)
Things left to do:
Related issues
https://qmacbis.atlassian.net/browse/MCR-2359
https://qmacbis.atlassian.net/browse/MCR-2360