-
Notifications
You must be signed in to change notification settings - Fork 0
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
CMDCT-4224: POC for update report validation using yup schemas #90
Conversation
.required(); | ||
|
||
const measureTemplatesSchema = object().shape({ | ||
[MeasureTemplateName["LTSS-1"]]: measurePageTemplateSchema, |
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.
im sure there's a way to do this in some sort of reduce function but i attempted to do this for way too long and typescript would cry and my tests would fail with every iteration of a reduce function i would write. so if anyone has any ideas on how to make a reduce function work here, hit me up
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.
In fact we can go a step more declarative than reduce, with Object.fromEntries()
:
const measureTemplateNames = Object.values(MeasureTemplateName);
const measureTemplatesSchema2 = object().shape(
Object.fromEntries(
measureTemplateNames.map((meas) => [meas, measurePageTemplateSchema])
)
);
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 told me not to get excited but too late because THIS WORKS!!!!!!! THANK 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.
ugh. never mind, it was working and then all of a sudden tests started failing. idk, this stuff is really hard to debug so it's kind of killing my soul here
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 great news. I got this working by fixing the enum!
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.
Right now I'm not familiar enough with the report shape to confirm that this validation code is completely correct. But I like the way you've put it together, and if any details need to be adjusted we can do that as they come up.
Very nice!
default: | ||
return mixed().notRequired(); // Fallback, although it should never be hit |
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.
Will this cooperate with stripUnknown
so that invalid element types will be removed from the object? If so, great!
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.
That's a great question and I will write some tests to see if I can confirm or deny this!
.required(); | ||
|
||
const measureTemplatesSchema = object().shape({ | ||
[MeasureTemplateName["LTSS-1"]]: measurePageTemplateSchema, |
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.
In fact we can go a step more declarative than reduce, with Object.fromEntries()
:
const measureTemplateNames = Object.values(MeasureTemplateName);
const measureTemplatesSchema2 = object().shape(
Object.fromEntries(
measureTemplateNames.map((meas) => [meas, measurePageTemplateSchema])
)
);
…that i want to work
@@ -0,0 +1,1154 @@ | |||
import { qmReportTemplate } from "../../forms/qm"; |
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.
File mockReport.ts
has 1141 lines of code (exceeds 1000 allowed). Consider refactoring.
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 am not going to not refactor because this is a mock data file and the mock reports are large and there's no getting around it.
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 lied, i cleaned this up quite a bit so that it's only like 200 lines now
Code Climate has analyzed commit 8354b5e and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 94.8% (90% is the threshold). This pull request will bring the total coverage in the repository to 93.6% (0.0% change). View more on Code Climate. |
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.
Small nitpick to update instances of qm
to qms
i.e, qmReportTemplate
to qmsReportTemplate
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.
May I request the qm to qms update 😬
I did this update on Friday. could i get an approval? |
Description
This is a working idea for update report validation. do not review unless your initials are B.M. or if you want to experience a world of Horrors
Related ticket(s)
CMDCT-4224
How to test