Issue1645 followup Fix PreferencesComponent file size validation behavior #1677
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,6 +21,7 @@ import { useTranslate } from '../../redux/componentHooks'; | |
| import TimeZoneSelect from '../TimeZoneSelect'; | ||
| import { defaultAdminState } from '../../redux/slices/adminSlice'; | ||
| import { checkboxStyle, labelStyle } from '../../styles/modalStyle'; | ||
| import { MAX_FILE_SIZE_LIMIT } from '../../../../common/preferencesValidationConstants'; | ||
|
|
||
| /** | ||
| * @returns Preferences Component for Administrative use | ||
|
|
@@ -72,11 +73,13 @@ export default function PreferencesComponent() { | |
|
|
||
| warningFileSize: (): boolean => { | ||
| return Number(localAdminPref.defaultWarningFileSize) < 0 | ||
| || Number(localAdminPref.defaultWarningFileSize) > MAX_FILE_SIZE_LIMIT | ||
| || Number(localAdminPref.defaultWarningFileSize) > Number(localAdminPref.defaultFileSizeLimit); | ||
| }, | ||
|
|
||
| fileSizeLimit: (): boolean => { | ||
| return Number(localAdminPref.defaultFileSizeLimit) < 0 | ||
| || Number(localAdminPref.defaultFileSizeLimit) > MAX_FILE_SIZE_LIMIT | ||
| || Number(localAdminPref.defaultWarningFileSize) > Number(localAdminPref.defaultFileSizeLimit); | ||
| } | ||
| }; | ||
|
|
@@ -324,12 +327,28 @@ export default function PreferencesComponent() { | |
| value={localAdminPref.defaultWarningFileSize} | ||
| onChange={e => makeLocalChanges('defaultWarningFileSize', Number(e.target.value))} | ||
| min='0' | ||
| max={Math.min( | ||
| Number(localAdminPref.defaultFileSizeLimit), | ||
| MAX_FILE_SIZE_LIMIT | ||
| )} | ||
| maxLength={50} | ||
| invalid={invalidFuncs.warningFileSize()} | ||
| /> | ||
| <FormFeedback> | ||
| {Number(localAdminPref.defaultWarningFileSize) < 0 ? ( | ||
|
Member
There was a problem hiding this comment. First, this new message is off since it indicated zero is not allowed. If that is why you added it then it can go. Second, I don't think it is needed. It isn't done on other pages. I tried removing the conditional and true part and I get all the messages desired. Thus, if not for the first item I still think this should go. Do you agree?
Contributor
There was a problem hiding this comment. I agree that since a value less than 0 cannot be typed (negative value), then there is no case that the "error.greater" message is displayed. Therefore, that condition is never reached. When the condition is removed, then the desired messages are still displayed. I have updated the FormFeedback line to the following: |
||
| <FormattedMessage | ||
| id="error.greater" | ||
| values={{ min: 0 }} | ||
| /> | ||
| ) : ( | ||
| <FormattedMessage | ||
| id="error.bounds" | ||
| values={{ | ||
| min: 0, | ||
| max: Number(localAdminPref.defaultFileSizeLimit) | ||
| }} | ||
| /> | ||
| )} | ||
| </FormFeedback> | ||
| </div> | ||
| <div> | ||
|
|
@@ -341,11 +360,18 @@ export default function PreferencesComponent() { | |
| value={localAdminPref.defaultFileSizeLimit} | ||
| onChange={e => makeLocalChanges('defaultFileSizeLimit', Number(e.target.value))} | ||
| min={Number(localAdminPref.defaultWarningFileSize)} | ||
| max={MAX_FILE_SIZE_LIMIT} | ||
| maxLength={50} | ||
| invalid={invalidFuncs.fileSizeLimit()} | ||
|
Member
There was a problem hiding this comment. I played with this and it isn't showing the max warning as I had hoped. I debugged for a little while but not completely. It seems to relate to the fact that it is using a general function (makeLocalChanges) to handle changes and not one that thinks it is a number. This may unwind back to how it is stored in OED (here and/or server). I looked at src/client/app/components/meters/CreateMeterModalComponent.tsx and it does it differently but seems to check the max value. Another issue is that src/server/routes/preferences.js has a max of 1000000000. I'm unsure where that came from. src/server/routes/meters.js on maxVal does not seem to have any limit and assumes it could not come through as a number if it was invalid (I think). I'm unclear on why OED has an upper limit on preferences and if the one in meter does what is desired. Again, it may relate to treating as a string instead of a number. I could not add above so am putting here. The date check here is close to but a little different than meter. Unsure if it matters but wonder about standardizing them. The old PR has a comment that also included unit for some of these. That may also help/need coordination. I'm unsure the right overall solution for this and welcome thoughts.
Contributor
Author
There was a problem hiding this comment. Hi @huss, @aduques and I dug into this a bit more and reproduced the issue. When entering a very large value, such as 9999999999999, for Default File Size Limit, the frontend accepts it without inline validation, but the backend rejects it with a Bad Request because preferences.js currently validates both file-size fields with a maximum of 1000000000. I noticed you mentioned you were unsure where that limit came from, so I was thinking about a few possible directions:
Since these settings appear to be used mainly for CSV or spreadsheet imports, I would not expect most deployments to approach a 1 GB upload. That made me wonder whether the current limit is an intentional design decision or primarily a safeguard against unrealistic values. A shared constant seems maintainable and would be easy to update later if larger files need to be supported. I’m happy to implement whichever direction makes the most sense. While tracing this, I also noticed what appears to be the same type of frontend/backend validation mismatch for Default Meter Reading Gap: the frontend effectively allows values above the backend maximum. I think that should probably be handled in a separate issue or PR to keep this one focused, but I wanted to mention it because it appears to be the same class of bug.
Member
There was a problem hiding this comment. @GoodKimchi & @aduques Thank you for thinking this through. My thoughts are below.
Yes. OED should not allow files that are too large. 1 GB seems more than enough. Aligning the front/back-end is a good idea and centralizing so they can never differ is even better. You can proceed with this unless you have thoughts/questions.
Nice catch. I agree they should be consistent. Reading gap is in seconds so 86400 is 60 days. That was undoubtedly deliberate as a more than max value to ever use. I think it would stop really silly values while still allowing it to be so big that one can turn off the warnings about gaps. So, that value should consistently be used. Do you want to create the issue or should I? If you want to work on it then put in a comment on the new issue or let me know. I welcome any thoughts.
Contributor
Author
There was a problem hiding this comment. Thanks @huss . That all makes sense. @aduques and I will go ahead with the shared constant approach for the file-size validation so the frontend and backend stay in sync.
Contributor
Author
There was a problem hiding this comment. Thanks for the guidance, @huss . We implemented the shared constant approach and pushed the changes. The frontend and backend now both use a shared MAX_FILE_SIZE_LIMIT constant from src/common, so the validation is consistent and the frontend prevents values that the backend would reject. We kept the Reading Gap changes out of this PR as discussed to keep it focused. @aduques has reviewed and tested the changes, so I believe it's ready for your review. I'll open a separate issue and follow-up PR for the Reading Gap consistency changes. |
||
| /> | ||
| <FormFeedback> | ||
| <FormattedMessage | ||
| id="error.bounds" | ||
| values={{ | ||
| min: Number(localAdminPref.defaultWarningFileSize), | ||
| max: MAX_FILE_SIZE_LIMIT | ||
| }} | ||
| /> | ||
| </FormFeedback> | ||
| </div> | ||
| <div> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| /* | ||
| * This Source Code Form is subject to the terms of the Mozilla Public | ||
| * License, v. 2.0. If a copy of the MPL was not distributed with this | ||
| * file, You can obtain one at http://mozilla.org/MPL/2.0/. | ||
| */ | ||
|
|
||
| export const MAX_FILE_SIZE_LIMIT: number; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| /* This Source Code Form is subject to the terms of the Mozilla Public | ||
| * License, v. 2.0. If a copy of the MPL was not distributed with this | ||
| * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ | ||
|
|
||
| /** | ||
| * Shared validation limits for Admin Preferences. | ||
| * Used by both frontend and backend to keep validation consistent. | ||
| */ | ||
| const PREFERENCES_VALIDATION_CONSTANTS = { | ||
| MAX_FILE_SIZE_LIMIT: 1000000000 | ||
| }; | ||
|
|
||
| module.exports = PREFERENCES_VALIDATION_CONSTANTS; |
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'm uncertain when the min is needed. It is not in the form feedback below. Since the defaultFileSizeLimit cannot exceed MAX_FILE_SIZE_LIMIT I thought that the check on defaultFileSizeLimit would be sufficient. What do you think?
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 agree that min in this case is redundant. I have removed the min property and have confirmed that you cannot type a value below 0 regardless. I believe that this min property was initially included to capture a case where the user might attempt to input a minimum value. I have not investigated what exactly prevents no negative values from being inputed, but I can confirm that the min property is not responsible and can be removed.