Issue1645 followup Fix PreferencesComponent file size validation behavior

[GoodKimchi]

Description

This pull request continues the work from PR #1603 and addresses issue #1645.

The change restores the intended client-side validation behavior for the default file size settings in PreferencesComponent.tsx. The maximum validation bound is restored to Infinity, which prevents invalid file size values from being submitted while keeping the validation consistent with similar fields on the meter and unit pages.

This pull request also removes a duplicated line in src/server/routes/preferences.js that was introduced during a merge conflict and fixes the related indentation.

Contributors:

• Oscar Bedolla (@GoodKimchi)
• Andrei Solomon Duque (@aduques)

Fixes #1645

Type of change

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

Checklist

• 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

npm run check passes.

npm run test one failure caused by a PostgreSQL deadlock in the test:

Retrieves 15 min reading when asked for 1 hour interval so looking at hourly readings

The reported error was deadlock detected. This is a known test issue currently affecting the development branch and is unrelated to the changes in this pull request. @aduques did not have this issue

[huss]

@GoodKimchi & @aduques Thank you for this contribution. I've made two comments on specific code to consider. I did not do a full review because I am unsure about the changes. It appears you now check if values are invalid at save and pop up a toast message if they are. What OED does on other pages is, when possible/reasonable, detect bad input when it happens, put a red error box around the item, put a red error message below and disable the save button. This means the user immediately knows the issue and cannot save the bad values. This should also be correctly carried to the unsaved warnings so it also will not save. There are limited circumstances where OED does wait until save to check values but that should be where immediate checks cause issues or are too complicated/interrelated to do at field entry. I'm not sure those apply here so I'm seeking your thoughts on whether the popup on save makes sense or if it should be the other way. I'm also noting the discard button does not work but I don't know yet if that is from your changes. Once these are settled/addressed, I should be able to do a fuller review. Please let me know if anything is not clear or you have thoughts/questions.

[aduques]

@huss I wanted to clarify our approach with the changes. Our changes has updated the error notifications that appear when the user tries to save with invalid changes, but we've also preserved the original red box and red text errors that appear as soon as the user tries to input invalid data. This will also disable the save button for the user.

The reason why the TODO DEBUG for disabled={!hasChanges} was included was because it was necessary to show the newly updated error notifications when a user attempts to save with invalid changes (to demonstrate the changes in this PR strictly). As mentioned before, this should never be shown to a user in a normal use case due to disabled={!hasChanges || Object.values(invalidFuncs).some(check => check())} preventing the user from encountering it in the first place. The TODO DEBUG will ultimately be removed as it doesn't satisfy the requirement of showing the red box and red text error and disabling the save functionality.

As intended, the red box and red text error displays when it detects an invalid input + disables the save button. As per the original comments from the original PR, the bounds have also been reverted back to Infinity to catch extremely large numbers that would bypass invalidFuncs.

I would appreciate confirmation if this approach satisfies the intended behavior, or if we should make further changes. In addition, I have noted the changes of the invalidFuncs not being carried over to the UnsavedWarning component and have that button disabled.

I don't recall making changes that affected the discard feature, but I will go through the file and make sure that the button properly fetches the current admin state as expected.

[huss]

I have not looked at the new code yet. Having said that, I would appreciate you explaining the advantage of the new error check and message on save. You state the user should never see it in a normal use case. So, does it only happen if the code is not working properly? If a bad value was passed to the server then hopefully it would be rejected and the user would see that without the new check. Thanks.

[aduques]

#1645 (comment)

By asking the question about the error message, "Failed to submit changes Bad Request," we were under the assumption that this error message was to be made more specific in addition to the end user never seeing it as the checks are in place to prevent it. The condition of preventing a user from attempting the save was always intended to be left untouched. The changes to the error check were included so that the invalid input of a large value in file size limit could be captured by invalidFuncs() (large value such as 9999999999999). I can see why this might have caused confusion as this addition to the error checks was only meant to check the specific case that was mentioned in the issue description. This error check addition should have been added with a TODO DEBUG as it was added to help display the new message.

Specifically these were supposed to be included as the TODO DEBUG and not meant to be added as a new condition for the error check:

  || Number(localAdminPref.defaultWarningFileSize) > Number.MAX_SAFE_INTEGER;
  || Number(localAdminPref.defaultFileSizeLimit) > Number.MAX_SAFE_INTEGER;

As for the new updated message it self, if I have made the wrong assumption about your answer to the question about the error message, "failed to submit changes Bad Request," and updated the error message unnecessarily, then I can quickly revert the additions if desired.

The benefit of this updated message was that it would be updated to follow a similar pattern as other save error messages on other pages and it would contain a specific message that lets a developer know which of the functions/checks in invalidFuncs() was raised. Before the changes, the error message would just say "Failed to submit changes Bad Request" when a save failed. However, with the changes, it would say "Failed to submit changes: (Default File Size Limit has invalid values)" when it detects that fileSizeLimit() returns true in invalidFuncs(). This lets the developer know which check produces the error given. Though, these messages won't be accessed due to the invalidFuncs() disabling the save button.

The core changes of this issue would just be strictly the comment suggestions of the original pull request. The added TODO DEBUGs were related to additions that I could have mistakenly thought were needed about the error message on save.

[huss]

Sorry this sat so long.

By asking the question about the error message, "Failed to submit changes Bad Request," we were under the assumption that this error message was to be made more specific in addition to the end user never seeing it as the checks are in place to prevent it..........

I agree with this goal. What I was wondering is if the user should (virtually) always see the updated messages without changing the save because the component is doing the same validation with a good message.

As for the new updated message it self, if I have made the wrong assumption about your answer to the question about the error message, "failed to submit changes Bad Request," and updated the error message unnecessarily, then I can quickly revert the additions if desired.

The benefit of this updated message was that it would be updated to follow a similar pattern as other save error messages on other pages and it would contain a specific message that lets a developer know which of the functions/checks in invalidFuncs() was raised. Before the changes, the error message would just say "Failed to submit changes Bad Request" when a save failed. However, with the changes, it would say "Failed to submit changes: (Default File Size Limit has invalid values)" when it detects that fileSizeLimit() returns true in invalidFuncs(). This lets the developer know which check produces the error given. Though, these messages won't be accessed due to the invalidFuncs() disabling the save button.

If the only change is to show the user the error message when a save happens then that is fine. I thought it might be redoing the checks and that I feel more ambivalent about. It is more code to maintain with little chance the check will fail (assuming I have the correct idea of the situation). And, the unlikely missed case from not having it in save will return a server error that you now show. Does this make sense? If I have the wrong idea then just let me know.

The core changes of this issue would just be strictly the comment suggestions of the original pull request. The added TODO DEBUGs were related to additions that I could have mistakenly thought were needed about the error message on save.

I'm okay with them if they are for debugging. Maybe you can remove them now so only the code you want permanently in OED is in the PR. That way I can see what will be merged. I can either test another way or look at an earlier commit to temporarily put back a debug statement. How does that sound?

As always, let me know anything unclear/thought/questions.

[huss]

The recent merge of PR #1672 is now causing a merge conflict. Please let me know if you need any help with this. The route moved from login to loginLogout and with new subroutes.

[aduques]

I agree with this goal. What I was wondering is if the user should (virtually) always see the updated messages without changing the save because the component is doing the same validation with a good message.

I'm not quite sure if I understand this statement, but I am interpreting it if a user types an invalid input, then the updated message (from showErrorNotification) would always be displayed like the red box/red-line text error. Or if the updated message be used to replace the existing message of the red-line text error. To clarify, the validation for save will remain the same. The only difference is the new updated error message for save (showErrorNotification). If it is desired to have the updated error notification be displayed on the page when the server detects invalid changes, I would be initially against the idea. The reason for this is because I don't recall showErrorNotification or showSuccessNotification being used when the component reloads (like with React.useEffect() or with the red-text/ red box errors). These notifications are primarily used for when a request has been made. If this is desired (given that I didn't misinterpret your statement), I would suggest that other pages have a similar change where the notifications appear when the server detects invalid changes so that there is consistency.

To close my thought, the idea of having the updated messages (error notification for save) appear for the user in the same way as the red box/red text errors is a good idea on the Admin Settings page. However, I would assume it might be problematic if this behavior only existed on Admin Settings page and not consistent with other pages. I would appreciate insight on this as I believe I may have misinterpreted the initial statement.

If desired, I would propose creating an issue that applies this behavior on other pages to let users know why the save button is disabled for them. If not already, the error messages would display to the users which checks are being raised to disable the save button. The error messages would keep in mind to the specific checks in each page, but this would required to be looked into more to see if it would make sense to add.

As for the other comments, you have answered all of my points and understood my thought process with the code additions. Any changes to the checks were strictly for testing and I will restore it back in the next commits. As for the updated error messages, I will assume that the changes are welcome and will leave it in for you to test along with the other changes.

[huss]

I'm feeling as if I am having trouble expressing my thoughts so they are clear. If you want to talk about this verbally then just let me know. Having said that, let me add this. I was not trying to propose that the error popup happens on page load/reload. What OED has on other pages (and I think here too) is that red boxes with error messages are placed on the page when issues are detected and the save is disabled. If associating with a given input box does not make sense then (I think) the message goes at the bottom of the page just before the save button. The idea was that this should make it clear why the save is disabled. (For now I'm not discussing a modification will happen on save to clean up the input that still allows the save.) If this is correct, then I was wondering why recheck on save on the preferences page since they should never get there if there are issues - at least in normal cases. If that is not the case then I would like to discuss which ones and why. Now, every page should display if the server rejects the request and why since the page checks either miss something or cannot readily check for issues. That is the final fail safe. That is okay and acceptable. Does this make more sense? I hope I'm understanding what you are saying.

Note PR #1672 changed the login/logout route to loginLogout/ and added subroutes. It was recently merged and is likely causing the merge conflict. Please let me know if you want any assistance in resolving it.

[aduques]

If associating with a given input box does not make sense then (I think) the message goes at the bottom of the page just before the save button.

I understand that having the showErrorNotification was not what you meant. I'm interpreting it that we should apply the red box / red text error messages that are used throughout OED to highlight textboxes with invalid inputs to the save button when it is disabled. The red box/red text errors will display the updated messages that specify which flags are raised in the error checks to let the user know why they can't save.

If this is correct, then I was wondering why recheck on save on the preferences page since they should never get there if there are issues - at least in normal cases. If that is not the case then I would like to discuss which ones and why.

I want to clarify that when I previously said,

"The TODO DEBUG was used to display the error message during the case of invalid inputs somehow bypassing the invalidFuncs checks. With the changes, the error message is retained, but the new addition will not affect the implementation of failed.to.submit.changes in the unsaved warning component."

I was not referring to a known bug that allows the user to bypassing the invalidFuncs check. I talked about this to describe the TODO DEBUG case where the invalidFuncs validation checks were removed to test out the updated error messages that displayed specific functions from invalidFuncs. I have not experienced any bugs with this guardrail, and as far as I am aware, invalidFuncs' only issue was the max bounds for fileSizeLimit not being Infinity (which is addressed and fixed in this PR).

I'm assuming the "recheck on save" refers to the addition of the getInvalidFieldNames() which would call invalidFuncs() again:

  onClick={() => {
					      const invalidFieldNames = getInvalidFieldNames();
					      if (invalidFieldNames) {
						      showErrorNotification(
							      translate('failed.to.submit.changes.saved') + '(' + invalidFieldNames + translate('failed.to.submit.changes.fields')
						      );
						      return;
					      }
  
					      submitPreferences(localAdminPref)
						      .unwrap()
						      .then(() => {
							      showSuccessNotification(translate('updated.preferences'));
						      })
						      .catch(err => {
							      showErrorNotification(translate('failed.to.submit.changes.saved') + err.data);
						      });
				      }}

The reason why this helper function calls invalidFuncs() again is to strictly confirm which of the functions in invalidFuncs is raised to determine how to update the error message. I understand that the first call of invalidFuncs() would prevent the recheck from even happening as the save is disabled (the original call that is a condition for disabling save). This recheck would only work with the TODO DEBUG case where the first check is removed and allowing the save button to be available.

With that in mind, I did think of alternatives, but I was hesitant to do so. This alternative solution might work with wanting to implement the red-box/red text error to the disabled save button. This alternative solution would include modifying the invalidFuncs() to update the error message (essentially what getInvalidFieldNames() does). This would prevent a recheck on invalidFuncs() as the initial check will handle disabling the save button if needed and updating the error message. I was hesitant to do this as I was not sure how changing invalidFuncs() might affect the behaviors elsewhere (if there is any beyond PreferencesComponents.tsx, but I assume not).

I think removing the "recheck" is definitely the approach since it would not be accessible in a normal use case when the first check disables the button. Something like the alternative solution I proposed above might be worth trying if you agree.

Note PR #1672 changed the login/logout route to loginLogout/ and added subroutes. It was recently merged and is likely causing the merge conflict. Please let me know if you want any assistance in resolving it.

We are currently working on merging the new changes and we will have a follow up message if we encounter any merge conflicts.

[aduques]

We have pulled and merged the recent changes for PR #1672. We didn't experience any merge conflicts, but noticed an issue with the login/logout. The login and logout still work, but there is an error message after the logout request in console. We are assuming this is not related to a merge conflict as the option to resolve a merge conflict did not pop up and the application started up without any issues.

Initially, I ran into an issue that was preventing me from logging in with either 'test' or 'test@example.com'. However, this was quickly resolved by refreshing the page and trying again (I'm assuming the changes were not live when I initially tried). I wanted to point this out as it seems other students ran into this issue based on the discord channel for codeday-sose.

[huss]

Thanks for the info on login/out. I received several reports yesterday. I posted to the OED Discord server on the developer channel about it so see that if you didn't already. It probably requires a DB change to work completely correctly.

[huss]

Thanks for the comment about the recheck. Given the red boxes & messages catch all the known issues and stops saves, I don't think OED needs to run a second set of checks on save to clarify the error message. It seems much more likely that the error message is from something else and not the checks being done in the component. Thus, the second check will not likely add any useful information. Note OED has decided not to provide the actual error from the server but say something generic as a recommenced security change. The detailed information is in the logs. So, if I understood your message and you agree with this, I think the secondary check on save can go along with all the debug code. Then I can do what is hopefully a final sweep. Please let me know any thoughts.

[aduques]

I understand that it is a design choice for OED to not include these actual error messages to the users and instead shows a generic error message. Therefore, the specific error messages will be scrapped from this PR.

However, I wanted to show you what the specific error messages would look like as a red text/ red box error on the save button. This code is removed from the PR and will not be committed, but I thought it would be interesting to consider adding these messages to OED where buttons would be disabled and it could give info to users as to why.
I've kept the specific messages as placeholder, but it could easily be replaced with generic messages to follow the design choice. I'd imagine this being something potentially helpful to users, but this idea would need to be refined more to give users information that is helpful and not already provided elsewhere. In addition, if this were to be considered in the future, it would require a better design implementation visually as I found what I've tried out to not be as polished visually.

The changes involve assigning the result of invalidFuncs to a variable, invalidFuncsResult, which allows it to be reused and only have one check as desired. We are able to get specific error messages by creating a dictionary/map which has the field name for each function in invalidFuncs. If the function returns true, the function name is used as a key to look up the field name in its bucket. The action of checking invalidFuncs and updating the error message is in the same pass and would not require a second check.

To summarize the code that will be commited in this PR, it will strictly be the requested changes of the original PR we were tasked with. The secondary check, specific error messages, and the debug code will all be removed for the final sweep.

I apologize for the time spent looking at the proposed ideas with the updated error messages for it to not be included in this PR, but I hope the basis of it can inspire future additions to OED.

[huss]

Please down worry at all about the discussion. I find your ideas interesting and they make me think. I'm open to UI changes but I want to be sure I understand the change. It can also be propose to a wider audience to see if they want it (I don't override, in general, the consensus of the group.) Currently OED tries to show each issue on the input page with a red box/message and not allowing save. Your proposal seems a similar look but done during submit. Is it correct that a user should not see your new messages if it is properly covered by the current style that stops saving? If it is only other ones and you need to know in advance to create your map then could these be added to the current checks that disable saving? If it involves an unexpected error then I'm unsure how OED could help the user with a message without showing the actual error (which we don't want to do for other potential issues). My hope is that over time the save button will not allow many errors to come back where the user sees the generic error pop up.

I still feel I'm missing something and I welcome your thoughts.

[aduques]

Your proposal seems a similar look but done during submit. Is it correct that a user should not see your new messages if it is properly covered by the current style that stops saving?

In the screenshot of the error message below the save button, it appears with the current behavior of the save button. So, the error message below the submit does not appear during the submit request, but instead it gets updated when the invalidFuncs() is called during the check. In other words, the current validation behavior that determines if the saved button is disabled is kept exactly the same. The only addition is that the error message would be displayed at the bottom to let the users know why it's still disabled.

As before, I am unaware of ways that users will be able to fail the invalidFuncs() checks (or any other unexpected errors that will raise these error messages), so I cannot provide a scenario at the moment. While I acknowledge that the error message itself can provide useful information, the current guardrails prevent the user as much as possible from even seeing these error messages (as intended).

If interested, here was the code snippets that I left out from the PR that were related to that error message:

  const fieldTranslationIds: { [key: string]: string } = {
      readingFreq: 'default.meter.reading.frequency',
      minDate: 'default.meter.minimum.date',
      maxDate: 'default.meter.maximum.date',
      readingGap: 'default.meter.reading.gap',
      meterErrors: 'default.meter.maximum.errors',
      warningFileSize: 'default.warning.file.size',
      fileSizeLimit: 'default.file.size.limit'
  };
  
  // Computed once by iterating invalidFuncs a single time - reused for the Submit button's disabled
  // state, the FormFeedback visibility, and the detailed list of which fields are invalid.
  const invalidFuncsResult = Object.entries(invalidFuncs)
      .filter(([, check]) => check())
      .map(([key]) => translate(fieldTranslationIds[key]))
      .join(', ');
  
  
  <div className='d-flex justify-content-end mt-3'>
      <Button
	      type='button'
	      onClick={discardChanges}
	      disabled={!hasChanges}
	      style={{ marginRight: '20px' }}
	      color='secondary'
      >
	      {translate('discard.changes')}
      </Button>
      <Button
	      type='submit'
	      onClick={() => {
		      submitPreferences(localAdminPref)
			      .unwrap()
			      .then(() => {
				      showSuccessNotification(translate('updated.preferences'));
			      })
			      .catch(err => {
				      showErrorNotification(translate('failed.to.submit.changes') + err.data);
			      });
	      }}
	      disabled={!hasChanges || !!invalidFuncsResult}
	      color='primary'
      >
	      {translate('submit')}
      </Button>
  </div>
  <FormFeedback style={{ display: invalidFuncsResult ? 'block' : 'none' }}>
      {translate('failed.to.submit.changes.saved') + '(' + invalidFuncsResult + translate('failed.to.submit.changes.fields')}
  </FormFeedback>

[huss]

Thank you for explaining this several times. I think I am finally starting to understand. I think the idea is fine but I'm not yet convinced it is worth doing across the pages. Here is my current thinking:

• It is code that needs to be maintained.
• The pages we are discussing are all admin-only pages. This means they are not the typical OED user and they are much more likely to look at the help pages and/or have experience with the UI. If these were used by many users then that would be different. For example, OED works to remove choices from graphics pages that are not allow and shows a message in large font on the center of the graphic or as popups that are specific.
• Many admin pages would not need scrolling to see the red box and text so it isn't that hard to see if you know to look for it (whenever the save is disabled).
• This is a common web UI technique. I acknowledge some fancy pages jump to the bad value(s) on save but that would be more work for what (I perceive) as small payback on admin pages.

As I said before, you are making me think about alternatives. Given what you have said and done, this already come to mind. A non-specific message could be placed near the save button to emphasize that the admin needs to find all the red boxes and fix them. This might help because OED does use red boxes for items that will be modified on save from what is shown and this clearly shows the difference. The unclickable save button does that too but if people think this would help then I'm open/okay with this. This would be consistent across pages so maintaining it would be much easier. I'm unsure if it would be easy to do but I would think it isn't too bad.

Let me know what you think and you can still advocate for your original solution.

[aduques]

I agree that my proposed code would primarily be used by admin users who will interact with the create/edit features, whereas actual non-admin users would never see these changes. I wanted to suggest these changes, but I can understand that at the moment it has niche applications.

I support the idea of having a generic message be displayed throughout the pages that explain why the buttons are disabled, but I understand that the effectiveness varies on each page. I believe that it would be important to make sure that the generic message doesn't just repeat what the red text/red box error messages display to be effective.

I think that my original solution is not ready for implementation and it has clear room to be improved on. This idea should definitely be reconsidered if OED has gotten a lot of reports from users stating they aren't clear as to why they cannot access the save buttons. However, the existing red text messages should already clear any confusion, so it is likely not the case. For right now, I think it would make sense to put this idea on hold until it becomes apparent that users need changes to clear any future confusions that they might have. Or, it would also make sense to revisit this idea if there is a future request to improve the UI to make it more new user friendly in any way.

As I continue to work on OED, I will keep a look out for any features that might be potentially confusing for end users and discuss these with you when they arise.

[huss]

As I continue to work on OED, I will keep a look out for any features that might be potentially confusing for end users and discuss these with you when they arise.

I appreciate your thoughts so far and any you have in the future. OED welcomes them.

[huss]

@GoodKimchi & @aduques Thank you for the updated code and your willingness to reconsider items done. I have now reviewed and tested. I made one comment to consider. I'm open to ideas on what to do and when/who might address it. Please let me know any thoughts.

[huss]

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.