-
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
Modify submitContract to have the same submitInfos on the contract and rates #2302
Conversation
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.
this looks good, probably worth tightening up the "which related rates do we submit" logic.
If you submit a contract and link it to another rate that was created by another contract, then unlock both contracts, submitting contract 2 should not submit the rate that was originally submitted by contract 1.
include: includeLatestSubmittedRateRev, | ||
}, | ||
// find the current contract with related rates | ||
const currentRev = await client.contractRevisionTable.findFirst({ |
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.
why move this out of the 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.
Generally you don't need a read operation to be in a transaction because it has no bearing on rollback. Also it can make the transaction longer, which in our case is probably not a huge deal since we're low volume, but anything that can make a row or table lock for longer I tend to want to avoid. Since this one is easy to break out (as opposed to the later findFirst
) I just pulled it to the top.
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 thought reading in a transaction would guarantee that you update based on the read data?
Like if you read latestStateNumber as 0, then update it as 1, there's no chance that someone else would do the same in-between you?
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.
Oh yeah that's true, I just didn't see us using currentRev
for that sort of data here. I'll move it back in.
return new Error(message) | ||
} | ||
// get the related rate revisions and any unsubmitted rates | ||
const relatedRateRevs = currentRev.draftRates.map((c) => c.revisions[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.
Technically, we should only be submitting rates that are parented by this contract. This is another thing that will change once we have unlock-resubmit rate, in which case you should only be able to submit DRAFT rates with this contract.
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.
Ok I wrote another test where I put together a contract with two rates, call submitRate
on one of the rates , and then call submitContract
, comparing the submitInfos
between them.
I filter out for draftRates
that don't already have a submitInfo
, which I took to mean it hasn't been submitted yet and are in DRAFT still. Let me know if that's not right though!
@@ -96,6 +94,22 @@ async function submitContract( | |||
}, | |||
}) | |||
|
|||
// we only want to update the rateRevision's submit info if it has not already been submitted | |||
for (const rev of unsubmittedRates) { | |||
await tx.rateRevisionTable.update({ |
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 could probably do an updateMany here or something but this is fine we aren't getting submissions with 100s of rates
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.
yeah that's a good point, I'll try to update that to an updateMany
.
@@ -159,10 +173,7 @@ async function submitContract( | |||
return await findContractWithHistory(tx, contractID) | |||
}) | |||
} catch (err) { | |||
console.error('Prisma error submitting contract', err) |
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.
did you get a type error here or something? Technically err is unknown so your change is more correct but I've never seen something complain
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.
Ah yeah I got into this habit with otel and forgot to put this error
into otel. I secretly don't like how much console stuff we output (particularly when trying to read our test suite output) and would rather everything get shipped to otel 😂 .
Summary
This modifies
submitContract
to connect thesubmitInfos
to the contract and the rate to be the same.Just wanted to call out that I couldn't get Prisma to accept the submitInfo connect directly on the contractRevisionTable update and instead had to update the
rateRevisionTable
directly, which feels a little weird to do insubmitContract
.Related issues
https://jiraent.cms.gov/browse/MCR-3973