Merge pull request #952 from freedomofpress/fix-crash-in-star-after-d…

…eletion

Fix starring behavior

securedrop_client/api_jobs/updatestar.py

@@ -1,9 +1,7 @@
 import logging
 import sdclientapi
 
-from typing import Tuple
-
-from sdclientapi import API
+from sdclientapi import API, RequestTimeoutError, ServerConnectionError
 from sqlalchemy.orm.session import Session
 
 from securedrop_client.api_jobs.base import ApiJob
@@ -12,12 +10,12 @@
 
 
 class UpdateStarJob(ApiJob):
-    def __init__(self, source_uuid: str, star_status: bool) -> None:
+    def __init__(self, source_uuid: str, is_starred: bool) -> None:
         super().__init__()
         self.source_uuid = source_uuid
-        self.star_status = star_status
+        self.is_starred = is_starred
 
-    def call_api(self, api_client: API, session: Session) -> Tuple[str, bool]:
+    def call_api(self, api_client: API, session: Session) -> str:
         '''
         Override ApiJob.
 
@@ -30,21 +28,31 @@ def call_api(self, api_client: API, session: Session) -> Tuple[str, bool]:
             # want to pass the default request timeout to remove_star and add_star instead of
             # setting it on the api object directly.
             api_client.default_request_timeout = 5
-            if self.star_status:
+            if self.is_starred:
                 api_client.remove_star(source_sdk_object)
             else:
                 api_client.add_star(source_sdk_object)
 
-            # Identify the source and *new* state of the star so the UI can be
-            # updated.
-            return self.source_uuid, not self.star_status
+            return self.source_uuid
+        except (RequestTimeoutError, ServerConnectionError) as e:
+            error_message = f'Failed to update star on source {self.source_uuid} due to error: {e}'
+            raise UpdateStarJobTimeoutError(error_message, self.source_uuid)
         except Exception as e:
-            error_message = "Failed to update star on source {uuid} due to {exception}".format(
-                uuid=self.source_uuid, exception=repr(e))
-            raise UpdateStarJobException(error_message, self.source_uuid)
+            error_message = f'Failed to update star on source {self.source_uuid} due to {e}'
+            raise UpdateStarJobError(error_message, self.source_uuid)
 
 
-class UpdateStarJobException(Exception):
-    def __init__(self, message: str, source_uuid: str):
+class UpdateStarJobError(Exception):
+    def __init__(self, message: str, source_uuid: str) -> None:
         super().__init__(message)
         self.source_uuid = source_uuid
+
+
+class UpdateStarJobTimeoutError(RequestTimeoutError):
+    def __init__(self, message: str, source_uuid: str) -> None:
+        super().__init__()
+        self.message = message
+        self.source_uuid = source_uuid
+
+    def __str__(self) -> str:
+        return self.message

securedrop_client/gui/__init__.py

@@ -58,12 +58,6 @@ def __init__(self, on: str, off: str, svg_size: str = None):
         # Make this a toggle button
         self.setCheckable(True)
 
-    def enable(self) -> None:
-        self.setEnabled(True)
-
-    def disable(self) -> None:
-        self.setEnabled(False)
-
     def set_icon(self, on: str, off: str) -> None:
         self.icon = load_icon(normal=on, normal_off=off)
         self.setIcon(self.icon)
@@ -116,12 +110,6 @@ def __init__(
         self.setIcon(self.icon)
         self.setIconSize(svg_size) if svg_size else self.setIconSize(QSize())
 
-    def enable(self) -> None:
-        self.setEnabled(True)
-
-    def disable(self) -> None:
-        self.setEnabled(False)
-
 
 class SvgLabel(QLabel):
     """

securedrop_client/gui/widgets.py

@@ -1089,7 +1089,6 @@ def __init__(self, controller: Controller, source: Source):
         # Store source
         self.source_uuid = source.uuid
         self.source = source
-        self.source_uuid = source.uuid
 
         # Set styles
         self.setStyleSheet(self.CSS)
@@ -1115,7 +1114,7 @@ def __init__(self, controller: Controller, source: Source):
         gutter_layout = QVBoxLayout(self.gutter)
         gutter_layout.setContentsMargins(0, 0, 0, 0)
         gutter_layout.setSpacing(0)
-        self.star = StarToggleButton(self.controller, self.source)
+        self.star = StarToggleButton(self.controller, self.source_uuid, source.is_starred)
         gutter_layout.addWidget(self.star)
         gutter_layout.addStretch()
 
@@ -1188,7 +1187,7 @@ def update(self):
 
             if self.source.document_count == 0:
                 self.paperclip.hide()
-            self.star.update()
+            self.star.update(self.source.is_starred)
         except sqlalchemy.exc.InvalidRequestError as e:
             logger.error(f"Could not update SourceWidget for source {self.source_uuid}: {e}")
             raise
@@ -1230,144 +1229,154 @@ class StarToggleButton(SvgToggleButton):
     }
     '''
 
-    def __init__(self, controller: Controller, source: Source):
-        super().__init__(
-            on='star_on.svg',
-            off='star_off.svg',
-            svg_size=QSize(16, 16))
+    def __init__(self, controller: Controller, source_uuid: str, is_starred: bool):
+        super().__init__(on='star_on.svg', off='star_off.svg', svg_size=QSize(16, 16))
 
         self.controller = controller
-        self.source = source
+        self.source_uuid = source_uuid
+        self.is_starred = is_starred
+        self.pending_count = 0
+        self.wait_until_next_sync = False
 
+        self.controller.authentication_state.connect(self.on_authentication_changed)
+        self.controller.star_update_failed.connect(self.on_star_update_failed)
+        self.controller.star_update_successful.connect(self.on_star_update_successful)
         self.installEventFilter(self)
 
         self.setObjectName('star_button')
         self.setStyleSheet(self.css)
         self.setFixedSize(QSize(20, 20))
 
-        self.controller.authentication_state.connect(self.on_authentication_changed)
-        self.on_authentication_changed(self.controller.is_authenticated)
+        self.pressed.connect(self.on_pressed)
+        self.setCheckable(True)
+        self.setChecked(self.is_starred)
 
-    def disable(self):
-        """
-        Disable the widget.
+        if not self.controller.is_authenticated:
+            self.disable_toggle()
 
-        Disable toggle by setting checkable to False. Unfortunately,
-        disabling toggle doesn't freeze state, rather it always
-        displays the off state when a user tries to toggle. In order
-        to save on state we update the icon's off state image to
-        display on (hack).
+    def disable_toggle(self):
         """
-        self.disable_api_call()
-        self.setCheckable(False)
-        if self.source.is_starred:
-            self.set_icon(on='star_on.svg', off='star_on.svg')
-
-        try:
-            while True:
-                self.pressed.disconnect(self.on_toggle_offline)
-        except Exception as e:
-            logger.warning("Could not disconnect on_toggle_offline from self.pressed: %s", e)
+        Unset `checkable` so that the star cannot be toggled.
 
-        try:
-            while True:
-                self.toggled.disconnect(self.on_toggle)
-        except Exception as e:
-            logger.warning("Could not disconnect on_toggle from self.toggled: %s", e)
+        Disconnect the `pressed` signal from previous handler and connect it to the offline handler.
+        """
+        self.pressed.disconnect()
+        self.pressed.connect(self.on_pressed_offline)
 
-        self.pressed.connect(self.on_toggle_offline)
+        # If the source is starred, we must update the icon so that the off state continues to show
+        # the source as starred. We could instead disable the button, which will continue to show
+        # the star as checked, but Qt will also gray out the star, which we don't want.
+        if self.is_starred:
+            self.set_icon(on='star_on.svg', off='star_on.svg')
+        self.setCheckable(False)
 
-    def enable(self):
+    def enable_toggle(self):
         """
         Enable the widget.
-        """
-        self.enable_api_call()
-        self.setCheckable(True)
-        self.set_icon(on='star_on.svg', off='star_off.svg')
-        self.setChecked(self.source.is_starred)
-
-        try:
-            while True:
-                self.toggled.disconnect(self.on_toggle)
-        except Exception:
-            pass
 
-        try:
-            while True:
-                self.pressed.disconnect(self.on_toggle_offline)
-        except Exception:
-            pass
+        Disconnect the pressed signal from previous handler, set checkable so that the star can be
+        toggled, and connect to the online toggle handler.
 
-        self.toggled.connect(self.on_toggle)
+        Note: We must update the icon in case it was modified after being disabled.
+        """
+        self.pressed.disconnect()
+        self.pressed.connect(self.on_pressed)
+        self.setCheckable(True)
+        self.set_icon(on='star_on.svg', off='star_off.svg')  # Undo icon change from disable_toggle
 
     def eventFilter(self, obj, event):
-        checkable = self.isCheckable()
+        """
+        If the button is checkable then we show a hover state.
+        """
+        if not self.isCheckable():
+            return QObject.event(obj, event)
+
         t = event.type()
-        if t == QEvent.HoverEnter and checkable:
+        if t == QEvent.HoverEnter:
             self.setIcon(load_icon('star_hover.svg'))
         elif t == QEvent.HoverLeave:
-            if checkable:
-                self.set_icon(on='star_on.svg', off='star_off.svg')
-            else:
-                if self.source.is_starred:
-                    self.set_icon(on='star_on.svg', off='star_on.svg')
+            self.set_icon(on='star_on.svg', off='star_off.svg')
+
         return QObject.event(obj, event)
 
-    def on_authentication_changed(self, authenticated: bool):
+    @pyqtSlot(bool)
+    def on_authentication_changed(self, authenticated: bool) -> None:
         """
         Set up handlers based on whether or not the user is authenticated. Connect to 'pressed'
         event instead of 'toggled' event when not authenticated because toggling will be disabled.
         """
         if authenticated:
-            self.enable()
+            self.pending_count = 0
+            self.enable_toggle()
+            self.setChecked(self.is_starred)
         else:
-            self.disable()
+            self.disable_toggle()
 
-    def on_toggle(self):
+    @pyqtSlot()
+    def on_pressed(self) -> None:
         """
         Tell the controller to make an API call to update the source's starred field.
         """
-        if self.is_api_call_enabled:
-            self.controller.update_star(self.source, self.on_update)
+        self.controller.update_star(self.source_uuid, self.isChecked())
+        self.is_starred = not self.is_starred
+        self.pending_count = self.pending_count + 1
+        self.wait_until_next_sync = True
 
-    def on_toggle_offline(self):
+    @pyqtSlot()
+    def on_pressed_offline(self) -> None:
         """
         Show error message when not authenticated.
         """
         self.controller.on_action_requiring_login()
 
-    def on_update(self, result):
+    @pyqtSlot(bool)
+    def update(self, is_starred: bool) -> None:
         """
-        The result is a uuid for the source and boolean flag for the new state
-        of the star.
+        If star was updated via the Journalist Interface or by another instance of the client, then
+        self.is_starred will not match the server and will need to be updated.
         """
-        enabled = result[1]
-        self.source.is_starred = enabled
-        self.controller.session.commit()
-        self.controller.update_sources()
-        self.setChecked(enabled)
+        if not self.controller.is_authenticated:
+            return
 
-    def update(self):
+        # Wait until ongoing star jobs are finished before checking if it matches with the server
+        if self.pending_count > 0:
+            return
+
+        # Wait until next sync to avoid the possibility of updating the star with outdated source
+        # information in case the server just received the star request.
+        if self.wait_until_next_sync:
+            self.wait_until_next_sync = False
+            return
+
+        if self.is_starred != is_starred:
+            self.is_starred = is_starred
+            self.setChecked(self.is_starred)
+
+    @pyqtSlot(str, bool)
+    def on_star_update_failed(self, source_uuid: str, is_starred: bool) -> None:
         """
-        Update the star to reflect its source's current state.
+        If the star update failed to update on the server, toggle back to previous state.
         """
-        self.controller.session.refresh(self.source)
-        self.disable_api_call()
-        self.setChecked(self.source.is_starred)
-        self.enable_api_call()
-
-    def disable_api_call(self):
-        self.is_api_call_enabled = False
+        if self.source_uuid == source_uuid:
+            self.is_starred = is_starred
+            self.pending_count = self.pending_count - 1
+            QTimer.singleShot(250, lambda: self.setChecked(self.is_starred))
 
-    def enable_api_call(self):
-        self.is_api_call_enabled = True
+    @pyqtSlot(str)
+    def on_star_update_successful(self, source_uuid: str) -> None:
+        """
+        If the star update succeeded, set pending to False so the sync can update the star field
+        """
+        if self.source_uuid == source_uuid:
+            self.pending_count = self.pending_count - 1
 
 
 class DeleteSourceMessageBox:
     """Use this to display operation details and confirm user choice."""
 
     def __init__(self, source, controller):
         self.source = source
+        self.source_uuid = source.uuid
         self.controller = controller
 
     def launch(self):
@@ -1383,7 +1392,7 @@ def launch(self):
             None, "", _(message), QMessageBox.Cancel | QMessageBox.Yes, QMessageBox.Cancel)
 
         if reply == QMessageBox.Yes:
-            logger.debug("Deleting source %s" % (self.source.uuid,))
+            logger.debug(f'Deleting source {self.source_uuid}')
             self.controller.delete_source(self.source)
 
     def _construct_message(self, source: Source) -> str: