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

QPPSF-5596 Non proprtion validation adjustments #66

Merged
merged 3 commits into from
Apr 2, 2020

Conversation

jbishtawi
Copy link
Contributor

@jbishtawi jbishtawi commented Jan 10, 2020

During validation the submission API adds a new field to the submission object, what we need to do is used the submission object passed in from the client vs the validated Submission since we want to make sure additional fields do not trip the logic

  • I needed to update the package.json because the prePush validation failed without the updates

When loading a bad IA-Quality submission, using the following file

2019-IA-Quality-bad.json.zip

image

When updating a negative non-proportion measure, using the file:

2019-non-proportion-quality.json.zip

image

When updating a negative non-proportion measure, using this file

2019-non-proportion-quality-bad.json.zip

image

@@ -47,8 +47,11 @@ export function fileUploader(submissionBody, submissionFormat, requestHeaders, b
};

return fileUploaderUtil.validateSubmission(submissionBody, submissionFormat, baseOptions)

Choose a reason for hiding this comment

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

Can you just remove the call to fileUploaderUtil.validateSubmission altogether, as it seems we are not using the result anywhere?

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 do not think so, we still need to validate.

But maybe someone else has a better suggestion or agrees with you.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, you need it to validate that the submission structure is valid. It will throw an error if it's not valid.

// QPPSF-5596, part of the validation logic for NonProportion measures for PY 2019, is to add a new field during validation,
// to prevent additional fields being added, we are going to send the original submission object
// to the api since the validation adds some properties
validatedSubmission = JSON.parse(submissionBody);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a point in calling fileUploaderUtil.validateSubmission any more if the result is not used? Could we just replace that call with return fileUploaderUtil.getExistingSubmission(JSON.parse(submissionBody), baseOptions) instead?

idk what the validation logic did before, but is it correct to assume basically we are offloading validation to the submission api?? or is there something else that the validateSubmission function does that we need?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

all good questions, I have no clue

Copy link
Contributor

Choose a reason for hiding this comment

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

No, see my comment above. Though I don't see the need for JSON.parse

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need the JSON.parse here because in getExistingSubmission method we do something like submission.nationalProviderIdentifierand the fact that submission is string when passed into validateSubmission we need to parse it so the properties can be read from it

Does my comment makes sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

@joshboley , do you know if we need anything from the returned validateSubmission object? Or is it okay that we continue to use the submissionBody so long as it passed validation?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, that's the point of this PR...don't use the submission returned from the validate submission endpoint. It will have extra fields that will break your subsequent POST

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, just making sure that that included everything. Thanks :)

@@ -59,6 +60,7 @@ const browserConfig = (env, argv) => {
},
devtool: env && env.production == 'true' ? 'source-map' : 'eval-source-map',
module: _module, // module is already defined
mode: env && env.production === 'true' ? 'production' : 'development',
Copy link
Contributor

Choose a reason for hiding this comment

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

What was the point of changing these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

those are editions, it would not build otherwise

Copy link
Contributor

Choose a reason for hiding this comment

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

https://webpack.js.org/configuration/mode/

this will toggle development vs production level optimizations

so for instance in development mode webpack will name chunks and whatnot, vs in production they do module concatenation and other goodies

Copy link
Contributor

Choose a reason for hiding this comment

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

Likely is new between webpack 3 and 4

@@ -40,6 +40,7 @@ const defaultConfig = (env, argv) => {
path: path.resolve(__dirname, 'dist')
},
devtool: env && env.production == 'true' ? 'source-map' : 'eval-source-map',
mode: env && env.production === 'true' ? 'production' : 'development',
Copy link
Contributor

Choose a reason for hiding this comment

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

And these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see @okeefem3 comment above

@jbishtawi jbishtawi merged commit 09db659 into develop Apr 2, 2020
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.

None yet

5 participants