From 6dd015589190e956cc084db557454eaaa05b7a09 Mon Sep 17 00:00:00 2001 From: John Hensley Date: Wed, 11 Mar 2020 16:41:46 -0400 Subject: [PATCH] Ensure that sources are properly refreshed on sync In widgets.SourceList.update, make sure that existing SourceWidgets are updated. This requires refreshing each source's state in the controller session. On sync, sources were being properly updated in the database via storage.update_sources, but without this refresh, a source being unstarred on the server, for example, wouldn't be reflected in the UI until some other operation caused the source to be refreshed in the controller's session. Commit after updating a source's is_starred field in widgets.StarToggleButton.on_update, preventing a sqlite3.OperationalError saying "database is locked". In sync.ApiSync.on_sync_failure, keep syncing no matter the error, and have the log message say less about the cause. Log more details about API job failure in api_jobs.base.ApiJob._do_call_api. --- securedrop_client/api_jobs/base.py | 6 ++ securedrop_client/gui/widgets.py | 88 ++++++++++++------ securedrop_client/logic.py | 2 +- securedrop_client/sync.py | 5 +- tests/gui/test_widgets.py | 142 +++++++++++++++++------------ tests/test_sync.py | 2 +- 6 files changed, 152 insertions(+), 93 deletions(-) diff --git a/securedrop_client/api_jobs/base.py b/securedrop_client/api_jobs/base.py index deb229227..4d5223ea7 100644 --- a/securedrop_client/api_jobs/base.py +++ b/securedrop_client/api_jobs/base.py @@ -1,4 +1,5 @@ import logging +import traceback from PyQt5.QtCore import QObject, pyqtSignal from sdclientapi import API, AuthError, RequestTimeoutError, ServerConnectionError @@ -77,6 +78,11 @@ def _do_call_api(self, api_client: API, session: Session) -> None: raise except Exception as e: self.failure_signal.emit(e) + logger.error( + "%s API call error: %s", + self.__class__.__name__, + traceback.format_exc() + ) raise else: self.success_signal.emit(result) diff --git a/securedrop_client/gui/widgets.py b/securedrop_client/gui/widgets.py index cb9c97187..9ab852d56 100644 --- a/securedrop_client/gui/widgets.py +++ b/securedrop_client/gui/widgets.py @@ -931,9 +931,20 @@ def update(self, sources: List[Source]) -> List[str]: # Create new widgets for new sources widget_uuids = [self.itemWidget(self.item(i)).source_uuid for i in range(self.count())] for source in sources: - if source.uuid not in widget_uuids: - new_source = SourceWidget(source) - new_source.setup(self.controller) + if source.uuid in widget_uuids: + try: + self.source_widgets[source.uuid].update() + except sqlalchemy.exc.InvalidRequestError as e: + logger.error( + "Could not update SourceWidget for source %s; deleting it. Error was: %s", + source.uuid, + e + ) + deleted_uuids.append(source.uuid) + self.source_widgets[source.uuid].deleteLater() + del self.source_widgets[list_widget.source_uuid] + else: + new_source = SourceWidget(self.controller, source) self.source_widgets[source.uuid] = new_source list_item = QListWidgetItem() @@ -1029,9 +1040,12 @@ class SourceWidget(QWidget): PREVIEW_WIDTH = 412 PREVIEW_HEIGHT = 60 - def __init__(self, source: Source): + def __init__(self, controller: Controller, source: Source): super().__init__() + self.controller = controller + self.controller.source_deleted.connect(self._on_source_deleted) + # Store source self.source_uuid = source.uuid self.source = source @@ -1061,7 +1075,7 @@ def __init__(self, source: Source): gutter_layout = QVBoxLayout(self.gutter) gutter_layout.setContentsMargins(0, 0, 0, 0) gutter_layout.setSpacing(0) - self.star = StarToggleButton(self.source) + self.star = StarToggleButton(self.controller, self.source) gutter_layout.addWidget(self.star) gutter_layout.addStretch() @@ -1117,23 +1131,25 @@ def __init__(self, source: Source): self.update() - def setup(self, controller): - """ - Pass through the controller object to this widget. - """ - self.controller = controller - self.controller.source_deleted.connect(self._on_source_deleted) - self.star.setup(self.controller) - def update(self): """ Updates the displayed values with the current values from self.source. """ - self.timestamp.setText(_(arrow.get(self.source.last_updated).format('DD MMM'))) - self.name.setText(self.source.journalist_designation) - self.set_snippet(self.source.uuid) - if self.source.document_count == 0: - self.paperclip.hide() + try: + self.controller.session.refresh(self.source) + self.timestamp.setText(_(arrow.get(self.source.last_updated).format('DD MMM'))) + self.name.setText(self.source.journalist_designation) + self.set_snippet(self.source.uuid) + if self.source.document_count == 0: + self.paperclip.hide() + self.star.update() + except sqlalchemy.exc.InvalidRequestError as e: + logger.error( + "Could not update SourceWidget for source %s: %s", + self.source.uuid, + e + ) + raise def set_snippet(self, source, uuid=None, content=None): """ @@ -1177,21 +1193,25 @@ class StarToggleButton(SvgToggleButton): } ''' - def __init__(self, source: Source): + def __init__(self, controller: Controller, source: Source): super().__init__( on='star_on.svg', off='star_off.svg', svg_size=QSize(16, 16)) - self.installEventFilter(self) + self.controller = controller self.source = source - if self.source.is_starred: - self.setChecked(True) + + self.installEventFilter(self) + self.toggle_event_enabled = True self.setObjectName('star_button') self.setStyleSheet(self.css) self.setFixedSize(QSize(20, 20)) + self.controller.authentication_state.connect(self.on_authentication_changed) + self.on_authentication_changed(self.controller.is_authenticated) + def disable(self): """ Disable the widget. @@ -1242,11 +1262,6 @@ def enable(self): self.toggled.connect(self.on_toggle) - def setup(self, controller): - self.controller = controller - self.controller.authentication_state.connect(self.on_authentication_changed) - self.on_authentication_changed(self.controller.is_authenticated) - def eventFilter(self, obj, event): t = event.type() if t == QEvent.HoverEnter: @@ -1269,7 +1284,8 @@ def on_toggle(self): """ Tell the controller to make an API call to update the source's starred field. """ - self.controller.update_star(self.source, self.on_update) + if self.toggle_event_enabled: + self.controller.update_star(self.source, self.on_update) def on_toggle_offline(self): """ @@ -1284,9 +1300,25 @@ def on_update(self, result): """ enabled = result[1] self.source.is_starred = enabled + self.controller.session.commit() self.controller.update_sources() self.setChecked(enabled) + def update(self): + """ + Update the star to reflect its source's current state. + """ + self.controller.session.refresh(self.source) + self.disable_toggle_event() + self.setChecked(self.source.is_starred) + self.enable_toggle_event() + + def disable_toggle_event(self): + self.toggle_event_enabled = False + + def enable_toggle_event(self): + self.toggle_event_enabled = True + class DeleteSourceMessageBox: """Use this to display operation details and confirm user choice.""" diff --git a/securedrop_client/logic.py b/securedrop_client/logic.py index 2a1dd2948..39c0227a3 100644 --- a/securedrop_client/logic.py +++ b/securedrop_client/logic.py @@ -468,7 +468,7 @@ def on_sync_failure(self, result: Exception) -> None: a sync fails is ApiInaccessibleError then we need to log the user out for security reasons and show them the login window in order to get a new token. """ - logger.debug('The SecureDrop server cannot be reached due to Error: {}'.format(result)) + logger.error('sync failure: {}'.format(result)) if isinstance(result, ApiInaccessibleError): # Don't show login window if the user is already logged out diff --git a/securedrop_client/sync.py b/securedrop_client/sync.py index 8a114ae5e..090662073 100644 --- a/securedrop_client/sync.py +++ b/securedrop_client/sync.py @@ -2,7 +2,7 @@ from PyQt5.QtCore import pyqtSignal, QObject, QThread, QTimer, Qt from sqlalchemy.orm import scoped_session -from sdclientapi import API, RequestTimeoutError, ServerConnectionError +from sdclientapi import API from securedrop_client.api_jobs.base import ApiInaccessibleError from securedrop_client.api_jobs.sync import MetadataSyncJob @@ -75,8 +75,7 @@ def on_sync_failure(self, result: Exception) -> None: Only start another sync on failure if the reason is a timeout request. ''' self.sync_failure.emit(result) - if isinstance(result, (RequestTimeoutError, ServerConnectionError)): - QTimer.singleShot(self.TIME_BETWEEN_SYNCS_MS, self.api_sync_bg_task.sync) + QTimer.singleShot(self.TIME_BETWEEN_SYNCS_MS, self.api_sync_bg_task.sync) class ApiSyncBackgroundTask(QObject): diff --git a/tests/gui/test_widgets.py b/tests/gui/test_widgets.py index 4a6b5a419..c7b3fa7cb 100644 --- a/tests/gui/test_widgets.py +++ b/tests/gui/test_widgets.py @@ -760,6 +760,46 @@ def test_SourceList_update_adds_new_sources(mocker): mock_sw.deleteLater.assert_not_called() +def test_SourceList_update_when_source_deleted(mocker, session, session_maker, homedir): + """ + Test that SourceWidget.update raises an exception when its source has been deleted. + + When SourceList.update calls SourceWidget.update and that + SourceWidget's source has been deleted, SourceList.update should + catch the resulting excpetion, delete the SourceWidget and add its + source UUID to the list of deleted source UUIDs. + """ + mock_gui = mocker.MagicMock() + controller = logic.Controller('http://localhost', mock_gui, session_maker, homedir) + + # create the source in another session + source = factory.Source() + session.add(source) + session.commit() + + # construct the SourceWidget with the source fetched in its + # controller's session + oss = controller.session.query(db.Source).filter_by(id=source.id).one() + + # add it to the SourceList + sl = SourceList() + sl.setup(controller) + deleted_uuids = sl.update([oss]) + assert not deleted_uuids + assert len(sl.source_widgets) == 1 + + # now delete it + session.delete(source) + session.commit() + + # and finally verify that updating raises an exception, causing + # the SourceWidget to be deleted + deleted_uuids = sl.update([source]) + assert len(deleted_uuids) == 1 + assert source.uuid in deleted_uuids + assert len(sl.source_widgets) == 0 + + def test_SourceList_update_maintains_selection(mocker): """ Maintains the selected item if present in new list @@ -877,36 +917,23 @@ def test_SourceWidget_init(mocker): """ The source widget is initialised with the passed-in source. """ + controller = mocker.MagicMock() mock_source = mocker.MagicMock() mock_source.journalist_designation = 'foo bar baz' - sw = SourceWidget(mock_source) + sw = SourceWidget(controller, mock_source) assert sw.source == mock_source -def test_SourceWidget_setup(mocker): - """ - The setup method adds the controller as an attribute on the SourceWidget. - """ - mock_controller = mocker.MagicMock() - mock_source = mocker.MagicMock(journalist_designation='mock') - sw = SourceWidget(mock_source) - sw.star = mocker.MagicMock() - - sw.setup(mock_controller) - - assert sw.controller == mock_controller - sw.star.setup.assert_called_once_with(mock_controller) - - def test_SourceWidget_html_init(mocker): """ The source widget is initialised with the given source name, with HTML escaped properly. """ + controller = mocker.MagicMock() mock_source = mocker.MagicMock() mock_source.journalist_designation = 'foo bar baz' - sw = SourceWidget(mock_source) + sw = SourceWidget(controller, mock_source) sw.name = mocker.MagicMock() sw.summary_layout = mocker.MagicMock() @@ -916,12 +943,13 @@ def test_SourceWidget_html_init(mocker): sw.name.setText.assert_called_once_with('foo bar baz') -def test_SourceWidget_update_attachment_icon(): +def test_SourceWidget_update_attachment_icon(mocker): """ Attachment icon identicates document count """ + controller = mocker.MagicMock() source = factory.Source(document_count=1) - sw = SourceWidget(source) + sw = SourceWidget(controller, source) sw.update() assert not sw.paperclip.isHidden() @@ -936,12 +964,13 @@ def test_SourceWidget_set_snippet(mocker): """ Snippets are set as expected. """ + controller = mocker.MagicMock() source = mocker.MagicMock() source.uuid = "a_uuid" source.journalist_designation = "Testy McTestface" msg = factory.Message(content="abcdefg") source.collection = [msg, ] - sw = SourceWidget(source) + sw = SourceWidget(controller, source) sw.set_snippet(source.uuid, msg.uuid, msg.content) assert sw.preview.text() == "abcdefg" @@ -951,10 +980,11 @@ def test_SourceWidget_update_truncate_latest_msg(mocker): If the latest message in the conversation is longer than 150 characters, truncate and add "..." to the end. """ + controller = mocker.MagicMock() source = mocker.MagicMock() source.journalist_designation = "Testy McTestface" source.collection = [factory.Message(content="a" * 151), ] - sw = SourceWidget(source) + sw = SourceWidget(controller, source) sw.update() assert sw.preview.text().endswith("...") @@ -966,8 +996,7 @@ def test_SourceWidget_delete_source(mocker, session, source): mock_delete_source_message = mocker.MagicMock( return_value=mock_delete_source_message_box_object) - sw = SourceWidget(source['source']) - sw.controller = mock_controller + sw = SourceWidget(mock_controller, source['source']) mocker.patch( "securedrop_client.gui.widgets.DeleteSourceMessageBox", @@ -990,8 +1019,7 @@ def test_SourceWidget_delete_source_when_user_chooses_cancel(mocker, session, so mock_message_box_question.return_value = QMessageBox.Cancel mock_controller = mocker.MagicMock() - sw = SourceWidget(source) - sw.controller = mock_controller + sw = SourceWidget(mock_controller, source) mocker.patch( "securedrop_client.gui.widgets.QMessageBox.question", @@ -1002,7 +1030,8 @@ def test_SourceWidget_delete_source_when_user_chooses_cancel(mocker, session, so def test_SourceWidget__on_source_deleted(mocker, session, source): - sw = SourceWidget(factory.Source(uuid='123')) + controller = mocker.MagicMock() + sw = SourceWidget(controller, factory.Source(uuid='123')) sw._on_source_deleted('123') assert sw.gutter.isHidden() assert sw.metadata.isHidden() @@ -1011,7 +1040,8 @@ def test_SourceWidget__on_source_deleted(mocker, session, source): def test_SourceWidget__on_source_deleted_wrong_uuid(mocker, session, source): - sw = SourceWidget(factory.Source(uuid='123')) + controller = mocker.MagicMock() + sw = SourceWidget(controller, factory.Source(uuid='123')) sw._on_source_deleted('321') assert not sw.gutter.isHidden() assert not sw.metadata.isHidden() @@ -1023,10 +1053,11 @@ def test_SourceWidget_uses_SecureQLabel(mocker): """ Ensure the source widget preview uses SecureQLabel and is not injectable """ + controller = mocker.MagicMock() source = mocker.MagicMock() source.journalist_designation = "Testy McTestface" source.collection = [factory.Message(content="a" * 121), ] - sw = SourceWidget(source) + sw = SourceWidget(controller, source) sw.update() assert isinstance(sw.preview, SecureQLabel) @@ -1038,45 +1069,33 @@ def test_SourceWidget_uses_SecureQLabel(mocker): def test_StarToggleButton_init_source_starred(mocker): + controller = mocker.MagicMock() source = factory.Source() source.is_starred = True - stb = StarToggleButton(source) + stb = StarToggleButton(controller, source) assert stb.source == source assert stb.isChecked() is True def test_StarToggleButton_init_source_unstarred(mocker): + controller = mocker.MagicMock() source = factory.Source() source.is_starred = False - stb = StarToggleButton(source) + stb = StarToggleButton(controller, source) assert stb.source == source assert stb.isChecked() is False -def test_StarToggleButton_setup(mocker): - star_toggle_button = StarToggleButton(source=mocker.MagicMock()) - controller = mocker.MagicMock() - controller.authentication_state = mocker.MagicMock() - controller.is_authenticated = 'mock' - on_authentication_changed_fn = mocker.patch.object( - StarToggleButton, 'on_authentication_changed') - - star_toggle_button.setup(controller) - - assert star_toggle_button.controller == controller - controller.authentication_state.connect.assert_called_once_with(on_authentication_changed_fn) - on_authentication_changed_fn.assert_called_with('mock') - - def test_StarToggleButton_eventFilter(mocker): """ Ensure the hover events are handled properly. """ - stb = StarToggleButton(source=mocker.MagicMock()) + controller = mocker.MagicMock() + stb = StarToggleButton(controller=controller, source=mocker.MagicMock()) stb.setIcon = mocker.MagicMock() stb.set_icon = mocker.MagicMock() # Hover enter @@ -1094,8 +1113,9 @@ def test_StarToggleButton_on_authentication_changed_while_authenticated_and_chec If on_authentication_changed is set up correctly, then calling toggle on a checked button should result in the button being unchecked. """ + controller = mocker.MagicMock() source = mocker.MagicMock() - stb = StarToggleButton(source=source) + stb = StarToggleButton(controller, source=source) stb.setChecked(True) stb.on_toggle = mocker.MagicMock() stb.on_authentication_changed(authenticated=True) @@ -1111,9 +1131,11 @@ def test_StarToggleButton_on_authentication_changed_while_authenticated_and_not_ If on_authentication_changed is set up correctly, then calling toggle on an unchecked button should result in the button being unchecked. """ + controller = mocker.MagicMock() source = mocker.MagicMock() source.is_starred = False - stb = StarToggleButton(source=source) + stb = StarToggleButton(controller, source=source) + stb.setChecked(False) stb.on_toggle = mocker.MagicMock() assert stb.isChecked() is False @@ -1130,8 +1152,9 @@ def test_StarToggleButton_on_authentication_changed_while_offline_mode(mocker): """ Ensure on_authentication_changed is set up correctly for offline mode. """ + controller = mocker.MagicMock() source = mocker.MagicMock() - stb = StarToggleButton(source=source) + stb = StarToggleButton(controller, source=source) stb.on_toggle_offline = mocker.MagicMock() stb.on_toggle = mocker.MagicMock() @@ -1146,9 +1169,9 @@ def test_StarToggleButton_on_toggle(mocker): """ Ensure correct star icon images are loaded for the enabled button. """ + controller = mocker.MagicMock() source = mocker.MagicMock() - stb = StarToggleButton(source) - stb.controller = mocker.MagicMock() + stb = StarToggleButton(controller, source) stb.on_toggle() @@ -1159,9 +1182,9 @@ def test_StarToggleButton_on_toggle_offline(mocker): """ Ensure toggle is disabled when offline. """ + controller = mocker.MagicMock() source = mocker.MagicMock() - stb = StarToggleButton(source) - stb.controller = mocker.MagicMock() + stb = StarToggleButton(controller, source) stb.on_toggle_offline() stb.controller.on_action_requiring_login.assert_called_once_with() @@ -1171,10 +1194,10 @@ def test_StarToggleButton_on_toggle_offline_when_checked(mocker): """ Ensure correct star icon images are loaded for the disabled button. """ + controller = mocker.MagicMock() source = mocker.MagicMock() source.is_starred = True - stb = StarToggleButton(source) - stb.controller = mocker.MagicMock() + stb = StarToggleButton(controller, source) set_icon_fn = mocker.patch('securedrop_client.gui.SvgToggleButton.set_icon') # go offline @@ -1191,11 +1214,11 @@ def test_StarToggleButton_on_update(mocker): Ensure the on_update callback updates the state of the source and UI element to the current "enabled" state. """ + controller = mocker.MagicMock() source = mocker.MagicMock() source.is_starred = True - stb = StarToggleButton(source) + stb = StarToggleButton(controller, source) stb.setChecked = mocker.MagicMock() - stb.controller = mocker.MagicMock() stb.on_update(("uuid", False)) assert source.is_starred is False stb.controller.update_sources.assert_called_once_with() @@ -3205,7 +3228,7 @@ def test_DeleteSource_from_source_menu_when_user_is_loggedout(mocker): def test_DeleteSource_from_source_widget_when_user_is_loggedout(mocker): mock_source = mocker.MagicMock(journalist_designation='mock') - mock_controller = mocker.MagicMock(logic.Controller) + mock_controller = mocker.MagicMock() mock_controller.api = None mock_event = mocker.MagicMock() mock_delete_source_message_box_obj = mocker.MagicMock() @@ -3218,8 +3241,7 @@ def test_DeleteSource_from_source_widget_when_user_is_loggedout(mocker): 'securedrop_client.gui.widgets.DeleteSourceMessageBox', mock_delete_source_message_box ): - source_widget = SourceWidget(mock_source) - source_widget.setup(mock_controller) + source_widget = SourceWidget(mock_controller, mock_source) source_widget.delete_source(mock_event) mock_delete_source_message_box_obj.launch.assert_not_called() diff --git a/tests/test_sync.py b/tests/test_sync.py index cf6fe93b2..dbe430157 100644 --- a/tests/test_sync.py +++ b/tests/test_sync.py @@ -166,7 +166,7 @@ def test_ApiSync_on_sync_failure(mocker, session_maker, homedir): api_sync.on_sync_failure(error) sync_failure.emit.assert_called_once_with(error) - singleShot_fn.assert_not_called() + singleShot_fn.assert_called_once_with(15000, api_sync.api_sync_bg_task.sync) @pytest.mark.parametrize("exception", [RequestTimeoutError, ServerConnectionError])