Skip to content
This repository has been archived by the owner on Mar 28, 2023. It is now read-only.

Settings invalid data #754

Merged
merged 3 commits into from
Sep 7, 2017
Merged

Settings invalid data #754

merged 3 commits into from
Sep 7, 2017

Conversation

rmisio
Copy link
Contributor

@rmisio rmisio commented Sep 1, 2017

This PR handles (the rare) case where you're settings have been saved with data that does not pass client validation. It is likely you saved a piece of data using another client and both that client and the server have less restrictive validation that the desktop client.

In such a case before this PR, unless you were on the tab containing the field with the bad data, the tab you were on would fail to save and the process would look broken.

Now, if you try and save a tab and the save is being blocked by an error in a different tab, you will get this alert:

image

That's not the ideal UX. It would be better to automatically take the user to the tab with the error and show the error in our standard way (i.e. red text above the field). We're deferring on that because:

1.) It's a good amount of work to make that happen. Each tab would need to enumerate the fields it's responsible for and then we'd need to programmatically select and save those tabs. This is complicated by the fact that some tabs like the Store and Shipping Addresses have a different UX than the other ones. There would probably have to be a default flow as well as giving each tab a way to customize the behavior to show a tab and any errors in it.
2.) It's hopefully rare that this situation will be in play.

To test this PR.

1.) Comment out the following lines in the smtpSettings model and restart the app.

      if (is.not.url(attrs.serverAddress)) {
        addError('serverAddress', app.polyglot.t('smtpModelErrors.serverAddress'));
      }

2.) In the settings >advanced tab, save out milly as the server address:

image

3.) uncomment the validation from line 1 and restart the app.
4.) attempt to save out any other tab that also uses the settings model (e.g. General).

@jjeffryes jjeffryes merged commit 278254b into master Sep 7, 2017
@jjeffryes jjeffryes deleted the settings-invalid-data branch September 7, 2017 16:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants