No sync on ui operations

securedrop_client/api_jobs/updatestar.py

@@ -1,6 +1,8 @@
 import logging
 import sdclientapi
 
+from typing import Tuple
+
 from sdclientapi import API
 from sqlalchemy.orm.session import Session
 
@@ -15,7 +17,7 @@ def __init__(self, source_uuid: str, star_status: bool) -> None:
         self.source_uuid = source_uuid
         self.star_status = star_status
 
-    def call_api(self, api_client: API, session: Session) -> str:
+    def call_api(self, api_client: API, session: Session) -> Tuple[str, bool]:
         '''
         Override ApiJob.
 
@@ -33,7 +35,9 @@ def call_api(self, api_client: API, session: Session) -> str:
             else:
                 api_client.add_star(source_sdk_object)
 
-            return self.source_uuid
+            # Identify the source and *new* state of the star so the UI can be
+            # updated.
+            return self.source_uuid, not self.star_status
         except Exception as e:
             error_message = "Failed to update star on source {uuid} due to {exception}".format(
                 uuid=self.source_uuid, exception=repr(e))

securedrop_client/gui/widgets.py

@@ -1106,7 +1106,7 @@ def on_toggle(self):
         """
         Tell the controller to make an API call to update the source's starred field.
         """
-        self.controller.update_star(self.source)
+        self.controller.update_star(self.source, self.on_update)
 
     def on_toggle_offline(self):
         """
@@ -1120,6 +1120,16 @@ def on_toggle_offline(self):
         if self.source.is_starred:
             self.set_icon(on='star_on.svg', off='star_on.svg')
 
+    def on_update(self, result):
+        """
+        The result is a uuid for the source and boolean flag for the new state
+        of the star.
+        """
+        enabled = result[1]
+        self.source.is_starred = enabled
+        self.controller.update_sources()
+        self.setChecked(enabled)
+
 
 class DeleteSourceMessageBox:
     """Use this to display operation details and confirm user choice."""

securedrop_client/logic.py

@@ -472,7 +472,6 @@ def on_update_star_success(self, result) -> None:
         After we star a source, we should sync the API such that the local database is updated.
         """
         self.gui.clear_error_status()  # remove any permanent error status message
-        self.sync_api()  # Syncing the API also updates the source list UI
 
     def on_update_star_failure(self, result: UpdateStarJobException) -> None:
         """
@@ -482,7 +481,7 @@ def on_update_star_failure(self, result: UpdateStarJobException) -> None:
         error = _('Failed to update star.')
         self.gui.update_error_status(error)
 
-    def update_star(self, source_db_object):
+    def update_star(self, source_db_object, callback):
         """
         Star or unstar. The callback here is the API sync as we first make sure
         that we apply the change to the server, and then update locally.
@@ -493,6 +492,7 @@ def update_star(self, source_db_object):
 
         job = UpdateStarJob(source_db_object.uuid, source_db_object.is_starred)
         job.success_signal.connect(self.on_update_star_success, type=Qt.QueuedConnection)
+        job.success_signal.connect(callback, type=Qt.QueuedConnection)
         job.failure_signal.connect(self.on_update_star_failure, type=Qt.QueuedConnection)
 
         self.api_job_queue.enqueue(job)
@@ -752,8 +752,11 @@ def on_delete_source_success(self, result) -> None:
         """
         Handler for when a source deletion succeeds.
         """
+        # Delete the local version of the source.
+        storage.delete_local_source_by_uuid(self.session, result)
         self.gui.clear_error_status()  # remove any permanent error status message
-        self.sync_api()
+        # Update the sources UI.
+        self.update_sources()
 
     def on_delete_source_failure(self, result: Exception) -> None:
         logging.info("failed to delete source at server")

securedrop_client/storage.py

@@ -47,6 +47,17 @@ def get_local_sources(session: Session) -> List[Source]:
     return session.query(Source).all()
 
 
+def delete_local_source_by_uuid(session: Session, uuid: str) -> None:
+    """
+    Delete the source with the referenced UUID.
+    """
+    source = session.query(Source).filter_by(uuid=uuid).one_or_none()
+    if source:
+        session.delete(source)
+        session.commit()
+        logger.info("Deleted source with UUID {} from local database.".format(uuid))
+
+
 def get_local_messages(session: Session) -> List[Message]:
     """
     Return all submission objects from the local database.

tests/gui/test_widgets.py

@@ -895,7 +895,7 @@ def test_StarToggleButton_on_toggle(mocker):
 
     stb.on_toggle()
 
-    stb.controller.update_star.assert_called_once_with(source)
+    stb.controller.update_star.assert_called_once_with(source, stb.on_update)
     assert stb.isCheckable() is True
 
 
@@ -929,6 +929,22 @@ def test_StarToggleButton_on_toggle_offline_when_checked(mocker):
     set_icon_fn.assert_called_with(on='star_on.svg', off='star_on.svg')
 
 
+def test_StarToggleButton_on_update(mocker):
+    """
+    Ensure the on_update callback updates the state of the source and UI
+    element to the current "enabled" state.
+    """
+    source = mocker.MagicMock()
+    source.is_starred = True
+    stb = StarToggleButton(source)
+    stb.setChecked = mocker.MagicMock()
+    stb.controller = mocker.MagicMock()
+    stb.on_update(("uuid", False))
+    assert source.is_starred is False
+    stb.controller.update_sources.assert_called_once_with()
+    stb.setChecked.assert_called_once_with(False)
+
+
 def test_LoginDialog_setup(mocker, i18n):
     """
     The LoginView is correctly initialised.

tests/test_logic.py

@@ -529,9 +529,10 @@ def test_Controller_update_star_not_logged_in(homedir, config, mocker, session_m
     mock_gui = mocker.MagicMock()
     co = Controller('http://localhost', mock_gui, session_maker, homedir)
     source_db_object = mocker.MagicMock()
+    mock_callback = mocker.MagicMock()
     co.on_action_requiring_login = mocker.MagicMock()
     co.api = None
-    co.update_star(source_db_object)
+    co.update_star(source_db_object, mock_callback)
     co.on_action_requiring_login.assert_called_with()
 
 
@@ -547,7 +548,7 @@ def test_Controller_on_update_star_success(homedir, config, mocker, session_make
     co.call_reset = mocker.MagicMock()
     co.sync_api = mocker.MagicMock()
     co.on_update_star_success(result)
-    co.sync_api.assert_called_once_with()
+    assert mock_gui.clear_error_status.called
 
 
 def test_Controller_on_update_star_failed(homedir, config, mocker, session_maker):
@@ -1270,10 +1271,12 @@ def test_Controller_on_delete_source_success(homedir, config, mocker, session_ma
     Using the `config` fixture to ensure the config is written to disk.
     '''
     mock_gui = mocker.MagicMock()
+    storage = mocker.patch('securedrop_client.logic.storage')
     co = Controller('http://localhost', mock_gui, session_maker, homedir)
-    co.sync_api = mocker.MagicMock()
-    co.on_delete_source_success(True)
-    co.sync_api.assert_called_with()
+    co.update_sources = mocker.MagicMock()
+    co.on_delete_source_success("uuid")
+    storage.delete_local_source_by_uuid.assert_called_once_with(co.session, "uuid")
+    assert co.update_sources.call_count == 1
 
 
 def test_Controller_on_delete_source_failure(homedir, config, mocker, session_maker):
@@ -1520,16 +1523,22 @@ def test_Controller_call_update_star_success(homedir, config, mocker, session_ma
     session.add(source)
     session.commit()
 
-    co.update_star(source)
+    mock_callback = mocker.MagicMock()
+
+    co.update_star(source, mock_callback)
 
     mock_job_cls.assert_called_once_with(
         source.uuid,
         source.is_starred
     )
 
     mock_queue.enqueue.assert_called_once_with(mock_job)
-    mock_success_signal.connect.assert_called_once_with(
-        co.on_update_star_success, type=Qt.QueuedConnection)
+    assert mock_success_signal.connect.call_count == 2
+    cal = mock_success_signal.connect.call_args_list
+    assert cal[0][0][0] == co.on_update_star_success
+    assert cal[0][1]['type'] == Qt.QueuedConnection
+    assert cal[1][0][0] == mock_callback
+    assert cal[1][1]['type'] == Qt.QueuedConnection
     mock_failure_signal.connect.assert_called_once_with(
         co.on_update_star_failure, type=Qt.QueuedConnection)
 

tests/test_storage.py

@@ -17,7 +17,7 @@
     delete_single_submission_or_reply_on_disk, rename_file, get_local_files, find_new_files, \
     source_exists, set_message_or_reply_content, mark_as_downloaded, mark_as_decrypted, get_file, \
     get_message, get_reply, update_and_get_user, update_missing_files, mark_as_not_downloaded, \
-    mark_all_pending_drafts_as_failed
+    mark_all_pending_drafts_as_failed, delete_local_source_by_uuid
 
 from securedrop_client import db
 from tests import factory
@@ -72,6 +72,22 @@ def test_get_local_sources(mocker):
     mock_session.query.assert_called_once_with(securedrop_client.db.Source)
 
 
+def test_delete_local_source_by_uuid(mocker):
+    """
+    Delete the referenced source in the session.
+    """
+    mock_session = mocker.MagicMock()
+    source = make_remote_source()
+    mock_session.query().filter_by().one_or_none.return_value = source
+    mock_session.query.reset_mock()
+    delete_local_source_by_uuid(mock_session, "uuid")
+    mock_session.query.assert_called_once_with(securedrop_client.db.Source)
+    mock_session.query().filter_by.assert_called_once_with(uuid="uuid")
+    assert mock_session.query().filter_by().one_or_none.call_count == 1
+    mock_session.delete.assert_called_once_with(source)
+    mock_session.commit.assert_called_once_with()
+
+
 def test_get_local_messages(mocker):
     """
     At this moment, just return all messages.