From 4eea2e7b1f5df14d9a8118c31914fd79ca06330e Mon Sep 17 00:00:00 2001 From: Allie Crevier Date: Wed, 22 Dec 2021 15:26:33 -0800 Subject: [PATCH 1/3] use signal for account deletion instead of sync --- securedrop_client/gui/widgets.py | 69 ++++++++++++++----- securedrop_client/logic.py | 16 ++++- tests/gui/test_widgets.py | 112 +++++++++++++++++++++++++++++++ 3 files changed, 178 insertions(+), 19 deletions(-) diff --git a/securedrop_client/gui/widgets.py b/securedrop_client/gui/widgets.py index 757b60430..134ce2c7d 100644 --- a/securedrop_client/gui/widgets.py +++ b/securedrop_client/gui/widgets.py @@ -607,6 +607,7 @@ def setup(self, controller: Controller) -> None: Pass through the controller object to this widget. """ self.controller = controller + self.controller.source_deletion_successful.connect(self._on_source_deletion_successful) self.source_list.setup(controller) def show_sources(self, sources: List[Source]) -> None: @@ -683,6 +684,20 @@ def refresh_source_conversations(self) -> None: except sqlalchemy.exc.InvalidRequestError as e: logger.debug("Error refreshing source conversations: %s", e) + def _delete_source(self, source_uuid: str) -> None: + """ + When we delete a source, we should delete its SourceConversationWrapper, + and remove the reference to it in self.source_conversations + """ + self.source_list.delete_sources([source_uuid]) + self.delete_conversation(source_uuid) + if not self.source_list.source_items: + self.empty_conversation_view.show_no_sources_message() + self.empty_conversation_view.show() + else: + self.empty_conversation_view.show_no_source_selected_message() + self.empty_conversation_view.show() + def delete_conversation(self, source_uuid: str) -> None: """ When we delete a source, we should delete its SourceConversationWrapper, @@ -709,6 +724,14 @@ def set_conversation(self, widget: QWidget) -> None: self.view_layout.addWidget(widget) widget.show() + @pyqtSlot(str, datetime) + def _on_source_deletion_successful(self, source_uuid: str, timestamp: datetime) -> None: + """ + Handle the signal for a source that was successfully deleted by deleting that source from + the source list and the corresponding conversation. + """ + self._delete_source(source_uuid) + class EmptyConversationView(QWidget): @@ -877,21 +900,8 @@ def update(self, sources: List[Source]) -> List[str]: continue # Delete widgets for sources not in the supplied sourcelist - deleted_uuids = [] - sources_to_delete = [ - self.source_items[uuid] for uuid in self.source_items if uuid not in sources_to_update - ] - for source_item in sources_to_delete: - if source_item.isSelected(): - self.setCurrentItem(None) - - source_widget = self.itemWidget(source_item) - self.takeItem(self.row(source_item)) - if source_widget.source_uuid in self.source_items: - del self.source_items[source_widget.source_uuid] - - deleted_uuids.append(source_widget.source_uuid) - source_widget.deleteLater() + sources_to_delete = [uuid for uuid in self.source_items if uuid not in sources_to_update] + deleted_uuids = self.delete_sources(sources_to_delete) # Update the remaining widgets for i in range(self.count()): @@ -921,6 +931,27 @@ def update(self, sources: List[Source]) -> List[str]: # conversation widgets return deleted_uuids + def delete_sources(self, source_uuids: List[str]) -> List[str]: + deleted_uuids = [] + + for uuid in source_uuids: + try: + source_item = self.source_items[uuid] + if source_item.isSelected(): + self.setCurrentItem(None) + + source_widget = self.itemWidget(source_item) + self.takeItem(self.row(source_item)) + if source_widget.source_uuid in self.source_items: + del self.source_items[source_widget.source_uuid] + + deleted_uuids.append(source_widget.source_uuid) + source_widget.deleteLater() + except KeyError: + continue + + return deleted_uuids + def initial_update(self, sources: List[Source]) -> None: """ Initialise the list with the passed in list of sources. @@ -1181,11 +1212,12 @@ def __init__( self.controller = controller self.controller.sync_started.connect(self._on_sync_started) self.controller.conversation_deleted.connect(self._on_conversation_deleted) - controller.conversation_deletion_successful.connect( + self.controller.conversation_deletion_successful.connect( self._on_conversation_deletion_successful ) self.controller.conversation_deletion_failed.connect(self._on_conversation_deletion_failed) self.controller.source_deleted.connect(self._on_source_deleted) + self.controller.source_deletion_successful.connect(self._on_source_deletion_successful) self.controller.source_deletion_failed.connect(self._on_source_deletion_failed) self.controller.authentication_state.connect(self._on_authentication_changed) source_selected_signal.connect(self._on_source_selected) @@ -1435,6 +1467,11 @@ def _on_source_deleted(self, source_uuid: str) -> None: if self.source_uuid == source_uuid: self.start_account_deletion() + @pyqtSlot(str, datetime) + def _on_source_deletion_successful(self, source_uuid: str, timestamp: datetime) -> None: + if self.source_uuid == source_uuid: + self.deletion_scheduled_timestamp = timestamp + @pyqtSlot(str) def _on_source_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 e1086491b..1f1210149 100644 --- a/securedrop_client/logic.py +++ b/securedrop_client/logic.py @@ -293,6 +293,15 @@ class Controller(QObject): """ 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 + """ + source_deletion_successful = pyqtSignal(str, datetime) + + """ + 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 @@ -1016,7 +1025,7 @@ def on_file_download_failure(self, exception: Exception) -> None: 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. + signal. """ logger.info("Conversation %s successfully deleted at server", uuid) self.conversation_deletion_successful.emit(uuid, datetime.utcnow()) @@ -1030,10 +1039,11 @@ def on_delete_conversation_failure(self, e: Exception) -> None: def on_delete_source_success(self, source_uuid: str) -> None: """ - Rely on sync to delete the source locally so we know for sure it was deleted + If the source has been successfully scheduled for deletion on the server, emit a + signal. """ logger.info("Source %s successfully deleted at server", source_uuid) - self.api_sync.sync() + self.source_deletion_successful.emit(source_uuid, datetime.utcnow()) def on_delete_source_failure(self, e: Exception) -> None: if isinstance(e, DeleteSourceJobException): diff --git a/tests/gui/test_widgets.py b/tests/gui/test_widgets.py index f07921c0f..4d399e0f5 100644 --- a/tests/gui/test_widgets.py +++ b/tests/gui/test_widgets.py @@ -947,6 +947,70 @@ def test_MainView_set_conversation(mocker): mv.view_layout.addWidget.assert_called_once_with(mock_widget) +def test_MainView__delete_source(mocker): + """ + Ensure that the source and conversation provided by source uuid are deleted. + """ + source = factory.Source(uuid="123") + conversation_wrapper = SourceConversationWrapper(source, mocker.MagicMock()) + mv = MainView(None) + mv.source_conversations = {} + mv.source_conversations["123"] = conversation_wrapper + mv.source_list.controller = mocker.MagicMock() + mv.source_list.update([source]) + assert len(mv.source_list.source_items) > 0 + + mv._delete_source("123") + + assert len(mv.source_conversations) == 0 + assert len(mv.source_list.source_items) == 0 + + +def test_MainView__delete_source_with_multiple_sources(mocker): + """ + Ensure that the source and conversation provided by source uuid are deleted. + """ + source1 = factory.Source(uuid="abc") + source2 = factory.Source(uuid="123") + mv = MainView(None) + mv.source_conversations = {} + mv.source_conversations["123"] = SourceConversationWrapper(source1, mocker.MagicMock()) + mv.source_conversations["123"] = SourceConversationWrapper(source2, mocker.MagicMock()) + mv.source_list.controller = mocker.MagicMock() + mv.source_list.update([source1, source2]) + assert len(mv.source_list.source_items) > 0 + + mv._delete_source("123") + mv._delete_source("abc") + + assert len(mv.source_conversations) == 0 + assert len(mv.source_list.source_items) == 0 + + +def test_MainView__on_source_deletion_successful(mocker): + """ + Ensure that the source and conversation provided by source uuid are deleted. + """ + source = factory.Source(uuid="123") + conversation_wrapper = SourceConversationWrapper(source, mocker.MagicMock()) + mv = MainView(None) + mv.source_conversations = {} + mv.source_conversations["123"] = conversation_wrapper + mv.source_list.controller = mocker.MagicMock() + mv.source_list.update([source]) + assert len(mv.source_list.source_items) > 0 + + mv._on_source_deletion_successful("123", datetime.now()) + + assert len(mv.source_conversations) == 0 + assert len(mv.source_list.source_items) == 0 + + +def test_MainView__on_source_deletion_handles_keyerror(mocker): + mv = MainView(None) + mv._on_source_deletion_successful("throw-keyerror", datetime.now()) + + def test_EmptyConversationView_show_no_sources_message(mocker): ecv = EmptyConversationView() @@ -2027,6 +2091,30 @@ def test_SourceWidget__on_source_deleted_wrong_uuid(mocker, session, source): assert not sw.timestamp.isHidden() +def test_SourceWidget_update_and_set_snippet_ineffective_after_deletion_successful(mocker): + """ + Ensure that a source widget is not updated by a stale sync if a deletion + request has been successfully received by the server. + """ + controller = mocker.MagicMock() + mark_seen_signal = mocker.MagicMock() + sw = SourceWidget(controller, factory.Source(uuid="123"), mark_seen_signal, mocker.MagicMock()) + sw._on_sync_started(datetime.now()) + + sw._on_source_deletion_successful("123", datetime.now()) + + sw.update_styles = mocker.MagicMock() + sw.set_snippet("mock_uuid", "msg_uuid", "msg_content") + sw.update_styles.assert_not_called() + + sw.set_snippet = mocker.MagicMock() + sw.set_snippet_to_conversation_deleted = mocker.MagicMock() + sw.update() + sw.set_snippet.assert_not_called() + sw.set_snippet_to_conversation_deleted.assert_not_called() + sw.update_styles.assert_not_called() + + def test_SourceWidget__on_source_deletion_failed(mocker, session, source): controller = mocker.MagicMock() mark_seen_signal = mocker.MagicMock() @@ -4154,6 +4242,30 @@ def test_SourceConversationWrapper_on_source_deleted(mocker): assert not scw.deletion_indicator.isHidden() +def test_SourceWidget_update_and_set_snippet_ineffective_after_deletion_started(mocker): + """ + Ensure that when deletion has started that the source widget is not + updated when set_snippet or update are called. + """ + controller = mocker.MagicMock() + sw = SourceWidget( + controller, factory.Source(uuid="123"), mocker.MagicMock(), mocker.MagicMock() + ) + sw._on_source_deleted("123") # Start source deletion + sw.update_styles = mocker.MagicMock() + sw.set_snippet_to_conversation_deleted = mocker.MagicMock() + + sw.set_snippet("123", "msg_uuid", "msg_content") + sw.update_styles.assert_not_called() + sw.set_snippet_to_conversation_deleted.assert_not_called() + + sw.set_snippet = mocker.MagicMock() + sw.update() + sw.update_styles.assert_not_called() + sw.set_snippet.assert_not_called() + sw.set_snippet_to_conversation_deleted.assert_not_called() + + def test_SourceConversationWrapper_on_source_deleted_wrong_uuid(mocker): scw = SourceConversationWrapper(factory.Source(uuid="123"), mocker.MagicMock()) scw.on_source_deleted("321") From 30159d12b089570b5e7d1a87ea0815e1520c51ab Mon Sep 17 00:00:00 2001 From: Allie Crevier Date: Wed, 22 Dec 2021 15:27:32 -0800 Subject: [PATCH 2/3] Revert "Introduce immediate sync upon source/conversation deletion" This reverts commit 40a224d1fbfd06d5d645e55645de279a9e9b660c. --- securedrop_client/sync.py | 14 ++------------ tests/test_sync.py | 4 ++++ 2 files changed, 6 insertions(+), 12 deletions(-) diff --git a/securedrop_client/sync.py b/securedrop_client/sync.py index c18437384..ad4b429c8 100644 --- a/securedrop_client/sync.py +++ b/securedrop_client/sync.py @@ -42,18 +42,12 @@ def __init__( self.sync_thread.started.connect(self.api_sync_bg_task.sync) - self.timer = QTimer() - self.timer.setInterval(self.TIME_BETWEEN_SYNCS_MS) - self.timer.timeout.connect(self.sync) - def start(self, api_client: API) -> None: """ Start metadata syncs. """ self.api_client = api_client - self.timer.start() - if not self.sync_thread.isRunning(): logger.debug("Starting sync thread") self.api_sync_bg_task.api_client = self.api_client @@ -74,18 +68,14 @@ def on_sync_success(self) -> None: Start another sync on success. """ self.sync_success.emit() + QTimer.singleShot(self.TIME_BETWEEN_SYNCS_MS, self.api_sync_bg_task.sync) def on_sync_failure(self, result: Exception) -> None: """ Only start another sync on failure if the reason is a timeout request. """ self.sync_failure.emit(result) - - def sync(self) -> None: - """ - Start an immediate sync. - """ - QTimer.singleShot(1, self.api_sync_bg_task.sync) + QTimer.singleShot(self.TIME_BETWEEN_SYNCS_MS, self.api_sync_bg_task.sync) class ApiSyncBackgroundTask(QObject): diff --git a/tests/test_sync.py b/tests/test_sync.py index 6355e2649..c9251f485 100644 --- a/tests/test_sync.py +++ b/tests/test_sync.py @@ -158,12 +158,14 @@ def test_ApiSync_on_sync_failure(mocker, session_maker, homedir): """ api_sync = ApiSync(mocker.MagicMock(), session_maker, mocker.MagicMock(), homedir) sync_failure = mocker.patch.object(api_sync, "sync_failure") + singleShot_fn = mocker.patch("securedrop_client.sync.QTimer.singleShot") error = Exception() api_sync.on_sync_failure(error) sync_failure.emit.assert_called_once_with(error) + singleShot_fn.assert_called_once_with(15000, api_sync.api_sync_bg_task.sync) @pytest.mark.parametrize("exception", [RequestTimeoutError, ServerConnectionError]) @@ -175,8 +177,10 @@ def test_ApiSync_on_sync_failure_because_of_timeout(mocker, session_maker, homed """ api_sync = ApiSync(mocker.MagicMock(), session_maker, mocker.MagicMock(), homedir) sync_failure = mocker.patch.object(api_sync, "sync_failure") + singleShot_fn = mocker.patch("securedrop_client.sync.QTimer.singleShot") error = exception() api_sync.on_sync_failure(error) sync_failure.emit.assert_called_once_with(error) + singleShot_fn.assert_called_once_with(15000, api_sync.api_sync_bg_task.sync) From f8010cc658e8a3d51cf6b5cf4b1cad80727ae86f Mon Sep 17 00:00:00 2001 From: Allie Crevier Date: Wed, 22 Dec 2021 21:30:08 -0800 Subject: [PATCH 3/3] hide source in gui so that it is not readded by stale sync data Signed-off-by: Allie Crevier --- securedrop_client/gui/widgets.py | 47 +++++++++++++++----- tests/gui/test_widgets.py | 75 ++++++++++++++++++++++++++------ 2 files changed, 97 insertions(+), 25 deletions(-) diff --git a/securedrop_client/gui/widgets.py b/securedrop_client/gui/widgets.py index 134ce2c7d..ba0494a66 100644 --- a/securedrop_client/gui/widgets.py +++ b/securedrop_client/gui/widgets.py @@ -684,20 +684,33 @@ def refresh_source_conversations(self) -> None: except sqlalchemy.exc.InvalidRequestError as e: logger.debug("Error refreshing source conversations: %s", e) - def _delete_source(self, source_uuid: str) -> None: + def _hide_source(self, source_uuid: str) -> None: """ - When we delete a source, we should delete its SourceConversationWrapper, - and remove the reference to it in self.source_conversations + Hide the source and corresponding conversation. """ - self.source_list.delete_sources([source_uuid]) - self.delete_conversation(source_uuid) - if not self.source_list.source_items: - self.empty_conversation_view.show_no_sources_message() + self._hide_conversation(source_uuid) + self.source_list.hide_source(source_uuid) + + visible_item_exists = False + for i in range(self.source_list.count()): + if not self.source_list.isRowHidden(i): + visible_item_exists = True + break + + if visible_item_exists: + self.empty_conversation_view.show_no_source_selected_message() self.empty_conversation_view.show() else: - self.empty_conversation_view.show_no_source_selected_message() + self.empty_conversation_view.show_no_sources_message() self.empty_conversation_view.show() + def _hide_conversation(self, source_uuid: str) -> None: + try: + conversation_wrapper = self.source_conversations[source_uuid] + conversation_wrapper.hide() + except KeyError: + logger.debug("No SourceConversationWrapper for {} to hide".format(source_uuid)) + def delete_conversation(self, source_uuid: str) -> None: """ When we delete a source, we should delete its SourceConversationWrapper, @@ -727,10 +740,12 @@ def set_conversation(self, widget: QWidget) -> None: @pyqtSlot(str, datetime) def _on_source_deletion_successful(self, source_uuid: str, timestamp: datetime) -> None: """ - Handle the signal for a source that was successfully deleted by deleting that source from - the source list and the corresponding conversation. + Hide the source until a sync removes it from the GUI to ensure that a stale sync does not + re-add a source. This method will no longer be necessary once Journalist API v2 is + implemented (see https://github.com/freedomofpress/securedrop/issues/5104) and we can know + explicitly when a source is new. """ - self._delete_source(source_uuid) + self._hide_source(source_uuid) class EmptyConversationView(QWidget): @@ -931,6 +946,16 @@ def update(self, sources: List[Source]) -> List[str]: # conversation widgets return deleted_uuids + def hide_source(self, uuid: str) -> None: + try: + source_item = self.source_items[uuid] + if source_item.isSelected(): + self.setCurrentItem(None) + + source_item.setHidden(True) + except KeyError: + logger.debug("No SourceListWidgetItem for {} to hide".format(uuid)) + def delete_sources(self, source_uuids: List[str]) -> List[str]: deleted_uuids = [] diff --git a/tests/gui/test_widgets.py b/tests/gui/test_widgets.py index 4d399e0f5..7262b5403 100644 --- a/tests/gui/test_widgets.py +++ b/tests/gui/test_widgets.py @@ -605,6 +605,21 @@ def test_MainView_show_sources_when_sources_are_deleted(mocker): mv.delete_conversation.assert_called_once_with(4) +def test_MainView_show_sources_does_not_reshow_deleted_sources(mocker): + """ + Ensure that the hidden source remains hidden. + """ + mv = MainView(None) + mv.source_list.controller = mocker.MagicMock() + source = factory.Source(uuid="123") + mv.source_list.update([factory.Source(uuid="abc"), source]) + mv._on_source_deletion_successful("123", datetime.now()) # "delete" the source + + mv.show_sources([source]) + + assert mv.source_list.source_items["123"].isHidden() + + def test_MainView_delete_conversation_when_conv_wrapper_exists(mocker): """ Ensure SourceConversationWrapper is deleted if it exists. @@ -947,9 +962,10 @@ def test_MainView_set_conversation(mocker): mv.view_layout.addWidget.assert_called_once_with(mock_widget) -def test_MainView__delete_source(mocker): +def test_MainView__hide_source(mocker): """ - Ensure that the source and conversation provided by source uuid are deleted. + Ensure that the source and conversation provided by source uuid are hidden, + and then deleted later by show_sources. """ source = factory.Source(uuid="123") conversation_wrapper = SourceConversationWrapper(source, mocker.MagicMock()) @@ -958,38 +974,49 @@ def test_MainView__delete_source(mocker): mv.source_conversations["123"] = conversation_wrapper mv.source_list.controller = mocker.MagicMock() mv.source_list.update([source]) + + mv._hide_source("123") + + assert len(mv.source_conversations) > 0 assert len(mv.source_list.source_items) > 0 - mv._delete_source("123") + mv.show_sources([]) # Now the server is telling us to delete the source + # Ensure the hidden source and conversation are actually deleted assert len(mv.source_conversations) == 0 assert len(mv.source_list.source_items) == 0 -def test_MainView__delete_source_with_multiple_sources(mocker): +def test_MainView__hide_source_with_multiple_sources(mocker): """ - Ensure that the source and conversation provided by source uuid are deleted. + Ensure that each source and conversation provided by source uuid are hidden, + and that both are deleted later by show_sources. """ source1 = factory.Source(uuid="abc") source2 = factory.Source(uuid="123") mv = MainView(None) mv.source_conversations = {} mv.source_conversations["123"] = SourceConversationWrapper(source1, mocker.MagicMock()) - mv.source_conversations["123"] = SourceConversationWrapper(source2, mocker.MagicMock()) + mv.source_conversations["abc"] = SourceConversationWrapper(source2, mocker.MagicMock()) mv.source_list.controller = mocker.MagicMock() mv.source_list.update([source1, source2]) - assert len(mv.source_list.source_items) > 0 - mv._delete_source("123") - mv._delete_source("abc") + mv._hide_source("123") + mv._hide_source("abc") + + assert len(mv.source_conversations) == 2 + assert len(mv.source_list.source_items) == 2 + + mv.show_sources([]) # Now the server is telling us to delete both sources + # Ensure hidden sources and conversations are actually deleted assert len(mv.source_conversations) == 0 assert len(mv.source_list.source_items) == 0 def test_MainView__on_source_deletion_successful(mocker): """ - Ensure that the source and conversation provided by source uuid are deleted. + Ensure that the source and conversation provided by source uuid are hidden. """ source = factory.Source(uuid="123") conversation_wrapper = SourceConversationWrapper(source, mocker.MagicMock()) @@ -998,15 +1025,30 @@ def test_MainView__on_source_deletion_successful(mocker): mv.source_conversations["123"] = conversation_wrapper mv.source_list.controller = mocker.MagicMock() mv.source_list.update([source]) - assert len(mv.source_list.source_items) > 0 + mv.source_conversations["123"].setHidden(False) mv._on_source_deletion_successful("123", datetime.now()) - assert len(mv.source_conversations) == 0 - assert len(mv.source_list.source_items) == 0 + assert mv.source_conversations["123"].isHidden() + assert mv.source_list.source_items["123"].isHidden() + + +def test_MainView__on_source_deletion_successful_does_not_select_next_source(mocker): + """ + Ensure that no other source is selected by default after a source deletion. + """ + source = factory.Source(uuid="123") + mv = MainView(None) + mv.setup(mocker.MagicMock()) + mv.source_list.update([source]) + mv.source_list.setCurrentItem(mv.source_list.itemAt(0, 0)) # select source + + mv._on_source_deletion_successful("123", datetime.now()) + assert not mv.source_list.get_selected_source() -def test_MainView__on_source_deletion_handles_keyerror(mocker): + +def test_MainView__on_source_deletion_handles_keyerror(): mv = MainView(None) mv._on_source_deletion_successful("throw-keyerror", datetime.now()) @@ -1047,6 +1089,11 @@ def test_SourceList_get_selected_source(mocker): assert current_source.id == sources[1].id +def test_SourceList_delete_sources_handles_keyerror(): + sl = SourceList() + sl.delete_sources([factory.Source(uuid="throw-keyerror")]) + + def test_SourceList_update_adds_new_sources(mocker): """ Check a new SourceWidget for each passed-in source is created and no widgets are cleared or