From 4e20eee7416e2806b0c07138cb0d2afc5e67c084 Mon Sep 17 00:00:00 2001 From: Rowen S Date: Fri, 25 Oct 2024 10:04:09 +0000 Subject: [PATCH] Address deletion condition when accessing orm object attributes in controller delete_sources. Ensure DeleteSourceDialog is shown even with no sources selected in widgets.py and improve comments. Fix new FileWidget functional test. --- client/securedrop_client/gui/widgets.py | 5 ++++- client/securedrop_client/logic.py | 12 +++++++++++- .../functional/test_deleted_file_filewidget.py | 14 +++++++------- 3 files changed, 22 insertions(+), 9 deletions(-) diff --git a/client/securedrop_client/gui/widgets.py b/client/securedrop_client/gui/widgets.py index c8be4ae13..f1997a3c2 100644 --- a/client/securedrop_client/gui/widgets.py +++ b/client/securedrop_client/gui/widgets.py @@ -553,7 +553,7 @@ def on_action_triggered(self) -> None: # The current source selection is continuously received by the controller # as the user selects and deselects; here we retrieve the selection targets = self.controller.get_selected_sources() - if targets: + if targets is not None: dialog = DeleteSourceDialog(targets) dialog.accepted.connect(lambda: self.controller.delete_sources(targets)) dialog.exec() @@ -904,6 +904,8 @@ def on_source_changed(self) -> None: # One source selected; prepare the conversation widget try: source = self.source_list.get_selected_source() + + # Avoid race between user selection and remote deletion if not source: return @@ -1299,6 +1301,7 @@ def schedule_source_management(slice_size: int = slice_size) -> None: QTimer.singleShot(1, schedule_source_management) def get_selected_source(self) -> Optional[Source]: + # if len == 0, return None if not self.selectedItems(): return None diff --git a/client/securedrop_client/logic.py b/client/securedrop_client/logic.py index f662cef7c..e9d42b3b6 100644 --- a/client/securedrop_client/logic.py +++ b/client/securedrop_client/logic.py @@ -1060,7 +1060,17 @@ def delete_sources(self, sources: list[db.Source]) -> None: the failure handler will display an error. """ for source in sources: - job = DeleteSourceJob(source.uuid) + try: + # Accessing source.uuid requires the source object to be + # present in the database. + # To avoid passing and referencing orm objects, a simplified + # ViewModel layer decoupled from the db that presents data to the API/GUI + # would be another possibility. + job = DeleteSourceJob(source.uuid) + except sqlalchemy.orm.exc.ObjectDeletedError: + logger.warning("DeleteSourceJob requested but source already deleted") + return + job.success_signal.connect(self.on_delete_source_success) job.failure_signal.connect(self.on_delete_source_failure) diff --git a/client/tests/functional/test_deleted_file_filewidget.py b/client/tests/functional/test_deleted_file_filewidget.py index 7b8f8f872..66c4681fb 100644 --- a/client/tests/functional/test_deleted_file_filewidget.py +++ b/client/tests/functional/test_deleted_file_filewidget.py @@ -40,18 +40,18 @@ def check_for_sources(): qtbot.wait(TIME_CLICK_ACTION) def conversation_with_file_is_rendered(): - assert gui.main_view.view_layout.itemAt(0) - conversation = gui.main_view.view_layout.itemAt(0).widget() + assert gui.main_view.view_layout.currentIndex() == gui.main_view.CONVERSATION_INDEX + conversation = gui.main_view.view_layout.widget(gui.main_view.view_layout.currentIndex()) assert isinstance(conversation, SourceConversationWrapper) - file_id = list(conversation.conversation_view.current_messages.keys())[2] - file_widget = conversation.conversation_view.current_messages[file_id] + file_id = list(conversation.current_messages.keys())[2] + file_widget = conversation.current_messages[file_id] assert isinstance(file_widget, FileWidget) # Get the selected source conversation that contains a file attachment qtbot.waitUntil(conversation_with_file_is_rendered, timeout=TIME_RENDER_CONV_VIEW) - conversation = gui.main_view.view_layout.itemAt(0).widget() - file_id = list(conversation.conversation_view.current_messages.keys())[2] - file_widget = conversation.conversation_view.current_messages[file_id] + conversation = gui.main_view.view_layout.widget(gui.main_view.view_layout.currentIndex()) + file_id = list(conversation.current_messages.keys())[2] + file_widget = conversation.current_messages[file_id] deleteLater = mocker.patch.object(file_widget, "deleteLater")