From 7a06344fa4bc99e9d8e4bcc17a596724581795a4 Mon Sep 17 00:00:00 2001 From: Ro Date: Wed, 4 Sep 2024 10:23:33 -0400 Subject: [PATCH 1/5] Add "Delete Sources" (batch delete) buttin in top bar. Change MainView layout to QVBoxLayout and add inner horizontal container to accommodate inner top bar. Update strings --- client/securedrop_client/gui/main.py | 8 +- client/securedrop_client/gui/widgets.py | 140 ++++++++++++++++-- client/securedrop_client/locale/messages.pot | 6 + .../resources/css/sdclient.css | 33 +++++ 4 files changed, 177 insertions(+), 10 deletions(-) diff --git a/client/securedrop_client/gui/main.py b/client/securedrop_client/gui/main.py index 130e15146..f5146a8d7 100644 --- a/client/securedrop_client/gui/main.py +++ b/client/securedrop_client/gui/main.py @@ -29,7 +29,7 @@ from securedrop_client.db import Source, User from securedrop_client.gui.auth import LoginDialog from securedrop_client.gui.shortcuts import Shortcuts -from securedrop_client.gui.widgets import BottomPane, LeftPane, MainView +from securedrop_client.gui.widgets import BottomPane, InnerTopPane, LeftPane, MainView from securedrop_client.logic import Controller from securedrop_client.resources import load_all_fonts, load_css, load_icon @@ -63,6 +63,11 @@ def __init__( self.setWindowTitle(_("SecureDrop Client {}").format(__version__)) self.setWindowIcon(load_icon(self.icon)) + # Top Pane to hold batch actions, eventually will also hold + # search bar for keyword filtering. The Top Pane is not a top-level + # layout element, but instead is nested inside the central widget view. + self.top_pane = InnerTopPane() + # Bottom Pane to display activity and error messages self.bottom_pane = BottomPane() @@ -104,6 +109,7 @@ def setup(self, controller: Controller) -> None: views used in the UI. """ self.controller = controller + self.top_pane.setup(self.controller) self.bottom_pane.setup(self.controller) self.left_pane.setup(self, self.controller) self.main_view.setup(self.controller) diff --git a/client/securedrop_client/gui/widgets.py b/client/securedrop_client/gui/widgets.py index 715b8e3ce..604cc5508 100644 --- a/client/securedrop_client/gui/widgets.py +++ b/client/securedrop_client/gui/widgets.py @@ -55,6 +55,7 @@ QSizePolicy, QSpacerItem, QStatusBar, + QToolBar, QToolButton, QVBoxLayout, QWidget, @@ -215,14 +216,16 @@ def __init__(self) -> None: layout.setContentsMargins(0, 0, 0, 0) layout.setSpacing(0) + self.user_profile = UserProfile() self.branding_barre = QLabel() self.branding_barre.setPixmap(load_image("left_pane.svg")) - self.user_profile = UserProfile() # Hide user profile widget until user logs in self.user_profile.hide() - # Add widgets to layout + # Add widgets to layout. An improvement + # to this layout could be to set the branding barre as a + # background layout for the other elements layout.addWidget(self.user_profile) layout.addWidget(self.branding_barre) @@ -424,6 +427,113 @@ def clear_message(self) -> None: self._hide() +class InnerTopPane(QWidget): + """ + Top pane of the MainView window. This pane holds the Batch Action layout, + and eventually will hold the keyword search/filter by codename bar. + """ + + def __init__(self) -> None: + super().__init__() + self.setObjectName("InnerTopPane") + + # Use a vertical layout so that the keyword search bar can be added later + layout = QVBoxLayout(self) + layout.setContentsMargins(0, 0, 0, 0) + layout.setSpacing(0) + layout.setAlignment(Qt.AlignVCenter) + self.setLayout(layout) + self.setAttribute(Qt.WA_StyledBackground, True) + + self.batch_actions = BatchActionWidget() + layout.addWidget(self.batch_actions) + + def setup(self, controller: Controller) -> None: + self.batch_actions.setup(controller) + + +class BatchActionWidget(QWidget): + def __init__(self) -> None: + super().__init__() + + # CSS style id + self.setObjectName("BatchActionWidget") + + # Solid background colour + self.setAttribute(Qt.WA_StyledBackground, True) + layout = QHBoxLayout() + self.setLayout(layout) + + self.toolbar = BatchActionToolbar() + + layout.addWidget(self.toolbar) + layout.addStretch() + + def setup(self, controller: Controller) -> None: + self.toolbar.setup(controller) + + +class BatchActionToolbar(QToolBar): + """ + A toolbar that contains batch actions (actions that target multiple + sources in the ConversationView, and therefore don't belong in the + individual conversation menu). Currently, this widget will hold the + "Delete Sources" (batch-delete) action. + + For user-facing naming consistency, these items won't be called + "batch/bulk ", but simply " s" (eg "Delete Sources"), where + the original nomenclature comes from the individual Source overflow QAction menu + items. Each item may have a tooltip, visible on hover, that provides a more + lengthy explanation (e.g., "Delete multiple source accounts"). + """ + + def __init__(self) -> None: + super().__init__() + self.setObjectName("BatchActionToolbar") + self.setContentsMargins(0, 0, 0, 0) + + palette = QPalette() + palette.setBrush( + QPalette.Background, QBrush(Qt.NoBrush) + ) # This makes the widget transparent + self.setPalette(palette) + + # Style and attributes + self.setMovable(False) + self.setToolButtonStyle(Qt.ToolButtonStyle.ToolButtonTextBesideIcon) + + # TODO: Icon needs changing? + # TODO: set keyboard shortcut + batch_delete_action = QAction( + QIcon(load_image("delete_close.svg")), _("DELETE SOURCES"), self + ) + batch_delete_action.setObjectName("BatchActionButton") + batch_delete_action.setToolTip(_("Delete selected source accounts")) + batch_delete_action.triggered.connect(self.delete_multiple_sources) + + # TODO: Enhancement: start with action disabled via setEnabled(False), + # enable when sources are selected + self.addAction(batch_delete_action) + + def delete_multiple_sources(self) -> None: + """ + Requires logged-in session. Delete currently-selected sources. + """ + logger.debug("delete_multiple_sources triggered") + if self.controller.api is None: + self.controller.on_action_requiring_login() + else: + # TODO rm: this is a placeholder + logger.debug("Delete sources clicked") + # TODO in followup PR + # An in-memory set of Sources selected by the user to be queued for deletion will + # live in the main app. If logged in, pass those to delete confirmation dialog, + # and if the user accepts the dialog, the sources will be deleted. + + def setup(self, controller: Controller) -> None: + self.controller = controller + + class UserProfile(QLabel): """ A widget that contains user profile information and options. @@ -603,8 +713,8 @@ def _on_clicked(self) -> None: class MainView(QWidget): """ - Represents the main content of the application (containing the source list - and main context view). + Represents the main content of the application (containing the source list, + main context view, and top actions pane). """ def __init__( @@ -620,12 +730,20 @@ def __init__( self.setObjectName("MainView") # Set layout - self._layout = QHBoxLayout(self) + self._layout = QVBoxLayout(self) + self._layout.setContentsMargins(0, 0, 0, 0) + self._layout.setSpacing(0) self.setLayout(self._layout) + # Top pane layout (actions/eventual searchbar) + self.top_pane = InnerTopPane() + + # Hold main conversation view and sourcelist + inner_container = QHBoxLayout(self) + # Set margins and spacing - self._layout.setContentsMargins(0, 0, 0, 0) - self._layout.setSpacing(0) + inner_container.setContentsMargins(0, 0, 0, 0) + inner_container.setSpacing(0) # Create SourceList widget self.source_list = SourceList() @@ -647,8 +765,11 @@ def __init__( self.view_layout.addWidget(self.empty_conversation_view) # Add widgets to layout - self._layout.addWidget(self.source_list, stretch=1) - self._layout.addWidget(self.view_holder, stretch=2) + inner_container.addWidget(self.source_list, stretch=1) + inner_container.addWidget(self.view_holder, stretch=2) + + self._layout.addWidget(self.top_pane) + self._layout.addLayout(inner_container, stretch=1) # Note: We should not delete SourceConversationWrapper when its source is unselected. This # is a temporary solution to keep copies of our objects since we do delete them. @@ -660,6 +781,7 @@ def setup(self, controller: Controller) -> None: """ self.controller = controller self.source_list.setup(controller) + self.top_pane.setup(controller) def show_sources(self, sources: list[Source]) -> None: """ diff --git a/client/securedrop_client/locale/messages.pot b/client/securedrop_client/locale/messages.pot index b5f68b206..2d85f4915 100644 --- a/client/securedrop_client/locale/messages.pot +++ b/client/securedrop_client/locale/messages.pot @@ -112,6 +112,12 @@ msgstr "" msgid "Last Refresh: never" msgstr "" +msgid "DELETE SOURCES" +msgstr "" + +msgid "Delete selected source accounts" +msgstr "" + msgid "{}" msgstr "" diff --git a/client/securedrop_client/resources/css/sdclient.css b/client/securedrop_client/resources/css/sdclient.css index 5aca95c7f..b371a2eda 100644 --- a/client/securedrop_client/resources/css/sdclient.css +++ b/client/securedrop_client/resources/css/sdclient.css @@ -115,6 +115,39 @@ QWidget { background-color: #85f6fe; } +#InnerTopPane { + background-color: #f3f5f9; +} + +#BatchActionWidget { + background-color: #fff; + min-height: 24px; + margin: 2px; +} + +#BatchActionToolbar QToolButton { + font-family: 'Montserrat'; + font-weight: 600; + font-size: 12px; + padding: 2px; + color: #2a319d; + background-color: #fff; + border: 2px solid #2a319d; +} + +#BatchActionToolbar QToolButton:disabled { + color: #a5a8d8; + background-color: #9495b9; + border: 2px solid #a5a8d8; +} + +#BatchActionToolbar QToolButton:hover, +#BatchActionToolbar QToolButton:checked { + background-color: #05a6fe; + border: 2px solid #05a6fe; + color: #fff; +} + #MainView { min-height: 558; } From 5e83f074e7642f02fafc324f030ba42f8e79c587 Mon Sep 17 00:00:00 2001 From: Ro Date: Thu, 3 Oct 2024 12:08:50 -0400 Subject: [PATCH 2/5] Support Batch Delete of sources: Provide pyqtSignal and Slot for toolbar to request and receive updated list of selected sources from sourcelist. Change list selection method of SourceList to allow for multi-select. Add support for multi-source selection. Add multi-source view pane in the conversationview. Add styled icon, adjust button and sourcelist selector color to match EmptyConversationView color. Use a QStackedLayout for the Conversation Pane views instead of hiding, showing, and deleting child widgets. Update functional and integration tests to reflect new MainView QStackedLayout. Improve delete sources explanation text and extract strings. --- client/securedrop_client/gui/actions.py | 2 +- client/securedrop_client/gui/main.py | 17 +- client/securedrop_client/gui/widgets.py | 400 ++++++++++++------ client/securedrop_client/locale/messages.pot | 14 +- client/securedrop_client/logic.py | 32 +- .../resources/css/sdclient.css | 37 +- .../images/delete_sources_toolbar_icon.svg | 11 + client/tests/functional/test_delete_source.py | 12 +- client/tests/functional/test_download_file.py | 6 +- client/tests/functional/test_export_wizard.py | 6 +- .../functional/test_offline_delete_source.py | 12 +- .../test_offline_read_conversation.py | 10 +- .../functional/test_offline_send_reply.py | 11 +- .../tests/functional/test_receive_message.py | 11 +- client/tests/functional/test_send_reply.py | 10 +- client/tests/gui/test_actions.py | 9 +- client/tests/gui/test_widgets.py | 279 +++++++----- client/tests/integration/test_placeholder.py | 6 +- .../test_styles_file_download_button.py | 4 +- .../test_styles_reply_status_bar.py | 4 +- .../tests/integration/test_styles_sdclient.py | 43 +- .../test_styles_speech_bubble_message.py | 2 +- .../test_styles_speech_bubble_status_bar.py | 2 +- client/tests/test_logic.py | 6 +- 24 files changed, 638 insertions(+), 308 deletions(-) create mode 100644 client/securedrop_client/resources/images/delete_sources_toolbar_icon.svg diff --git a/client/securedrop_client/gui/actions.py b/client/securedrop_client/gui/actions.py index 0f3088cbe..2858d8c13 100644 --- a/client/securedrop_client/gui/actions.py +++ b/client/securedrop_client/gui/actions.py @@ -93,7 +93,7 @@ def __init__( # but when triggered from this menu, only applies to one source self._confirmation_dialog = confirmation_dialog(set([self.source])) self._confirmation_dialog.accepted.connect( - lambda: self.controller.delete_source(self.source) + lambda: self.controller.delete_sources(set([self.source])) ) self.triggered.connect(self.trigger) diff --git a/client/securedrop_client/gui/main.py b/client/securedrop_client/gui/main.py index f5146a8d7..b0b368a09 100644 --- a/client/securedrop_client/gui/main.py +++ b/client/securedrop_client/gui/main.py @@ -29,7 +29,7 @@ from securedrop_client.db import Source, User from securedrop_client.gui.auth import LoginDialog from securedrop_client.gui.shortcuts import Shortcuts -from securedrop_client.gui.widgets import BottomPane, InnerTopPane, LeftPane, MainView +from securedrop_client.gui.widgets import BottomPane, LeftPane, MainView from securedrop_client.logic import Controller from securedrop_client.resources import load_all_fonts, load_css, load_icon @@ -63,11 +63,6 @@ def __init__( self.setWindowTitle(_("SecureDrop Client {}").format(__version__)) self.setWindowIcon(load_icon(self.icon)) - # Top Pane to hold batch actions, eventually will also hold - # search bar for keyword filtering. The Top Pane is not a top-level - # layout element, but instead is nested inside the central widget view. - self.top_pane = InnerTopPane() - # Bottom Pane to display activity and error messages self.bottom_pane = BottomPane() @@ -80,6 +75,7 @@ def __init__( self.main_pane.setLayout(layout) self.left_pane = LeftPane() self.main_view = MainView(self.main_pane, app_state) + layout.addWidget(self.left_pane) layout.addWidget(self.main_view) @@ -109,10 +105,15 @@ def setup(self, controller: Controller) -> None: views used in the UI. """ self.controller = controller - self.top_pane.setup(self.controller) self.bottom_pane.setup(self.controller) self.left_pane.setup(self, self.controller) self.main_view.setup(self.controller) + + # Listen for changes to the selected sources in sourcelist + self.main_view.source_list.selected_sources.connect( + self.controller.on_receive_selected_sources + ) + self.show_login() def show_main_window(self, db_user: User | None = None) -> None: @@ -190,6 +191,7 @@ def set_logged_in_as(self, db_user: User) -> None: """ self.left_pane.set_logged_in_as(db_user) self.bottom_pane.set_logged_in() + self.main_view.set_logged_in() def logout(self) -> None: """ @@ -197,6 +199,7 @@ def logout(self) -> None: """ self.left_pane.set_logged_out() self.bottom_pane.set_logged_out() + self.main_view.set_logged_out() def update_sync_status(self, message: str, duration: int = 0) -> None: """ diff --git a/client/securedrop_client/gui/widgets.py b/client/securedrop_client/gui/widgets.py index 604cc5508..34aae742f 100644 --- a/client/securedrop_client/gui/widgets.py +++ b/client/securedrop_client/gui/widgets.py @@ -42,6 +42,7 @@ QResizeEvent, ) from PyQt5.QtWidgets import ( + QAbstractItemView, QAction, QGridLayout, QHBoxLayout, @@ -54,6 +55,7 @@ QScrollArea, QSizePolicy, QSpacerItem, + QStackedLayout, QStatusBar, QToolBar, QToolButton, @@ -451,6 +453,18 @@ def __init__(self) -> None: def setup(self, controller: Controller) -> None: self.batch_actions.setup(controller) + def set_logged_out(self) -> None: + """ + Disable action toolbar if logged out. + """ + self.batch_actions.toolbar.hide_action() + + def set_logged_in(self) -> None: + """ + Enable action toolbar if logged in. + """ + self.batch_actions.toolbar.show_action() + class BatchActionWidget(QWidget): def __init__(self) -> None: @@ -501,37 +515,51 @@ def __init__(self) -> None: # Style and attributes self.setMovable(False) self.setToolButtonStyle(Qt.ToolButtonStyle.ToolButtonTextBesideIcon) + self.setSizePolicy(QSizePolicy.Preferred, QSizePolicy.Fixed) - # TODO: Icon needs changing? - # TODO: set keyboard shortcut - batch_delete_action = QAction( - QIcon(load_image("delete_close.svg")), _("DELETE SOURCES"), self + self.delete_sources_action = QAction( + QIcon(load_image("delete_sources_toolbar_icon.svg")), _("DELETE SOURCES"), self ) - batch_delete_action.setObjectName("BatchActionButton") - batch_delete_action.setToolTip(_("Delete selected source accounts")) - batch_delete_action.triggered.connect(self.delete_multiple_sources) + self.delete_sources_action.setObjectName("BatchActionButton") + self.delete_sources_action.setToolTip( + _( + "Delete selected source accounts. " + "Ctrl+click sources below to select multiple sources." + ) + ) + self.delete_sources_action.triggered.connect(self.on_action_triggered) + self.button = self.widgetForAction(self.delete_sources_action) - # TODO: Enhancement: start with action disabled via setEnabled(False), - # enable when sources are selected - self.addAction(batch_delete_action) + # Add spacer. "select all" checkbox can replace the spacer in future + spacer = QWidget() + spacer.setFixedSize(14, 14) # sourcewidget spacer size + self.addWidget(spacer) + self.addAction(self.delete_sources_action) - def delete_multiple_sources(self) -> None: - """ - Requires logged-in session. Delete currently-selected sources. - """ - logger.debug("delete_multiple_sources triggered") + def setup(self, controller: Controller) -> None: + self.controller = controller + + def hide_action(self) -> None: + self.delete_sources_action.setVisible(False) + + def show_action(self) -> None: + self.delete_sources_action.setVisible(True) + + @pyqtSlot() + def on_action_triggered(self) -> None: if self.controller.api is None: self.controller.on_action_requiring_login() else: - # TODO rm: this is a placeholder - logger.debug("Delete sources clicked") - # TODO in followup PR - # An in-memory set of Sources selected by the user to be queued for deletion will - # live in the main app. If logged in, pass those to delete confirmation dialog, - # and if the user accepts the dialog, the sources will be deleted. - - def setup(self, controller: Controller) -> None: - self.controller = controller + # 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: + dialog = DeleteSourceDialog(targets) + dialog.accepted.connect(lambda: self.controller.delete_sources(targets)) + dialog.exec() + else: + # No selected sources should return an empty set, not None + logger.error("Toolbar action triggered without valid data from controller.") class UserProfile(QLabel): @@ -717,6 +745,14 @@ class MainView(QWidget): main context view, and top actions pane). """ + # Index items for StackedLayout. CONVERSATION_INDEX should remain the + # biggest int value, for future ease of caching and cleaning up additional + # optional pages (eg rendered conversations) in higher index positions + NO_SOURCES_INDEX = 0 + NOTHING_SELECTED_INDEX = 1 + MULTI_SELECTED_INDEX = 2 + CONVERSATION_INDEX = 3 + def __init__( self, parent: Optional[QWidget], @@ -735,11 +771,12 @@ def __init__( self._layout.setSpacing(0) self.setLayout(self._layout) - # Top pane layout (actions/eventual searchbar) + # Top Pane to hold batch actions, eventually will also hold + # search bar for keyword filtering self.top_pane = InnerTopPane() # Hold main conversation view and sourcelist - inner_container = QHBoxLayout(self) + inner_container = QHBoxLayout() # Set margins and spacing inner_container.setContentsMargins(0, 0, 0, 0) @@ -747,6 +784,7 @@ def __init__( # Create SourceList widget self.source_list = SourceList() + self.source_list.setSelectionMode(QAbstractItemView.ExtendedSelection) self.source_list.itemSelectionChanged.connect(self.on_source_changed) if app_state is not None: self.source_list.source_selection_changed.connect( @@ -757,12 +795,22 @@ def __init__( # Create widgets self.view_holder = QWidget() self.view_holder.setObjectName("MainView_view_holder") - self.view_layout = QVBoxLayout() + + # Layout where only one view shows at a time. Suitable for the case + # where we show either a conversation or a contextually-appropriate + # message ("Select a source...", "Nothing to see yet", etc) + self.view_layout = QStackedLayout() self.view_holder.setLayout(self.view_layout) self.view_layout.setContentsMargins(0, 0, 0, 0) self.view_layout.setSpacing(0) - self.empty_conversation_view = EmptyConversationView() - self.view_layout.addWidget(self.empty_conversation_view) + + self.view_layout.insertWidget(self.NO_SOURCES_INDEX, EmptyConversationView()) + self.view_layout.insertWidget(self.NOTHING_SELECTED_INDEX, NothingSelectedView()) + self.view_layout.insertWidget(self.MULTI_SELECTED_INDEX, MultiSelectView()) + + # Placeholder widget at the CONVERSATION_INDEX, dynamically replaced by conversation view + # as soon as a source conversation is selected + self.view_layout.insertWidget(self.CONVERSATION_INDEX, NothingSelectedView()) # Add widgets to layout inner_container.addWidget(self.source_list, stretch=1) @@ -783,25 +831,22 @@ def setup(self, controller: Controller) -> None: self.source_list.setup(controller) self.top_pane.setup(controller) + def set_logged_out(self) -> None: + """ + Logged-out context. Called by parent. + """ + self.top_pane.set_logged_out() + + def set_logged_in(self) -> None: + """ + Logged-in context. Called by parent. + """ + self.top_pane.set_logged_in() + def show_sources(self, sources: list[Source]) -> None: """ Update the sources list in the GUI with the supplied list of sources. """ - # If no sources are supplied, display the EmptyConversationView with the no-sources message. - # - # If there are sources but no source is selected in the GUI, display the - # EmptyConversationView with the no-source-selected messaging. - # - # Otherwise, hide the EmptyConversationView. - if not sources: - self.empty_conversation_view.show_no_sources_message() - self.empty_conversation_view.show() - elif not self.source_list.get_selected_source(): - self.empty_conversation_view.show_no_source_selected_message() - self.empty_conversation_view.show() - else: - self.empty_conversation_view.hide() - # If the source list in the GUI is empty, then we will run the optimized initial update. # Otherwise, do a regular source list update. if not self.source_list.source_items: @@ -812,20 +857,94 @@ def show_sources(self, sources: list[Source]) -> None: # Then call the function to remove the wrapper and its children. self.delete_conversation(source_uuid) + # Show the correct conversation pane gui element depending on + # the number of sources a) available and b) selected. + # An improved approach will be to create an `on_source_context_update` + # pyQtSlot that subscribes/listens for storage updates and calls + # `show_sources` and `show_conversation_context`. + self._on_update_conversation_context() + + def _on_update_conversation_context(self) -> None: + """ + Show the correct view type based on the number of available and selected sources. + + If there are no sources, show the empty conversation view. + If there are sources, but none are selected, show the "Select a source" view. + If there are sources and exactly one has been selected, show the conversation + with that source. + If multiple sources are selected, show the "Multiple sources selected" view. + + This method can be triggered by a click event (list index changed) or by a "sync" + event (sourcelist updated), which redraws the list. + + In the latter case, supply list[Source] and set is_redraw_event to True. + + Return number of selected sources. + """ + selected = len(self.source_list.selectedItems()) + if selected == 0 and self.source_list.count() == 0: + self.view_layout.setCurrentIndex(self.NO_SOURCES_INDEX) + elif selected == 0: + self.view_layout.setCurrentIndex(self.NOTHING_SELECTED_INDEX) + elif selected > 1: + self.view_layout.setCurrentIndex(self.MULTI_SELECTED_INDEX) + else: + # Exactly one source selected + self.view_layout.setCurrentIndex(self.CONVERSATION_INDEX) + + @pyqtSlot() def on_source_changed(self) -> None: """ - Show conversation for the selected source. + Show conversation for the selected source, or, if multiple sources are selected, + show multi select view. + """ + + selected = len(self.source_list.selectedItems()) + if selected == 1: + # One source selected; prepare the conversation widget + try: + source = self.source_list.get_selected_source() + if not source: + return + + self.controller.session.refresh(source) + + # Immediately show the selected source as seen in the UI and then make a + # request to mark source as seen. + self.source_list.source_selected.emit(source.uuid) + self.controller.mark_seen(source) + + # Get or create the SourceConversationWrapper + if source.uuid in self.source_conversations: + conversation_wrapper = self.source_conversations[source.uuid] + conversation_wrapper.conversation_view.update_conversation( # type: ignore[has-type] + source.collection + ) + else: + conversation_wrapper = SourceConversationWrapper( + source, self.controller, self._state + ) + self.source_conversations[source.uuid] = conversation_wrapper + + # Put this widget into the QStackedLayout at the correct position + self.set_conversation(conversation_wrapper) + logger.debug(f"Set conversation to the selected source with uuid: {source.uuid}") + + except sqlalchemy.exc.InvalidRequestError as e: + logger.debug(e) + + # Now show the right widget depending on the selection + self._on_update_conversation_context() + + def refresh_source_conversations(self) -> None: + """ + Refresh the selected source conversation. """ try: source = self.source_list.get_selected_source() if not source: return - self.controller.session.refresh(source) - - # Immediately show the selected source as seen in the UI and then make a request to mark - # source as seen. - self.source_list.source_selected.emit(source.uuid) self.controller.mark_seen(source) # Get or create the SourceConversationWrapper @@ -840,26 +959,6 @@ def on_source_changed(self) -> None: ) self.source_conversations[source.uuid] = conversation_wrapper - self.set_conversation(conversation_wrapper) - logger.debug(f"Set conversation to the selected source with uuid: {source.uuid}") - - except sqlalchemy.exc.InvalidRequestError as e: - logger.debug(e) - - def refresh_source_conversations(self) -> None: - """ - Refresh the selected source conversation. - """ - try: - source = self.source_list.get_selected_source() - if not source: - return - self.controller.session.refresh(source) - self.controller.mark_seen(source) - conversation_wrapper = self.source_conversations[source.uuid] - conversation_wrapper.conversation_view.update_conversation( # type: ignore[has-type] - source.collection - ) except sqlalchemy.exc.InvalidRequestError as e: logger.debug("Error refreshing source conversations: %s", e) @@ -876,21 +975,26 @@ def delete_conversation(self, source_uuid: str) -> None: except KeyError: logger.debug(f"No SourceConversationWrapper for {source_uuid} to delete") - def set_conversation(self, widget: QWidget) -> None: + def set_conversation(self, conversation: SourceConversationWrapper) -> None: """ - Update the view holder to contain the referenced widget. + Replace rendered conversation at CONVERSATION_INDEX. Does not change + QStackedLayout current index. """ - old_widget = self.view_layout.takeAt(0) + self.view_layout.insertWidget(self.CONVERSATION_INDEX, conversation) - if old_widget and old_widget.widget(): - old_widget.widget().hide() + # At the moment, we don't keep these widgets as pages in the stacked layout, + # and we store an in-memory dict of {uuids: widget}s to avoid recreating a widget every + # time a conversation is revisited. A fixed-size cache could be implemented here instead. + layoutitem = self.view_layout.itemAt(self.CONVERSATION_INDEX + 1) + if layoutitem: + self.view_layout.removeWidget(layoutitem.widget()) - self.empty_conversation_view.hide() - self.view_layout.addWidget(widget) - widget.show() +class ConversationPaneView(QWidget): + """ + Base widget element for the ConversationPane. + """ -class EmptyConversationView(QWidget): MARGIN = 30 NEWLINE_HEIGHT_PX = 35 @@ -898,17 +1002,20 @@ def __init__(self) -> None: super().__init__() self.setObjectName("EmptyConversationView") + self._layout = QVBoxLayout() + self.setContentsMargins(self.MARGIN, self.MARGIN, self.MARGIN, self.MARGIN) + self._layout.setAlignment(Qt.AlignCenter) + self.setLayout(self._layout) - # Set layout - layout = QHBoxLayout() - layout.setContentsMargins(self.MARGIN, self.MARGIN, self.MARGIN, self.MARGIN) - self.setLayout(layout) - # Create widgets - self.no_sources = QWidget() - self.no_sources.setObjectName("EmptyConversationView_no_sources") - no_sources_layout = QVBoxLayout() - self.no_sources.setLayout(no_sources_layout) +class EmptyConversationView(ConversationPaneView): + """ + Displayed in conversation pane when sourcelist is empty. + """ + + def __init__(self) -> None: + super().__init__() + no_sources_instructions = QLabel(_("Nothing to see just yet!")) no_sources_instructions.setObjectName("EmptyConversationView_instructions") no_sources_instructions.setWordWrap(True) @@ -920,16 +1027,21 @@ def __init__(self) -> None: _("This is where you will read messages, reply to sources, and work with files.") ) no_sources_instruction_details2.setWordWrap(True) - no_sources_layout.addWidget(no_sources_instructions) - no_sources_layout.addSpacing(self.NEWLINE_HEIGHT_PX) - no_sources_layout.addWidget(no_sources_instruction_details1) - no_sources_layout.addSpacing(self.NEWLINE_HEIGHT_PX) - no_sources_layout.addWidget(no_sources_instruction_details2) - - self.no_source_selected = QWidget() - self.no_source_selected.setObjectName("EmptyConversationView_no_source_selected") - no_source_selected_layout = QVBoxLayout() - self.no_source_selected.setLayout(no_source_selected_layout) + self._layout.addWidget(no_sources_instructions) + self._layout.addSpacing(self.NEWLINE_HEIGHT_PX) + self._layout.addWidget(no_sources_instruction_details1) + self._layout.addSpacing(self.NEWLINE_HEIGHT_PX) + self._layout.addWidget(no_sources_instruction_details2) + + +class NothingSelectedView(ConversationPaneView): + """ + Displayed in conversation pane when sources are present but none are selected. + """ + + def __init__(self) -> None: + super().__init__() + no_source_selected_instructions = QLabel(_("Select a source from the list, to:")) no_source_selected_instructions.setObjectName("EmptyConversationView_instructions") no_source_selected_instructions.setWordWrap(True) @@ -960,24 +1072,50 @@ def __init__(self) -> None: bullet3_layout.addWidget(bullet3_bullet) bullet3_layout.addWidget(QLabel(_("Send a response"))) bullet3_layout.addStretch() - no_source_selected_layout.addWidget(no_source_selected_instructions) - no_source_selected_layout.addSpacing(self.NEWLINE_HEIGHT_PX) - no_source_selected_layout.addWidget(bullet1) - no_source_selected_layout.addWidget(bullet2) - no_source_selected_layout.addWidget(bullet3) - no_source_selected_layout.addSpacing(self.NEWLINE_HEIGHT_PX * 4) + no_source_selected_end_instructions = QLabel( + _("Or, select multiple sources with Ctrl+click.") + ) + no_source_selected_end_instructions.setObjectName("EmptyConversationView_instructions") + no_source_selected_end_instructions.setWordWrap(True) + self._layout.addWidget(no_source_selected_instructions) + self._layout.addSpacing(self.NEWLINE_HEIGHT_PX) + self._layout.addWidget(bullet1) + self._layout.addWidget(bullet2) + self._layout.addWidget(bullet3) + self._layout.addSpacing(self.NEWLINE_HEIGHT_PX * 4) + self._layout.addWidget(no_source_selected_end_instructions) + self._layout.addSpacing(self.NEWLINE_HEIGHT_PX * 4) - # Add widgets - layout.addWidget(self.no_sources, alignment=Qt.AlignCenter) - layout.addWidget(self.no_source_selected, alignment=Qt.AlignCenter) - def show_no_sources_message(self) -> None: - self.no_sources.show() - self.no_source_selected.hide() +class MultiSelectView(ConversationPaneView): + """ + Displayed in conversation pane when multiple sources are selected. + """ - def show_no_source_selected_message(self) -> None: - self.no_sources.hide() - self.no_source_selected.show() + def __init__(self) -> None: + super().__init__() + multi_sources_instructions = QLabel(_("Multiple Sources Selected")) + multi_sources_instructions.setObjectName("EmptyConversationView_instructions") + multi_sources_instructions.setWordWrap(True) + multi_sources_instruction_details1 = QLabel( + _( + "Select or de-select sources using Ctrl+click, Shift+click, " + "or by dragging the mouse." + ) + ) + multi_sources_instruction_details1.setWordWrap(True) + multi_sources_instruction_details2 = QLabel( + _( + "Use the top toolbar to delete multiple sources at once. " + "You will be shown a confirmation dialog before anything is deleted." + ) + ) + multi_sources_instruction_details2.setWordWrap(True) + self._layout.addWidget(multi_sources_instructions) + self._layout.addSpacing(self.NEWLINE_HEIGHT_PX) + self._layout.addWidget(multi_sources_instruction_details1) + self._layout.addSpacing(self.NEWLINE_HEIGHT_PX) + self._layout.addWidget(multi_sources_instruction_details2) class SourceListWidgetItem(QListWidgetItem): @@ -1002,9 +1140,13 @@ class SourceList(QListWidget): Displays the list of sources. """ + # State machine signals source_selection_changed = pyqtSignal(state.SourceId) source_selection_cleared = pyqtSignal() + # Bulk-context signal (toolbar) + selected_sources = pyqtSignal(object) # set[Source] + NUM_SOURCES_TO_ADD_AT_A_TIME = 32 INITIAL_UPDATE_SCROLLBAR_WIDTH = 20 @@ -1202,12 +1344,29 @@ def set_snippet(self, source_uuid: str, collection_item_uuid: str, content: str) @pyqtSlot() def _on_item_selection_changed(self) -> None: - source = self.get_selected_source() - if source is not None: - self.source_selection_changed.emit(state.SourceId(source.uuid)) + """ + 0..n items may be selected. If multiple items are selected, to avoid confusion, + don't preview any individual source conversation, but instead show a contextual message. + """ + + logger.debug(f"{len(self.selectedItems())} selected") + selected = [] + for item in self.selectedItems(): + widget = self.itemWidget(item) + if isinstance(widget, SourceWidget): + selected.append(widget.source) + + # Show conversation view if one source selected + if len(selected) == 1: + source = self.get_selected_source() + if source: + self.source_selection_changed.emit(state.SourceId(source.uuid)) else: self.source_selection_cleared.emit() + # Update listeners (action toolbar) with current selection + self.selected_sources.emit(set(selected)) + class SourcePreview(SecureQLabel): PREVIEW_WIDTH_DIFFERENCE = 140 @@ -3542,9 +3701,12 @@ class SourceMenu(QMenu): This menu provides below functionality via menu actions: - 1. Delete source - - Note: At present this only supports "delete" operation. + * Delete Source + * Delete Conversation + * Download Files + * Export Transcript + * Export Conversation and Transcript + * Print Transcript """ SOURCE_MENU_CSS = load_css("source_menu.css") diff --git a/client/securedrop_client/locale/messages.pot b/client/securedrop_client/locale/messages.pot index 2d85f4915..15c8b1ec1 100644 --- a/client/securedrop_client/locale/messages.pot +++ b/client/securedrop_client/locale/messages.pot @@ -115,7 +115,7 @@ msgstr "" msgid "DELETE SOURCES" msgstr "" -msgid "Delete selected source accounts" +msgid "Delete selected source accounts. Ctrl+click sources below to select multiple sources." msgstr "" msgid "{}" @@ -148,6 +148,18 @@ msgstr "" msgid "Send a response" msgstr "" +msgid "Or, select multiple sources with Ctrl+click." +msgstr "" + +msgid "Multiple Sources Selected" +msgstr "" + +msgid "Select or de-select sources using Ctrl+click, Shift+click, or by dragging the mouse." +msgstr "" + +msgid "Use the top toolbar to delete multiple sources at once. You will be shown a confirmation dialog before anything is deleted." +msgstr "" + msgid "Deleting files and messages…" msgstr "" diff --git a/client/securedrop_client/logic.py b/client/securedrop_client/logic.py index 565774ce5..b92fa0ff3 100644 --- a/client/securedrop_client/logic.py +++ b/client/securedrop_client/logic.py @@ -417,6 +417,9 @@ def __init__( # type: ignore[no-untyped-def] ): os.chmod(self.last_sync_filepath, 0o600) + # Store currently-selected sources + self._selected_sources: set[db.Source] | None = None + @pyqtSlot(int) def _on_main_queue_updated(self, num_items: int) -> None: if num_items > 0: @@ -607,6 +610,9 @@ def login_offline_mode(self) -> None: # may have attempted online mode login, then switched to offline) self.gui.clear_clipboard() self.gui.show_main_window() + + # All child elements of main window should show logged_out state + self.gui.logout() self.update_sources() def on_action_requiring_login(self) -> None: @@ -1044,21 +1050,22 @@ def on_delete_source_failure(self, e: Exception) -> None: self.source_deletion_failed.emit(e.source_uuid) @login_required - def delete_source(self, source: db.Source) -> None: + def delete_sources(self, sources: set[db.Source]) -> None: """ - Performs a delete operation on source record. + Performs a delete operation on one or more source records. - This method will submit a job to delete the source record on - the server. If the job succeeds, the success handler will + This method will submit a job to delete each target source record on + the server. If a given job succeeds, the success handler will synchronize the server records with the local state. If not, the failure handler will display an error. """ - job = DeleteSourceJob(source.uuid) - job.success_signal.connect(self.on_delete_source_success) - job.failure_signal.connect(self.on_delete_source_failure) + for source in sources: + job = DeleteSourceJob(source.uuid) + job.success_signal.connect(self.on_delete_source_success) + job.failure_signal.connect(self.on_delete_source_failure) - self.add_job.emit(job) - self.source_deleted.emit(source.uuid) + self.add_job.emit(job) + self.source_deleted.emit(source.uuid) @login_required def delete_conversation(self, source: db.Source) -> None: @@ -1183,3 +1190,10 @@ def update_failed_replies(self) -> None: failed_replies = storage.mark_all_pending_drafts_as_failed(self.session) for failed_reply in failed_replies: self.reply_failed.emit(failed_reply.uuid) + + @pyqtSlot(object) + def on_receive_selected_sources(self, sources: set[db.Source]) -> None: + self._selected_sources = sources + + def get_selected_sources(self) -> set[db.Source] | None: + return self._selected_sources diff --git a/client/securedrop_client/resources/css/sdclient.css b/client/securedrop_client/resources/css/sdclient.css index b371a2eda..a60b60e8e 100644 --- a/client/securedrop_client/resources/css/sdclient.css +++ b/client/securedrop_client/resources/css/sdclient.css @@ -121,30 +121,36 @@ QWidget { #BatchActionWidget { background-color: #fff; - min-height: 24px; margin: 2px; } +#BatchActionToolbar { + min-height: 42px; +} + +#BatchActionToolbar QToolButton:disabled { + color: #f9f9ff; + background-color: #a5b3e9; + border: 2px solid #a5b3e9; +} + #BatchActionToolbar QToolButton { font-family: 'Montserrat'; font-weight: 600; font-size: 12px; padding: 2px; - color: #2a319d; + color: #a5b3e9; background-color: #fff; - border: 2px solid #2a319d; -} - -#BatchActionToolbar QToolButton:disabled { - color: #a5a8d8; - background-color: #9495b9; - border: 2px solid #a5a8d8; + border: 2px solid #a5b3e9; + border-radius: 4%; + margin-left: 2px; + margin-right: 2px; } #BatchActionToolbar QToolButton:hover, #BatchActionToolbar QToolButton:checked { - background-color: #05a6fe; - border: 2px solid #05a6fe; + background-color: #a5b3e9; + border: 2px solid #a5b3e9; color: #fff; } @@ -164,7 +170,7 @@ QWidget { #EmptyConversationView QLabel { font-family: Montserrat; font-weight: 400; - font-size: 35px; + font-size: 30px; color: #a5b3e9; } @@ -174,7 +180,7 @@ QWidget { #EmptyConversationView QLabel#EmptyConversationView_bullet { margin: 4px 0px 0px 0px; - font-size: 35px; + font-size: 30px; font-weight: 600; } @@ -187,11 +193,12 @@ QListView#SourceList { } QListView#SourceList::item:selected { - background-color: rgba(244, 245, 255, 0.84); + /* #a5b3e94d aka #e4e8f8 */ + background-color: rgba(165, 179, 233, 0.30); } QListView#SourceList::item:hover { - background-color: #f9f9ff; + background-color: #e4e8f8; } #SourceWidget_container { diff --git a/client/securedrop_client/resources/images/delete_sources_toolbar_icon.svg b/client/securedrop_client/resources/images/delete_sources_toolbar_icon.svg new file mode 100644 index 000000000..8ab2687b4 --- /dev/null +++ b/client/securedrop_client/resources/images/delete_sources_toolbar_icon.svg @@ -0,0 +1,11 @@ + + + + Delete-X + Created with Sketch. + + + + + + \ No newline at end of file diff --git a/client/tests/functional/test_delete_source.py b/client/tests/functional/test_delete_source.py index e368797ba..5c26bfcdc 100644 --- a/client/tests/functional/test_delete_source.py +++ b/client/tests/functional/test_delete_source.py @@ -8,6 +8,7 @@ from flaky import flaky from PyQt5.QtCore import Qt +from securedrop_client.gui.widgets import SourceConversationWrapper from tests.conftest import TIME_CLICK_ACTION, TIME_RENDER_CONV_VIEW, TIME_RENDER_SOURCE_LIST @@ -30,18 +31,21 @@ def check_for_sources(): qtbot.wait(TIME_CLICK_ACTION) def check_for_conversation(): - assert gui.main_view.view_layout.itemAt(0) - assert gui.main_view.view_layout.itemAt(0).widget() + assert gui.main_view.view_layout.currentIndex() == gui.main_view.CONVERSATION_INDEX + assert isinstance( + gui.main_view.view_layout.widget(gui.main_view.view_layout.currentIndex()), + SourceConversationWrapper, + ) # Get the selected source conversation qtbot.waitUntil(check_for_conversation, timeout=TIME_RENDER_CONV_VIEW) - conversation = gui.main_view.view_layout.itemAt(0).widget() + conversation = gui.main_view.view_layout.widget(gui.main_view.view_layout.currentIndex()) # Delete the selected source # Note: The qtbot object cannot interact with QAction items (as used in the delete button/menu) # so we programatically delete the source rather than using the GUI via qtbot source_count = gui.main_view.source_list.count() - controller.delete_source(conversation.conversation_title_bar.source) + controller.delete_sources(set([conversation.conversation_title_bar.source])) def check_source_list(): assert gui.main_view.source_list.count() == source_count - 1 diff --git a/client/tests/functional/test_download_file.py b/client/tests/functional/test_download_file.py index df31d8ccd..40701305d 100644 --- a/client/tests/functional/test_download_file.py +++ b/client/tests/functional/test_download_file.py @@ -40,7 +40,8 @@ def check_for_sources(): def conversation_with_file_is_rendered(): assert gui.main_view.view_layout.itemAt(0) - conversation = gui.main_view.view_layout.itemAt(0).widget() + 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] @@ -48,7 +49,8 @@ def conversation_with_file_is_rendered(): # 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() + conversation = gui.main_view.view_layout.widget(gui.main_view.view_layout.currentIndex()) + file_id = list(conversation.conversation_view.current_messages.keys())[2] file_widget = conversation.conversation_view.current_messages[file_id] diff --git a/client/tests/functional/test_export_wizard.py b/client/tests/functional/test_export_wizard.py index a1351bbdd..8d084489c 100644 --- a/client/tests/functional/test_export_wizard.py +++ b/client/tests/functional/test_export_wizard.py @@ -49,14 +49,16 @@ def check_for_sources(): def conversation_with_file_is_rendered(): assert gui.main_view.view_layout.itemAt(0) - conversation = gui.main_view.view_layout.itemAt(0).widget() + conversation = gui.main_view.view_layout.widget(gui.main_view.view_layout.currentIndex()) + assert isinstance(conversation, SourceConversationWrapper) file_widget = list(conversation.conversation_view.current_messages.values())[2] 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() + conversation = gui.main_view.view_layout.widget(gui.main_view.view_layout.currentIndex()) + file_widget = list(conversation.conversation_view.current_messages.values())[2] # If the file is not downloaded, click on the download button diff --git a/client/tests/functional/test_offline_delete_source.py b/client/tests/functional/test_offline_delete_source.py index 1a108752e..25eca0f61 100644 --- a/client/tests/functional/test_offline_delete_source.py +++ b/client/tests/functional/test_offline_delete_source.py @@ -8,6 +8,7 @@ from flaky import flaky from PyQt5.QtCore import Qt +from securedrop_client.gui.widgets import SourceConversationWrapper from tests.conftest import TIME_CLICK_ACTION, TIME_RENDER_CONV_VIEW, TIME_RENDER_SOURCE_LIST @@ -31,17 +32,20 @@ def check_for_sources(): qtbot.wait(TIME_CLICK_ACTION) def check_for_conversation(): - assert gui.main_view.view_layout.itemAt(0) - assert gui.main_view.view_layout.itemAt(0).widget() + assert gui.main_view.view_layout.currentIndex() == gui.main_view.CONVERSATION_INDEX + assert isinstance( + gui.main_view.view_layout.widget(gui.main_view.view_layout.currentIndex()), + SourceConversationWrapper, + ) # Get the selected source conversation qtbot.waitUntil(check_for_conversation, timeout=TIME_RENDER_CONV_VIEW) - conversation = gui.main_view.view_layout.itemAt(0).widget() + conversation = gui.main_view.view_layout.widget(gui.main_view.view_layout.currentIndex()) # Attempt to delete the selected source # Note: The qtbot object cannot interact with QAction items (as used in the delete button/menu) # so we programatically attempt to delete the source rather than using the GUI via qtbot - controller.delete_source(conversation.conversation_title_bar.source) + controller.delete_sources(set([conversation.conversation_title_bar.source])) def check_for_error(): msg = gui.bottom_pane.error_status_bar.status_bar.currentMessage() diff --git a/client/tests/functional/test_offline_read_conversation.py b/client/tests/functional/test_offline_read_conversation.py index 21d6e291a..7fcb44fc9 100644 --- a/client/tests/functional/test_offline_read_conversation.py +++ b/client/tests/functional/test_offline_read_conversation.py @@ -8,6 +8,7 @@ from flaky import flaky from PyQt5.QtCore import Qt +from securedrop_client.gui.widgets import SourceConversationWrapper from tests.conftest import TIME_CLICK_ACTION, TIME_RENDER_CONV_VIEW, TIME_RENDER_SOURCE_LIST @@ -30,12 +31,15 @@ def check_for_sources(): qtbot.wait(TIME_CLICK_ACTION) def check_for_conversation(): - assert gui.main_view.view_layout.itemAt(0) - assert gui.main_view.view_layout.itemAt(0).widget() + assert gui.main_view.view_layout.currentIndex() == gui.main_view.CONVERSATION_INDEX + assert isinstance( + gui.main_view.view_layout.widget(gui.main_view.view_layout.currentIndex()), + SourceConversationWrapper, + ) # Get the selected source conversation qtbot.waitUntil(check_for_conversation, timeout=TIME_RENDER_CONV_VIEW) - conversation = gui.main_view.view_layout.itemAt(0).widget() + conversation = gui.main_view.view_layout.widget(gui.main_view.view_layout.currentIndex()) # Verify that the conversation widgets exist assert len(list(conversation.conversation_view.current_messages.keys())) > 0 diff --git a/client/tests/functional/test_offline_send_reply.py b/client/tests/functional/test_offline_send_reply.py index f63e24c46..bec6a871b 100644 --- a/client/tests/functional/test_offline_send_reply.py +++ b/client/tests/functional/test_offline_send_reply.py @@ -8,6 +8,7 @@ from flaky import flaky from PyQt5.QtCore import Qt +from securedrop_client.gui.widgets import SourceConversationWrapper from tests.conftest import TIME_CLICK_ACTION, TIME_RENDER_CONV_VIEW, TIME_RENDER_SOURCE_LIST @@ -30,12 +31,16 @@ def check_for_sources(): qtbot.wait(TIME_CLICK_ACTION) def check_for_conversation(): - assert gui.main_view.view_layout.itemAt(0) - assert gui.main_view.view_layout.itemAt(0).widget() + assert gui.main_view.view_layout.currentIndex() == gui.main_view.CONVERSATION_INDEX + assert isinstance( + gui.main_view.view_layout.widget(gui.main_view.view_layout.currentIndex()), + SourceConversationWrapper, + ) # Get the selected source conversation qtbot.waitUntil(check_for_conversation, timeout=TIME_RENDER_CONV_VIEW) - conversation = gui.main_view.view_layout.itemAt(0).widget() + + conversation = gui.main_view.view_layout.widget(gui.main_view.view_layout.currentIndex()) assert not conversation.reply_box.text_edit.isEnabled() assert not conversation.reply_box.send_button.isVisible() diff --git a/client/tests/functional/test_receive_message.py b/client/tests/functional/test_receive_message.py index ea39c7abd..88f6220a4 100644 --- a/client/tests/functional/test_receive_message.py +++ b/client/tests/functional/test_receive_message.py @@ -8,6 +8,7 @@ from flaky import flaky from PyQt5.QtCore import Qt +from securedrop_client.gui.widgets import SourceConversationWrapper from tests.conftest import TIME_CLICK_ACTION, TIME_RENDER_CONV_VIEW, TIME_RENDER_SOURCE_LIST @@ -32,12 +33,16 @@ def check_for_sources(): qtbot.wait(TIME_CLICK_ACTION) def check_for_conversation(): - assert gui.main_view.view_layout.itemAt(0) - assert gui.main_view.view_layout.itemAt(0).widget() + assert gui.main_view.view_layout.currentIndex() == gui.main_view.CONVERSATION_INDEX + assert isinstance( + gui.main_view.view_layout.widget(gui.main_view.view_layout.currentIndex()), + SourceConversationWrapper, + ) # Get the selected source conversation qtbot.waitUntil(check_for_conversation, timeout=TIME_RENDER_CONV_VIEW) - conversation = gui.main_view.view_layout.itemAt(0).widget() + conversation = gui.main_view.view_layout.widget(gui.main_view.view_layout.currentIndex()) + message_id = list(conversation.conversation_view.current_messages.keys())[0] message = conversation.conversation_view.current_messages[message_id] diff --git a/client/tests/functional/test_send_reply.py b/client/tests/functional/test_send_reply.py index ab05bd96a..ba889e476 100644 --- a/client/tests/functional/test_send_reply.py +++ b/client/tests/functional/test_send_reply.py @@ -8,6 +8,7 @@ from flaky import flaky from PyQt5.QtCore import Qt +from securedrop_client.gui.widgets import SourceConversationWrapper from tests.conftest import TIME_CLICK_ACTION, TIME_RENDER_CONV_VIEW, TIME_RENDER_SOURCE_LIST @@ -30,12 +31,15 @@ def check_for_sources(): qtbot.wait(TIME_CLICK_ACTION) def check_for_conversation(): - assert gui.main_view.view_layout.itemAt(0) - assert gui.main_view.view_layout.itemAt(0).widget() + assert gui.main_view.view_layout.currentIndex() == gui.main_view.CONVERSATION_INDEX + assert isinstance( + gui.main_view.view_layout.widget(gui.main_view.view_layout.currentIndex()), + SourceConversationWrapper, + ) # Get the selected source conversation qtbot.waitUntil(check_for_conversation, timeout=TIME_RENDER_CONV_VIEW) - conversation = gui.main_view.view_layout.itemAt(0).widget() + conversation = gui.main_view.view_layout.widget(gui.main_view.view_layout.currentIndex()) item_count = len(list(conversation.conversation_view.current_messages.keys())) # Focus on the reply box and type a message diff --git a/client/tests/gui/test_actions.py b/client/tests/gui/test_actions.py index aa7a3cdba..25bb7b955 100644 --- a/client/tests/gui/test_actions.py +++ b/client/tests/gui/test_actions.py @@ -102,7 +102,10 @@ def test_deletes_source_when_dialog_accepted(self): self.action.trigger() - self._controller.delete_source.assert_called_once_with(self._source) + self._controller.delete_sources.assert_called_once() + assert ( + self._source in self._controller.delete_sources.call_args[0][0] + ), self._controller.delete_sources.call_args[0][0] def test_does_not_delete_source_when_dialog_rejected(self): # Reject the confirmation dialog from a separate thread. @@ -110,7 +113,7 @@ def test_does_not_delete_source_when_dialog_rejected(self): self.action.trigger() - assert not self._controller.delete_source.called + assert not self._controller.delete_sources.called def test_requires_authenticated_journalist(self): controller = mock.MagicMock(Controller, api=None) # no authenticated user @@ -122,7 +125,7 @@ def test_requires_authenticated_journalist(self): self.action.trigger() assert not confirmation_dialog.exec.called - assert not controller.delete_source.called + assert not controller.delete_sources.called controller.on_action_requiring_login.assert_called_once() diff --git a/client/tests/gui/test_widgets.py b/client/tests/gui/test_widgets.py index f73abc34e..5579d1937 100644 --- a/client/tests/gui/test_widgets.py +++ b/client/tests/gui/test_widgets.py @@ -3,7 +3,7 @@ """ import math -from datetime import datetime +from datetime import datetime, timedelta from gettext import gettext as _ from unittest.mock import Mock, PropertyMock @@ -31,6 +31,8 @@ LoginButton, MainView, MessageWidget, + MultiSelectView, + NothingSelectedView, ReplyBoxWidget, ReplyTextEdit, ReplyTextEditPlaceholder, @@ -533,21 +535,17 @@ def test_MainView_show_sources_with_none_selected(mocker): # Set up SourceList so that SourceList.get_selected_source() returns a source mv.source_list = SourceList() + mv.source_list.controller = mocker.MagicMock() source_widget = SourceWidget( mocker.MagicMock(), factory.Source(uuid="stub_uuid"), mocker.MagicMock(), mocker.MagicMock() ) source_item = SourceListWidgetItem(mv.source_list) mv.source_list.setItemWidget(source_item, source_widget) mv.source_list.source_items["stub_uuid"] = source_item - mocker.patch.object(mv.source_list, "update_sources") - mv.empty_conversation_view = mocker.MagicMock() - - mv.show_sources([1, 2, 3]) + mv.show_sources([factory.Source(), factory.Source(), factory.Source()]) - mv.source_list.update_sources.assert_called_once_with([1, 2, 3]) - mv.empty_conversation_view.show_no_source_selected_message.assert_called_once_with() - mv.empty_conversation_view.show.assert_called_once_with() + assert mv.view_layout.currentIndex() == mv.NOTHING_SELECTED_INDEX def test_MainView_show_sources_from_cold_start(mocker): @@ -570,13 +568,16 @@ def test_MainView_show_sources_with_no_sources_at_all(mocker): """ mv = MainView(None) mv.source_list = mocker.MagicMock() - mv.empty_conversation_view = mocker.MagicMock() + mv.source_list.count = mocker.MagicMock(return_value=0) + mv.source_list.selectedItems = mocker.MagicMock(return_value=[]) + + mv._on_update_conversation_context = mocker.MagicMock(wraps=mv._on_update_conversation_context) mv.show_sources([]) mv.source_list.update_sources.assert_called_once_with([]) - mv.empty_conversation_view.show_no_sources_message.assert_called_once_with() - mv.empty_conversation_view.show.assert_called_once_with() + mv._on_update_conversation_context.assert_called() + assert mv.view_layout.currentIndex() == mv.NO_SOURCES_INDEX def test_MainView_show_sources_when_sources_are_deleted(mocker): @@ -584,18 +585,20 @@ def test_MainView_show_sources_when_sources_are_deleted(mocker): Ensure that show_sources also deletes the SourceConversationWrapper for a deleted source. """ mv = MainView(None) + sources = [factory.Source(), factory.Source(), factory.Source(), factory.Source()] mv.source_list = mocker.MagicMock() - mv.empty_conversation_view = mocker.MagicMock() + mv.source_list.count = mocker.MagicMock(return_value=len(sources)) + mv.source_list.getSelectedItems = mocker.MagicMock(return_value=[]) mv.source_list.update_sources = mocker.MagicMock(return_value=[]) mv.delete_conversation = mocker.MagicMock() - mv.show_sources([1, 2, 3, 4]) + mv.show_sources(sources) - mv.source_list.update_sources = mocker.MagicMock(return_value=[4]) + mv.source_list.update_sources = mocker.MagicMock(return_value=[sources[-1]]) - mv.show_sources([1, 2, 3]) + mv.show_sources(sources[:-1]) - mv.delete_conversation.assert_called_once_with(4) + mv.delete_conversation.assert_called_once_with(sources[-1]) def test_MainView_delete_conversation_when_conv_wrapper_exists(mocker): @@ -635,17 +638,66 @@ def test_MainView_on_source_changed(mocker): """ mv = MainView(None) mv.set_conversation = mocker.MagicMock() + source = factory.Source() mv.source_list = mocker.MagicMock() - mv.source_list.get_selected_source = mocker.MagicMock(return_value=factory.Source()) + mv.source_list.get_selected_source = mocker.MagicMock(return_value=source) + mv.source_list.selectedItems = mocker.MagicMock(return_value=[source]) + mv.source_list.count = mocker.MagicMock(return_value=3) mv.controller = mocker.MagicMock(is_authenticated=True) mocker.patch("securedrop_client.gui.widgets.source_exists", return_value=True) - scw = mocker.MagicMock() - mocker.patch("securedrop_client.gui.widgets.SourceConversationWrapper", return_value=scw) - mv.on_source_changed() mv.source_list.get_selected_source.assert_called_once_with() - mv.set_conversation.assert_called_once_with(scw) + mv.set_conversation.assert_called_once() + assert mv.set_conversation.call_args[0][0].source == source + + +def test_MainView_on_source_changed_shows_correct_context(mocker, homedir, session, session_maker): + """ + Ensure correct context presented based on number of sources selected. + """ + # Build sourcelist + sources = [] + for i in range(10): + s = factory.Source() + sources.append(s) + session.add(s) + + session.commit() + + mv = MainView(None) + + mock_gui = mocker.MagicMock() + controller = logic.Controller("http://localhost", mock_gui, session_maker, homedir, None) + controller.api = mocker.MagicMock() + controller.session.refresh = mocker.MagicMock() + + mv.setup(controller) + mv.show() + + assert mv.view_layout.currentIndex() == mv.NO_SOURCES_INDEX + + mv.source_list.update_sources(sources) + mv.show_sources(sources) + + assert mv.view_layout.currentIndex() == mv.NOTHING_SELECTED_INDEX + + # Select a source, ensure the correct view context is shown + mv.source_list.setCurrentRow(0) + assert mv.view_layout.currentIndex() == mv.CONVERSATION_INDEX + + mv.source_list.setCurrentRow(1) + + assert mv.view_layout.currentIndex() == mv.CONVERSATION_INDEX + assert ( + mv.controller.session.refresh.call_args[0][0] + == mv.source_list.itemWidget(mv.source_list.item(1)).source + ) + + # Now ensure the "multiple sources selected" view is shown + mv.source_list.selectAll() + + assert mv.view_layout.currentIndex() == mv.MULTI_SELECTED_INDEX def test_MainView_on_source_changed_does_not_raise_InvalidRequestError(mocker): @@ -655,21 +707,24 @@ def test_MainView_on_source_changed_does_not_raise_InvalidRequestError(mocker): """ mv = MainView(None) mv.set_conversation = mocker.MagicMock() - mv.source_list = mocker.MagicMock() - mv.source_list.get_selected_source = mocker.MagicMock(return_value=factory.Source()) + source = factory.Source() + mv.source_list.count = mocker.MagicMock(return_value=1) + mv.source_list.get_selected_source = mocker.MagicMock(return_value=source) + mv.source_list.selectedItems = mocker.MagicMock(return_value=[source]) mv.controller = mocker.MagicMock(is_authenticated=True) ex = sqlalchemy.exc.InvalidRequestError() - mv.controller.session.refresh.side_effect = ex + mv.controller.session = mocker.MagicMock() + mv.controller.session.refresh = mocker.MagicMock(side_effect=ex) mocker.patch("securedrop_client.gui.widgets.source_exists", return_value=True) - scw = mocker.MagicMock() - mocker.patch("securedrop_client.gui.widgets.SourceConversationWrapper", return_value=scw) mock_logger = mocker.MagicMock() + mocker.patch("securedrop_client.gui.widgets.logger", mock_logger) mv.on_source_changed() + mv.controller.session.refresh.assert_called() - assert mock_logger.debug.call_count == 1 + assert mock_logger.debug.call_count == 1, mock_logger.debug.call_args def test_MainView_on_source_changed_when_source_no_longer_exists(mocker): @@ -679,11 +734,12 @@ def test_MainView_on_source_changed_when_source_no_longer_exists(mocker): mv = MainView(None) mv.set_conversation = mocker.MagicMock() mv.source_list = mocker.MagicMock() + mv.source_list.selectedItems = mocker.MagicMock() mv.source_list.get_selected_source = mocker.MagicMock(return_value=None) mv.on_source_changed() - mv.source_list.get_selected_source.assert_called_once_with() + assert mv.view_layout.currentIndex() == mv.NOTHING_SELECTED_INDEX mv.set_conversation.assert_not_called() @@ -692,9 +748,13 @@ def test_MainView_on_source_changed_updates_conversation_view(mocker, session): Test that the source collection is displayed in the conversation view. """ mv = MainView(None) + source = factory.Source() + # mv.source_list = mocker.MagicMock() + mv.source_list.count = mocker.MagicMock(return_value=1) + mv.source_list.selectedItems = mocker.MagicMock(return_value=[source]) + mv.source_list.get_selected_source = mocker.MagicMock(return_value=source) mv.controller = mocker.MagicMock(is_authenticated=True) - source = factory.Source() session.add(source) file = factory.File(source=source, filename="0-mock-doc.gpg") message = factory.Message(source=source, filename="0-mock-msg.gpg") @@ -704,9 +764,6 @@ def test_MainView_on_source_changed_updates_conversation_view(mocker, session): session.add(reply) session.commit() source_selected = mocker.patch("securedrop_client.gui.widgets.SourceList.source_selected") - mocker.patch( - "securedrop_client.gui.widgets.SourceList.get_selected_source", return_value=source - ) add_message_fn = mocker.patch( "securedrop_client.gui.widgets.ConversationView.add_message", new=mocker.Mock() ) @@ -732,56 +789,54 @@ def test_MainView_on_source_changed_SourceConversationWrapper_is_preserved(mocke SourceConversationWrapper when we click away from a given source. We should create it the first time, and then it should persist. """ - mv = MainView(None) - mv.set_conversation = mocker.MagicMock() - source_selected = mocker.patch("securedrop_client.gui.widgets.SourceList.source_selected") - mv.controller = mocker.MagicMock(is_authenticated=True) source = factory.Source() source2 = factory.Source() session.add(source) session.add(source2) session.commit() - source_conversation_init = mocker.patch( - "securedrop_client.gui.widgets.SourceConversationWrapper.__init__", return_value=None - ) + mv = MainView(None) + mv.source_list.count = mocker.MagicMock(return_value=2) + mv.source_list.source_selected = mocker.MagicMock() - # We expect on the first call, SourceConversationWrapper.__init__ should be called. - mocker.patch( - "securedrop_client.gui.widgets.SourceList.get_selected_source", return_value=source + # Side effect == first one return value, then the second return value. + # Simulate repeated calls + mv.source_list.get_selected_source = mocker.MagicMock(side_effect=[source, source2, source]) + + # Called twice per redraw event + mv.source_list.selectedItems = mocker.MagicMock( + side_effect=[[source], [source], [source2], [source2], [source], [source]] ) + mv.set_conversation = mocker.MagicMock(wraps=mv.set_conversation) + + mv.controller = mocker.MagicMock(is_authenticated=True) + mv.on_source_changed() assert mv.set_conversation.call_count == 1 - assert source_conversation_init.call_count == 1 - source_selected.emit.assert_called_once_with(source.uuid) + assert source.uuid in mv.source_conversations + # We haven't created this widget yet since the conversation hasn't been clicked yet + assert source2.uuid not in mv.source_conversations + mv.source_list.source_selected.emit.assert_called_once_with(source.uuid) # Reset mocked objects for the next call of on_source_changed. - source_conversation_init.reset_mock() mv.set_conversation.reset_mock() - source_selected.reset_mock() + mv.source_list.source_selected.reset_mock() - # Now click on another source (source2). Since this is the first time we have clicked - # on source2, we expect on the first call, SourceConversationWrapper.__init__ should be - # called. - mocker.patch( - "securedrop_client.gui.widgets.SourceList.get_selected_source", return_value=source2 - ) + # Now click on another source (source2) ensure correct widget is constructed. mv.on_source_changed() assert mv.set_conversation.call_count == 1 - assert source_conversation_init.call_count == 1 - source_selected.emit.assert_called_once_with(source2.uuid) + assert source.uuid in mv.source_conversations + assert source2.uuid in mv.source_conversations + mv.source_list.source_selected.emit.assert_called_once_with(source2.uuid) # Reset mocked objects for the next call of on_source_changed. - source_conversation_init.reset_mock() mv.set_conversation.reset_mock() - source_selected.reset_mock() + mv.source_list.source_selected.reset_mock() # But if we click back (call on_source_changed again) to the source, # its SourceConversationWrapper should _not_ be recreated. - mocker.patch( - "securedrop_client.gui.widgets.SourceList.get_selected_source", return_value=source - ) conversation_wrapper = mv.source_conversations[source.uuid] + assert conversation_wrapper is not None conversation_wrapper.conversation_view = mocker.MagicMock() conversation_wrapper.conversation_view.update_conversation = mocker.MagicMock() @@ -791,8 +846,7 @@ def test_MainView_on_source_changed_SourceConversationWrapper_is_preserved(mocke # Conversation should be redrawn even for existing source (bug #467). assert conversation_wrapper.conversation_view.update_conversation.call_count == 1 - assert source_conversation_init.call_count == 0 - source_selected.emit.assert_called_once_with(source.uuid) + mv.source_list.source_selected.emit.assert_called_once_with(source.uuid) def test_MainView_refresh_source_conversations(homedir, mocker, qtbot, session_maker, session): @@ -802,7 +856,8 @@ def test_MainView_refresh_source_conversations(homedir, mocker, qtbot, session_m source1 = factory.Source(uuid="rsc-123") session.add(source1) - source2 = factory.Source(uuid="rsc-456") + # Less recent update time (default is datetime.now()) + source2 = factory.Source(uuid="rsc-456", last_updated=(datetime.now() - timedelta(days=1))) session.add(source2) session.commit() @@ -819,29 +874,32 @@ def test_MainView_refresh_source_conversations(homedir, mocker, qtbot, session_m mv.source_list.update_sources(sources) mv.show() - # get the conversations created - mocker.patch( - "securedrop_client.gui.widgets.SourceList.get_selected_source", return_value=source1 + # Inspect + mv.on_source_changed = mocker.MagicMock(wraps=mv.on_source_changed) + mv.source_list.itemSelectionChanged = mocker.MagicMock( + wraps=mv.source_list.itemSelectionChanged ) - mv.on_source_changed() - mocker.patch( - "securedrop_client.gui.widgets.SourceList.get_selected_source", return_value=source2 - ) - mv.on_source_changed() + # Select one source, then another + mv.source_list.setCurrentRow(0) + + assert mv.source_list.get_selected_source() == source1 + # assert mv.on_source_changed.call_count == 1 + mv.source_list.setCurrentRow(1) + assert mv.source_list.get_selected_source() == source2 + # assert mv.on_source_changed.call_count == 2 assert len(mv.source_conversations) == 2 + # Nothing selected + mv.source_list.setCurrentRow(-1) + # refresh with no source selected - mocker.patch("securedrop_client.gui.widgets.SourceList.get_selected_source", return_value=None) mv.refresh_source_conversations() + assert mv.view_layout.currentIndex() == mv.NOTHING_SELECTED_INDEX - # refresh with source1 selected while its conversation is being deleted - mocker.patch( - "securedrop_client.gui.widgets.SourceList.get_selected_source", return_value=source1 - ) - mv.on_source_changed() - + # # refresh with source1 selected while its conversation is being deleted + mv.source_list.setCurrentRow(0) assert len(mv.source_list.source_items) == 2 scw1 = mv.source_conversations[source1.uuid] @@ -931,31 +989,58 @@ def test_MainView_set_conversation(mocker): (i.e. that area of the screen on the right hand side). """ mv = MainView(None) - mv.view_layout = mocker.MagicMock() - mock_widget = mocker.MagicMock() - mv.set_conversation(mock_widget) + scw = SourceConversationWrapper(factory.Source(), mocker.MagicMock()) + mv.set_conversation(scw) - mv.view_layout.takeAt.assert_called_once_with(0) - mv.view_layout.addWidget.assert_called_once_with(mock_widget) + assert mv.view_layout.widget(mv.CONVERSATION_INDEX) == scw -def test_EmptyConversationView_show_no_sources_message(mocker): - ecv = EmptyConversationView() +def test_EmptyConversationView(mocker): + mv = MainView(None) + mv.source_list = mocker.MagicMock() + mv.source_list.count = mocker.MagicMock(return_value=0) + mv.source_list.selectedItems = mocker.MagicMock(return_value=[]) + mv.show() + assert mv.view_layout.count() == 4 # Sanity - are all the pages there? + mv.show_sources([]) + assert mv.view_layout.currentIndex() == mv.NO_SOURCES_INDEX + assert isinstance(mv.view_layout.widget(mv.view_layout.currentIndex()), EmptyConversationView) - ecv.show_no_sources_message() - assert not ecv.no_sources.isHidden() - assert ecv.no_source_selected.isHidden() +def test_NothingSelectedView(mocker): + mv = MainView(None) + mv.show() + mv.source_list = mocker.MagicMock() + mv.source_list.count = mocker.MagicMock(return_value=4) + mv.source_list.selectedItems = mocker.MagicMock(return_value=[]) + + # Sanity check - make sure that all base QStackedWidget pages + # (Empty, NothingSelected, MultiSelected, Conversation) are rendered + assert mv.view_layout.count() == 4 + + mv.show_sources([factory.Source(), factory.Source(), factory.Source()]) + assert isinstance(mv.view_layout.widget(mv.view_layout.currentIndex()), NothingSelectedView) + assert mv.view_layout.currentIndex() == mv.NOTHING_SELECTED_INDEX -def test_EmptyConversationView_show_no_source_selected_message(mocker): - ecv = EmptyConversationView() +def test_MultiSelectedView(mocker): + mv = MainView(None) + sources = [factory.Source(), factory.Source(), factory.Source()] + mv.source_list = mocker.MagicMock() + mv.source_list.count = mocker.MagicMock(return_value=len(sources)) + mv.source_list.selectedItems = mocker.MagicMock(return_value=sources[:-1]) + mv._on_update_conversation_context = mocker.MagicMock(wraps=mv._on_update_conversation_context) + mv.show_sources(sources) - ecv.show_no_source_selected_message() + mv.show() - assert ecv.no_sources.isHidden() - assert not ecv.no_source_selected.isHidden() + # Sanity check - make sure that all base QStackedWidget pages + # (Empty, NothingSelected, MultiSelected, Conversation) are rendered + assert mv.view_layout.count() == 4 + mv._on_update_conversation_context.assert_called() + assert isinstance(mv.view_layout.widget(mv.view_layout.currentIndex()), MultiSelectView) + assert mv.view_layout.currentIndex() == mv.MULTI_SELECTED_INDEX def test_SourceList_get_selected_source(mocker): @@ -3791,14 +3876,12 @@ def test_SourceConversationWrapper_on_source_deleted(mocker): mv.source_list = mocker.MagicMock() mv.source_list.get_selected_source = mocker.MagicMock(return_value=source) mv.controller = mocker.MagicMock(is_authenticated=True) - mv.show() + + # Detached sourceconversationwrapper, just for unit testing scw = SourceConversationWrapper(source, mv.controller, None) - mocker.patch("securedrop_client.gui.widgets.SourceConversationWrapper", return_value=scw) mv.on_source_changed() scw.on_source_deleted("123") - assert mv.isVisible() - assert scw.isVisible() assert not scw.conversation_title_bar.isHidden() assert not scw.reply_box.isHidden() assert not scw.reply_box.text_edit.isEnabled() @@ -3847,15 +3930,13 @@ def test_SourceConversationWrapper_on_conversation_deleted(mocker): mv.source_list = mocker.MagicMock() mv.source_list.get_selected_source = mocker.MagicMock(return_value=source) mv.controller = mocker.MagicMock(is_authenticated=True) + mocker.patch("securedrop_client.gui.widgets.source_exists", return_value=True) mv.show() scw = SourceConversationWrapper(source, mv.controller, None) - mocker.patch("securedrop_client.gui.widgets.SourceConversationWrapper", return_value=scw) mv.on_source_changed() scw.on_conversation_deleted("123") - assert mv.isVisible() - assert scw.isVisible() assert not scw.conversation_title_bar.isHidden() assert not scw.reply_box.isHidden() assert not scw.reply_box.text_edit.isEnabled() diff --git a/client/tests/integration/test_placeholder.py b/client/tests/integration/test_placeholder.py index 6d0c69b6c..747e0bb38 100644 --- a/client/tests/integration/test_placeholder.py +++ b/client/tests/integration/test_placeholder.py @@ -2,7 +2,7 @@ def test_styles_for_placeholder(main_window): - wrapper = main_window.main_view.view_layout.itemAt(0).widget() + wrapper = main_window.main_view.view_layout.widget(main_window.main_view.CONVERSATION_INDEX) reply_box = wrapper.reply_box reply_text_edit = reply_box.text_edit @@ -34,7 +34,9 @@ def test_styles_for_placeholder(main_window): def test_styles_for_placeholder_no_key(main_window_no_key): - wrapper = main_window_no_key.main_view.view_layout.itemAt(0).widget() + wrapper = main_window_no_key.main_view.view_layout.itemAt( + main_window_no_key.main_view.CONVERSATION_INDEX + ).widget() reply_box = wrapper.reply_box reply_text_edit = reply_box.text_edit diff --git a/client/tests/integration/test_styles_file_download_button.py b/client/tests/integration/test_styles_file_download_button.py index e4e99d1d9..58ca631fc 100644 --- a/client/tests/integration/test_styles_file_download_button.py +++ b/client/tests/integration/test_styles_file_download_button.py @@ -5,7 +5,7 @@ def test_styles(mocker, main_window): - wrapper = main_window.main_view.view_layout.itemAt(0).widget() + wrapper = main_window.main_view.view_layout.widget(main_window.main_view.CONVERSATION_INDEX) conversation_scrollarea = wrapper.conversation_view._scroll file_widget = conversation_scrollarea.widget().layout().itemAt(0).widget() download_button = file_widget.download_button @@ -28,7 +28,7 @@ def test_styles(mocker, main_window): def test_styles_animated(mocker, main_window): - wrapper = main_window.main_view.view_layout.itemAt(0).widget() + wrapper = main_window.main_view.view_layout.widget(main_window.main_view.CONVERSATION_INDEX) conversation_scrollarea = wrapper.conversation_view._scroll file_widget = conversation_scrollarea.widget().layout().itemAt(0).widget() download_button = file_widget.download_button diff --git a/client/tests/integration/test_styles_reply_status_bar.py b/client/tests/integration/test_styles_reply_status_bar.py index 2d7d8f458..a5f8bf742 100644 --- a/client/tests/integration/test_styles_reply_status_bar.py +++ b/client/tests/integration/test_styles_reply_status_bar.py @@ -2,7 +2,7 @@ def test_styles(mocker, main_window): - wrapper = main_window.main_view.view_layout.itemAt(0).widget() + wrapper = main_window.main_view.view_layout.widget(main_window.main_view.CONVERSATION_INDEX) conversation_scrollarea = wrapper.conversation_view._scroll reply_widget = conversation_scrollarea.widget().layout().itemAt(2).widget() reply_widget.sender_is_current_user = False @@ -25,7 +25,7 @@ def test_styles(mocker, main_window): def test_styles_for_replies_from_authenticated_user(mocker, main_window): - wrapper = main_window.main_view.view_layout.itemAt(0).widget() + wrapper = main_window.main_view.view_layout.widget(main_window.main_view.CONVERSATION_INDEX) conversation_scrollarea = wrapper.conversation_view._scroll reply_widget = conversation_scrollarea.widget().layout().itemAt(2).widget() reply_widget.sender_is_current_user = True diff --git a/client/tests/integration/test_styles_sdclient.py b/client/tests/integration/test_styles_sdclient.py index cab18faa8..299b99ca8 100644 --- a/client/tests/integration/test_styles_sdclient.py +++ b/client/tests/integration/test_styles_sdclient.py @@ -64,11 +64,18 @@ def test_class_name_matches_css_object_name(mocker, main_window): assert main_view.__class__.__name__ == "MainView" assert main_view.objectName() == "MainView" assert "MainView" in main_view.view_holder.objectName() - empty_conversation_view = main_view.empty_conversation_view - empty_conversation_view.__class__.__name__ == "EmptyConversationView" - empty_conversation_view.objectName() == "EmptyConversationView" - "EmptyConversationView" in empty_conversation_view.no_sources.objectName() - "EmptyConversationView" in empty_conversation_view.no_source_selected.objectName() + + # All views use the "EmptyConversationView" styling + for index in [ + main_view.NO_SOURCES_INDEX, + main_view.NOTHING_SELECTED_INDEX, + main_view.MULTI_SELECTED_INDEX, + ]: + view = main_view.view_layout.widget(index) + view.objectName() == "EmptyConversationView" + "EmptyConversationView" in view.objectName() + "EmptyConversationView" in view.objectName() + source_list = main_view.source_list source_widget = source_list.itemWidget(source_list.item(0)) @@ -83,7 +90,7 @@ def test_class_name_matches_css_object_name(mocker, main_window): assert star.__class__.__name__ == "StarToggleButton" assert "StarToggleButton" in star.objectName() - wrapper = main_view.view_layout.itemAt(0).widget() + wrapper = main_view.view_layout.itemAt(main_view.CONVERSATION_INDEX).widget() assert wrapper.__class__.__name__ == "SourceConversationWrapper" assert wrapper.deletion_indicator.objectName() == "SourceDeletionIndicator" reply_box = wrapper.reply_box @@ -229,14 +236,12 @@ def test_styles_for_main_view(mocker, main_window): main_view = main_window.main_view assert main_view.minimumSize().height() == 558 assert main_view.view_holder.palette().color(QPalette.Background).name() == "#f9f9ff" - - assert main_view.empty_conversation_view.minimumSize().width() == 640 - no_sources = main_view.empty_conversation_view.no_sources + no_sources = main_view.view_layout.widget(0) assert no_sources.layout().count() == 5 no_sources_instructions = no_sources.layout().itemAt(0).widget() assert no_sources_instructions.font().family() == "Montserrat" assert QFont.DemiBold - 1 == no_sources_instructions.font().weight() - assert no_sources_instructions.font().pixelSize() == 35 + assert no_sources_instructions.font().pixelSize() == 30 assert no_sources_instructions.palette().color(QPalette.Foreground).name() == "#a5b3e9" no_sources_spacer1 = no_sources.layout().itemAt(1) assert no_sources_spacer1.minimumSize().height() == 35 @@ -244,7 +249,7 @@ def test_styles_for_main_view(mocker, main_window): no_sources_instruction_details1 = no_sources.layout().itemAt(2).widget() assert no_sources_instruction_details1.font().family() == "Montserrat" assert QFont.Normal == no_sources_instruction_details1.font().weight() - assert no_sources_instruction_details1.font().pixelSize() == 35 + assert no_sources_instruction_details1.font().pixelSize() == 30 assert no_sources_instruction_details1.palette().color(QPalette.Foreground).name() == "#a5b3e9" no_sources_spacer2 = no_sources.layout().itemAt(3) assert no_sources_spacer2.minimumSize().height() == 35 @@ -252,34 +257,34 @@ def test_styles_for_main_view(mocker, main_window): no_sources_instruction_details2 = no_sources.layout().itemAt(4).widget() assert no_sources_instruction_details2.font().family() == "Montserrat" assert QFont.Normal == no_sources_instruction_details2.font().weight() - assert no_sources_instruction_details2.font().pixelSize() == 35 + assert no_sources_instruction_details2.font().pixelSize() == 30 assert no_sources_instruction_details2.palette().color(QPalette.Foreground).name() == "#a5b3e9" - no_source_selected = main_view.empty_conversation_view.no_source_selected - assert no_source_selected.layout().count() == 6 + no_source_selected = main_view.view_layout.widget(1) + assert no_source_selected.layout().count() == 8 no_source_selected_instructions = no_source_selected.layout().itemAt(0).widget() assert no_source_selected_instructions.font().family() == "Montserrat" assert QFont.DemiBold - 1 == no_source_selected_instructions.font().weight() - assert no_source_selected_instructions.font().pixelSize() == 35 + assert no_source_selected_instructions.font().pixelSize() == 30 assert no_source_selected_instructions.palette().color(QPalette.Foreground).name() == "#a5b3e9" no_source_selected_spacer1 = no_source_selected.layout().itemAt(1) assert no_source_selected_spacer1.minimumSize().height() == 35 assert no_source_selected_spacer1.maximumSize().height() == 35 bullet1_bullet = no_source_selected.layout().itemAt(2).widget().layout().itemAt(0).widget() assert bullet1_bullet.getContentsMargins() == (0, 4, 0, 0) - bullet1_bullet.font().pixelSize() == 35 + bullet1_bullet.font().pixelSize() == 30 QFont.Bold == bullet1_bullet.font().weight() assert bullet1_bullet.font().family() == "Montserrat" assert bullet1_bullet.palette().color(QPalette.Foreground).name() == "#a5b3e9" bullet2_bullet = no_source_selected.layout().itemAt(3).widget().layout().itemAt(0).widget() assert bullet2_bullet.getContentsMargins() == (0, 4, 0, 0) - bullet2_bullet.font().pixelSize() == 35 + bullet2_bullet.font().pixelSize() == 30 QFont.Bold == bullet2_bullet.font().weight() assert bullet2_bullet.font().family() == "Montserrat" assert bullet2_bullet.palette().color(QPalette.Foreground).name() == "#a5b3e9" bullet3_bullet = no_source_selected.layout().itemAt(4).widget().layout().itemAt(0).widget() assert bullet3_bullet.getContentsMargins() == (0, 4, 0, 0) - bullet3_bullet.font().pixelSize() == 35 + bullet3_bullet.font().pixelSize() == 30 QFont.Bold == bullet3_bullet.font().weight() assert bullet3_bullet.font().family() == "Montserrat" assert bullet3_bullet.palette().color(QPalette.Foreground).name() == "#a5b3e9" @@ -314,7 +319,7 @@ def test_styles_source_list(mocker, main_window): def test_styles_for_conversation_view(mocker, main_window): - wrapper = main_window.main_view.view_layout.itemAt(0).widget() + wrapper = main_window.main_view.view_layout.widget(main_window.main_view.CONVERSATION_INDEX) deletion_indicator = wrapper.deletion_indicator assert deletion_indicator.deletion_message.font().family() == "Montserrat" assert QFont.Normal == deletion_indicator.deletion_message.font().weight() diff --git a/client/tests/integration/test_styles_speech_bubble_message.py b/client/tests/integration/test_styles_speech_bubble_message.py index c1d1bfc7e..35b015056 100644 --- a/client/tests/integration/test_styles_speech_bubble_message.py +++ b/client/tests/integration/test_styles_speech_bubble_message.py @@ -2,7 +2,7 @@ def test_styles(mocker, main_window): - wrapper = main_window.main_view.view_layout.itemAt(0).widget() + wrapper = main_window.main_view.view_layout.widget(main_window.main_view.CONVERSATION_INDEX) conversation_scrollarea = wrapper.conversation_view._scroll speech_bubble = conversation_scrollarea.widget().layout().itemAt(1).widget() diff --git a/client/tests/integration/test_styles_speech_bubble_status_bar.py b/client/tests/integration/test_styles_speech_bubble_status_bar.py index 0930db3f7..aece7cc4a 100644 --- a/client/tests/integration/test_styles_speech_bubble_status_bar.py +++ b/client/tests/integration/test_styles_speech_bubble_status_bar.py @@ -2,7 +2,7 @@ def test_styles(mocker, main_window): - wrapper = main_window.main_view.view_layout.itemAt(0).widget() + wrapper = main_window.main_view.view_layout.widget(main_window.main_view.CONVERSATION_INDEX) conversation_scrollarea = wrapper.conversation_view._scroll speech_bubble = conversation_scrollarea.widget().layout().itemAt(1).widget() diff --git a/client/tests/test_logic.py b/client/tests/test_logic.py index 4c03b4fa9..b7c11ba0b 100644 --- a/client/tests/test_logic.py +++ b/client/tests/test_logic.py @@ -1919,13 +1919,13 @@ def test_Controller_delete_source_not_logged_in(homedir, config, mocker, session source_db_object = mocker.MagicMock() co.on_action_requiring_login = mocker.MagicMock() co.api = None - co.delete_source(source_db_object) + co.delete_sources(set([source_db_object])) co.on_action_requiring_login.assert_called_with() def test_Controller_delete_source(homedir, config, mocker, session_maker, session): """ - Check that a DeleteSourceJob is submitted when delete_source is called. + Check that a DeleteSourceJob is submitted when delete_sources is called. """ mock_gui = mocker.MagicMock() co = Controller("http://localhost", mock_gui, session_maker, homedir, None) @@ -1945,7 +1945,7 @@ def test_Controller_delete_source(homedir, config, mocker, session_maker, sessio session.add(source) session.commit() - co.delete_source(source) + co.delete_sources(set([source])) assert len(source_deleted_emissions) == 1 assert source_deleted_emissions[0] == [source.uuid] From 41b429934126f0bc95e16960a85423c38f17467a Mon Sep 17 00:00:00 2001 From: Rowen S Date: Thu, 24 Oct 2024 19:46:15 +0000 Subject: [PATCH 3/5] Refactor: list[Source] instead of set[Source] of selectedItems --- client/securedrop_client/gui/actions.py | 6 +++--- client/securedrop_client/gui/source/delete/dialog.py | 6 +++--- client/securedrop_client/gui/widgets.py | 4 ++-- client/securedrop_client/logic.py | 8 ++++---- client/tests/functional/test_delete_source.py | 2 +- client/tests/functional/test_offline_delete_source.py | 2 +- client/tests/gui/source/delete/test_dialog.py | 8 ++++---- client/tests/test_logic.py | 4 ++-- 8 files changed, 20 insertions(+), 20 deletions(-) diff --git a/client/securedrop_client/gui/actions.py b/client/securedrop_client/gui/actions.py index 2858d8c13..73257dbf6 100644 --- a/client/securedrop_client/gui/actions.py +++ b/client/securedrop_client/gui/actions.py @@ -81,7 +81,7 @@ def __init__( source: Source, parent: QMenu, controller: Controller, - confirmation_dialog: Callable[[set[Source]], QDialog], + confirmation_dialog: Callable[[list[Source]], QDialog], ) -> None: self.source = source self.controller = controller @@ -91,9 +91,9 @@ def __init__( # DeleteSource Dialog can accept more than one source (bulk delete), # but when triggered from this menu, only applies to one source - self._confirmation_dialog = confirmation_dialog(set([self.source])) + self._confirmation_dialog = confirmation_dialog([self.source]) self._confirmation_dialog.accepted.connect( - lambda: self.controller.delete_sources(set([self.source])) + lambda: self.controller.delete_sources([self.source]) ) self.triggered.connect(self.trigger) diff --git a/client/securedrop_client/gui/source/delete/dialog.py b/client/securedrop_client/gui/source/delete/dialog.py index 2592a2f57..6453c23b4 100644 --- a/client/securedrop_client/gui/source/delete/dialog.py +++ b/client/securedrop_client/gui/source/delete/dialog.py @@ -27,7 +27,7 @@ class DeleteSourceDialog(ModalDialog): """Used to confirm deletion of source accounts.""" - def __init__(self, sources: set[Source]) -> None: + def __init__(self, sources: list[Source]) -> None: super().__init__(show_header=False, dangerous=True) self.sources = sources @@ -50,7 +50,7 @@ def __init__(self, sources: set[Source]) -> None: self.confirmation_label.setText(_("Are you sure this is what you want?")) self.adjustSize() - def make_body_text(self, sources: set[Source]) -> str: + def make_body_text(self, sources: list[Source]) -> str: message_tuple = ( "

", _("Delete entire account for: {source_or_sources}?"), @@ -74,7 +74,7 @@ def make_body_text(self, sources: set[Source]) -> str: source_or_sources=f"{self._get_source_names(sources)}" ) - def _get_source_names(self, sources: set[Source]) -> str: + def _get_source_names(self, sources: list[Source]) -> str: """ Helper. Return a comma-separated list of journalist designations. """ diff --git a/client/securedrop_client/gui/widgets.py b/client/securedrop_client/gui/widgets.py index 34aae742f..c8be4ae13 100644 --- a/client/securedrop_client/gui/widgets.py +++ b/client/securedrop_client/gui/widgets.py @@ -1145,7 +1145,7 @@ class SourceList(QListWidget): source_selection_cleared = pyqtSignal() # Bulk-context signal (toolbar) - selected_sources = pyqtSignal(object) # set[Source] + selected_sources = pyqtSignal(object) # list[Source] NUM_SOURCES_TO_ADD_AT_A_TIME = 32 INITIAL_UPDATE_SCROLLBAR_WIDTH = 20 @@ -1365,7 +1365,7 @@ def _on_item_selection_changed(self) -> None: self.source_selection_cleared.emit() # Update listeners (action toolbar) with current selection - self.selected_sources.emit(set(selected)) + self.selected_sources.emit(selected) class SourcePreview(SecureQLabel): diff --git a/client/securedrop_client/logic.py b/client/securedrop_client/logic.py index b92fa0ff3..f662cef7c 100644 --- a/client/securedrop_client/logic.py +++ b/client/securedrop_client/logic.py @@ -418,7 +418,7 @@ def __init__( # type: ignore[no-untyped-def] os.chmod(self.last_sync_filepath, 0o600) # Store currently-selected sources - self._selected_sources: set[db.Source] | None = None + self._selected_sources: list[db.Source] | None = None @pyqtSlot(int) def _on_main_queue_updated(self, num_items: int) -> None: @@ -1050,7 +1050,7 @@ def on_delete_source_failure(self, e: Exception) -> None: self.source_deletion_failed.emit(e.source_uuid) @login_required - def delete_sources(self, sources: set[db.Source]) -> None: + def delete_sources(self, sources: list[db.Source]) -> None: """ Performs a delete operation on one or more source records. @@ -1192,8 +1192,8 @@ def update_failed_replies(self) -> None: self.reply_failed.emit(failed_reply.uuid) @pyqtSlot(object) - def on_receive_selected_sources(self, sources: set[db.Source]) -> None: + def on_receive_selected_sources(self, sources: list[db.Source]) -> None: self._selected_sources = sources - def get_selected_sources(self) -> set[db.Source] | None: + def get_selected_sources(self) -> list[db.Source] | None: return self._selected_sources diff --git a/client/tests/functional/test_delete_source.py b/client/tests/functional/test_delete_source.py index 5c26bfcdc..dfe6e67ed 100644 --- a/client/tests/functional/test_delete_source.py +++ b/client/tests/functional/test_delete_source.py @@ -45,7 +45,7 @@ def check_for_conversation(): # Note: The qtbot object cannot interact with QAction items (as used in the delete button/menu) # so we programatically delete the source rather than using the GUI via qtbot source_count = gui.main_view.source_list.count() - controller.delete_sources(set([conversation.conversation_title_bar.source])) + controller.delete_sources([conversation.conversation_title_bar.source]) def check_source_list(): assert gui.main_view.source_list.count() == source_count - 1 diff --git a/client/tests/functional/test_offline_delete_source.py b/client/tests/functional/test_offline_delete_source.py index 25eca0f61..ef0c106e2 100644 --- a/client/tests/functional/test_offline_delete_source.py +++ b/client/tests/functional/test_offline_delete_source.py @@ -45,7 +45,7 @@ def check_for_conversation(): # Attempt to delete the selected source # Note: The qtbot object cannot interact with QAction items (as used in the delete button/menu) # so we programatically attempt to delete the source rather than using the GUI via qtbot - controller.delete_sources(set([conversation.conversation_title_bar.source])) + controller.delete_sources([conversation.conversation_title_bar.source]) def check_for_error(): msg = gui.bottom_pane.error_status_bar.status_bar.currentMessage() diff --git a/client/tests/gui/source/delete/test_dialog.py b/client/tests/gui/source/delete/test_dialog.py index 29c7417ec..dc854026c 100644 --- a/client/tests/gui/source/delete/test_dialog.py +++ b/client/tests/gui/source/delete/test_dialog.py @@ -5,7 +5,7 @@ @pytest.fixture( - params=[set(), set([factory.Source()]), set([factory.Source(), factory.Source()])], + params=[[], [factory.Source()], [factory.Source(), factory.Source()]], ) def dialog(request): """ @@ -22,7 +22,7 @@ def dialog(request): class TestDeleteSourceDialog: def test_dialog_setup(self, dialog): assert type(dialog) is DeleteSourceDialog - assert type(dialog.sources) is set + assert type(dialog.sources) is list assert dialog.dangerous def test_default_button_is_safer_choice(self, dialog): @@ -69,7 +69,7 @@ def test_correct_format_body_text(self): For n > 1 sources, ensure the warning text includes all the journalist desginators. """ - sources = set() + sources = [] names = [ "source one", "source two", @@ -82,7 +82,7 @@ def test_correct_format_body_text(self): for item in names: source = factory.Source(journalist_designation=item) - sources.update([source]) + sources.append(source) dialog = DeleteSourceDialog(sources) diff --git a/client/tests/test_logic.py b/client/tests/test_logic.py index b7c11ba0b..ec7f65fa7 100644 --- a/client/tests/test_logic.py +++ b/client/tests/test_logic.py @@ -1919,7 +1919,7 @@ def test_Controller_delete_source_not_logged_in(homedir, config, mocker, session source_db_object = mocker.MagicMock() co.on_action_requiring_login = mocker.MagicMock() co.api = None - co.delete_sources(set([source_db_object])) + co.delete_sources([source_db_object]) co.on_action_requiring_login.assert_called_with() @@ -1945,7 +1945,7 @@ def test_Controller_delete_source(homedir, config, mocker, session_maker, sessio session.add(source) session.commit() - co.delete_sources(set([source])) + co.delete_sources([source]) assert len(source_deleted_emissions) == 1 assert source_deleted_emissions[0] == [source.uuid] From c267ce7a4977f8ec96d03d946e6280c07550bd01 Mon Sep 17 00:00:00 2001 From: Rowen S Date: Fri, 25 Oct 2024 10:04:09 +0000 Subject: [PATCH 4/5] 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 +++++++++++- .../tests/functional/test_deleted_file_filewidget.py | 6 +++--- 3 files changed, 18 insertions(+), 5 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..6b0d673b3 100644 --- a/client/tests/functional/test_deleted_file_filewidget.py +++ b/client/tests/functional/test_deleted_file_filewidget.py @@ -40,8 +40,8 @@ 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] @@ -49,7 +49,7 @@ def conversation_with_file_is_rendered(): # 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() + conversation = gui.main_view.view_layout.widget(gui.main_view.view_layout.currentIndex()) file_id = list(conversation.conversation_view.current_messages.keys())[2] file_widget = conversation.conversation_view.current_messages[file_id] From f6bd2c92ddb6a3c17b01a4e9c2a67ebdd05d44fe Mon Sep 17 00:00:00 2001 From: Cory Francis Myers Date: Mon, 28 Oct 2024 23:34:39 -0300 Subject: [PATCH 5/5] test(test_delete_sources): parameterized functional test for deletion of one or more sources This is a complete rewrite of this test, which now (a) distinguishes between single- and multi-select behavior and (b) checks that the individual sources selected for deletion are confirmed and deleted, not only the *number* of sources selected. This includes a workaround for #2273. Here we also change securedrop_client.gui.widgets.BatchActionToolbar.on_action_triggered() to call QDialog.open(), which is modal with QDialog.setModal(), rather than QDialog.exec(), which is modal *and* blocking, which blocks pytest timers as well as the main event loop. --- client/securedrop_client/gui/widgets.py | 3 +- .../functional/data/test_delete_source.yml | 1192 --------- .../functional/data/test_delete_sources.yml | 2258 +++++++++++++++++ client/tests/functional/test_delete_source.py | 53 - .../tests/functional/test_delete_sources.py | 101 + 5 files changed, 2361 insertions(+), 1246 deletions(-) delete mode 100644 client/tests/functional/data/test_delete_source.yml create mode 100644 client/tests/functional/data/test_delete_sources.yml delete mode 100644 client/tests/functional/test_delete_source.py create mode 100644 client/tests/functional/test_delete_sources.py diff --git a/client/securedrop_client/gui/widgets.py b/client/securedrop_client/gui/widgets.py index f1997a3c2..26c16dcb7 100644 --- a/client/securedrop_client/gui/widgets.py +++ b/client/securedrop_client/gui/widgets.py @@ -555,8 +555,9 @@ def on_action_triggered(self) -> None: targets = self.controller.get_selected_sources() if targets is not None: dialog = DeleteSourceDialog(targets) + self._last_dialog = dialog # FIXME: workaround for #2273 dialog.accepted.connect(lambda: self.controller.delete_sources(targets)) - dialog.exec() + dialog.open() else: # No selected sources should return an empty set, not None logger.error("Toolbar action triggered without valid data from controller.") diff --git a/client/tests/functional/data/test_delete_source.yml b/client/tests/functional/data/test_delete_source.yml deleted file mode 100644 index bdcb0c603..000000000 --- a/client/tests/functional/data/test_delete_source.yml +++ /dev/null @@ -1,1192 +0,0 @@ -interactions: -- request: - body: '{"username": "journalist", "passphrase": "correct horse battery staple - profanity oil chewy", "one_time_code": "668631"}' - headers: {} - method: POST - uri: api/v1/token - response: !!python/object:securedrop_client.sdk.JSONResponse - data: - expiration: '2024-02-29T23:51:33.301258Z' - journalist_first_name: null - journalist_last_name: null - journalist_uuid: d53edd4d-1802-40ce-9c69-86d9b6b75ac8 - token: ImxEbER6d3I2enhzdE41d0dqbUpfR1QwdjRmQ0hMVDZ0ZldKQjlqZEJHeHci.ZeD8ZQ.4YCgvY1FDiEt7Rs7ashjW4AgPAs - headers: - connection: close - content-length: '290' - content-type: application/json - date: Thu, 29 Feb 2024 21:51:33 GMT - server: Werkzeug/2.2.3 Python/3.8.10 - status: 200 -- request: - body: null - headers: - Accept: - - application/json - Authorization: - - Token ImxEbER6d3I2enhzdE41d0dqbUpfR1QwdjRmQ0hMVDZ0ZldKQjlqZEJHeHci.ZeD8ZQ.4YCgvY1FDiEt7Rs7ashjW4AgPAs - Content-Type: - - application/json - method: GET - uri: api/v1/users - response: !!python/object:securedrop_client.sdk.JSONResponse - data: - users: - - first_name: null - last_name: null - username: journalist - uuid: d53edd4d-1802-40ce-9c69-86d9b6b75ac8 - - first_name: null - last_name: null - username: dellsberg - uuid: 16b3778d-28e5-48fe-afa5-abc2f1f99e62 - - first_name: null - last_name: null - username: deleted - uuid: bfb91841-1eae-4d73-8e42-aa763de1f01b - headers: - connection: close - content-length: '474' - content-type: application/json - date: Thu, 29 Feb 2024 21:51:33 GMT - server: Werkzeug/2.2.3 Python/3.8.10 - status: 200 -- request: - body: null - headers: - Accept: - - application/json - Authorization: - - Token ImxEbER6d3I2enhzdE41d0dqbUpfR1QwdjRmQ0hMVDZ0ZldKQjlqZEJHeHci.ZeD8ZQ.4YCgvY1FDiEt7Rs7ashjW4AgPAs - Content-Type: - - application/json - method: GET - uri: api/v1/sources - response: !!python/object:securedrop_client.sdk.JSONResponse - data: - sources: - - add_star_url: /api/v1/sources/71fb3b51-4ee6-4c37-bd13-a6d5a5df71f9/add_star - interaction_count: 6 - is_flagged: false - is_starred: false - journalist_designation: mindless stevia - key: - fingerprint: 3565064B66EC1B1FFFF9F91E60C9B9DB9B742F40 - public: '-----BEGIN PGP PUBLIC KEY BLOCK----- - - Comment: 3565 064B 66EC 1B1F FFF9 F91E 60C9 B9DB 9B74 2F40 - - Comment: Source Key = num_to_delete + + qtbot.waitUntil(check_for_sources, timeout=TIME_RENDER_SOURCE_LIST) + + # Select num_to_delete sources at random. + source_uuids_to_delete = random.sample( + sorted(gui.main_view.source_list.source_items), num_to_delete + ) + for i, source_uuid in enumerate(source_uuids_to_delete): + source_item = gui.main_view.source_list.source_items[source_uuid] + source_widget = gui.main_view.source_list.itemWidget(source_item) + + # Simulate a non- initial click... + if i == 0: + qtbot.mouseClick(source_widget, Qt.LeftButton) + # ...followed by subsequent -clicks. + else: + qtbot.mouseClick( + source_widget, Qt.LeftButton, modifier=Qt.KeyboardModifier.ControlModifier + ) + + qtbot.wait(TIME_CLICK_ACTION) + + def check_for_conversation(): + """ + Check that the GUI is showing the right view for the number of + sources selected. + """ + if num_to_delete == 1: + index = gui.main_view.CONVERSATION_INDEX + widget = SourceConversationWrapper + else: + index = gui.main_view.MULTI_SELECTED_INDEX + widget = MultiSelectView + + assert gui.main_view.view_layout.currentIndex() == index + assert isinstance( + gui.main_view.view_layout.widget(gui.main_view.view_layout.currentIndex()), widget + ) + + qtbot.waitUntil(check_for_conversation, timeout=TIME_RENDER_CONV_VIEW) + + # Delete the selected source. We can't qtbot.mouseClick() on a QAction, but + # we can trigger it directly. + gui.main_view.top_pane.batch_actions.toolbar.delete_sources_action.trigger() + + def check_and_accept_dialog(): + """Check that the dialog confirms deletion of all sources selected.""" + dialog = ( + gui.main_view.top_pane.batch_actions.toolbar._last_dialog + ) # FIXME: workaround for #2273 + assert set(source_uuids_to_delete) == {source.uuid for source in dialog.sources} + dialog.accept() + + qtbot.waitUntil(check_and_accept_dialog, timeout=TIME_CLICK_ACTION) + + def check_source_list(): + """ + Check that all of the sources selected for deletion have been removed + from the source list. + """ + for source_uuid in source_uuids_to_delete: + assert source_uuid not in gui.main_view.source_list.source_items + + qtbot.waitUntil(check_source_list, timeout=TIME_RENDER_SOURCE_LIST)