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-3547] Add view for adding a mccrs record number #1996

Merged
merged 31 commits into from
Oct 25, 2023

Conversation

pearl-truss
Copy link
Contributor

@pearl-truss pearl-truss commented Oct 20, 2023

Summary

Adds a form for CMS users to fill out to update the MC-CRS record number which is a field on the contract table

figma designs

Related issues

https://qmacbis.atlassian.net/browse/MCR-3547

To Do items

  • Update the header banner on the Add MC-CRS ID page to display the submission name. Currently it just says CMS

Screenshots

Screenshot 2023-10-20 at 10 51 53 AM Screenshot 2023-10-20 at 10 52 59 AM Screenshot 2023-10-20 at 10 53 27 AM

Test cases covered

  • Validations for the MC-CRS record number (must be 4 digits exactly, a number, not blank)
  • The add MC-CRS record number link is only displayed to CMS users

QA guidance

  • Create a submission as a state user
  • View the submission summary as a CMS user
  • Click "Add MC-CRS record number" link
  • You should be able to enter a 4 digit number and click save to be taken back to the submission summary page
  • Once back on the submission summary page you should see: MC-CRS record number:[the number you entered]

services/app-web/src/pages/MccrsId/MccrsId.tsx Outdated Show resolved Hide resolved
services/app-web/src/pages/MccrsId/MccrsId.tsx Outdated Show resolved Hide resolved
services/app-web/src/pages/MccrsId/MccrsId.test.tsx Outdated Show resolved Hide resolved
},
})

expect(screen.getByTestId('textInput')).toBeInTheDocument()
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Some of these tests could probably be combined into one. This text input could be moved to 'renders without errors'. And the validation tests could be all in one test.

} from '../../testHelpers/jestHelpers'
import { MccrsId } from './MccrsId'

describe('MCCRSID', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: A test for failing useUpdateContractMutation would be a good addition. This one might be tricky because you would have to create a new mocked mutation for useUpdateContractMutation that fails. See services/app-web/src/testHelpers/apolloMocks/healthPlanPackageGQLMock.ts for some examples and we could also pair if you need!

Copy link
Contributor

@JasonLin0991 JasonLin0991 left a comment

Choose a reason for hiding this comment

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

Almost there just two more things.

{showMCCRSRecordNumber && (
<Route
path={RoutesRecord.SUBMISSIONS_MCCRSID}
element={<MccrsId />}
Copy link
Contributor

Choose a reason for hiding this comment

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

This change caused a flicker of the 404 not found page before going to the MCCRSID page.

Screen.Recording.2023-10-24.at.3.21.17.PM.mov

Copy link
Contributor

@JasonLin0991 JasonLin0991 Oct 24, 2023

Choose a reason for hiding this comment

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

This might be a tricky fix. I think it's because we use a hook for feature flags inside the route's component, and that toggles what routes are valid. I wouldn't be opposed to moving this into a separate bug ticket.

Thoughts? @macrael @haworku

Copy link
Contributor

@haworku haworku Oct 24, 2023

Choose a reason for hiding this comment

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

Ughhh! And also yes this shouldn't block this PR. To me the flicker is a follow on bug ticket (sure) or just can be wrapped into edit work as we finish out this epic. @pearl-truss happy to take a look at this - lmk

Copy link
Contributor

Choose a reason for hiding this comment

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

I think an answer is to only display the blank page until flags have been loaded

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't been able to reproduce this yet. I tried switching the flag on and off and loading the app in an incognito window. If there's anything else I can try, that would be helpful. And I can include a fix for this in the edit ticket.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think locally, it happens so fast that you might not see it. If you visit the deployed app for this PR you should be able to see it there

Copy link
Contributor

Choose a reason for hiding this comment

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

On local, throttling network speeds might get the bug to show up.
Screenshot 2023-10-24 at 10 41 12 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, thanks Jason :)

showError={Boolean(
showFieldErrors(errors.mccrsId)
)}
type="text"
Copy link
Contributor

Choose a reason for hiding this comment

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

We should trim the empty spaces before the MC-CRS number, but should we trim spaces in general? For example, 12 34.

This is what the URL looks like with spaces before the ID
Screenshot 2023-10-24 at 3 31 16 PM

Screenshot 2023-10-24 at 3 31 31 PM

Copy link
Contributor

@haworku haworku Oct 24, 2023

Choose a reason for hiding this comment

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

Yes let's trim all whitespace before building those urls, good catch. Since we are following on quickly with edit work I think this doesn't have to block the initial PR.

@pearl-truss pearl-truss merged commit d9e413e into main Oct 25, 2023
27 checks passed
@pearl-truss pearl-truss deleted the mcr-3547-add-view-mccrs-id branch October 25, 2023 20:34
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.

4 participants