[Bugfix] Multiselect settings must have a selection #850
Conversation
src/seedsigner/models/settings.py
Outdated
| if type(new_settings[entry.attr_name]) == str: | ||
| # Break comma-separated SettingsQR input into List | ||
| new_settings[entry.attr_name] = new_settings[entry.attr_name].split(",") | ||
| elif new_settings[entry.attr_name] is None: |
There was a problem hiding this comment.
| elif new_settings[entry.attr_name] is None: | |
| elif not new_settings[entry.attr_name]: |
When all options are de-selected in a multiselect setting, its value is [], this led to the condition failing ([] is NOT None), and so the default values were not being assigned. Simply changing the condition to not ... should work.
The test for this passed because the value was explicitly being set to None.
There was a problem hiding this comment.
Oof, good catch.
I've reworked this section and built up the related tests to explicitly test for when the value is "", ",", or None.
There was a problem hiding this comment.
LGTM now, just a minor nit: might as well add [] to the list in tests.
|
Tested ACK. Verified that loading invalid (empty) persistent settings sets the default values and that it is impossible to exit a multiselect settings menu without selecting atleast one option. |
2 participants
Description
Bug found by @Chaitanya-Keyal in discussion here.
Users can de-select all options in a multiselect, leaving their configuration in an invalid state. This can ultimately cause an exception during the xpub export flow.
Current multiselect settings:
Changes in this PR
New UX flow: Enforce selecting at least one option
If users try to exit the multiselect options Setting screen with no options selected, they are presented with this warning screen and then re-routed back to the Setting screen. There is no way to escape this flow until at least one option is selected.
Check when loading persistent Settings
If the persistent settings json has a multiselect with no options selected, the Setting's defaults are loaded instead. There's no good flow to notify the user of the issue: imagine the case where all three multiselects are invalid. The resulting additional flow logic to notify the user and have them fix each one would be a mess.
So instead we just ensure that the Views that depend on those multiselect Settings can function properly by initializing the defaults.
Check when loading SettingsQR
We already have failure exceptions for invalid SettingsQRs so we add a new one for multiselects with no options selected.
Discussion
Currently it doesn't make sense to have a multiselect with no options selected. But if we ever need to relax this for some future Setting, we can add a bool to
SettingsDefinitionlikecan_select_noneand adjust the logic in these 3 places accordingly.Misc notes
Various tests and flow tests either amended or created to verify each of the 3 new behaviors.
Adds screenshot shown above.
This pull request is categorized as a:
Checklist
pytestand made sure all unit tests pass before submitting the PRIf you modified or added functionality/workflow, did you add new unit tests?
I have tested this PR on the following platforms/os: