From 8b049a4546e112f7228986b7f34a9363c2a89e7d Mon Sep 17 00:00:00 2001 From: Allie Crevier Date: Fri, 5 Nov 2021 09:14:01 -0700 Subject: [PATCH] fix #1285: skip conversation update if the sync is stale Signed-off-by: Allie Crevier --- securedrop_client/gui/widgets.py | 36 +++++++++++++++++++++ securedrop_client/logic.py | 15 ++++++++- tests/gui/test_widgets.py | 54 ++++++++++++++++++++++++++++++++ 3 files changed, 104 insertions(+), 1 deletion(-) diff --git a/securedrop_client/gui/widgets.py b/securedrop_client/gui/widgets.py index be40cde385..1cbf38851c 100644 --- a/securedrop_client/gui/widgets.py +++ b/securedrop_client/gui/widgets.py @@ -3242,11 +3242,20 @@ def __init__(self, source_db_object: Source, controller: Controller) -> None: super().__init__() self.source = source_db_object + self.source_uuid = source_db_object.uuid self.controller = controller + self.controller.sync_started.connect(self._on_sync_started) + controller.conversation_deletion_successful.connect( + self._on_conversation_deletion_successful + ) + # To hold currently displayed messages. self.current_messages = {} # type: Dict[str, QWidget] + self.deletion_scheduled_timestamp = datetime.utcnow() + self.sync_started_timestamp = datetime.utcnow() + self.setObjectName("ConversationView") # Set layout @@ -3278,6 +3287,20 @@ def __init__(self, source_db_object: Source, controller: Controller) -> None: except sqlalchemy.exc.InvalidRequestError as e: logger.debug("Error initializing ConversationView: %s", e) + @pyqtSlot(datetime) + def _on_sync_started(self, timestamp: datetime) -> None: + self.sync_started_timestamp = timestamp + + @pyqtSlot(str, datetime) + def _on_conversation_deletion_successful(self, source_uuid: str, timestamp: datetime) -> None: + if self.source_uuid == source_uuid: + self.deletion_scheduled_timestamp = timestamp + for message in self.current_messages.values(): + message.hide() + self.scroll.hide() + self.deleted_conversation_items_marker.hide() + self.deleted_conversation_marker.show() + def update_deletion_markers(self) -> None: try: if self.source.collection: @@ -3311,6 +3334,11 @@ def update_conversation(self, collection: list) -> None: passed into this method in case of a mismatch between where the widget has been and now is in terms of its index in the conversation. """ + # If the sync started before the deletion finished, then the sync is stale and we do + # not want to update the conversation. + if self.sync_started_timestamp < self.deletion_scheduled_timestamp: + return + self.controller.session.refresh(self.source) # Keep a temporary copy of the current conversation so we can delete any @@ -3499,6 +3527,9 @@ def __init__(self, source: Source, controller: Controller) -> None: self.source_uuid = source.uuid controller.conversation_deleted.connect(self.on_conversation_deleted) controller.conversation_deletion_failed.connect(self.on_conversation_deletion_failed) + controller.conversation_deletion_successful.connect( + self._on_conversation_deletion_successful + ) controller.source_deleted.connect(self.on_source_deleted) controller.source_deletion_failed.connect(self.on_source_deletion_failed) @@ -3533,6 +3564,11 @@ def on_conversation_deleted(self, source_uuid: str) -> None: if self.source_uuid == source_uuid: self.start_conversation_deletion() + @pyqtSlot(str, datetime) + def _on_conversation_deletion_successful(self, source_uuid: str, timestamp: datetime) -> None: + if self.source_uuid == source_uuid: + self.end_conversation_deletion() + @pyqtSlot(str) def on_conversation_deletion_failed(self, source_uuid: str) -> None: if self.source_uuid == source_uuid: diff --git a/securedrop_client/logic.py b/securedrop_client/logic.py index 0bc4d827b1..e1086491b9 100644 --- a/securedrop_client/logic.py +++ b/securedrop_client/logic.py @@ -290,6 +290,15 @@ class Controller(QObject): """ source_deletion_failed = pyqtSignal(str) + """ + This signal indicates that a deletion attempt was successful at the server. + + Emits: + str: the source UUID + datetime: the timestamp for when the deletion succeeded + """ + conversation_deletion_successful = pyqtSignal(str, datetime) + """ This signal lets the queue manager know to add the job to the appropriate network queue. @@ -1005,8 +1014,12 @@ def on_file_download_failure(self, exception: Exception) -> None: self.gui.update_error_status(_("The file download failed. Please try again.")) def on_delete_conversation_success(self, uuid: str) -> None: + """ + If the source collection has been successfully scheduled for deletion on the server, emit a + signal and sync. + """ logger.info("Conversation %s successfully deleted at server", uuid) - self.api_sync.sync() + self.conversation_deletion_successful.emit(uuid, datetime.utcnow()) def on_delete_conversation_failure(self, e: Exception) -> None: if isinstance(e, DeleteConversationJobException): diff --git a/tests/gui/test_widgets.py b/tests/gui/test_widgets.py index 3c8a803f13..297bd30bf2 100644 --- a/tests/gui/test_widgets.py +++ b/tests/gui/test_widgets.py @@ -4337,6 +4337,19 @@ def test_SourceConversationWrapper_on_conversation_deleted_wrong_uuid(mocker): assert scw.deletion_indicator.isHidden() +def test_SourceConversationWrapper__on_conversation_deletion_successful(mocker): + scw = SourceConversationWrapper(factory.Source(uuid="123"), mocker.MagicMock()) + scw.on_conversation_deleted("123") + + scw._on_conversation_deletion_successful("123", datetime.now()) + + assert not scw.conversation_title_bar.isHidden() + assert not scw.conversation_view.isHidden() + assert not scw.reply_box.isHidden() + assert scw.conversation_deletion_indicator.isHidden() + assert scw.deletion_indicator.isHidden() + + def test_SourceConversationWrapper_on_conversation_deletion_failed(mocker): scw = SourceConversationWrapper(factory.Source(uuid="123"), mocker.MagicMock()) scw.on_conversation_deleted("123") @@ -4424,6 +4437,47 @@ def test_ConversationView_ConversationScrollArea_resize(mocker): file_widget_adjust_width.assert_called_with(cv.scroll.widget().width()) +def test_ConversationView__on_sync_started(mocker, session): + cv = ConversationView(factory.Source(), mocker.MagicMock()) + timestamp = datetime.now() + cv._on_sync_started(timestamp) + assert cv.sync_started_timestamp == timestamp + + +def test_ConversationView__on_conversation_deletion_successful(mocker, session): + source = factory.Source() + message = factory.Message(source=source) + session.add(message) + session.add(source) + session.commit() + cv = ConversationView(source, mocker.MagicMock()) + timestamp = datetime.now() + + cv._on_conversation_deletion_successful(cv.source.uuid, timestamp) + + assert cv.deletion_scheduled_timestamp == timestamp + assert cv.scroll.isHidden() + assert cv.deleted_conversation_items_marker.isHidden() + assert not cv.deleted_conversation_marker.isHidden() + assert cv.current_messages[message.uuid].isHidden() + + +def test_ConversationView_update_conversation_skips_if_sync_is_stale(mocker): + """ + If the sync started before the source was scheduled for deletion, do not update the conversation + """ + cv = ConversationView(factory.Source(), mocker.MagicMock()) + cv.update_deletion_markers = mocker.MagicMock() + cv.sync_started_timestamp = datetime.now() + cv.deletion_scheduled_timestamp = datetime.now() + cv.update_conversation([]) + cv.update_deletion_markers.assert_not_called() + # Also test that a new message will not get added if the sync is stale + cv.update_conversation([factory.Message()]) + assert not cv.current_messages + cv.update_deletion_markers.assert_not_called() + + def test_ConversationView_update_conversation_position_follow(mocker, homedir): """ Check the signal handler sets the correct value for the scrollbar to be