WIP: Refactor Meter Validation (#1217)#1622
Conversation
…d improve validation logic
- Change function to accept minValue and maxValue directly instead of full meter array
- Extract minValue (meter[27]) and maxValue (meter[28]) at the call site in uploadMeters
- Replace throws inside the function with returned { minMaxErrorMsg, value } object for consistency with other validators
- Replace isNaN() with safer typeof and Number.isNaN(Number()) check per MDN guidance
- Remove hardcoded range limits (-9007199254740991 / 9007199254740991) — valid number strings are now accepted without boundary checks per PM feedback
- Replace Number() early conversion with delayed conversion — values are now converted only after validation is confirmed
- Fall back to Number.MIN_SAFE_INTEGER and Number.MAX_SAFE_INTEGER as DB defaults when one value is missing, ensuring the min > max check always runs against a meaningful boundary
- Add null checks alongside undefined and empty string checks for consistency
- Add quick exit when both values are empty
- Updated isValidAreaUnit to return an object with a boolean and a specific error message. - Added rowIndex parameter to provide location specific feedback for CSV errors. - Improved error messaging to include the exact unrecognized unit provided in the row.
…ct checks - Refactored function to take maxErrorValue directly instead of the meter array. - Added checks for undefined, null, and empty strings to handle optional CSV fields. - Implemented stricter numeric validation using Number.isNaN(Number()) before range check. - Modify error messaging to include the exact invalid value and current row index.
- Updated isValidTimeSort to return an object with a success flag and error message. - Added rowIndex parameter to pinpoint the exact location of invalid time sort values. - Enhanced error feedback by suggesting valid options (increasing or decreasing).
- Updated isValidMeterType to return a structured error object instead of a boolean. - Added rowIndex to the function signature for precise CSV error location reporting. - Enhanced error feedback by dynamically listing all valid meter types from Meter.type
…validation logic
- Add rowIndex parameter for error reporting
- Replace boolean return with { areaMsg, value } object for consistency with other validators
- Add descriptive error messages indicating the invalid value and row number
- Updated function call to reflect the changes
- Converted function from throwing direct errors to returning structured objects. - Added guard clause to check for null, undefined, and empty string inputs. - Implemented stricter numeric validation including Number.isFinite check. - Modified error messaging to include the specific row index and invalid value.
- Changed function signature to accept gapValue directly.
- Replaced direct Error throws with result objects { gapMsg, value }.
- Added checks for empty strings, null, and non-finite numbers.
- Modified error message formatting to include row index and invalid input.
…checks
- Updated validateArea to handle both numeric validation and 'none' unit logic.
- Implemented strict type and finite number checks to prevent invalid data.
- Refactored to accept direct values (areaValue, areaUnitString) for easier testing.
- Aligned return structure with the { areaMsg, value } pattern.
- Replaced hardcoded 'none' string with Unit.areaUnitType.NONE
- Updated isDuplicate to return an object with a boolean and a specific error message. - Added rowIndex parameter to provide location specific feedback for CSV errors. - Included guard clause to handle undefined, null, and empty inputs.
- Updated number check to include typeof and Number.isNaN(Number()) - Included check for Infinity | -Infinity inputs
- Updated function signature to return an Object containing an error message and a boolean success flag. - Included rowIndex as a parameter for error reporting. - Updated function call to reflect these changes.
- Merged isValidDate and correctDateTimeFormat into a single validateDateRange function.
- Implemented whitespace trimming for min/max date inputs.
- Updated parsing to use Moment.js directly with strict format validation.
- Update return structure to { dateMsg, value } for consistent error reporting.
- Improved error messaging to include the row index and the specific invalid date value.
- refactored to return a {boolMsg, value] object.
|
@DestinyVeloz Thank you for the PR. I'm sorry for the delay in looking at it due to my recent issues. While doing an overview, I wanted to know how this relates to PR #1579 (and the included work in that in PR #1403)? I think I understand that you were one of the developers of PR #1579 and this is supposed to continue that work. However, I don't see any commits from that previous work nor the previous PR it included. Am I missing something in the git record or can you give me a better understanding of this? I wanted to figure that out before doing anything. Also, were you hoping for feedback at this time? |
@huss Yes I was a part of the team that worked on PR #1579 (which integrated validation structure from #1403). For this PR I started with a clean updated feature branch off of the latest development branch to continue working on the issue (improving/creating validation functions for CSV inputs). I think because I branched directly off the current development baseline rather than changing it directly onto the historical PR branch, those older individual commits from #1579 wont show up as unique additions in this specific PR's git history. It's meant to build directly on top of the current state of the main repository. I am new to git & github, so maybe the way I submitted this WIP PR is a mistake on my end. But yes I was hoping for feedback. |
|
@DestinyVeloz Thank you for you comment and information. I think removing the history of commits can be viewed as an issue. The work put in by other people will not show up in the development branch so it is hard to see what they did. I would have thought some of their work had value so including it would be appropriate. Do you feel what was done before should be ignored and a new method should be used? If not, then using it without attribution is something OED would like to avoid. I welcome your thoughts/ideas. I can appreciate you are new to git. Know there are ways to start from the current development version on some files but leave the commit history. However, this is normally done if the other changes are not going to be used. There are other options as well. What might be appropriate depends on how the previous work is being viewed so knowing your thoughts above would help. I'm not going to look at the actual code until this is resolved. |
|
@huss Thank you for pointing that out, and I completely agree with you. I absolutely do not want to ignore or erase the valuable work of previous contributors. My initial approach was purely an attempt to present a "clean" file to make reviewing easier, but I see now how that disrupts attribution in the projects history. To answer your question, no the previous work should not be ignored. This PR is a direct continuation and polish of the previous code, not a complete rewrite. Since I am still gaining experience with complex git workflows, I would love to get your guidance on the best way to resolve this. Should I close this current PR and open a new feature branch that chains onto the original PR #1579 branch first, and then apply my fresh refactors on top of that? I definitely want to make sure everyones contributions are fully attributed. |
@DestinyVeloz You may be newer to git but your ideas are fine. It does make sense to close the current PR. You can give a reason if you want. It is also fine to wait until the new PR is in place and then reference it in the old PR by saying there is a newer one so the old one is being closed. That will link them in GitHub. As for how to move forward, here is what many people do:
There are other ways but that is what I know best. If you have questions or feel you need help then just let me know. You can also message me on Discord (easily on the OED Discord server linked in the contact page on our website) to discuss and/or set up a call to work together on getting started. I hope this helps and I appreciate your willingness to shift your work to preserve the git history. |
2 participants
Description
Contributor Note: I am taking full ownership of this issue moving forward as there has been no recent communication from previous assignees. All current and future work on this refactor will be handled by me.
This PR refactors the uploadMeters function and its associated validation logic to improve maintainability, error reporting, and data integrity. The core validation logic has been decoupled into standalone helper functions that follow a standardized return-object pattern {value, msg}.
Changes Made
Partly Addresses [#1217]
Type of change
Checklist
Limitations
To do
Functions that need updating
isValidArea - returns an error when the area is empty/undefined, but empty should be allowed.
isValidTimeSort - fails when timeSortValue is undefined; should likely allow undefined/empty as valid default case
isValidMeterType - Needs a guard clause for missing values
isDuplicate - treats null/"" as invalid but undefined as valid; this inconsistency may cause issues.
Unused res Parameter: I noticed that the res object is currently passed into uploadMeters but never utilized. Should I remove this to clean up the signature, or is it being kept for future middleware compatibility?