From 4a4e6726f47903839a49f992532c0741750a738f Mon Sep 17 00:00:00 2001 From: Allie Crevier Date: Wed, 9 Mar 2022 17:07:52 -0800 Subject: [PATCH 1/2] Simplify how a reply is added from the replybox --- securedrop_client/gui/widgets.py | 31 +++------------- tests/gui/test_widgets.py | 61 +++----------------------------- 2 files changed, 8 insertions(+), 84 deletions(-) diff --git a/securedrop_client/gui/widgets.py b/securedrop_client/gui/widgets.py index 20af9bfa0..b46dedc19 100644 --- a/securedrop_client/gui/widgets.py +++ b/securedrop_client/gui/widgets.py @@ -3116,39 +3116,16 @@ def add_reply(self, reply: Union[DraftReply, Reply], sender: User, index: int) - self.scroll.add_widget_to_conversation(index, conversation_item, Qt.AlignRight) self.current_messages[reply.uuid] = conversation_item - def add_reply_from_reply_box(self, uuid: str, content: str) -> None: - """ - Add a reply from the reply box. - """ - if not self.controller.authenticated_user: - logger.error("User is no longer authenticated so cannot send reply.") - return - - index = len(self.current_messages) - conversation_item = ReplyWidget( - self.controller, - uuid, - content, - "PENDING", - self.controller.reply_ready, - self.controller.reply_download_failed, - self.controller.reply_succeeded, - self.controller.reply_failed, - index, - self.scroll.widget().width(), - self.controller.authenticated_user, - True, - ) - self.scroll.add_widget_to_conversation(index, conversation_item, Qt.AlignRight) - self.current_messages[uuid] = conversation_item - def on_reply_sent(self, source_uuid: str, reply_uuid: str, reply_text: str) -> None: """ Add the reply text sent from ReplyBoxWidget to the conversation. """ self.reply_flag = True if source_uuid == self.source.uuid: - self.add_reply_from_reply_box(reply_uuid, reply_text) + try: + self.update_conversation(self.source.collection) + except sqlalchemy.exc.InvalidRequestError as e: + logger.debug(e) self.update_deletion_markers() diff --git a/tests/gui/test_widgets.py b/tests/gui/test_widgets.py index 5e1f125b1..b9590fbf4 100644 --- a/tests/gui/test_widgets.py +++ b/tests/gui/test_widgets.py @@ -4448,12 +4448,12 @@ def test_ConversationView_on_reply_sent(mocker): controller = mocker.MagicMock() controller.authenticated_user = factory.User() cv = ConversationView(source, controller) - cv.add_reply_from_reply_box = mocker.MagicMock() + cv.update_conversation = mocker.MagicMock() assert cv.reply_flag is False cv.on_reply_sent(source_uuid=source.uuid, reply_uuid="abc123", reply_text="test message") - cv.add_reply_from_reply_box.assert_called_with("abc123", "test message") + cv.update_conversation.assert_called_with(source.collection) assert cv.reply_flag is True @@ -4474,12 +4474,12 @@ def test_ConversationView_on_reply_sent_does_not_add_message_intended_for_differ def test_ConversationView_on_reply_sent_with_deleted_source(mocker): """ - Check that replying to a deleted source causes an error and is logged. + Check that replying to a deleted source handles exception. """ source = factory.Source() controller = mocker.MagicMock() cv = ConversationView(source, controller) - cv.add_reply_from_reply_box = mocker.MagicMock() + cv.update_conversation = mocker.MagicMock() def collection_error(): raise sqlalchemy.exc.InvalidRequestError() @@ -4498,59 +4498,6 @@ def collection_error(): ) -def test_ConversationView_add_reply_from_reply_box(mocker, session, session_maker, homedir): - """ - Adding a reply from reply box results in a new ReplyWidget added to the layout. - """ - controller = logic.Controller("http://localhost", mocker.MagicMock(), session_maker, homedir) - controller.authenticated_user = factory.User() - - source = factory.Source() - session.add(source) - session.commit() - cv = ConversationView(source, controller) - cv.add_reply_from_reply_box(uuid="abc123", content=">^..^<") - - # Check that we added the correct widget to the layout - reply_widget = cv.scroll.conversation_layout.itemAt(0).widget() - assert isinstance(reply_widget, ReplyWidget) - - assert reply_widget.uuid == "abc123" - assert reply_widget.message.text() == ">^..^<" - assert reply_widget.index == 0 - assert reply_widget.status == "PENDING" - assert not reply_widget.failed_to_decrypt - assert reply_widget.controller == controller - assert reply_widget.sender_is_current_user - assert cv.current_messages["abc123"].sender == controller.authenticated_user - assert reply_widget.sender == controller.authenticated_user - - -def test_ConversationView_add_reply_from_reply_box_does_not_send_when_not_authenticated(mocker): - """ - Ensure we do not send a reply from reply box when the user is no longer authenticated. - """ - source = factory.Source() - reply_ready = mocker.MagicMock() - reply_download_failed = mocker.MagicMock() - reply_succeeded = mocker.MagicMock() - reply_failed = mocker.MagicMock() - controller = mocker.MagicMock( - reply_ready=reply_ready, - reply_download_failed=reply_download_failed, - reply_succeeded=reply_succeeded, - reply_failed=reply_failed, - ) - controller.authenticated_user = None - cv = ConversationView(source, controller) - cv.scroll.conversation_layout = mocker.MagicMock() - reply_widget = mocker.patch("securedrop_client.gui.widgets.ReplyWidget") - cv.add_reply_from_reply_box(uuid="abc123", content="test message") - - reply_widget.assert_not_called() - cv.scroll.conversation_layout.insertWidget.assert_not_called() - - def test_ConversationView_add_reply(mocker, homedir, session, session_maker): """ Adding a reply from a source results in a new ReplyWidget added to the layout. From 3aeef6eeea404dbc55e9f4fcc210e6e812ce29f3 Mon Sep 17 00:00:00 2001 From: Allie Crevier Date: Wed, 9 Mar 2022 23:59:35 -0800 Subject: [PATCH 2/2] Remove old way of checking deletion timestamp Signed-off-by: Allie Crevier --- securedrop_client/gui/widgets.py | 38 ++++++------------- tests/gui/test_widgets.py | 64 ++------------------------------ 2 files changed, 16 insertions(+), 86 deletions(-) diff --git a/securedrop_client/gui/widgets.py b/securedrop_client/gui/widgets.py index b46dedc19..7b95d4850 100644 --- a/securedrop_client/gui/widgets.py +++ b/securedrop_client/gui/widgets.py @@ -1329,11 +1329,6 @@ def set_snippet( if self.deleting or self.deleting_conversation: return - # If the sync started before the deletion finished, then the sync is stale and we do - # not want to update the source widget. - if self.sync_started_timestamp < self.deletion_scheduled_timestamp: - return - # If the source collection is empty yet the interaction_count is greater than zero, then we # known that the conversation has been deleted. if not self.source.server_collection: @@ -2944,18 +2939,15 @@ def _on_conversation_deletion_successful(self, source_uuid: str, timestamp: date logger.debug(f"Could not update ConversationView: {e}") def update_deletion_markers(self) -> None: - try: - if self.source.collection: - self.scroll.show() - if self.source.collection[0].file_counter > 1: - self.deleted_conversation_marker.hide() - self.deleted_conversation_items_marker.show() - elif self.source.interaction_count > 0: - self.scroll.hide() - self.deleted_conversation_items_marker.hide() - self.deleted_conversation_marker.show() - except sqlalchemy.exc.InvalidRequestError as e: - logger.debug(f"Could not Update deletion markers in the ConversationView: {e}") + if self.source.collection: + self.scroll.show() + if self.source.collection[0].file_counter > 1: + self.deleted_conversation_marker.hide() + self.deleted_conversation_items_marker.show() + elif self.source.interaction_count > 0: + self.scroll.hide() + self.deleted_conversation_items_marker.hide() + self.deleted_conversation_marker.show() def update_conversation(self, collection: list) -> None: """ @@ -2976,11 +2968,6 @@ 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 @@ -3116,7 +3103,7 @@ def add_reply(self, reply: Union[DraftReply, Reply], sender: User, index: int) - self.scroll.add_widget_to_conversation(index, conversation_item, Qt.AlignRight) self.current_messages[reply.uuid] = conversation_item - def on_reply_sent(self, source_uuid: str, reply_uuid: str, reply_text: str) -> None: + def on_reply_sent(self, source_uuid: str) -> None: """ Add the reply text sent from ReplyBoxWidget to the conversation. """ @@ -3126,7 +3113,6 @@ def on_reply_sent(self, source_uuid: str, reply_uuid: str, reply_text: str) -> N self.update_conversation(self.source.collection) except sqlalchemy.exc.InvalidRequestError as e: logger.debug(e) - self.update_deletion_markers() class SourceConversationWrapper(QWidget): @@ -3268,7 +3254,7 @@ class ReplyBoxWidget(QWidget): A textbox where a journalist can enter a reply. """ - reply_sent = pyqtSignal(str, str, str) + reply_sent = pyqtSignal(str) def __init__(self, source: Source, controller: Controller) -> None: super().__init__() @@ -3365,7 +3351,7 @@ def send_reply(self) -> None: self.text_edit.setText("") reply_uuid = str(uuid4()) self.controller.send_reply(self.source.uuid, reply_uuid, reply_text) - self.reply_sent.emit(self.source.uuid, reply_uuid, reply_text) + self.reply_sent.emit(self.source.uuid) @pyqtSlot(bool) def _on_authentication_changed(self, authenticated: bool) -> None: diff --git a/tests/gui/test_widgets.py b/tests/gui/test_widgets.py index b9590fbf4..5b24d90de 100644 --- a/tests/gui/test_widgets.py +++ b/tests/gui/test_widgets.py @@ -1940,41 +1940,6 @@ def test_SourceWidget_set_snippet(mocker, session_maker, session, homedir): sw.set_snippet(source_uuid, "mock_file_uuid", "something new") -def test_SourceWidget_set_snippet_does_not_update_if_sync_is_stale(mocker): - """ - Ensure that the snippet does not get updated when an update is based on a sync that started - before a deletion operation succeeded. - """ - controller = mocker.MagicMock() - source = mocker.MagicMock() - source.journalist_designation = "Testy McTestface" - source.collection = [factory.Message(content="abc123")] - source.server_collection = source.collection - sw = SourceWidget(controller, source, mocker.MagicMock(), mocker.MagicMock()) - sw.update() - assert sw.preview.text() == "abc123" - - # Make the sync stale - sw.sync_started_timestamp = datetime.now() - sw.deletion_scheduled_timestamp = datetime.now() - - # Set up source data so that it appears that the conversation was deleted - source.collection = [] - source.interaction_count = 1 - source.server_collection = source.collection - - # Assert the preview snippet does not get updated because the sync is stale - sw.set_snippet(source.uuid, "mock_file_uuid") - assert sw.preview.text() == "abc123" - - # The sync is no longer stale so the preview should show that the are no more source - # conversation items because they've been deleted - sw.deletion_scheduled_timestamp = datetime.now() - sw.sync_started_timestamp = datetime.now() - sw.set_snippet(source.uuid, "mock_file_uuid") - assert sw.preview.text() == "— All files and messages deleted for this source —" - - def test_SourceWidget_update_truncate_latest_msg(mocker): """ If the latest message in the conversation is longer than 150 characters, @@ -4325,22 +4290,6 @@ def test_ConversationView__on_conversation_deletion_successful_does_not_hide_dra assert not cv.current_messages[draft_reply.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 @@ -4451,7 +4400,7 @@ def test_ConversationView_on_reply_sent(mocker): cv.update_conversation = mocker.MagicMock() assert cv.reply_flag is False - cv.on_reply_sent(source_uuid=source.uuid, reply_uuid="abc123", reply_text="test message") + cv.on_reply_sent(source_uuid=source.uuid) cv.update_conversation.assert_called_with(source.collection) assert cv.reply_flag is True @@ -4467,7 +4416,7 @@ def test_ConversationView_on_reply_sent_does_not_add_message_intended_for_differ cv = ConversationView(source, controller) cv.add_reply = mocker.MagicMock() - cv.on_reply_sent("different_source_id", "mock", "mock") + cv.on_reply_sent("different_source_id") assert not cv.add_reply.called @@ -4484,18 +4433,13 @@ def test_ConversationView_on_reply_sent_with_deleted_source(mocker): def collection_error(): raise sqlalchemy.exc.InvalidRequestError() - debug_logger = mocker.patch("securedrop_client.gui.widgets.logger.debug") mock_collection = mocker.patch( "securedrop_client.db.Source.collection", new_callable=PropertyMock ) ire = sqlalchemy.exc.InvalidRequestError() mock_collection.side_effect = ire - cv.on_reply_sent(source.uuid, "mock", "mock") - - debug_logger.assert_any_call( - f"Could not Update deletion markers in the ConversationView: {ire}" - ) + cv.on_reply_sent(source.uuid) def test_ConversationView_add_reply(mocker, homedir, session, session_maker): @@ -4772,7 +4716,7 @@ def test_ReplyBoxWidget_send_reply(mocker): scw.reply_box.send_reply() - scw.reply_box.reply_sent.emit.assert_called_once_with("abc123", "456xyz", "Alles für Alle") + scw.reply_box.reply_sent.emit.assert_called_once_with("abc123") scw.reply_box.text_edit.setText.assert_called_once_with("") controller.send_reply.assert_called_once_with("abc123", "456xyz", "Alles für Alle")