ensure no empty seen requests

securedrop_client/api_jobs/seen.py

@@ -17,6 +17,10 @@ def call_api(self, api_client: API, session: Session) -> None:
         """
         Override ApiJob.
 
-        Mark files, messages, and replies as seen
+        Mark files, messages, and replies as seen. Do not make the request if there are no items to
+        be marked as seen.
         """
+        if not self.files and not self.messages and not self.replies:
+            return
+
         api_client.seen(self.files, self.messages, self.replies)

securedrop_client/gui/widgets.py

@@ -641,9 +641,10 @@ def on_source_changed(self):
 
             self.controller.session.refresh(source)
 
-            # If logged in, mark source as seen
-            if self.controller.authenticated:
-                self.source_list.mark_seen.emit(source.uuid)
+            # Immediately show the selected source as seen in the UI and then make a request to mark
+            # source as seen.
+            self.source_list.source_selected.emit(source.uuid)
+            self.controller.mark_seen(source)
 
             # Get or create the SourceConversationWrapper
             if source.uuid in self.source_conversations:
@@ -800,7 +801,7 @@ class SourceList(QListWidget):
 
     NUM_SOURCES_TO_ADD_AT_A_TIME = 32
 
-    mark_seen = pyqtSignal(str)
+    source_selected = pyqtSignal(str)
 
     def __init__(self):
         super().__init__()
@@ -872,7 +873,9 @@ def update(self, sources: List[Source]) -> List[str]:
 
         # Add widgets for new sources
         for uuid in sources_to_add:
-            source_widget = SourceWidget(self.controller, sources_to_add[uuid], self.mark_seen)
+            source_widget = SourceWidget(
+                self.controller, sources_to_add[uuid], self.source_selected
+            )
             source_item = SourceListWidgetItem(self)
             source_item.setSizeHint(source_widget.sizeHint())
             self.insertItem(0, source_item)
@@ -907,7 +910,7 @@ def schedule_source_management(slice_size=slice_size):
             for source in sources_slice:
                 try:
                     source_uuid = source.uuid
-                    source_widget = SourceWidget(self.controller, source, self.mark_seen)
+                    source_widget = SourceWidget(self.controller, source, self.source_selected)
                     source_item = SourceListWidgetItem(self)
                     source_item.setSizeHint(source_widget.sizeHint())
                     self.insertItem(0, source_item)
@@ -1003,14 +1006,14 @@ class SourceWidget(QWidget):
 
     SOURCE_CSS = load_css("source.css")
 
-    def __init__(self, controller: Controller, source: Source, mark_seen_signal: pyqtSignal):
+    def __init__(self, controller: Controller, source: Source, source_selected_signal: pyqtSignal):
         super().__init__()
 
         self.controller = controller
         self.controller.source_deleted.connect(self._on_source_deleted)
         self.controller.source_deletion_failed.connect(self._on_source_deletion_failed)
         self.controller.authentication_state.connect(self._on_authentication_changed)
-        mark_seen_signal.connect(self._on_mark_seen)
+        source_selected_signal.connect(self._on_source_selected)
 
         # Store source
         self.source = source
@@ -1171,53 +1174,19 @@ def _on_authentication_changed(self, authenticated: bool) -> None:
             self.update_styles()
 
     @pyqtSlot(str)
-    def _on_mark_seen(self, source_uuid: str):
+    def _on_source_selected(self, selected_source_uuid: str):
         """
-        Immediately show the source widget as having been seen and tell the controller to make a
-        seen API request to mark all files, messages, and replies as unseen by the current user as
-        seen.
+        Show widget as having been seen.
         """
-        if self.source_uuid != source_uuid:
+        if self.source_uuid != selected_source_uuid:
             return
 
-        # Avoid marking as seen when switching to offline mode (this is an edge case since
-        # we do not emit the mark_seen signal from the SourceList if not authenticated)
-        if not self.controller.authenticated_user:
+        if self.seen:
             return
-        else:
-            journalist_id = self.controller.authenticated_user.id
 
-        # immediately update styles to mark as seen
         self.seen = True
         self.update_styles()
 
-        # Prepare the lists of uuids to mark as seen by the current user. Continue to process the
-        # next item if the source conversation item has already been seen by the current user or if
-        # it no longer exists.
-        try:
-            files = []  # type: List[str]
-            messages = []  # type: List[str]
-            replies = []  # type: List[str]
-            source_items = self.source.collection
-            for item in source_items:
-                try:
-                    if item.seen_by(journalist_id):
-                        continue
-
-                    if isinstance(item, File):
-                        files.append(item.uuid)
-                    elif isinstance(item, Message):
-                        messages.append(item.uuid)
-                    elif isinstance(item, Reply):
-                        replies.append(item.uuid)
-                except sqlalchemy.exc.InvalidRequestError as e:
-                    logger.debug(e)
-                    continue
-
-            self.controller.mark_seen(files, messages, replies)
-        except sqlalchemy.exc.InvalidRequestError as e:
-            logger.debug(e)
-
     @pyqtSlot(str)
     def _on_source_deleted(self, source_uuid: str):
         if self.source_uuid == source_uuid:

securedrop_client/logic.py

@@ -27,6 +27,7 @@
 
 import arrow
 import sdclientapi
+import sqlalchemy.orm.exc
 from PyQt5.QtCore import QObject, QProcess, Qt, QThread, QTimer, pyqtSignal
 from sdclientapi import RequestTimeoutError, ServerConnectionError
 from sqlalchemy.orm.session import sessionmaker
@@ -613,11 +614,50 @@ def update_sources(self):
         sources = list(storage.get_local_sources(self.session))
         self.gui.show_sources(sources)
 
-    def mark_seen(self, files: List[str], messages: List[str], replies: List[str]):
-        job = SeenJob(files, messages, replies)
-        job.success_signal.connect(self.on_seen_success, type=Qt.QueuedConnection)
-        job.failure_signal.connect(self.on_seen_failure, type=Qt.QueuedConnection)
-        self.add_job.emit(job)
+    def mark_seen(self, source: db.Source) -> None:
+        """
+        Mark all unseen conversation items of the supplied source as seen by the current
+        authenticated user.
+        """
+        try:
+            # If user is logged out then just return
+            if not self.authenticated_user:
+                return
+
+            # Prepare the lists of uuids to mark as seen by the current user. Continue to process
+            # the next item if the source conversation item has already been seen by the current
+            # user or if it no longer exists (individual conversation items can be deleted via the
+            # web journalist interface).
+            current_user_id = self.authenticated_user.id
+            files = []  # type: List[str]
+            messages = []  # type: List[str]
+            replies = []  # type: List[str]
+            source_items = source.collection
+            for item in source_items:
+                try:
+                    if item.seen_by(current_user_id):
+                        continue
+
+                    if isinstance(item, db.File):
+                        files.append(item.uuid)
+                    elif isinstance(item, db.Message):
+                        messages.append(item.uuid)
+                    elif isinstance(item, db.Reply):
+                        replies.append(item.uuid)
+                except sqlalchemy.exc.InvalidRequestError as e:
+                    logger.debug(e)
+                    continue
+
+            # If there's nothing to be marked as seen, just return.
+            if not files and not messages and not replies:
+                return
+
+            job = SeenJob(files, messages, replies)
+            job.success_signal.connect(self.on_seen_success, type=Qt.QueuedConnection)
+            job.failure_signal.connect(self.on_seen_failure, type=Qt.QueuedConnection)
+            self.add_job.emit(job)
+        except sqlalchemy.exc.InvalidRequestError as e:
+            logger.debug(e)
 
     def on_seen_success(self) -> None:
         pass

securedrop_client/resources/css/source.css

@@ -58,4 +58,12 @@
     font-weight: 600;
     font-size: 13px;
     color: #000;
+}
+
+#SourceWidget_container QPushButton {
+    border: none;
+}
+
+#StarToggleButton {
+    border: none;
 }

tests/api_jobs/test_seen.py

@@ -2,21 +2,65 @@
 from tests import factory
 
 
-def test_seen(homedir, mocker, session, source):
+def test_seen(homedir, mocker, session):
     """
-    Check if we call add_star method if a source is not stared.
+    Check that the job makes the seen api request with the expected items.
     """
-    file = factory.File(id=1, source=source["source"])
-    message = factory.Message(id=2, source=source["source"])
-    reply = factory.Reply(source=factory.Source())
+    api_client = mocker.MagicMock()
+    file = factory.File()
+    message = factory.Message()
+    reply = factory.Reply()
     session.add(file)
     session.add(message)
     session.add(reply)
 
+    job = SeenJob([file.uuid], [message.uuid], [reply.uuid])
+    job.call_api(api_client, session)
+
+    api_client.seen.assert_called_once_with([file.uuid], [message.uuid], [reply.uuid])
+
+
+def test_seen_skips_making_request_if_no_items_to_mark_seen(homedir, mocker, session, source):
+    """
+    Check that the job does not make the seen api request if there are no items to mark as seen.
+    """
+    api_client = mocker.MagicMock()
+
+    job = SeenJob([], [], [])
+    job.call_api(api_client, session)
+
+    api_client.seen.assert_not_called()
+
+
+def test_seen_with_file_only(homedir, mocker, session, source):
+    api_client = mocker.MagicMock()
+    file = factory.File()
+    session.add(file)
+
+    job = SeenJob([file.uuid], [], [])
+    job.call_api(api_client, session)
+
+    api_client.seen.assert_called_once_with([file.uuid], [], [])
+
+
+def test_seen_with_message_only(homedir, mocker, session, source):
+    api_client = mocker.MagicMock()
+    message = factory.Message()
+    session.add(message)
+
+    job = SeenJob([], [message.uuid], [])
+    job.call_api(api_client, session)
+
+    api_client.seen.assert_called_once_with([], [message.uuid], [])
+
+
+def test_seen_with_reply_only(homedir, mocker, session, source):
     api_client = mocker.MagicMock()
+    reply = factory.Reply()
+    session.add(reply)
 
-    job = SeenJob([file], [message], [reply])
+    job = SeenJob([], [], [reply.uuid])
 
     job.call_api(api_client, session)
 
-    api_client.seen.assert_called_once_with([file], [message], [reply])
+    api_client.seen.assert_called_once_with([], [], [reply.uuid])