Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[bugfix] Prevent SettingsQR from trying to enable Persistent Settings when SD card is not inserted #464

Merged
merged 6 commits into from
Sep 3, 2023

Conversation

kdmukai
Copy link
Contributor

@kdmukai kdmukai commented Aug 31, 2023

fixes #457

The problem

If you remove the SD card and then scan in a SettingsQR that attempts to enable Persistent Settings, you get a System Error exception.

The fix

  • Simply ignore the attempt to enable Persistent Settings when the SD card is not inserted.
  • Refine the SettingsQR status message to reflect where the new settings are being stored:

SettingsIngestSettingsQRView_not_persistent SettingsIngestSettingsQRView_persistent

Additional changes

  • The test suite now mocks out the MicroSD class so that individual tests can run scenarios with varying MicroSD.is_inserted states. Instead of the original class' is_inserted property, we just provide it as an exposed class var that any test can directly alter as needed. The setup for each test automatically resets it to True (see BaseTest.setup_method()).
  • Adds test flows for scanning in SettingsQRs with various combinations of Persistent Settings vs MicroSD state.
  • Adds the above screenshots to the screenshot generator w/slight reorg of the Settings group.
  • Moves SettingsUpdatedScreen out of scan_screens.py and renamed into settings_screens.py.
  • Fixes a previously innocuous bug in TestSettings that omitted super().setup_class(). Had no effect before, but runs into problems with the updates in BaseTest in this PR without it.

@jdlcdl
Copy link

jdlcdl commented Sep 1, 2023

As of a48620f

ACK tested

It's nice having the separate confirmation screens that say exactly where settings are.

Tested on pi2 Raspi-os and pi0 seedsigner-os, all tests passing too.

@kdmukai
Copy link
Contributor Author

kdmukai commented Sep 1, 2023

All credit goes to you on this one, @jdlcdl! You found the bug AND figured out the cause that was totally eluding me.

@newtonick newtonick added bug Something isn't working merge conflicts has merge conflicts that need resolution labels Sep 1, 2023
@jdlcdl
Copy link

jdlcdl commented Sep 1, 2023

In case it helps:

I've resolved this conflict locally by keeping most changes for both pr_464 and latest dev, in two separate sections of ./tests/screenshot_generator/generator.py with all tests passing and 6 additional successful screenshots bringing total number to 100.

 diff --cc tests/screenshot_generator/generator.py
index 6592ed3,c50e719..0000000
--- a/tests/screenshot_generator/generator.py
+++ b/tests/screenshot_generator/generator.py
@@@ -124,12 -123,7 +124,13 @@@ def test_generate_screenshots(target_lo
              PowerOptionsView,
              RestartView,
              PowerOffView,
 +            NotYetImplementedView,
 +            (UnhandledExceptionView, dict(error=UnhandledExceptionViewFood)),
 +            NetworkMismatchErrorView,
 +            (OptionDisabledView, dict(settings_attr=SettingsConstants.SETTING__MESSAGE_SIGNING)),
 +
 +
+             (settings_views.SettingsIngestSettingsQRView, dict(data="settings::v1 name=Uncle_Jim's_noob_mode")),
          ],
          "Seed Views": [
              seed_views.SeedsMenuView,
@@@ -221,12 -215,19 +222,24 @@@
              tools_views.ToolsAddressExplorerAddressListView,
              #tools_views.ToolsAddressExplorerAddressView,
          ],
 -        "Settings Views": settings_views_list,
 +        "Settings Views": settings_views_list + [
 +            settings_views.IOTestView,
 +            settings_views.DonateView,
 +            (settings_views.SettingsIngestSettingsQRView, dict(data=settingsqr_data_persistent), "SettingsIngestSettingsQRView_persistent"),
 +            (settings_views.SettingsIngestSettingsQRView, dict(data=settingsqr_data_not_persistent), "SettingsIngestSettingsQRView_not_persistent"),
 +        ],
+         "Misc Error Views": [
+             NotYetImplementedView,
+             (UnhandledExceptionView, dict(error=UnhandledExceptionViewFood)),
+             NetworkMismatchErrorView,
+             (OptionDisabledView, dict(settings_attr=SettingsConstants.SETTING__MESSAGE_SIGNING)),
+             (ErrorView, dict(
+                 title="Error",
+                 status_headline="Unknown QR Type",
+                 text="QRCode is invalid or is a data format not yet supported.",
+                 button_text="Back",
+             )),
+         ],
      }

@jdlcdl
Copy link

jdlcdl commented Sep 2, 2023

as of rebased d810d12

ACK tested.

on pi0 w/ hash 2b76dd99f767efb22472cca7f8d29fb254dba7e6ea682843342822d5db7f0fb3

@newtonick newtonick removed the merge conflicts has merge conflicts that need resolution label Sep 3, 2023
@newtonick
Copy link
Collaborator

ACK and tested

@newtonick newtonick merged commit 193ebfc into SeedSigner:dev Sep 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug Report: Enabling Persistent Settings w/ No MicroSD Inserted Produces Error
3 participants