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

GV Refactor - omsValidator #2436

Merged
merged 24 commits into from
Oct 7, 2024
Merged

GV Refactor - omsValidator #2436

merged 24 commits into from
Oct 7, 2024

Conversation

ailZhou
Copy link
Collaborator

@ailZhou ailZhou commented Sep 16, 2024

Description

omsValidator is used to run any validaiton related to the Optional Measure Stratification (OMS) section. So the omsValidator.tsx script has to handle checking the OMS N/D/R fields and act as a gateway to call other OMS related components that get passed in as a callback function.

This code is a full refactor as the previous code was hard to merge across every year. The full refactor could still be refactored better (espcially the getOMSRates function), so if you have any better ideas of how to make the code even more readable, please let me know!

So What Do These New Code Do

  • errorToFillNDR & errorForNDRSelection: These functions were added to cut down on having to write the same error message several times. Not necessary but it does reduce repetitive code.
  • getOMSRates: This is the meat of the whole component. The function goes through all the checkboxes and retrieves the rate information that is needed to be checked against the validator. I really want to refactor this to be smaller but hopefully it is more readable than before.
  • validateNDR, validateFields & validateValues: These three functions are the different types of checks we use to validate the data in the rates field.
    • validateNDR is used to check our typical N/D/R sets
    • validateFields is used for more unique ones that don't have N/D/R sets, like AIF-HH & IU-HH.
    • validateValues is used for PCR rates
  • buildOPMLocationDictionary: This builds a label lookup for Other Performance Measure's OPM section. It matches the structure they have set for looking up OMS labels.
  • omsValidations: This is how the other scripts will call this script. The order of operations are:
    • Initial setup by checking if OMS data is OPM - OMS data, build the opm label dictionary and get the rate data and storing it in omsRates
    • Loop through omsRates and check each rates,
      • if rateData exist, we run it against specific validate functions and get those error labels to store in errorArray
      • if rateData does not exist, create and store error in errorArray variable
      • Then we run any callback functions that are being passed in
      • Then we run validatePartialRateCompletionOMS, this I had kept from the previous code because I didn't understand why it had to be structured like this so I choose not to adjust it.
    • After we look through omsRates, we should have all the error messages needed and then we return it back to the measure validation to be rendered

Preemptively Answering Questions You May Have
What happened to the checkIsFilled prop?
I am still confused about the existence of this prop, from what I could see, it was always true so it was never used. I guess the idea was to allow the callbacks to run but turn off n/d/r validation checks? I honestly think that we should refactor this more so that we wouldn't be running other OMS validation through here as if it was a highway but that's a battle for latter.

Why was there an addition to validateOPMRates script?
That chunk of code that I added to validateOPMRates was originally in the omsValidator, I thought it made more sense to move it.

In the omsValidator unit test, it looks like you just changed the expected return of errors.length? Are you just trying to avoid writing a real test?
So my changes does add more validation messages back but they are also more accurate. It was not consistent on when you would see the validation message and it was not generating a message for each qualifier in a category and only generate for the first one. There is a chance that the numbers is still wrong but its not any less wrong than before 😫.

If this is still all too confusing and you want a walkthrough, please let me know!

Related ticket(s)

CMDCT-3965


How to test

  1. Sign into QMR, any state user
  2. In Reporting Year 2024, select an adult core set
  3. Enter AAB-AD
  4. In the Measure Specification Section, select the first radio button
  5. Scroll to the Performance Measure Section, and fill our all the N/D/R sets
  6. Now Screen down to the the OMS section
    Here is what the validation is at different stages of selection:

Screenshot 2024-09-25 at 10 05 08 AM

Screenshot 2024-09-25 at 10 05 14 AM

Screenshot 2024-09-25 at 10 05 46 AM

Screenshot 2024-09-25 at 10 05 52 AM

Screenshot 2024-09-25 at 10 06 39 AM

Screenshot 2024-09-25 at 10 06 49 AM

Screenshot 2024-09-25 at 10 07 31 AM

Screenshot 2024-09-25 at 10 07 39 AM

Notes

N/A


Pre-review checklist

  • I have added thorough tests, if necessary
  • I have updated relevant documentation, if necessary
  • I have performed a self-review of my code
  • I have manually tested this PR in the deployed cloud environment

Pre-merge checklist

Review

  • Design: This work has been reviewed and approved by design, if necessary
  • Product: This work has been reviewed and approved by product owner, if necessary

Security

If either of the following are true, notify the team's ISSO (Information System Security Officer).

  • These changes are significant enough to require an update to the SIA.
  • These changes are significant enough to require a penetration test.

convert to a different template: test → val | val → prod

@ailZhou ailZhou added the in progress In progress label Sep 16, 2024
@ailZhou ailZhou marked this pull request as ready for review September 24, 2024 19:45
@ailZhou ailZhou added ready for review Ready for all the reviews! and removed in progress In progress labels Sep 25, 2024
@ailZhou ailZhou requested review from ajaitasaini and removed request for BearHanded September 25, 2024 14:10
Copy link
Contributor

@rocio-desantiago rocio-desantiago left a comment

Choose a reason for hiding this comment

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

Tested in deploy link and works as expected from instructions.
The file services/ui-src/src/shared/globalValidations/omsValidator/index.ts could be a good one to huddle on as a team as it's a bit complex for my tiny brain 😬

Copy link
Contributor

@benmartin-coforma benmartin-coforma left a comment

Choose a reason for hiding this comment

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

Yay!

Copy link

codeclimate bot commented Oct 7, 2024

Code Climate has analyzed commit a36c5ab and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (90% is the threshold).

This pull request will bring the total coverage in the repository to 88.1% (0.0% change).

View more on Code Climate.

@ailZhou ailZhou merged commit 667b264 into master Oct 7, 2024
23 checks passed
@ailZhou ailZhou deleted the cmdct-3965-oms-val branch October 7, 2024 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review Ready for all the reviews!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants