Issue 1217 - csv upload validation #1403
Conversation
…ion for checking each value towards the bottom of the uploadMeters.js file as well as calling them when the submission is sent to the server
… made sure the correct data was being sent back to the client side
…ifferent caps/lowercase
Boolean value validations
Min max validation
Merge branch 'Issue1217-csv-validation' of https://github.com/SageMar/OED into Issue1217-csv-validation
|
@SageMar & @cmatthews444 I saw you made some changes since my comments and wanted to see where this PR is at. Are you planning to do more work on this, do you need any help or is that status something else? Thanks. |
|
@huss We are definitely still here finishing it up. It's been difficult to find the time between other projects this quarter, but we're aiming to fix the last errors and at least get in the validations we listed in the opening post for this PR. When we last left off with the code, we had 3 errors still occurring which we were working to figure out what they mean. It would actually help a ton if you're able to give clarification on them as they're not quite as straightforward as the others we got. If it's easier to chat about this over Discord we can schedule a time on Friday (if you're available, I believe we can both make ourselves available any time on Friday other than 1pm PST), but if they're not too complicated to sum up here that would be great.
|
Both of these failed because it expected OED to report a success code (200) but it received an error code (400). I would debug this by adding a console.log to the code to print out the response text since that might indicate what went wrong.
This one is curious because the returned error is Do you want to work on these yourselves? I can also help (either working with you or by myself). Let me know what you prefer and if you want/need more information. |
I think we'll at least give it a try ourselves with that information. If we're still stuck by Tuesday, I'll reach out again! Thank you! |
…ecked, now it converts strings to ints and functions
|
@huss In case you were curious about what caused the issues, it seems the confusing error on pipe101 was what effected all of them. For come reason those files in particular had the number under area coming in as undefined, so using parseInt() on the value ended up fixing all three. It passed tests and appears to be working for everything on our end, but let us know if there's anything else that needs to be changed! Thanks for always being so helpful. |
huss
left a comment
huss
left a comment
There was a problem hiding this comment.
Thanks to @SageMar & @cmatthews444 for continued work on this issue. I reviewed/tested and made some comments. Please let me know if you need any help with this.
|
@huss We are planning on working on this later this week, will reach and let you know if we need help from there. |
|
@huss Do you have any time to meet with us on Friday in the Discord channel? Just looking to get a little clarification on some of the fixes. We're both available anytime after 11AM PST. Thanks! |
Yup, that works great! See you then |
…checked for validity before calling it to check min<max
… decimals are passing through
|
@SageMar & @cmatthews444 I wanted to find out the status of this PR. Is it ready or is more work needed? Sorry that I lost track of this. |
Yes, it is ready! I had forgotten to push up the last change to make timezone validation use moment.tz, but that's included now. I believe we fixed all the comments you had before. Thank you for your patience with us! |
huss
left a comment
huss
left a comment
There was a problem hiding this comment.
Thanks to @SageMar & @cmatthews444 for continued work on this issue. The changes are good where I made a couple of comments to address.
I did systematic testing of the code and found these checks are not being made:
- gap should be zero or greater.
- variation should be zero or greater.
- duplicates should be 1 to 9.
- if area unit is none then area cannot be specified
- max error must be 0 to 75
- the max date cannot be before the min date; a bonus would be to validate they are valid dates but the check may fail anyway if they are not and that is fine
I realize this is a non-trivial number of changes so let me know if doing them is an issue.
| // do nothing, pass it through | ||
| } else if (isNaN(minValue) || minValue < -9007199254740991 || minValue > maxValue) { | ||
| throw new CSVPipelineError( | ||
| `Invalid min/max values in row ${rowIndex + 1}: min="${meter[27]}", max="${meter[28]}". ` + |
There was a problem hiding this comment.
Here and below you directly use the meter value. Since you assigned it a variable above I think it would be best to use that to be consistent.
| const minValue = Number(meter[27]); | ||
| const maxValue = Number(meter[28]); | ||
|
|
||
| if (isNaN(minValue) && isNaN(maxValue)) { |
There was a problem hiding this comment.
I feel the logic is tricky. For example, assume only maxValue is a NaN. The first else if will compare minValue > maxValue in the first else if but maxValue is a NaN. Also, the second if below stops the else if so I don't see how the exception can occur in this case. Maybe this logic is better and simpler to understand (open to ideas):
// Check minValue is okay. If maxValue is a NaN then skip last check but it will throw an exception below.
if (isNaN(minValue) || minValue < -9007199254740991 || (!isNaN(maxValue) && minValue > maxValue)) {
exception
}
// Check maxValue is okay.
// A check for minValue > maxValue is not needed since it would have caused an exception in the
// if above unless minValue or maxValue is a NaN but those cause the exception anyway.
if (isNaN(maxValue) || maxValue > 9007199254740991) {
exception
}
|
@SageMar & @cmatthews444 I saw your commit on 7 June and appreciate your long efforts on this work. I wanted to see if are or are planning to continue work on this. Please leave a comment with any information. If OED does not hear anything by 1 Aug. then we will try to see about someone else finishing up the needed items to address. |
Thank you for your patience with us through this whole process - I had hoped we would be able to do that last round of changes, but unfortunately we have both been extremely busy. It may be best to pass it on to someone else now in order to get it done in a reasonable time frame. Thank you again for working with us for so long! |
|
Thanks for the quick response. I've unassigned the issue and will look for someone else to work on this. |
3 participants
Description
"The create meter page stops admins from entering bad values by constraining choices and/or checking before saving. It would be good if the CSV meter upload page did similar checks." After a talk with Steve, it was decided that this validation should be done in JavaScript on the back-end, so these validations are inside the uploadMeters.js file.
Done in collaboration with @cmatthews444
Partly Addresses #1217
This adds validations for the following variables:
Type of change
(Check the ones that apply by placing an "x" instead of the space in the [ ] so it becomes [x])
Checklist
(Note what you have done by placing an "x" instead of the space in the [ ] so it becomes [x]. It is hoped you do all of them.)
Limitations
This does not cover every single validation done on a CSV file - it only covers those listed above.