fixup: error hint logic, tests

client/securedrop_client/gui/conversation/export/export_wizard.py

@@ -17,6 +17,7 @@
     PassphraseWizardPage,
     PreflightPage,
 )
+from securedrop_client.resources import load_movie
 
 logger = logging.getLogger(__name__)
 
@@ -31,9 +32,12 @@ class ExportWizard(QWizard):
     FILENAME_WIDTH_PX = 260
     BUTTON_CSS = resource_string(__name__, "dialog_button.css").decode("utf-8")
 
-    # TODO: If the drive is unlocked, we don't need a passphrase;
-    # if we do, it's populated later
-    PASS_PLACEHOLDER_FIELD = ""
+    # If the drive is unlocked, we don't need a passphrase; if we do need one,
+    # it's populated later. Why is this a space instead of the empty string?
+    # If we need to quit the dialog due to unrecoverable error and the user has
+    # not entered a passphrase, we're prevented from bypassing the unlock page
+    # unless we provide input that is different frome the field's default value.
+    PASS_PLACEHOLDER_FIELD = " "
 
     def __init__(self, export: Export, summary_text: str, filepaths: List[str]) -> None:
         parent = QApplication.activeWindow()
@@ -66,40 +70,31 @@ def text(self) -> str:
         return self.body.text()
 
     def _style_buttons(self) -> None:
-        # When the dialog launches, the export preflight call is executed
-        # on the first screen, as well as for validation on the USB insert page.
-        # Otherwise, all calls are to export
         self.next_button = self.button(QWizard.WizardButton.NextButton)
         self.next_button.clicked.connect(self.request_export)
         self.next_button.setStyleSheet(self.BUTTON_CSS)
         self.cancel_button = self.button(QWizard.WizardButton.CancelButton)
         self.cancel_button.setStyleSheet(self.BUTTON_CSS)
 
         # Activestate animation
-        # TODO: we may not use this
-        # self.button_animation = load_movie("activestate-wide.gif")
-        # self.button_animation.setScaledSize(QSize(32, 32))
-        # self.button_animation.frameChanged.connect(self.animate_activestate)
+        self.button_animation = load_movie("activestate-wide.gif")
+        self.button_animation.setScaledSize(QSize(32, 32))
+        self.button_animation.frameChanged.connect(self.animate_activestate)
 
-    # TODO: may not style buttons like this
     def animate_activestate(self) -> None:
         self.next_button.setIcon(QIcon(self.button_animation.currentPixmap()))
 
-    # TODO
     def start_animate_activestate(self) -> None:
         self.button_animation.start()
-        self.next_button.setText("")
         self.next_button.setMinimumSize(QSize(142, 43))
         # Reset widget stylesheets
         self.next_button.setStyleSheet("")
         self.next_button.setObjectName("ModalDialog_primary_button_active")
         self.next_button.setStyleSheet(self.BUTTON_CSS)
 
-    # TODO- move to parent class
     def stop_animate_activestate(self) -> None:
         self.next_button.setIcon(QIcon())
         self.button_animation.stop()
-        self.next_button.setText(_("CONTINUE"))
         # Reset widget stylesheets
         self.next_button.setStyleSheet("")
         self.next_button.setObjectName("ModalDialog_primary_button")
@@ -132,14 +127,17 @@ def _set_focus(self, which: QWizard.WizardButton) -> None:
         self.button(which).setFocus()
 
     def request_export(self) -> None:
+        logger.debug("Request export")
+        # Registered fields let us access the passphrase field
+        # of the PassphraseRequestPage from the wizard parent
         passphrase_untrusted = self.field("passphrase")
         if str(passphrase_untrusted) is not None:
-            # Export is shell-escaped
             self.export.export(self.filepaths, str(passphrase_untrusted))
         else:
             self.export.export(self.filepaths, self.PASS_PLACEHOLDER_FIELD)
 
     def request_export_preflight(self) -> None:
+        logger.debug("Request preflight check")
         self.export.run_export_preflight_checks()
 
     @pyqtSlot(object)
@@ -179,10 +177,16 @@ def on_status_received(self, status: ExportStatus) -> None:
 
         # Someone may have yanked out or unmounted a USB
         if target:
-            # If the user is stuck on the same page, show them a hint
-            should_show_hint = target == self.currentId()
             self.rewind(target)
-            self.currentPage().update_content(status, should_show_hint)
+
+        # If the user is stuck, show them a hint.
+        should_show_hint = target == self.currentId()
+        logger.debug(f"{status.value} - should show hint is {should_show_hint}")
+
+        page = self.currentPage()
+        # Sometimes self.currentPage() is None
+        if page and should_show_hint:
+            page.update_content(status, should_show_hint)
 
         # Update status
         self.current_status = status
@@ -206,15 +210,20 @@ def end_wizard_with_error(self, error: ExportStatus) -> None:
         if page:
             # Disable "next" on terminal error pages
             page.set_complete(False)
-            if isinstance(page, PassphraseWizardPage):
+            if isinstance(page, FinalPage):
+                page.update_content(error)
+            elif isinstance(page, PassphraseWizardPage):
                 # Advance one page and display the error
                 self.next()
-                self.page(self.currentId()).update_content(error)
-            elif isinstance(page, FinalPage):
-                page.update_content(error)
+                page = self.currentPage()
+                if page:
+                    page.update_content(error)
             else:
-                # Should be unreachable
-                logger.error("Tried to end early, but user should cancel")
+                # The passphrase field is required, so if we need to end
+                # early we need to "fill it in"
+                self.field("passphrase").setText(self.PASS_PLACEHOLDER_FIELD)
+                while self.currentId() != Pages.EXPORT_DONE:
+                    self.next()
 
     # readywhen: valid usb inserted
     def _create_preflight(self) -> QWizardPage:

client/securedrop_client/gui/conversation/export/export_wizard_page.py

@@ -214,6 +214,7 @@ def __init__(self, export, summary):
         )
 
         super().__init__(export, header=header, body=body)
+        self.start_animate_header()
         self.export.run_export_preflight_checks()
 
     def nextId(self):
@@ -229,6 +230,15 @@ def nextId(self):
         else:
             return super().nextId()
 
+    def on_status_received(self, status: ExportStatus):
+        self.stop_animate_header()
+        if status in (ExportStatus.DEVICE_LOCKED, ExportStatus.DEVICE_WRITABLE):
+            header = _(
+                "Ready to export:<br />" '<span style="font-weight:normal">{}</span>'
+            ).format(self.header_text)
+            self.header.setText(header)
+        self.status = status
+
 
 class InsertUSBPage(ExportWizardPage):
     def __init__(self, export, summary):

client/tests/gui/test_widgets.py

@@ -3600,12 +3600,12 @@ def test_FileWidget__on_export_clicked(mocker, session, source):
     controller.run_export_preflight_checks = mocker.MagicMock()
     controller.downloaded_file_exists = mocker.MagicMock(return_value=True)
 
-    wizard = mocker.patch("securedrop_client.gui.conversation.ExportWizard")
+    wizard = mocker.patch("securedrop_client.gui.conversation.export.ExportWizard")
 
     fw._on_export_clicked()
     wizard.assert_called_once_with(
         export_device(), file.filename, [file_location]
-    ), wizard.call_args
+    ), f"{wizard.call_args}"
 
 
 def test_FileWidget__on_export_clicked_missing_file(mocker, session, source):