-
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
Submission Summary V2 with FetchContract #2350
Conversation
if (contract.draftRevision) { | ||
return contract.draftRevision | ||
} else { | ||
return contract.packageSubmissions[0].contractRevision | ||
return getLastContractSubmission(contract)?.contractRevision | ||
} | ||
} | ||
const isContractOnly = (contract: Contract): boolean => { |
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.
No change needed now just sharing thought when I see all these ?
introduced to our utility functions.
Have all these optional checks around the contract revision's existence means the boolean becomes less sure - we could be getting false
for malformatted data, not just for the actual intended check. I like seeing utilities, especially on the frontend like this (which are literally determining display state) be more strictly typed. We would have to handle data validation/ correctness checks much earlier in the React component, divert to an error state if theres any question about the data shape. Also starting to wonder if helpers should take the ContractFormData
not the Contract
along those lines - maybe have the component itself figure out which form data to pass? Not sure.
…e-review into mcr-4014-submission-submmary-v2
const isSubsequentSubmission = r.kind === 'submit' | ||
// We want to know if this contract has multiple submissions. To have multiple submissions, there must be minimum | ||
// more than the initial contract revision. | ||
const hasSubsequentSubmissions = revisionHistory.length > 1 |
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.
Previously this was set to >=3:
// We want to know if this package has multiple submissions.
// To have multiple submissions, there must be minimum
// 3 revisions in revisionHistory the initial submission revision,
// unlock revision and resubmission revision.
const hasSubsequentSubmissions = revisionHistory.length >= 3
I thought it might make sense to change it to > 1 because each packageSubmission has an array of revs but the multiple submissions part might not be relevant anymore?
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.
The code change you made looks good to me. I would delete the comment totally- I agree that it doesn't make sense for the new world where related rate changes can result in entries in that same package submission array
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.
looks great!
const result: flatRevisions[] = [] | ||
//Reverse revisions to order from earliest to latest revision. This is to correctly set version for each | ||
// contract & recontract. | ||
const reversedRevisions = [...contract.packageSubmissions].reverse() |
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 put package submissions in time order? I was matching revisions b/c calling contract.packageSubmissions[0] feels like a nice way to always get the most recent submission but could be convinced otherwise
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 do if we want to keep the order the same as what currently appears (unless I'm missing something?)
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 don't think so. Multiple pages use that contract.packageSubmissions[0]
logic already and its working well. This is edge case - I think it's literally just for that integer in the previous submission urls why we reverse in this order on this one page - everywhere else fine to keep current backend ordering and assume latest first.
//Use unshift to push the latest revision unlock info to the beginning of the array | ||
result.unshift(newUnlock) | ||
} | ||
if (r.contractRevision.submitInfo) { |
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 is necessary b/c it is optional in the types, but just to reiterate every revision in packageSubmissions must have this set.
@@ -77,8 +78,8 @@ export const RateDetailsSummarySectionV2 = ({ | |||
contract.draftRevision?.formData || | |||
getLastContractSubmission(contract)?.contractRevision.formData | |||
const rates = | |||
contract.draftRates || |
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.
little weird to me that we're just grabbing one or the other.
I think we should be pulling from one or the other depending on isEditing. if isEditing then we are reading from draftRates, if not, then the latest submitted 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.
@macrael I believe it's because of types that we do this.Everything is optional, so you want to narrow it early to avoid that optionality messing up the UI. If you do the current implementation of isEditing
then either value could still be undefined which trickles down.
Def think we could use type assertions or more precise tools eventually. Maybe we need a design ticket on that? However, right now coming out GraphQL we don't have a union type that' s either a draft or a locked package, you have a type two optional subfields that determine what is being used, so you need to clear that early.
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.
WIth this change, getting draftRates after checking for last submission, as a state user you won't see the current version of the rates if you are editing a submission that has been submitted before. Looking at it again I think this is a bug
data: fetchContractData, | ||
loading: fetchContractLoading, | ||
error: fetchContractError, | ||
} = useFetchContractQuery({ |
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 know Hana isn't sold on it but I still think that the ApolloQueryWrapper is useful, makes some impossible cases impossible
const result: flatRevisions[] = [] | ||
//Reverse revisions to order from earliest to latest revision. This is to correctly set version for each | ||
// contract & recontract. | ||
const reversedRevisions = [...contract.packageSubmissions].reverse() |
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.
🔍 Is this code working properly with linked rates?
I am under the impression that we would only want contract related package submissions in our change right now. I think at this point we only seek parity with current ChangeHistory
display (e.g. even if there are linked rates we are not yet adding in linked rates to change history). The problem I see is right now we are mapping over all the packageSubmissions, won't there be extra fields then in the Change history currently?
I would think we need to filter only contract changes from packageSubmission
array into the reversedRevisions
array.
Change history work with the full package submissions list is entirely different epic I believe
@macrael can you weigh in?
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 think that if you filtered out all packages that dont have reason: "CONTRACT_SUBMISSION" then you will have the same number of entries as before. That will also give us the correct unlock reasons, we should only ever show unlocks from CONTRACT_SUBMISSION, not from RATE* updates
I definitely want this code to be using packageSubmissions and get off of the old revisions code
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.
@haworku that makes sense to me
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.
@pearl-truss sounds like @macrael agrees, we should be filtering only the contract_submission packages here - can you make that change and ignore rate stuff? Otherwise looks good.
@@ -216,7 +232,13 @@ const CMSUserRoutes = ({ | |||
)} | |||
<Route | |||
path={RoutesRecord.SUBMISSIONS_SUMMARY} | |||
element={<SubmissionSummary />} | |||
element={ |
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.
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.
Ugh! Scoop creep. Make it a new ticket. Yes it should because thats the only way CMS can see linked rates stuff. CMS absolutely needs to be seeing the v2 page once linked rates is on.
Maybe we could get away fetching with new API and doing the unlock with the old HPP API and the old unlock on this v2 page for this one case on the V2 page. Need to look
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 created the ticket here
Summary
This PR
fetchContract
APIsubmitContract
so you should be able to test this without needing to switch feature flags on and offmccrsID
tofetchContract
Related issues
https://jiraent.cms.gov/browse/MCR-4014
Screenshots
Test cases covered
QA guidance