Merge pull request #850 from kdmukai/multiselect_settings_selection_required

[Bugfix] Multiselect settings must have a selection

Author: newtonick

src/seedsigner/models/settings.py

@@ -81,6 +81,10 @@ def parse_settingsqr(cls, data: str) -> tuple[str, dict]:
         for entry in data.split()[split_index:]:
             abbreviated_name, value = entry.split("=")
 
+            # Empty values ("some_setting= other_setting=E") are invalid
+            if value == "":
+                raise InvalidSettingsQRData(f"{abbreviated_name} cannot be empty")
+
             # Parse multi-value settings; integer-ize where needed
             if "," in value:
                 values_updated = []
@@ -103,6 +107,7 @@ def parse_settingsqr(cls, data: str) -> tuple[str, dict]:
                 values = [value]
             else:
                 values = value
+
             for v in values:
                 if v not in [opt[0] for opt in settings_entry.selection_options]:
                     if settings_entry.attr_name == SettingsConstants.SETTING__PERSISTENT_SETTINGS and v == SettingsConstants.OPTION__ENABLED:
@@ -154,8 +159,13 @@ def update(self, new_settings: dict):
                 # Clean the incoming data, if necessary
                 if entry.type == SettingsConstants.TYPE__MULTISELECT:
                     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(",")
+                        # Break comma-separated multiselect options into List; avoid empty
+                        # values.
+                        new_settings[entry.attr_name] = [value for value in new_settings[entry.attr_name].split(",") if value.strip()]
+
+                    if not new_settings[entry.attr_name]:
+                        # Multiselect cannot be empty; load defaults to avoid issues
+                        new_settings[entry.attr_name] = entry.default_value
 
         for key, value in new_settings.items():
             self.set_value(key, value)

src/seedsigner/views/settings_views.py

@@ -204,6 +204,13 @@ def run(self):
         )
 
         if ret_value == RET_CODE__BACK_BUTTON:
+            if self.settings_entry.type == SettingsConstants.TYPE__MULTISELECT:
+                # After the user finishes toggling multiselect options, initial_value will
+                # have their final selections when they hit BACK to exit. All current
+                # multiselect settings require at least one option to be selected.
+                if not initial_value:
+                    return Destination(SettingsSelectionRequiredWarningView, view_args={"attr_name": self.settings_entry.attr_name})
+
             if self.blocking_view:
                 return Destination(self.blocking_view, clear_history=True)
             return settings_menu_view_destination
@@ -261,6 +268,37 @@ def run(self):
 
 
 
+class SettingsSelectionRequiredWarningView(View):
+    def __init__(self, attr_name: str):
+        super().__init__()
+        self.settings_entry = SettingsDefinition.get_settings_entry(attr_name)
+
+
+    def run(self):
+        from seedsigner.gui.screens.screen import WarningScreen
+
+        # TRANSLATOR_NOTE: Title of a warning dialog when configuring a setting that requires at least one option to be selected.
+        title = _("Selection Required")
+
+        # TRANSLATOR_NOTE: The name of the setting being configured (e.g. "Script types") will be inserted.
+        text = _("At least one option must be selected for \"{}\".").format(self.settings_entry.display_name)
+
+        # TRANSLATOR_NOTE: Text for the button that returns the user to the setting configuration screen.
+        button_text = _("Return to setting")
+
+        self.run_screen(
+            WarningScreen,
+            title=title,
+            status_headline=None,
+            text=text,
+            button_data=[ButtonOption(button_text)],
+            show_back_button=False,
+        )
+
+        return Destination(SettingsEntryUpdateSelectionView, view_args=dict(attr_name=self.settings_entry.attr_name))
+
+
+
 class SettingsIngestSettingsQRView(View):
     def __init__(self, data: str):
         from seedsigner.hardware.microsd import MicroSD

tests/screenshot_generator/generator.py

@@ -427,6 +427,7 @@ def PSBTOpReturnView_raw_hex_data_cb_before():
                 ScreenshotConfig(settings_views.DonateView),
                 ScreenshotConfig(settings_views.SettingsIngestSettingsQRView, dict(data=settingsqr_data_persistent), screenshot_name="SettingsIngestSettingsQRView_persistent"),
                 ScreenshotConfig(settings_views.SettingsIngestSettingsQRView, dict(data=settingsqr_data_not_persistent), screenshot_name="SettingsIngestSettingsQRView_not_persistent"),
+                ScreenshotConfig(settings_views.SettingsSelectionRequiredWarningView, dict(attr_name=SettingsConstants.SETTING__SCRIPT_TYPES)),
             ],
             "Misc Error Views": [
                 ScreenshotConfig(NotYetImplementedView),

tests/test_flows_settings.py

@@ -37,17 +37,35 @@ def test_persistent_settings(self):
 
 
     def test_multiselect(self):
-        """ Multiselect Settings options should stay in-place; requires BACK to exit. """
+        """
+        Multiselect Settings options should stay in-place; requires BACK to exit. If no
+        selections are made, route to the warning screen and return the user to the
+        settings entry until at least one option is selected.
+        """
         # Which option are we testing?
-        settings_entry = SettingsDefinition.get_settings_entry(SettingsConstants.SETTING__XPUB_QR_FORMAT)
+        settings_entry = SettingsDefinition.get_settings_entry(SettingsConstants.SETTING__SIG_TYPES)
+
+        # Enable all options to start
+        self.settings.set_value(settings_entry.attr_name, [option[0] for option in settings_entry.selection_options])
+
+        # Sanity check, we only expect two options for this setting
+        assert len(settings_entry.selection_options) == 2
 
         self.run_sequence([
             FlowStep(MainMenuView, button_data_selection=MainMenuView.SETTINGS),
             FlowStep(settings_views.SettingsMenuView, button_data_selection=settings_views.SettingsMenuView.ADVANCED),
             FlowStep(settings_views.SettingsMenuView, button_data_selection=ButtonOption(settings_entry.display_name)),
-            FlowStep(settings_views.SettingsEntryUpdateSelectionView, screen_return_value=0),  # select/deselect first option
-            FlowStep(settings_views.SettingsEntryUpdateSelectionView, screen_return_value=1),  # select/deselect second option
-            FlowStep(settings_views.SettingsEntryUpdateSelectionView, screen_return_value=1),  # select/deselect second option
+            FlowStep(settings_views.SettingsEntryUpdateSelectionView, screen_return_value=0),  # deselect first option
+            FlowStep(settings_views.SettingsEntryUpdateSelectionView, screen_return_value=1),  # deselect second option
+            FlowStep(settings_views.SettingsEntryUpdateSelectionView, screen_return_value=1),  # select second option
+            FlowStep(settings_views.SettingsEntryUpdateSelectionView, screen_return_value=1),  # deselect second option
+            FlowStep(settings_views.SettingsEntryUpdateSelectionView, screen_return_value=RET_CODE__BACK_BUTTON),  # BACK to exit
+
+            # Both options were deselected, should route to the warning screen
+            FlowStep(settings_views.SettingsSelectionRequiredWarningView),
+            FlowStep(settings_views.SettingsEntryUpdateSelectionView, screen_return_value=0),  # select first option
+
+            # Now we can exit
             FlowStep(settings_views.SettingsEntryUpdateSelectionView, screen_return_value=RET_CODE__BACK_BUTTON),  # BACK to exit
             FlowStep(settings_views.SettingsMenuView),
         ])

tests/test_settings.py

@@ -1,3 +1,4 @@
+import json
 import pytest
 from base import BaseTest
 from seedsigner.models.settings import InvalidSettingsQRData, Settings
@@ -35,6 +36,68 @@ def test_settings_defaults(self):
             assert settings.get_value(settings_entry.attr_name) == settings_entry.default_value
 
 
+    def test_load_persistent_settings(self):
+        """ Settings should load previously saved persistent settings from disk, if any
+        exist. """
+        # Initial Settings will start with defaults
+        settings = Settings.get_instance()
+
+        # Enable persistent settings and make another change
+        settings.set_value(SettingsConstants.SETTING__PERSISTENT_SETTINGS, SettingsConstants.OPTION__ENABLED)
+
+        assert settings.get_value(SettingsConstants.SETTING__QR_DENSITY) != SettingsConstants.DENSITY__HIGH
+        settings.set_value(SettingsConstants.SETTING__QR_DENSITY, SettingsConstants.DENSITY__HIGH)
+
+        # Hold on to the settings.json content
+        settings_json = None
+        with open(Settings.SETTINGS_FILENAME) as settings_file:
+            settings_json = json.loads(settings_file.read())
+
+        # Now wipe out the Settings singleton
+        BaseTest.reset_settings()
+
+        # This also deletes settings.json, so recreate it
+        with open(Settings.SETTINGS_FILENAME, "w") as settings_file:
+            settings_file.write(json.dumps(settings_json))
+
+        # Now instantiate the Settings singleton again; it should load from disk
+        settings = Settings.get_instance()
+
+        # Persistent setting change should have survived
+        assert settings.get_value(SettingsConstants.SETTING__QR_DENSITY) == SettingsConstants.DENSITY__HIGH
+
+
+    def test_load_empty_multiselect_settings(self):
+        """ Empty multiselect settings should load defaults. """
+        # Initial Settings will start with defaults
+        settings = Settings.get_instance()
+
+        # Enable persistent settings to write settings.json to disk
+        settings.set_value(SettingsConstants.SETTING__PERSISTENT_SETTINGS, SettingsConstants.OPTION__ENABLED)
+
+        # Hold on to the settings.json content
+        settings_dict = None
+        with open(Settings.SETTINGS_FILENAME) as settings_file:
+            settings_dict = json.loads(settings_file.read())
+
+        def _verify_defaults_loaded(attr_name: str):
+            # Verify that the multiselect setting has loaded its defaults
+            settings = Settings.get_instance()
+            cur_setting_value = settings.get_value(attr_name)
+            assert cur_setting_value == SettingsDefinition.get_settings_entry(attr_name).default_value
+
+        # Alter the settings to test against various empty values
+        for empty_value in ["", ",", [], None]:
+            settings_dict[SettingsConstants.SETTING__SIG_TYPES] = empty_value
+            settings.update(settings_dict)
+            _verify_defaults_loaded(SettingsConstants.SETTING__SIG_TYPES)
+
+        # One last test: remove the multiselect setting entirely
+        del settings_dict[SettingsConstants.SETTING__SIG_TYPES]
+        settings.update(settings_dict)
+        _verify_defaults_loaded(SettingsConstants.SETTING__SIG_TYPES)
+
+
     def test_parse_settingsqr_data(self):
         """
         SettingsQR parser should successfully parse a valid settingsqr input string and
@@ -108,6 +171,14 @@ def test_settingsqr_fails_unrecognized_option(self):
         assert "passphrase" in str(e.value)
 
 
+    def test_settingsqr_fails_empty_values(self):
+        """ SettingsQR parser should fail if a setting is empty """
+        settingsqr_data = "settings::v1 persistent=D sigs= camera=180"
+        with pytest.raises(InvalidSettingsQRData) as e:
+            Settings.parse_settingsqr(settingsqr_data)
+        assert "sigs" in str(e.value)
+
+
     def test_settingsqr_parses_line_break_separators(self):
         """ SettingsQR parser should read line breaks as acceptable separators """
         settingsqr_data = "settings::v1\nname=Foo\nsigs=ss,ms\nscripts=nat,nes,tr\npassphrase=E\n"