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

MCR-3900 Can continue or save as draft with new API #2313

Merged
merged 7 commits into from
Mar 12, 2024

Conversation

haworku
Copy link
Contributor

@haworku haworku commented Mar 8, 2024

Summary

Happy path for Continue and Save As Draft - using the new updateDraftContractRates mutation on Continue

  • Change formik object to be called rateForms instead of rates. I'm sorry for the code diff. It was getting very confusing to me in the code when we had rates as in GQL rates and when we had rate forms coming from Formik. Tried to make naming more distinct.
  • I moved LinkYourRates a level up so that I can control form data using the FieldArray.replace in next PR
  • Fix route navigate call

Related issues

https://jiraent.cms.gov/browse/MCR-3900

Screenshots

Test cases covered

  • Added tests for the new generateUpdatedRates utility
  • Skip RateDetails tests again

QA guidance

  • You can add and remove data from rate details V2 now with existing form data
    Note: Linked Rates radio and dropdown won't hook in yet - thats the next set of PRs

@haworku
Copy link
Contributor Author

haworku commented Mar 8, 2024

I think I'm seeing an issue with the API. It seems to be creatingdraftRates on the contract that are malformatted due to the stateNumber, I believe? Can investigate next week. Surfacing for visibility @macrael

Getting this error back

 Error from API fetch ApolloError: Issue finding a contract with history with id 2b18aa60-fec4-458e-82dd-b49d41690ef3. Message: [
  {
    "code": "too_small",
    "minimum": 1,
    "type": "number",
    "inclusive": true,
    "exact": false,
    "message": "Number must be greater than or equal to 1",
    "path": [
      "draftRevision",
      "rateRevisions",
      0,
      "rate",
      "stateNumber"
    ]
  },
  {
    "code": "too_small",
    "minimum": 1,
    "type": "number",
    "inclusive": true,
    "exact": false,
    "message": "Number must be greater than or equal to 1",
    "path": [
      "draftRates",
      0,
      "stateNumber"
    ]
  }

RateRevision: {
fields: {
formData: {
merge: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Getting GQL warnings (not blocking but they clutter the console) because form datas dont have id but are nested objects that are changing.

https://www.apollographql.com/docs/react/caching/cache-field-behavior/#merging-non-normalized-objects

Following docs, I believe this is the proper way to address but welcome other eyes.

Copy link
Contributor

@macrael macrael Mar 11, 2024

Choose a reason for hiding this comment

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

interesting, funny because these aren't really stand alone objects, we just made formData to wrap the data up nicely. This looks right to me based on those docs

@haworku haworku marked this pull request as ready for review March 11, 2024 22:15
@haworku haworku requested review from macrael, pearl-truss and ruizajtruss and removed request for macrael March 11, 2024 22:15
Copy link
Contributor

@macrael macrael left a comment

Choose a reason for hiding this comment

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

Looks good. I am a little surprised at the dual headed nature of this component but that's for another time

attribute: boolean | Date | string | null | undefined
): string => {
function formatForForm<T> (
attribute: T
Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect you could just use any here and it would work the same as the generic parameter

@@ -16,7 +16,7 @@ describe('LinkYourRates', () => {
onSubmit={(values) => console.info('submitted', values)}
>
<form>
<LinkYourRates fieldNamePrefix="rates.1" index={1} />
<LinkYourRates fieldNamePrefix="rateForms.1" index={1} />
Copy link
Contributor

Choose a reason for hiding this comment

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

good name change

// Set up data for form. Either based on contract API (for multi rate) or rates API (for edit and submit of standalone rate)
const ratesFromContract =
fetchContractData?.fetchContract.contract.draftRates // TODO WHEN WE IMPLEMENT UPDATE API, THIS SHOULD ALSO LOAD FROM ANY LINKED RATES
const initialRequestLoading = fetchContractLoading || fetchRateLoading
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, now I think I see where you are skipping requests. I'm a little surprised by this pattern, seems like a lot of overhead is required in order to use this component for both stand alone rate edit and inside of the RateDetails array. No need to change this now but I'd like to look at that after summit again.

}

// convert from FormikRateForm to GQL RateFormData used in API
const convertRateFormToGQLRateFormData = (
Copy link
Contributor

Choose a reason for hiding this comment

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

I like having these explicit converters

Copy link
Contributor

Choose a reason for hiding this comment

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

Same, this is a great addition!

@haworku
Copy link
Contributor Author

haworku commented Mar 12, 2024

am a little surprised at the dual headed nature of this component but that's for another time

Yes hoping we remove this and that we get to actually do this right. I still would like to see a SINGLE and MULTIPLE RateDetails v2 also with separation of concerns. However, I was finding keeping that interface clean and also figuring out how to encapsulate the API requests and their handling challenging. A bit hard to refactor and do forward facing work with the short timeline. Putting them together helped me see more clearly what's the same (for standalone v multiple rates) and whats different in one file. Made me more confident I didnt totally break single rate experience

Copy link
Contributor

@pearl-truss pearl-truss left a comment

Choose a reason for hiding this comment

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

LGTM

@haworku haworku merged commit 619f2f2 into main Mar 12, 2024
28 checks passed
@haworku haworku deleted the MCR-3900-continue-button branch March 12, 2024 14:05
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.

3 participants