From c843e670d6029cd4e603ad36fff174efcec9ffc1 Mon Sep 17 00:00:00 2001 From: "Nicholas H.Tollervey" Date: Thu, 9 Jan 2020 16:20:18 +0000 Subject: [PATCH 1/2] A working PoC which requires feedback and tests. Fixes #473. --- securedrop_client/gui/widgets.py | 101 ++++++++++++++++++++++--------- 1 file changed, 71 insertions(+), 30 deletions(-) diff --git a/securedrop_client/gui/widgets.py b/securedrop_client/gui/widgets.py index e54accd4e..c19ddd644 100644 --- a/securedrop_client/gui/widgets.py +++ b/securedrop_client/gui/widgets.py @@ -1557,9 +1557,10 @@ class SpeechBubble(QWidget): TOP_MARGIN = 28 BOTTOM_MARGIN = 10 - def __init__(self, message_id: str, text: str, update_signal) -> None: + def __init__(self, message_id: str, text: str, update_signal, index: int) -> None: super().__init__() self.message_id = message_id + self.index = index # Set styles self.setObjectName('speech_bubble') @@ -1620,8 +1621,8 @@ class MessageWidget(SpeechBubble): Represents an incoming message from the source. """ - def __init__(self, message_id: str, message: str, update_signal) -> None: - super().__init__(message_id, message, update_signal) + def __init__(self, message_id: str, message: str, update_signal, index: int) -> None: + super().__init__(message_id, message, update_signal, index) class ReplyWidget(SpeechBubble): @@ -1702,8 +1703,9 @@ def __init__( update_signal, message_succeeded_signal, message_failed_signal, + index: int, ) -> None: - super().__init__(message_id, message, update_signal) + super().__init__(message_id, message, update_signal, index) self.message_id = message_id error_icon = SvgLabel('error_icon.svg', svg_size=QSize(12, 12)) @@ -1833,6 +1835,7 @@ def __init__( file_uuid: str, controller: Controller, file_ready_signal: pyqtBoundSignal, + index: int, ) -> None: """ Given some text and a reference to the controller, make something to display a file. @@ -1841,6 +1844,7 @@ def __init__( self.controller = controller self.file = self.controller.get_file(file_uuid) + self.index = index # Set styles self.setObjectName('file_widget') @@ -2388,6 +2392,8 @@ def __init__(self, source_db_object: Source, controller: Controller): self.source = source_db_object self.controller = controller + self.current_messages = {} # To hold currently displayed messages. + # Set styles self.setStyleSheet(self.CSS) @@ -2428,44 +2434,73 @@ def clear_conversation(self): child.widget().deleteLater() def update_conversation(self, collection: list) -> None: - # clear all old items - self.clear_conversation() + """ + Given a list of conversation items that reflect the new state of the + conversation, this method does two things: + + * Checks if the conversation item already exists in the conversation. + If so, it checks that it's still in the same position. If it isn't, + the item is removed from its current position and re-added at the + new position. Then the index meta-data on the widget is updated to + reflect this change. + * If the item is a new item, this is created (as before) and inserted + into the conversation at the correct index. + + Things to note, speech bubbles and files have an index attribute which + defines where they currently are. This is the attribute that's checked + when the new conversation state (i.e. the collection argument) is + 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. + """ self.controller.session.refresh(self.source) - # add new items - for conversation_item in collection: - if isinstance(conversation_item, Message): - self.add_message(conversation_item) - elif isinstance(conversation_item, (DraftReply, Reply)): - self.add_reply(conversation_item) + for index, conversation_item in enumerate(collection): + item_widget = self.current_messages.get(conversation_item.uuid) + if item_widget: + # check an already displayed item. + if item_widget.index != index: + # The existing widget is out of order, remove / re-add it + # and update index details. + self.conversation_layout.removeWidget(item_widget) + item_widget.index = index + self.conversation_layout.insertWidget(index, item_widget) + # TODO: Check if text in item has changed, then update the + # widget to reflect this change. + # TODO: Check for any other possible changes of state in the + # message so this can be reflected in the widget. + # TODO: Get rid of the damn todos and write/update tests. ;-) else: - self.add_file(conversation_item) + # add a new item to be displayed. + if isinstance(conversation_item, Message): + self.add_message(conversation_item, index) + elif isinstance(conversation_item, (DraftReply, Reply)): + self.add_reply(conversation_item, index) + else: + self.add_file(conversation_item, index) - def add_file(self, file: File): + def add_file(self, file: File, index): """ Add a file from the source. """ - conversation_item = FileWidget(file.uuid, self.controller, self.controller.file_ready) - self.conversation_layout.addWidget(conversation_item, alignment=Qt.AlignLeft) + conversation_item = FileWidget(file.uuid, self.controller, self.controller.file_ready, index) + self.conversation_layout.insertWidget(index, conversation_item, alignment=Qt.AlignLeft) + self.current_messages[file.uuid] = conversation_item def update_conversation_position(self, min_val, max_val): """ Handler called when a new item is added to the conversation. Ensures it's scrolled to the bottom and thus visible. """ - current_val = self.scroll.verticalScrollBar().value() - viewport_height = self.scroll.viewport().height() - - if current_val + viewport_height > max_val: - self.scroll.verticalScrollBar().setValue(max_val) + self.scroll.verticalScrollBar().setValue(max_val) - def add_message(self, message: Message) -> None: + def add_message(self, message: Message, index) -> None: """ Add a message from the source. """ - conversation_item = MessageWidget(message.uuid, str(message), self.controller.message_ready) - self.conversation_layout.addWidget(conversation_item, alignment=Qt.AlignLeft) + conversation_item = MessageWidget(message.uuid, str(message), self.controller.message_ready, index) + self.conversation_layout.insertWidget(index, conversation_item, alignment=Qt.AlignLeft) + self.current_messages[message.uuid] = conversation_item - def add_reply(self, reply: Union[DraftReply, Reply]) -> None: + def add_reply(self, reply: Union[DraftReply, Reply], index) -> None: """ Add a reply from a journalist to the source. """ @@ -2481,28 +2516,34 @@ def add_reply(self, reply: Union[DraftReply, Reply]) -> None: send_status, self.controller.reply_ready, self.controller.reply_succeeded, - self.controller.reply_failed) - self.conversation_layout.addWidget(conversation_item, alignment=Qt.AlignRight) + self.controller.reply_failed, + index) + self.conversation_layout.insertWidget(index, conversation_item, alignment=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. """ + index = len(self.current_messages) conversation_item = ReplyWidget( uuid, content, 'PENDING', self.controller.reply_ready, self.controller.reply_succeeded, - self.controller.reply_failed) + self.controller.reply_failed, + index) self.conversation_layout.addWidget(conversation_item, alignment=Qt.AlignRight) 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. """ - if source_uuid == self.source.uuid: - self.add_reply_from_reply_box(reply_uuid, reply_text) + # TODO: replace this with UI indication that the reply is "in flight" + # For now, just do nothing. + # if source_uuid == self.source.uuid: + # self.add_reply_from_reply_box(reply_uuid, reply_text) class SourceConversationWrapper(QWidget): From abac2cc6ea70d8051a2510ce73bdb1d850b4a1e4 Mon Sep 17 00:00:00 2001 From: "Nicholas H.Tollervey" Date: Mon, 13 Jan 2020 12:26:39 +0000 Subject: [PATCH 2/2] Revert scroll to bottom (to be addressed in existing separate PR). Fix #61 to supercede #685. Make on_reply_sent work properly without duplicate entries. --- securedrop_client/gui/widgets.py | 51 ++++--- tests/factory.py | 20 ++- tests/gui/test_widgets.py | 228 +++++++++++++++++++++++-------- 3 files changed, 225 insertions(+), 74 deletions(-) diff --git a/securedrop_client/gui/widgets.py b/securedrop_client/gui/widgets.py index c19ddd644..d7b408a38 100644 --- a/securedrop_client/gui/widgets.py +++ b/securedrop_client/gui/widgets.py @@ -31,7 +31,8 @@ QPushButton, QVBoxLayout, QLineEdit, QScrollArea, QDialog, QAction, QMenu, QMessageBox, \ QToolButton, QSizePolicy, QPlainTextEdit, QStatusBar, QGraphicsDropShadowEffect -from securedrop_client.db import DraftReply, Source, Message, File, Reply, User +from securedrop_client.db import (DraftReply, Source, Message, File, Reply, User, + ReplySendStatusCodes) from securedrop_client.storage import source_exists from securedrop_client.export import ExportStatus, ExportError from securedrop_client.gui import SecureQLabel, SvgLabel, SvgPushButton, SvgToggleButton @@ -2392,7 +2393,8 @@ def __init__(self, source_db_object: Source, controller: Controller): self.source = source_db_object self.controller = controller - self.current_messages = {} # To hold currently displayed messages. + # To hold currently displayed messages. + self.current_messages = {} # type: Dict[str, QWidget] # Set styles self.setStyleSheet(self.CSS) @@ -2419,6 +2421,9 @@ def __init__(self, source_db_object: Source, controller: Controller): self.scroll.setWidget(self.container) self.scroll.setWidgetResizable(True) + # Flag to show if the current user has sent a reply. See issue #61. + self.reply_flag = False + # Completely unintuitive way to ensure the view remains scrolled to the bottom. sb = self.scroll.verticalScrollBar() sb.rangeChanged.connect(self.update_conversation_position) @@ -2462,12 +2467,21 @@ def update_conversation(self, collection: list) -> None: # and update index details. self.conversation_layout.removeWidget(item_widget) item_widget.index = index - self.conversation_layout.insertWidget(index, item_widget) - # TODO: Check if text in item has changed, then update the + if isinstance(item_widget, ReplyWidget): + self.conversation_layout.insertWidget(index, item_widget, + alignment=Qt.AlignRight) + else: + self.conversation_layout.insertWidget(index, item_widget, + alignment=Qt.AlignLeft) + # Check if text in item has changed, then update the # widget to reflect this change. - # TODO: Check for any other possible changes of state in the - # message so this can be reflected in the widget. - # TODO: Get rid of the damn todos and write/update tests. ;-) + if not isinstance(item_widget, FileWidget): + if item_widget.message.text() != conversation_item.content: + item_widget.message.setText(conversation_item.content) + # Check if this is a draft reply then ensure it's removed. + if isinstance(conversation_item, DraftReply): + if conversation_item.send_status.name == ReplySendStatusCodes.PENDING.value: + self.conversation_layout.removeWidget(item_widget) else: # add a new item to be displayed. if isinstance(conversation_item, Message): @@ -2481,7 +2495,8 @@ def add_file(self, file: File, index): """ Add a file from the source. """ - conversation_item = FileWidget(file.uuid, self.controller, self.controller.file_ready, index) + conversation_item = FileWidget(file.uuid, self.controller, self.controller.file_ready, + index) self.conversation_layout.insertWidget(index, conversation_item, alignment=Qt.AlignLeft) self.current_messages[file.uuid] = conversation_item @@ -2490,13 +2505,16 @@ def update_conversation_position(self, min_val, max_val): Handler called when a new item is added to the conversation. Ensures it's scrolled to the bottom and thus visible. """ - self.scroll.verticalScrollBar().setValue(max_val) + if self.reply_flag and max_val > 0: + self.scroll.verticalScrollBar().setValue(max_val) + self.reply_flag = False def add_message(self, message: Message, index) -> None: """ Add a message from the source. """ - conversation_item = MessageWidget(message.uuid, str(message), self.controller.message_ready, index) + conversation_item = MessageWidget(message.uuid, str(message), self.controller.message_ready, + index) self.conversation_layout.insertWidget(index, conversation_item, alignment=Qt.AlignLeft) self.current_messages[message.uuid] = conversation_item @@ -2534,16 +2552,16 @@ def add_reply_from_reply_box(self, uuid: str, content: str) -> None: self.controller.reply_succeeded, self.controller.reply_failed, index) - self.conversation_layout.addWidget(conversation_item, alignment=Qt.AlignRight) + self.conversation_layout.insertWidget(index, conversation_item, alignment=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. """ - # TODO: replace this with UI indication that the reply is "in flight" - # For now, just do nothing. - # if source_uuid == self.source.uuid: - # self.add_reply_from_reply_box(reply_uuid, reply_text) + self.reply_flag = True + if source_uuid == self.source.uuid: + self.add_reply_from_reply_box(reply_uuid, reply_text) class SourceConversationWrapper(QWidget): @@ -2692,10 +2710,11 @@ def send_reply(self) -> None: """ reply_text = self.text_edit.toPlainText().strip() if reply_text: + self.text_edit.clearFocus() # Fixes #691 + 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.text_edit.setText('') def _on_authentication_changed(self, authenticated: bool) -> None: if authenticated: diff --git a/tests/factory.py b/tests/factory.py index 4a66e7f59..2ef97f5b5 100644 --- a/tests/factory.py +++ b/tests/factory.py @@ -13,6 +13,7 @@ FILE_COUNT = 0 REPLY_COUNT = 0 DRAFT_REPLY_COUNT = 0 +REPLY_SEND_STATUS_COUNT = 0 USER_COUNT = 0 @@ -54,7 +55,7 @@ def Message(**attrs): global MESSAGE_COUNT MESSAGE_COUNT += 1 defaults = dict( - uuid='source-uuid-{}'.format(MESSAGE_COUNT), + uuid='msg-uuid-{}'.format(MESSAGE_COUNT), filename='{}-msg.gpg'.format(MESSAGE_COUNT), size=123, download_url='http://wat.onion/abc', @@ -72,7 +73,7 @@ def Reply(**attrs): global REPLY_COUNT REPLY_COUNT += 1 defaults = dict( - uuid='source-uuid-{}'.format(REPLY_COUNT), + uuid='reply-uuid-{}'.format(REPLY_COUNT), filename='{}-reply.gpg'.format(REPLY_COUNT), size=123, is_decrypted=True, @@ -95,6 +96,7 @@ def DraftReply(**attrs): file_counter=1, uuid='draft-reply-uuid-{}'.format(REPLY_COUNT), content='content', + send_status_id=1, ) defaults.update(attrs) @@ -102,11 +104,23 @@ def DraftReply(**attrs): return db.DraftReply(**defaults) +def ReplySendStatus(**attrs): + global REPLY_SEND_STATUS_COUNT + REPLY_SEND_STATUS_COUNT += 1 + defaults = dict( + name=db.ReplySendStatusCodes.PENDING.value, + ) + + defaults.update(attrs) + + return db.ReplySendStatus(**defaults) + + def File(**attrs): global FILE_COUNT FILE_COUNT += 1 defaults = dict( - uuid='source-uuid-{}'.format(FILE_COUNT), + uuid='file-uuid-{}'.format(FILE_COUNT), filename='{}-doc.gz.gpg'.format(FILE_COUNT), original_filename='{}-doc.txt'.format(FILE_COUNT), size=123, diff --git a/tests/gui/test_widgets.py b/tests/gui/test_widgets.py index 443590b21..07f2f3dc3 100644 --- a/tests/gui/test_widgets.py +++ b/tests/gui/test_widgets.py @@ -1279,7 +1279,7 @@ def test_SpeechBubble_init(mocker): mock_connect = mocker.Mock() mock_signal.connect = mock_connect - sb = SpeechBubble('mock id', 'hello', mock_signal) + sb = SpeechBubble('mock id', 'hello', mock_signal, 0) ss = sb.styleSheet() sb.message.text() == 'hello' @@ -1294,7 +1294,7 @@ def test_SpeechBubble_update_text(mocker): mock_signal = mocker.MagicMock() msg_id = 'abc123' - sb = SpeechBubble(msg_id, 'hello', mock_signal) + sb = SpeechBubble(msg_id, 'hello', mock_signal, 0) new_msg = 'new message' sb._update_text(msg_id, new_msg) @@ -1312,7 +1312,7 @@ def test_SpeechBubble_html_init(mocker): """ mock_signal = mocker.MagicMock() - bubble = SpeechBubble('mock id', 'hello', mock_signal) + bubble = SpeechBubble('mock id', 'hello', mock_signal, 0) assert bubble.message.text() == 'hello' @@ -1321,7 +1321,7 @@ def test_SpeechBubble_with_apostrophe_in_text(mocker): mock_signal = mocker.MagicMock() message = "I'm sure, you are reading my message." - bubble = SpeechBubble('mock id', message, mock_signal) + bubble = SpeechBubble('mock id', message, mock_signal, 0) assert bubble.message.text() == message @@ -1333,7 +1333,7 @@ def test_MessageWidget_init(mocker): mock_connected = mocker.Mock() mock_signal.connect = mock_connected - MessageWidget('mock id', 'hello', mock_signal) + MessageWidget('mock id', 'hello', mock_signal, 0) assert mock_connected.called @@ -1361,6 +1361,7 @@ def test_ReplyWidget_init(mocker): mock_update_signal, mock_success_signal, mock_failure_signal, + 0, ) assert mock_update_connected.called @@ -1379,7 +1380,7 @@ def test_FileWidget_init_file_not_downloaded(mocker, source, session): get_file = mocker.MagicMock(return_value=file) controller = mocker.MagicMock(get_file=get_file) - fw = FileWidget('mock', controller, mocker.MagicMock()) + fw = FileWidget('mock', controller, mocker.MagicMock(), 0) assert fw.controller == controller assert fw.file.is_downloaded is False @@ -1402,7 +1403,7 @@ def test_FileWidget_init_file_downloaded(mocker, source, session): get_file = mocker.MagicMock(return_value=file) controller = mocker.MagicMock(get_file=get_file) - fw = FileWidget('mock', controller, mocker.MagicMock()) + fw = FileWidget('mock', controller, mocker.MagicMock(), 0) assert fw.controller == controller assert fw.file.is_downloaded is True @@ -1431,7 +1432,7 @@ def test_FileWidget_event_handler(mocker, session, source): test_event = QEvent(QEvent.MouseButtonPress) test_event.button = mocker.MagicMock(return_value=Qt.LeftButton) - fw = FileWidget(file_.uuid, mock_controller, mock_signal) + fw = FileWidget(file_.uuid, mock_controller, mock_signal, 0) fw._on_left_click = mocker.MagicMock() fw.eventFilter(fw, test_event) @@ -1454,7 +1455,7 @@ def test_FileWidget_on_left_click_download(mocker, session, source): mock_get_file = mocker.MagicMock(return_value=file_) mock_controller = mocker.MagicMock(get_file=mock_get_file) - fw = FileWidget(file_.uuid, mock_controller, mock_signal) + fw = FileWidget(file_.uuid, mock_controller, mock_signal, 0) fw.download_button = mocker.MagicMock() mock_get_file.assert_called_once_with(file_.uuid) mock_get_file.reset_mock() @@ -1482,7 +1483,7 @@ def test_FileWidget_start_button_animation(mocker, session, source): session.commit() mock_get_file = mocker.MagicMock(return_value=file_) mock_controller = mocker.MagicMock(get_file=mock_get_file) - fw = FileWidget(file_.uuid, mock_controller, mock_signal) + fw = FileWidget(file_.uuid, mock_controller, mock_signal, 0) fw.download_button = mocker.MagicMock() fw.start_button_animation() # Check indicators of activity have been updated. @@ -1504,7 +1505,7 @@ def test_FileWidget_on_left_click_open(mocker, session, source): mock_get_file = mocker.MagicMock(return_value=file_) mock_controller = mocker.MagicMock(get_file=mock_get_file) - fw = FileWidget(file_.uuid, mock_controller, mock_signal) + fw = FileWidget(file_.uuid, mock_controller, mock_signal, 0) fw._on_left_click() fw.controller.on_file_open.assert_called_once_with(file_.uuid) @@ -1525,7 +1526,7 @@ def test_FileWidget_set_button_animation_frame(mocker, session, source): mock_get_file = mocker.MagicMock(return_value=file_) mock_controller = mocker.MagicMock(get_file=mock_get_file) - fw = FileWidget(file_.uuid, mock_controller, mock_signal) + fw = FileWidget(file_.uuid, mock_controller, mock_signal, 0) fw.download_button = mocker.MagicMock() fw.set_button_animation_frame(1) assert fw.download_button.setIcon.call_count == 1 @@ -1540,7 +1541,7 @@ def test_FileWidget_update(mocker, session, source): session.commit() get_file = mocker.MagicMock(return_value=file) controller = mocker.MagicMock(get_file=get_file) - fw = FileWidget(file.uuid, controller, mocker.MagicMock()) + fw = FileWidget(file.uuid, controller, mocker.MagicMock(), 0) fw.update() @@ -1560,7 +1561,7 @@ def test_FileWidget_on_file_download_updates_items_when_uuid_matches(mocker, sou get_file = mocker.MagicMock(return_value=file) controller = mocker.MagicMock(get_file=get_file) - fw = FileWidget(file.uuid, controller, mocker.MagicMock()) + fw = FileWidget(file.uuid, controller, mocker.MagicMock(), 0) fw.update = mocker.MagicMock() fw._on_file_downloaded(file.uuid) @@ -1586,7 +1587,7 @@ def test_FileWidget_on_file_download_updates_items_when_uuid_does_not_match( get_file = mocker.MagicMock(return_value=file) controller = mocker.MagicMock(get_file=get_file) - fw = FileWidget(file.uuid, controller, mocker.MagicMock()) + fw = FileWidget(file.uuid, controller, mocker.MagicMock(), 0) fw.clear = mocker.MagicMock() fw.update = mocker.MagicMock() @@ -1612,7 +1613,7 @@ def test_FileWidget__on_export_clicked(mocker, session, source): get_file = mocker.MagicMock(return_value=file) controller = mocker.MagicMock(get_file=get_file) - fw = FileWidget(file.uuid, controller, mocker.MagicMock()) + fw = FileWidget(file.uuid, controller, mocker.MagicMock(), 0) fw.update = mocker.MagicMock() mocker.patch('securedrop_client.gui.widgets.QDialog.exec') controller.run_export_preflight_checks = mocker.MagicMock() @@ -1639,7 +1640,7 @@ def test_FileWidget__on_export_clicked_missing_file(mocker, session, source): get_file = mocker.MagicMock(return_value=file) controller = mocker.MagicMock(get_file=get_file) - fw = FileWidget(file.uuid, controller, mocker.MagicMock()) + fw = FileWidget(file.uuid, controller, mocker.MagicMock(), 0) fw.update = mocker.MagicMock() mocker.patch('securedrop_client.gui.widgets.QDialog.exec') controller.run_export_preflight_checks = mocker.MagicMock() @@ -1663,7 +1664,7 @@ def test_FileWidget__on_print_clicked(mocker, session, source): get_file = mocker.MagicMock(return_value=file) controller = mocker.MagicMock(get_file=get_file) - fw = FileWidget(file.uuid, controller, mocker.MagicMock()) + fw = FileWidget(file.uuid, controller, mocker.MagicMock(), 0) fw.update = mocker.MagicMock() mocker.patch('securedrop_client.gui.widgets.QDialog.exec') controller.print_file = mocker.MagicMock() @@ -1690,7 +1691,7 @@ def test_FileWidget__on_print_clicked_missing_file(mocker, session, source): get_file = mocker.MagicMock(return_value=file) controller = mocker.MagicMock(get_file=get_file) - fw = FileWidget(file.uuid, controller, mocker.MagicMock()) + fw = FileWidget(file.uuid, controller, mocker.MagicMock(), 0) fw.update = mocker.MagicMock() mocker.patch('securedrop_client.gui.widgets.QDialog.exec') controller.print_file = mocker.MagicMock() @@ -2054,13 +2055,17 @@ def test_ConversationView_update_conversation_position_follow(mocker, homedir): """ Check the signal handler sets the correct value for the scrollbar to be the maximum possible value, when the scrollbar is near the bottom, meaning - it is following the conversation. + it is following the conversation. This should only work if the user has + submitted a reply to a source. """ mocked_source = mocker.MagicMock() mocked_controller = mocker.MagicMock() cv = ConversationView(mocked_source, mocked_controller) + # Flag to denote a reply was sent by the user. + cv.reply_flag = True + cv.scroll.verticalScrollBar().value = mocker.MagicMock(return_value=5900) cv.scroll.viewport().height = mocker.MagicMock(return_value=500) cv.scroll.verticalScrollBar().setValue = mocker.MagicMock() @@ -2111,14 +2116,14 @@ def test_ConversationView_add_message(mocker, session, source): mock_msg_widget = mocker.patch('securedrop_client.gui.widgets.MessageWidget', return_value=mock_msg_widget_res) - cv.add_message(message) + cv.add_message(message, 0) # check that we built the widget was called with the correct args - mock_msg_widget.assert_called_once_with(message.uuid, content, mock_message_ready_signal) + mock_msg_widget.assert_called_once_with(message.uuid, content, mock_message_ready_signal, 0) # check that we added the correct widget to the layout - cv.conversation_layout.addWidget.assert_called_once_with( - mock_msg_widget_res, alignment=Qt.AlignLeft) + cv.conversation_layout.insertWidget.assert_called_once_with( + 0, mock_msg_widget_res, alignment=Qt.AlignLeft) def test_ConversationView_add_message_no_content(mocker, session, source): @@ -2144,15 +2149,15 @@ def test_ConversationView_add_message_no_content(mocker, session, source): mock_msg_widget = mocker.patch('securedrop_client.gui.widgets.MessageWidget', return_value=mock_msg_widget_res) - cv.add_message(message) + cv.add_message(message, 0) # check that we built the widget was called with the correct args mock_msg_widget.assert_called_once_with( - message.uuid, '', mock_message_ready_signal) + message.uuid, '', mock_message_ready_signal, 0) # check that we added the correct widget to the layout - cv.conversation_layout.addWidget.assert_called_once_with( - mock_msg_widget_res, alignment=Qt.AlignLeft) + cv.conversation_layout.insertWidget.assert_called_once_with( + 0, mock_msg_widget_res, alignment=Qt.AlignLeft) def test_ConversationView_on_reply_sent(mocker): @@ -2164,9 +2169,11 @@ def test_ConversationView_on_reply_sent(mocker): cv = ConversationView(source, controller) cv.add_reply_from_reply_box = mocker.MagicMock() + assert cv.reply_flag is False cv.on_reply_sent(source.uuid, 'abc123', 'test message') cv.add_reply_from_reply_box.assert_called_with('abc123', 'test message') + assert cv.reply_flag is True def test_ConversationView_on_reply_sent_does_not_add_message_intended_for_different_source(mocker): @@ -2203,9 +2210,9 @@ def test_ConversationView_add_reply_from_reply_box(mocker): cv.add_reply_from_reply_box('abc123', 'test message') reply_widget.assert_called_once_with( - 'abc123', 'test message', 'PENDING', reply_ready, reply_succeeded, reply_failed) - cv.conversation_layout.addWidget.assert_called_once_with( - reply_widget_res, alignment=Qt.AlignRight) + 'abc123', 'test message', 'PENDING', reply_ready, reply_succeeded, reply_failed, 0) + cv.conversation_layout.insertWidget.assert_called_once_with( + 0, reply_widget_res, alignment=Qt.AlignRight) def test_ConversationView_add_reply(mocker, session, source): @@ -2235,7 +2242,7 @@ def test_ConversationView_add_reply(mocker, session, source): mock_reply_widget = mocker.patch('securedrop_client.gui.widgets.ReplyWidget', return_value=reply_widget_res) - cv.add_reply(reply) + cv.add_reply(reply, 0) # check that we built the widget was called with the correct args mock_reply_widget.assert_called_once_with( @@ -2244,11 +2251,12 @@ def test_ConversationView_add_reply(mocker, session, source): 'SUCCEEDED', mock_reply_ready_signal, mock_reply_succeeded_signal, - mock_reply_failed_signal) + mock_reply_failed_signal, + 0) # check that we added the correct widget to the layout - cv.conversation_layout.addWidget.assert_called_once_with( - reply_widget_res, alignment=Qt.AlignRight) + cv.conversation_layout.insertWidget.assert_called_once_with( + 0, reply_widget_res, alignment=Qt.AlignRight) def test_ConversationView_add_reply_no_content(mocker, session, source): @@ -2279,7 +2287,7 @@ def test_ConversationView_add_reply_no_content(mocker, session, source): mock_reply_widget = mocker.patch('securedrop_client.gui.widgets.ReplyWidget', return_value=reply_widget_res) - cv.add_reply(reply) + cv.add_reply(reply, 0) # check that we built the widget was called with the correct args mock_reply_widget.assert_called_once_with( @@ -2288,11 +2296,12 @@ def test_ConversationView_add_reply_no_content(mocker, session, source): 'SUCCEEDED', mock_reply_ready_signal, mock_reply_succeeded_signal, - mock_reply_failed_signal) + mock_reply_failed_signal, + 0) # check that we added the correct widget to the layout - cv.conversation_layout.addWidget.assert_called_once_with( - reply_widget_res, alignment=Qt.AlignRight) + cv.conversation_layout.insertWidget.assert_called_once_with( + 0, reply_widget_res, alignment=Qt.AlignRight) def test_ConversationView_add_downloaded_file(mocker, homedir, source, session): @@ -2315,13 +2324,13 @@ def test_ConversationView_add_downloaded_file(mocker, homedir, source, session): mocker.patch('securedrop_client.gui.widgets.QHBoxLayout.addWidget') mocker.patch('securedrop_client.gui.widgets.FileWidget.setLayout') - cv.add_file(file) + cv.add_file(file, 0) mock_label.assert_called_with('123B') # default factory filesize - assert cv.conversation_layout.addWidget.call_count == 1 + assert cv.conversation_layout.insertWidget.call_count == 1 - cal = cv.conversation_layout.addWidget.call_args_list - assert isinstance(cal[0][0][0], FileWidget) + cal = cv.conversation_layout.insertWidget.call_args_list + assert isinstance(cal[0][0][1], FileWidget) def test_ConversationView_add_not_downloaded_file(mocker, homedir, source, session): @@ -2342,11 +2351,11 @@ def test_ConversationView_add_not_downloaded_file(mocker, homedir, source, sessi mocker.patch('securedrop_client.gui.widgets.QHBoxLayout.addWidget') mocker.patch('securedrop_client.gui.widgets.FileWidget.setLayout') - cv.add_file(file) - assert cv.conversation_layout.addWidget.call_count == 1 + cv.add_file(file, 0) + assert cv.conversation_layout.insertWidget.call_count == 1 - cal = cv.conversation_layout.addWidget.call_args_list - assert isinstance(cal[0][0][0], FileWidget) + cal = cv.conversation_layout.insertWidget.call_args_list + assert isinstance(cal[0][0][1], FileWidget) def test_DeleteSourceMessageBox_init(mocker, source): @@ -2638,7 +2647,8 @@ def test_ReplyWidget_success_failure_slots(mocker): 'PENDING', mock_update_signal, mock_success_signal, - mock_failure_signal) + mock_failure_signal, + 0) # ensure we have connected the slots mock_success_signal.connect.assert_called_once_with(widget._on_reply_success) @@ -2845,11 +2855,12 @@ def test_ReplyTextEdit_set_logged_in(mocker): def test_update_conversation_maintains_old_items(mocker, session): """ - Calling update_conversation deletes and adds old items back to layout + Calling update_conversation maintains old item state / position if there's + no change to the conversation collection. """ source = factory.Source() session.add(source) - session.flush() + session.commit() file_ = factory.File(filename='1-source-doc.gpg', source=source) session.add(file_) @@ -2870,13 +2881,82 @@ def test_update_conversation_maintains_old_items(mocker, session): assert cv.conversation_layout.count() == 3 +def test_update_conversation_removes_draft_items(mocker, session): + """ + Calling update_conversation removes items that were added as drafts. + """ + source = factory.Source() + session.add(source) + send_status = factory.ReplySendStatus() + session.add(send_status) + session.commit() + + file_ = factory.File(filename='1-source-doc.gpg', source=source) + session.add(file_) + message = factory.Message(filename='2-source-msg.gpg', source=source) + session.add(message) + draft_reply = factory.DraftReply(source=source, send_status=send_status) + session.add(draft_reply) + session.commit() + + mock_get_file = mocker.MagicMock(return_value=file_) + mock_controller = mocker.MagicMock(get_file=mock_get_file) + + cv = ConversationView(source, mock_controller) + assert cv.conversation_layout.count() == 3 # precondition with draft + + # add the new message and persist + new_message = factory.Message(filename='4-source-msg.gpg', source=source) + session.add(new_message) + session.commit() + + # New message added, draft message removed. + cv.update_conversation(cv.source.collection) + assert cv.conversation_layout.count() == 3 + + +def test_update_conversation_keeps_failed_draft_items(mocker, session): + """ + Calling update_conversation keeps items that were added as drafts but which + have failed. + """ + source = factory.Source() + session.add(source) + send_status = factory.ReplySendStatus(name="FAILED") + session.add(send_status) + session.commit() + + file_ = factory.File(filename='1-source-doc.gpg', source=source) + session.add(file_) + message = factory.Message(filename='2-source-msg.gpg', source=source) + session.add(message) + draft_reply = factory.DraftReply(source=source, send_status=send_status) + session.add(draft_reply) + session.commit() + + mock_get_file = mocker.MagicMock(return_value=file_) + mock_controller = mocker.MagicMock(get_file=mock_get_file) + + cv = ConversationView(source, mock_controller) + assert cv.conversation_layout.count() == 3 # precondition with draft + + # add the new message and persist + new_message = factory.Message(filename='4-source-msg.gpg', source=source) + session.add(new_message) + session.commit() + + # New message added, draft message retained. + cv.update_conversation(cv.source.collection) + assert cv.conversation_layout.count() == 4 + + def test_update_conversation_adds_new_items(mocker, session): """ Calling update_conversation adds new items to layout """ source = factory.Source() session.add(source) - session.flush() + session.commit() file_ = factory.File(filename='1-source-doc.gpg', source=source) session.add(file_) @@ -2901,6 +2981,42 @@ def test_update_conversation_adds_new_items(mocker, session): assert cv.conversation_layout.count() == 4 +def test_update_conversation_position_updates(mocker, session): + """ + Calling update_conversation adds new items to layout + """ + source = factory.Source() + session.add(source) + session.commit() + + file_ = factory.File(filename='1-source-doc.gpg', source=source) + session.add(file_) + message = factory.Message(filename='2-source-msg.gpg', source=source) + session.add(message) + reply = factory.Reply(filename='3-source-reply.gpg', source=source) + session.add(reply) + session.commit() + + mock_get_file = mocker.MagicMock(return_value=file_) + mock_controller = mocker.MagicMock(get_file=mock_get_file) + + cv = ConversationView(source, mock_controller) + assert cv.conversation_layout.count() == 3 # precondition + + # Change the position of the Reply. + reply_widget = cv.current_messages[reply.uuid] + reply_widget.index = 1 + + # add the new message and persist + new_message = factory.Message(filename='4-source-msg.gpg', source=source) + session.add(new_message) + session.commit() + + cv.update_conversation(cv.source.collection) + assert cv.conversation_layout.count() == 4 + assert reply_widget.index == 2 # re-ordered. + + def test_update_conversation_content_updates(mocker, session): """ Subsequent calls to update_conversation update the content of the conversation_item @@ -2911,15 +3027,17 @@ def test_update_conversation_content_updates(mocker, session): mock_controller.session = session source = factory.Source() session.add(source) - session.flush() + session.commit() message = factory.Message(filename='2-source-msg.gpg', source=source, content=None) session.add(message) session.commit() cv = ConversationView(source, mock_controller) + cv.current_messages = {} # Reset! - cv.conversation_layout.addWidget = mocker.MagicMock() + cv.conversation_layout.insertWidget = mocker.MagicMock() + cv.conversation_layout.removeWidget = mocker.MagicMock() # this is the MessageWidget that __init__() would return mock_msg_widget_res = mocker.MagicMock() # mock MessageWidget so we can inspect the __init__ call to see what content @@ -2947,7 +3065,7 @@ def test_update_conversation_content_updates(mocker, session): cv.update_conversation(cv.source.collection) # Check that the widget was updated with the expected content. - assert mock_msg_widget.call_args[0][1] == expected_content + assert mock_msg_widget_res.message.setText.call_args[0][0] == expected_content def test_clear_conversation_deletes_items(mocker, homedir): @@ -2958,7 +3076,7 @@ def test_clear_conversation_deletes_items(mocker, homedir): mock_source = mocker.MagicMock() message = db.Message(uuid='uuid', content='message', filename='1-foo') cv = ConversationView(mock_source, mock_controller) - cv.add_message(message) + cv.add_message(message, 0) assert cv.conversation_layout.count() == 1 cv.clear_conversation()