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

Issue 1217 - csv upload validation #1403

Open
wants to merge 18 commits into
base: development
Choose a base branch
from

Conversation

SageMar
Copy link
Contributor

@SageMar SageMar commented Dec 5, 2024

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:

  • Enabled
  • Disabled
  • Type
  • Timezone
  • Area
  • Cumulative
  • Reset
  • Timesort
  • End Only
  • Area Unit
  • Min Val
  • Max Val
  • Disabled Checks

Type of change

(Check the ones that apply by placing an "x" instead of the space in the [ ] so it becomes [x])

  • Note merging this changes the database configuration.
  • This change requires a documentation update

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.)

  • I have followed the OED pull request ideas
  • I have removed text in ( ) from the issue request
  • You acknowledge that every person contributing to this work has signed the OED Contributing License Agreement and each author is listed in the Description section.

Limitations

This does not cover every single validation done on a CSV file - it only covers those listed above.

}

// Validates read values to either false or true
if (value !== 'true' && value !== 'false' && value !== true && value !== false) {
Copy link
Member

Choose a reason for hiding this comment

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

I know this is a draft PR but a number of the tests were failing. I debugged one of them. The issue is that endOnlyTime on a meter can be undefined. In src/server/sql/meter/create_meters_table.sql it will set it to a default value if undefined. Thus, some tests need to allow for this.

I also noticed that a few inputs go through normalizeBoolean() that allows for yes/no in addition to true/false. FYI.

Please let me know if you need any more information or help.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for those comments, they're very helpful. We took a short break over the holidays so we'll get to work resolving those ASAP.

Happy new year!

@huss
Copy link
Member

huss commented Jan 20, 2025

@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.

@SageMar
Copy link
Contributor Author

SageMar commented Jan 20, 2025

@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.

  1. Meter testing files starting 'pipe100' doing 'Second meter upload where incorrectly provides meter identifier so fails' with 2

  2. Meter testing files starting 'pipe101' doing 'Second meter with same name so fails but first meter exists' with 1 requests

  3. Meter testing files starting 'pipe103' doing 'Uploading meters using Email. First succeeds then the second meter upload fails because it incorrectly provides meter identifier' with 2 requests

@huss
Copy link
Member

huss commented Jan 20, 2025

@SageMar

  1. Meter testing files starting 'pipe100' doing 'Second meter upload where incorrectly provides meter identifier so fails' with 2
  2. Meter testing files starting 'pipe103' doing 'Uploading meters using Email. First succeeds then the second meter upload fails because it incorrectly provides meter identifier' with 2 requests

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.

  1. Meter testing files starting 'pipe101' doing 'Second meter with same name so fails but first meter exists' with 1 requests

This one is curious because the returned error is Failed to upload meters due to internal OED Error: For meter pipe101 the area entry of 33 is invalid. Area must be a number greater than 0. but the value of 33 is greater than 0. I'm guessing you added a test that the area is valid. It may not be working as desired and/or it is returning new info so the test needs to be updated. I would first want to figure out why the test seems to have a message that is inconsistent on 33 > 0.

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.

@SageMar
Copy link
Contributor Author

SageMar commented Jan 24, 2025

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.

@huss

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
@SageMar SageMar marked this pull request as ready for review January 24, 2025 21:11
@SageMar
Copy link
Contributor Author

SageMar commented Jan 24, 2025

@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.

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.

3 participants