From 3002633308f917945eeb545e6fdaa7f71718c04b Mon Sep 17 00:00:00 2001 From: Allie Crevier Date: Wed, 18 Mar 2020 17:56:26 -0700 Subject: [PATCH 01/10] remove unused code --- securedrop_client/gui/__init__.py | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/securedrop_client/gui/__init__.py b/securedrop_client/gui/__init__.py index 9a838510a..1bf762bfd 100644 --- a/securedrop_client/gui/__init__.py +++ b/securedrop_client/gui/__init__.py @@ -58,12 +58,6 @@ def __init__(self, on: str, off: str, svg_size: str = None): # Make this a toggle button self.setCheckable(True) - def enable(self) -> None: - self.setEnabled(True) - - def disable(self) -> None: - self.setEnabled(False) - def set_icon(self, on: str, off: str) -> None: self.icon = load_icon(normal=on, normal_off=off) self.setIcon(self.icon) @@ -116,12 +110,6 @@ def __init__( self.setIcon(self.icon) self.setIconSize(svg_size) if svg_size else self.setIconSize(QSize()) - def enable(self) -> None: - self.setEnabled(True) - - def disable(self) -> None: - self.setEnabled(False) - class SvgLabel(QLabel): """ From 8433cc0fdc7f7281ec73a2e5baa2acdaea82178e Mon Sep 17 00:00:00 2001 From: Allie Crevier Date: Wed, 18 Mar 2020 18:06:26 -0700 Subject: [PATCH 02/10] remove extra success callback for star --- securedrop_client/gui/widgets.py | 18 ++---------------- securedrop_client/logic.py | 6 ++---- 2 files changed, 4 insertions(+), 20 deletions(-) diff --git a/securedrop_client/gui/widgets.py b/securedrop_client/gui/widgets.py index 06219421b..1821587c7 100644 --- a/securedrop_client/gui/widgets.py +++ b/securedrop_client/gui/widgets.py @@ -1231,10 +1231,7 @@ class StarToggleButton(SvgToggleButton): ''' def __init__(self, controller: Controller, source: Source): - super().__init__( - on='star_on.svg', - off='star_off.svg', - svg_size=QSize(16, 16)) + super().__init__(on='star_on.svg', off='star_off.svg', svg_size=QSize(16, 16)) self.controller = controller self.source = source @@ -1328,7 +1325,7 @@ def on_toggle(self): Tell the controller to make an API call to update the source's starred field. """ if self.is_api_call_enabled: - self.controller.update_star(self.source, self.on_update) + self.controller.update_star(self.source) def on_toggle_offline(self): """ @@ -1336,17 +1333,6 @@ def on_toggle_offline(self): """ self.controller.on_action_requiring_login() - def on_update(self, result): - """ - The result is a uuid for the source and boolean flag for the new state - of the star. - """ - 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. diff --git a/securedrop_client/logic.py b/securedrop_client/logic.py index 3b99591d8..6db8c94f1 100644 --- a/securedrop_client/logic.py +++ b/securedrop_client/logic.py @@ -505,14 +505,12 @@ def on_update_star_failure(self, result: UpdateStarJobException) -> None: self.gui.update_error_status(_('Failed to update star.')) @login_required - def update_star(self, source_db_object, callback): + def update_star(self, source_db_object): """ - Star or unstar. The callback here is the API sync as we first make sure - that we apply the change to the server, and then update locally. + Star or unstar. """ job = UpdateStarJob(source_db_object.uuid, source_db_object.is_starred) job.success_signal.connect(self.on_update_star_success, type=Qt.QueuedConnection) - job.success_signal.connect(callback, type=Qt.QueuedConnection) job.failure_signal.connect(self.on_update_star_failure, type=Qt.QueuedConnection) self.api_job_queue.enqueue(job) From 0fee0680e62f6b4d6dbb646c75d23da361e0e4dc Mon Sep 17 00:00:00 2001 From: Allie Crevier Date: Wed, 18 Mar 2020 18:14:28 -0700 Subject: [PATCH 03/10] remove source db object from star --- securedrop_client/gui/widgets.py | 25 +++++++++++++------------ securedrop_client/logic.py | 4 ++-- 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/securedrop_client/gui/widgets.py b/securedrop_client/gui/widgets.py index 1821587c7..f9a129c72 100644 --- a/securedrop_client/gui/widgets.py +++ b/securedrop_client/gui/widgets.py @@ -1188,7 +1188,7 @@ def update(self): if self.source.document_count == 0: self.paperclip.hide() - self.star.update() + self.star.update(self.source.is_starred) except sqlalchemy.exc.InvalidRequestError as e: logger.error(f"Could not update SourceWidget for source {self.source_uuid}: {e}") raise @@ -1234,7 +1234,8 @@ def __init__(self, controller: Controller, source: Source): super().__init__(on='star_on.svg', off='star_off.svg', svg_size=QSize(16, 16)) self.controller = controller - self.source = source + self.source_uuid = source.uuid + self.is_starred = source.is_starred self.installEventFilter(self) @@ -1257,7 +1258,7 @@ def disable(self): """ self.disable_api_call() self.setCheckable(False) - if self.source.is_starred: + if self.is_starred: self.set_icon(on='star_on.svg', off='star_on.svg') try: @@ -1281,7 +1282,7 @@ def enable(self): self.enable_api_call() self.setCheckable(True) self.set_icon(on='star_on.svg', off='star_off.svg') - self.setChecked(self.source.is_starred) + self.setChecked(self.is_starred) try: while True: @@ -1306,7 +1307,7 @@ def eventFilter(self, obj, event): if checkable: self.set_icon(on='star_on.svg', off='star_off.svg') else: - if self.source.is_starred: + if self.is_starred: self.set_icon(on='star_on.svg', off='star_on.svg') return QObject.event(obj, event) @@ -1325,7 +1326,7 @@ def on_toggle(self): Tell the controller to make an API call to update the source's starred field. """ if self.is_api_call_enabled: - self.controller.update_star(self.source) + self.controller.update_star(self.source_uuid, self.is_starred) def on_toggle_offline(self): """ @@ -1333,14 +1334,14 @@ def on_toggle_offline(self): """ self.controller.on_action_requiring_login() - def update(self): + def update(self, is_starred: bool): """ Update the star to reflect its source's current state. """ - self.controller.session.refresh(self.source) - self.disable_api_call() - self.setChecked(self.source.is_starred) - self.enable_api_call() + if not self.controller.is_authenticated(): + return + + self.setChecked(self.is_starred) def disable_api_call(self): self.is_api_call_enabled = False @@ -1467,7 +1468,7 @@ def __init__(self): effect.setBlurRadius(8) effect.setColor(QColor('#aa000000')) self.setGraphicsEffect(effect) - self.update() + self.selectedIndexes class LoginErrorBar(QWidget): diff --git a/securedrop_client/logic.py b/securedrop_client/logic.py index 6db8c94f1..9bc1a5965 100644 --- a/securedrop_client/logic.py +++ b/securedrop_client/logic.py @@ -505,11 +505,11 @@ def on_update_star_failure(self, result: UpdateStarJobException) -> None: self.gui.update_error_status(_('Failed to update star.')) @login_required - def update_star(self, source_db_object): + def update_star(self, source_uuid: str, is_starred: bool): """ Star or unstar. """ - job = UpdateStarJob(source_db_object.uuid, source_db_object.is_starred) + job = UpdateStarJob(source_uuid, is_starred) job.success_signal.connect(self.on_update_star_success, type=Qt.QueuedConnection) job.failure_signal.connect(self.on_update_star_failure, type=Qt.QueuedConnection) From 5f6c18378013f6e130eefe991eb1ff6ff4987673 Mon Sep 17 00:00:00 2001 From: Allie Crevier Date: Wed, 18 Mar 2020 18:42:35 -0700 Subject: [PATCH 04/10] use source_uuid instead of source.uuid --- securedrop_client/gui/widgets.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/securedrop_client/gui/widgets.py b/securedrop_client/gui/widgets.py index f9a129c72..c73e022b9 100644 --- a/securedrop_client/gui/widgets.py +++ b/securedrop_client/gui/widgets.py @@ -1355,6 +1355,7 @@ class DeleteSourceMessageBox: def __init__(self, source, controller): self.source = source + self.source_uuid = source.uuid self.controller = controller def launch(self): @@ -1370,7 +1371,7 @@ def launch(self): None, "", _(message), QMessageBox.Cancel | QMessageBox.Yes, QMessageBox.Cancel) if reply == QMessageBox.Yes: - logger.debug("Deleting source %s" % (self.source.uuid,)) + logger.debug(f'Deleting source {self.source_uuid}') self.controller.delete_source(self.source) def _construct_message(self, source: Source) -> str: From c4a7e061b176a17e4cf58ebba9bafd6b98f53bb9 Mon Sep 17 00:00:00 2001 From: Allie Crevier Date: Wed, 18 Mar 2020 18:44:37 -0700 Subject: [PATCH 05/10] simplify starring --- securedrop_client/gui/widgets.py | 121 ++++++++++++++----------------- 1 file changed, 54 insertions(+), 67 deletions(-) diff --git a/securedrop_client/gui/widgets.py b/securedrop_client/gui/widgets.py index c73e022b9..4b0972b6e 100644 --- a/securedrop_client/gui/widgets.py +++ b/securedrop_client/gui/widgets.py @@ -1237,117 +1237,104 @@ def __init__(self, controller: Controller, source: Source): self.source_uuid = source.uuid self.is_starred = source.is_starred + self.controller.authentication_state.connect(self.on_authentication_changed) self.installEventFilter(self) 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) + self.pressed.connect(self.on_pressed) + self.setCheckable(True) + self.setChecked(self.is_starred) - def disable(self): + if not self.controller.is_authenticated: + self.disable_toggle() + + def disable_toggle(self): """ - Disable the widget. + Unset `checkable` so that the star cannot be toggled. - Disable toggle by setting checkable to False. Unfortunately, - disabling toggle doesn't freeze state, rather it always - displays the off state when a user tries to toggle. In order - to save on state we update the icon's off state image to - display on (hack). + Disconnect the `pressed` signal from previous handler and connect it to the offline handler. """ - self.disable_api_call() - self.setCheckable(False) + self.pressed.disconnect() + self.pressed.connect(self.on_pressed_offline) + + # If the source is starred, we must update the icon so that the off state continues to show + # the source as starred. We could instead disable the button, which will continue to show + # the star as checked, but Qt will also gray out the star, which we don't want. if self.is_starred: self.set_icon(on='star_on.svg', off='star_on.svg') + self.setCheckable(False) - try: - while True: - self.pressed.disconnect(self.on_toggle_offline) - except Exception as e: - logger.warning("Could not disconnect on_toggle_offline from self.pressed: %s", e) - - try: - while True: - self.toggled.disconnect(self.on_toggle) - except Exception as e: - logger.warning("Could not disconnect on_toggle from self.toggled: %s", e) - - self.pressed.connect(self.on_toggle_offline) - - def enable(self): + def enable_toggle(self): """ Enable the widget. - """ - self.enable_api_call() - self.setCheckable(True) - self.set_icon(on='star_on.svg', off='star_off.svg') - self.setChecked(self.is_starred) - try: - while True: - self.toggled.disconnect(self.on_toggle) - except Exception: - pass - - try: - while True: - self.pressed.disconnect(self.on_toggle_offline) - except Exception: - pass + Disconnect the pressed signal from previous handler, set checkable so that the star can be + toggled, and connect to the online toggle handler. - self.toggled.connect(self.on_toggle) + Note: We must update the icon in case it was modified after being disabled. + """ + self.pressed.disconnect() + self.pressed.connect(self.on_pressed) + self.setCheckable(True) + self.set_icon(on='star_on.svg', off='star_off.svg') # Undo icon change from disable_toggle def eventFilter(self, obj, event): - checkable = self.isCheckable() + """ + If the button is checkable then we show a hover state. + """ + if not self.isCheckable(): + return QObject.event(obj, event) + t = event.type() - if t == QEvent.HoverEnter and checkable: + if t == QEvent.HoverEnter: self.setIcon(load_icon('star_hover.svg')) elif t == QEvent.HoverLeave: - if checkable: - self.set_icon(on='star_on.svg', off='star_off.svg') - else: - if self.is_starred: - self.set_icon(on='star_on.svg', off='star_on.svg') + self.set_icon(on='star_on.svg', off='star_off.svg') + return QObject.event(obj, event) - def on_authentication_changed(self, authenticated: bool): + @pyqtSlot(bool) + def on_authentication_changed(self, authenticated: bool) -> None: """ Set up handlers based on whether or not the user is authenticated. Connect to 'pressed' event instead of 'toggled' event when not authenticated because toggling will be disabled. """ if authenticated: - self.enable() + self.enable_toggle() + self.setChecked(self.is_starred) else: - self.disable() + self.disable_toggle() - def on_toggle(self): + @pyqtSlot() + def on_pressed(self) -> None: """ Tell the controller to make an API call to update the source's starred field. """ - if self.is_api_call_enabled: - self.controller.update_star(self.source_uuid, self.is_starred) + self.controller.update_star(self.source_uuid, self.isChecked()) + self.is_starred = not self.is_starred - def on_toggle_offline(self): + @pyqtSlot() + def on_pressed_offline(self) -> None: """ Show error message when not authenticated. """ self.controller.on_action_requiring_login() - def update(self, is_starred: bool): + @pyqtSlot(bool) + def update(self, is_starred: bool) -> None: """ - Update the star to reflect its source's current state. + If star was updated via the Journalist Interface or by another instance of the client, then + self.is_starred will not match the server and will need to be updated. """ - if not self.controller.is_authenticated(): + if not self.controller.is_authenticated: return - self.setChecked(self.is_starred) - - def disable_api_call(self): - self.is_api_call_enabled = False - - def enable_api_call(self): - self.is_api_call_enabled = True + if self.is_starred != is_starred: + self.is_starred = is_starred + self.setChecked(self.is_starred) class DeleteSourceMessageBox: From 1944f951abe7758eb87e169becde3b778cb20218 Mon Sep 17 00:00:00 2001 From: Allie Crevier Date: Wed, 18 Mar 2020 18:53:14 -0700 Subject: [PATCH 06/10] if star fails, toggle back to previous state --- securedrop_client/api_jobs/updatestar.py | 13 +++++++------ securedrop_client/gui/widgets.py | 12 +++++++++++- securedrop_client/logic.py | 11 ++++++++++- 3 files changed, 28 insertions(+), 8 deletions(-) diff --git a/securedrop_client/api_jobs/updatestar.py b/securedrop_client/api_jobs/updatestar.py index 0bb3ae9d3..cafca2471 100644 --- a/securedrop_client/api_jobs/updatestar.py +++ b/securedrop_client/api_jobs/updatestar.py @@ -12,10 +12,10 @@ class UpdateStarJob(ApiJob): - def __init__(self, source_uuid: str, star_status: bool) -> None: + def __init__(self, source_uuid: str, is_starred: bool) -> None: super().__init__() self.source_uuid = source_uuid - self.star_status = star_status + self.is_starred = is_starred def call_api(self, api_client: API, session: Session) -> Tuple[str, bool]: ''' @@ -30,21 +30,22 @@ def call_api(self, api_client: API, session: Session) -> Tuple[str, bool]: # want to pass the default request timeout to remove_star and add_star instead of # setting it on the api object directly. api_client.default_request_timeout = 5 - if self.star_status: + if self.is_starred: api_client.remove_star(source_sdk_object) else: api_client.add_star(source_sdk_object) # Identify the source and *new* state of the star so the UI can be # updated. - return self.source_uuid, not self.star_status + return self.source_uuid, not self.is_starred except Exception as e: error_message = "Failed to update star on source {uuid} due to {exception}".format( uuid=self.source_uuid, exception=repr(e)) - raise UpdateStarJobException(error_message, self.source_uuid) + raise UpdateStarJobException(error_message, self.source_uuid, self.is_starred) class UpdateStarJobException(Exception): - def __init__(self, message: str, source_uuid: str): + def __init__(self, message: str, source_uuid: str, is_starred: bool) -> None: super().__init__(message) self.source_uuid = source_uuid + self.is_starred = is_starred diff --git a/securedrop_client/gui/widgets.py b/securedrop_client/gui/widgets.py index 4b0972b6e..7b4674f36 100644 --- a/securedrop_client/gui/widgets.py +++ b/securedrop_client/gui/widgets.py @@ -1238,6 +1238,7 @@ def __init__(self, controller: Controller, source: Source): self.is_starred = source.is_starred self.controller.authentication_state.connect(self.on_authentication_changed) + self.controller.star_update_failed.connect(self.on_star_update_failed) self.installEventFilter(self) self.setObjectName('star_button') @@ -1336,6 +1337,15 @@ def update(self, is_starred: bool) -> None: self.is_starred = is_starred self.setChecked(self.is_starred) + @pyqtSlot(str) + def on_star_update_failed(self, source_uuid: str, is_starred: bool) -> None: + """ + If the star update failed to update on the server, toggle back to previous state. + """ + if self.source_uuid == source_uuid: + self.is_starred = is_starred + self.setChecked(is_starred) + class DeleteSourceMessageBox: """Use this to display operation details and confirm user choice.""" @@ -1456,7 +1466,7 @@ def __init__(self): effect.setBlurRadius(8) effect.setColor(QColor('#aa000000')) self.setGraphicsEffect(effect) - self.selectedIndexes + self.update() class LoginErrorBar(QWidget): diff --git a/securedrop_client/logic.py b/securedrop_client/logic.py index 9bc1a5965..8219bdc3e 100644 --- a/securedrop_client/logic.py +++ b/securedrop_client/logic.py @@ -190,6 +190,14 @@ class Controller(QObject): """ source_deleted = pyqtSignal(str) + """ + This signal indicates that a star update request failed. + + Emits: + str: the source UUID + """ + star_update_failed = pyqtSignal(str) + def __init__(self, hostname: str, gui, session_maker: sessionmaker, home: str, proxy: bool = True, qubes: bool = True) -> None: """ @@ -501,8 +509,9 @@ def update_sources(self): def on_update_star_success(self, result) -> None: pass - def on_update_star_failure(self, result: UpdateStarJobException) -> None: + def on_update_star_failure(self, error: UpdateStarJobException) -> None: self.gui.update_error_status(_('Failed to update star.')) + self.gui.star_update_failed.emit(error.source_uuid, error.is_starred) @login_required def update_star(self, source_uuid: str, is_starred: bool): From 82532c31a9352d22b9abe20fce09c5cbd9e49c74 Mon Sep 17 00:00:00 2001 From: Allie Crevier Date: Wed, 18 Mar 2020 19:14:22 -0700 Subject: [PATCH 07/10] support starring retries on timeouts --- securedrop_client/api_jobs/updatestar.py | 31 ++++++++++++------- securedrop_client/gui/widgets.py | 25 ++++++++++++++++ securedrop_client/logic.py | 38 ++++++++++++++---------- 3 files changed, 67 insertions(+), 27 deletions(-) diff --git a/securedrop_client/api_jobs/updatestar.py b/securedrop_client/api_jobs/updatestar.py index cafca2471..77bd0a428 100644 --- a/securedrop_client/api_jobs/updatestar.py +++ b/securedrop_client/api_jobs/updatestar.py @@ -1,9 +1,7 @@ import logging import sdclientapi -from typing import Tuple - -from sdclientapi import API +from sdclientapi import API, RequestTimeoutError, ServerConnectionError from sqlalchemy.orm.session import Session from securedrop_client.api_jobs.base import ApiJob @@ -17,7 +15,7 @@ def __init__(self, source_uuid: str, is_starred: bool) -> None: self.source_uuid = source_uuid self.is_starred = is_starred - def call_api(self, api_client: API, session: Session) -> Tuple[str, bool]: + def call_api(self, api_client: API, session: Session) -> str: ''' Override ApiJob. @@ -35,17 +33,28 @@ def call_api(self, api_client: API, session: Session) -> Tuple[str, bool]: else: api_client.add_star(source_sdk_object) - # Identify the source and *new* state of the star so the UI can be - # updated. - return self.source_uuid, not self.is_starred + return self.source_uuid + except (RequestTimeoutError, ServerConnectionError) as e: + error_message = f'Failed to update star on source {self.source_uuid} due to {e}' + raise UpdateStarJobTimeoutError(error_message, self.source_uuid, self.is_starred) except Exception as e: - error_message = "Failed to update star on source {uuid} due to {exception}".format( - uuid=self.source_uuid, exception=repr(e)) - raise UpdateStarJobException(error_message, self.source_uuid, self.is_starred) + error_message = f'Failed to update star on source {self.source_uuid} due to {e}' + raise UpdateStarJobError(error_message, self.source_uuid, self.is_starred) -class UpdateStarJobException(Exception): +class UpdateStarJobError(Exception): def __init__(self, message: str, source_uuid: str, is_starred: bool) -> None: super().__init__(message) self.source_uuid = source_uuid self.is_starred = is_starred + + +class UpdateStarJobTimeoutError(RequestTimeoutError): + def __init__(self, message: str, source_uuid: str, is_starred: bool) -> None: + super().__init__() + self.message = message + self.source_uuid = source_uuid + self.is_starred = is_starred + + def __str__(self) -> str: + return self.message diff --git a/securedrop_client/gui/widgets.py b/securedrop_client/gui/widgets.py index 7b4674f36..9caf3bda4 100644 --- a/securedrop_client/gui/widgets.py +++ b/securedrop_client/gui/widgets.py @@ -1236,9 +1236,12 @@ def __init__(self, controller: Controller, source: Source): self.controller = controller self.source_uuid = source.uuid self.is_starred = source.is_starred + self.pending_count = 0 + self.wait_until_next_sync = False self.controller.authentication_state.connect(self.on_authentication_changed) self.controller.star_update_failed.connect(self.on_star_update_failed) + self.controller.star_update_successful.connect(self.on_star_update_successful) self.installEventFilter(self) self.setObjectName('star_button') @@ -1304,6 +1307,7 @@ def on_authentication_changed(self, authenticated: bool) -> None: event instead of 'toggled' event when not authenticated because toggling will be disabled. """ if authenticated: + self.pending_count = 0 self.enable_toggle() self.setChecked(self.is_starred) else: @@ -1316,6 +1320,8 @@ def on_pressed(self) -> None: """ self.controller.update_star(self.source_uuid, self.isChecked()) self.is_starred = not self.is_starred + self.pending_count = self.pending_count + 1 + self.wait_until_next_sync = True @pyqtSlot() def on_pressed_offline(self) -> None: @@ -1333,6 +1339,16 @@ def update(self, is_starred: bool) -> None: if not self.controller.is_authenticated: return + # Wait until ongoing star jobs are finished before checking if it matches with the server + if self.pending_count > 0: + return + + # Wait until next sync to avoid the possibility of updating the star with outdated source + # information in case the server just received the star request. + if self.wait_until_next_sync: + self.wait_until_next_sync = False + return + if self.is_starred != is_starred: self.is_starred = is_starred self.setChecked(self.is_starred) @@ -1345,6 +1361,15 @@ def on_star_update_failed(self, source_uuid: str, is_starred: bool) -> None: if self.source_uuid == source_uuid: self.is_starred = is_starred self.setChecked(is_starred) + self.pending_count = self.pending_count - 1 + + @pyqtSlot(str) + def on_star_update_successful(self, source_uuid: str) -> None: + """ + If the star update succeeded, set pending to False so the sync can update the star field + """ + if self.source_uuid == source_uuid: + self.pending_count = self.pending_count - 1 class DeleteSourceMessageBox: diff --git a/securedrop_client/logic.py b/securedrop_client/logic.py index 8219bdc3e..e578fc8ac 100644 --- a/securedrop_client/logic.py +++ b/securedrop_client/logic.py @@ -34,14 +34,13 @@ from securedrop_client import storage from securedrop_client import db from securedrop_client.api_jobs.base import ApiInaccessibleError -from securedrop_client.api_jobs.downloads import ( - DownloadChecksumMismatchException, DownloadDecryptionException, FileDownloadJob, - MessageDownloadJob, ReplyDownloadJob, -) +from securedrop_client.api_jobs.downloads import DownloadChecksumMismatchException, \ + DownloadDecryptionException, FileDownloadJob, MessageDownloadJob, ReplyDownloadJob from securedrop_client.api_jobs.sources import DeleteSourceJob from securedrop_client.api_jobs.uploads import SendReplyJob, SendReplyJobError, \ SendReplyJobTimeoutError -from securedrop_client.api_jobs.updatestar import UpdateStarJob, UpdateStarJobException +from securedrop_client.api_jobs.updatestar import UpdateStarJob, UpdateStarJobError, \ + UpdateStarJobTimeoutError from securedrop_client.crypto import GpgHelper from securedrop_client.export import Export from securedrop_client.queue import ApiJobQueue @@ -190,6 +189,14 @@ class Controller(QObject): """ source_deleted = pyqtSignal(str) + """ + This signal indicates that a star update request succeeded. + + Emits: + str: the source UUID + """ + star_update_successful = pyqtSignal(str) + """ This signal indicates that a star update request failed. @@ -506,12 +513,16 @@ def update_sources(self): sources.sort(key=lambda x: x.last_updated) self.gui.show_sources(sources) - def on_update_star_success(self, result) -> None: - pass + def on_update_star_success(self, source_uuid: str) -> None: + self.star_update_successful.emit(source_uuid) - def on_update_star_failure(self, error: UpdateStarJobException) -> None: - self.gui.update_error_status(_('Failed to update star.')) - self.gui.star_update_failed.emit(error.source_uuid, error.is_starred) + def on_update_star_failure( + self, + error: Union[UpdateStarJobError, UpdateStarJobTimeoutError] + ) -> None: + if isinstance(error, UpdateStarJobError): + self.gui.update_error_status(_('Failed to update star.')) + self.star_update_failed.emit(error.source_uuid, error.is_starred) @login_required def update_star(self, source_uuid: str, is_starred: bool): @@ -814,12 +825,7 @@ def send_reply(self, source_uuid: str, reply_uuid: str, message: str) -> None: self.session.add(draft_reply) self.session.commit() - job = SendReplyJob( - source_uuid, - reply_uuid, - message, - self.gpg, - ) + job = SendReplyJob(source_uuid, reply_uuid, message, self.gpg) job.success_signal.connect(self.on_reply_success, type=Qt.QueuedConnection) job.failure_signal.connect(self.on_reply_failure, type=Qt.QueuedConnection) From 8c1a66911560d2f7b7ad4494f53bc05f0c804e42 Mon Sep 17 00:00:00 2001 From: Allie Crevier Date: Thu, 19 Mar 2020 17:50:00 -0700 Subject: [PATCH 08/10] update test --- securedrop_client/api_jobs/updatestar.py | 2 +- securedrop_client/gui/widgets.py | 9 +- tests/api_jobs/test_updatestar.py | 32 +- tests/gui/test_init.py | 36 --- tests/gui/test_widgets.py | 357 ++++++++++++++++++----- tests/test_logic.py | 82 +++--- 6 files changed, 370 insertions(+), 148 deletions(-) diff --git a/securedrop_client/api_jobs/updatestar.py b/securedrop_client/api_jobs/updatestar.py index 77bd0a428..e2473a686 100644 --- a/securedrop_client/api_jobs/updatestar.py +++ b/securedrop_client/api_jobs/updatestar.py @@ -35,7 +35,7 @@ def call_api(self, api_client: API, session: Session) -> str: return self.source_uuid except (RequestTimeoutError, ServerConnectionError) as e: - error_message = f'Failed to update star on source {self.source_uuid} due to {e}' + error_message = f'Failed to update star on source {self.source_uuid} due to error: {e}' raise UpdateStarJobTimeoutError(error_message, self.source_uuid, self.is_starred) except Exception as e: error_message = f'Failed to update star on source {self.source_uuid} due to {e}' diff --git a/securedrop_client/gui/widgets.py b/securedrop_client/gui/widgets.py index 9caf3bda4..49f278291 100644 --- a/securedrop_client/gui/widgets.py +++ b/securedrop_client/gui/widgets.py @@ -1089,7 +1089,6 @@ def __init__(self, controller: Controller, source: Source): # Store source self.source_uuid = source.uuid self.source = source - self.source_uuid = source.uuid # Set styles self.setStyleSheet(self.CSS) @@ -1115,7 +1114,7 @@ def __init__(self, controller: Controller, source: Source): gutter_layout = QVBoxLayout(self.gutter) gutter_layout.setContentsMargins(0, 0, 0, 0) gutter_layout.setSpacing(0) - self.star = StarToggleButton(self.controller, self.source) + self.star = StarToggleButton(self.controller, self.source_uuid, source.is_starred) gutter_layout.addWidget(self.star) gutter_layout.addStretch() @@ -1230,12 +1229,12 @@ class StarToggleButton(SvgToggleButton): } ''' - def __init__(self, controller: Controller, source: Source): + def __init__(self, controller: Controller, source_uuid: str, is_starred: bool): super().__init__(on='star_on.svg', off='star_off.svg', svg_size=QSize(16, 16)) self.controller = controller - self.source_uuid = source.uuid - self.is_starred = source.is_starred + self.source_uuid = source_uuid + self.is_starred = is_starred self.pending_count = 0 self.wait_until_next_sync = False diff --git a/tests/api_jobs/test_updatestar.py b/tests/api_jobs/test_updatestar.py index 6f95efb34..17c8a798f 100644 --- a/tests/api_jobs/test_updatestar.py +++ b/tests/api_jobs/test_updatestar.py @@ -1,6 +1,9 @@ import pytest -from securedrop_client.api_jobs.updatestar import UpdateStarJob, UpdateStarJobException +from securedrop_client.api_jobs.updatestar import UpdateStarJob, UpdateStarJobError, \ + UpdateStarJobTimeoutError +from sdclientapi import RequestTimeoutError, ServerConnectionError + from tests import factory @@ -61,9 +64,9 @@ def test_unstar_if_star(homedir, mocker, session, session_maker): api_client.remove_star.assert_called_once_with(mock_sdk_source) -def test_failure_to_star(homedir, mocker, session, session_maker): +def test_call_api_raises_UpdateStarJobError(homedir, mocker, session, session_maker): ''' - Check if we call remove_star method if a source is stared. + Check that UpdateStarJobError is raised if remove_star fails due to an exception. ''' source = factory.Source() source.is_starred = True @@ -80,5 +83,26 @@ def test_failure_to_star(homedir, mocker, session, session_maker): source.is_starred ) - with pytest.raises(UpdateStarJobException): + with pytest.raises(UpdateStarJobError): + job.call_api(api_client, session) + + +@pytest.mark.parametrize("exception", [RequestTimeoutError, ServerConnectionError]) +def test_call_api_raises_UpdateStarJobTimeoutError(mocker, session, exception): + ''' + Check that UpdateStarJobTimeoutError is raised if remove_star fails due to a timeout. + ''' + source = factory.Source() + source.is_starred = True + session.add(source) + session.commit() + + api_client = mocker.MagicMock() + api_client.remove_star = mocker.MagicMock() + api_client.remove_star.side_effect = exception() + + job = UpdateStarJob(source.uuid, source.is_starred) + + error = f'Failed to update star on source {source.uuid} due to error' + with pytest.raises(UpdateStarJobTimeoutError, match=error): job.call_api(api_client, session) diff --git a/tests/gui/test_init.py b/tests/gui/test_init.py index 8858285fe..a16096379 100644 --- a/tests/gui/test_init.py +++ b/tests/gui/test_init.py @@ -28,24 +28,6 @@ def test_SvgToggleButton_init(mocker): setIconSize_fn.assert_called_once_with(svg_size) -def test_SvgToggleButton_enable(mocker): - """ - Ensure enable. - """ - stb = SvgToggleButton(on='mock_on', off='mock_off') - stb.enable() - assert stb.isEnabled() is True - - -def test_SvgToggleButton_disable(mocker): - """ - Ensure disable. - """ - stb = SvgToggleButton(on='mock_on', off='mock_off') - stb.disable() - assert stb.isEnabled() is False - - def test_SvgToggleButton_toggle(mocker): """ Make sure we're not calling this a toggle button for no reason. @@ -95,24 +77,6 @@ def test_SvgPushButton_init(mocker): setIconSize_fn.assert_called_once_with(svg_size) -def test_SvgPushButton_enable(mocker): - """ - Ensure enable. - """ - spb = SvgPushButton(normal='mock1', disabled='mock2', active='mock3', selected='mock4') - spb.enable() - assert spb.isEnabled() is True - - -def test_SvgPushButton_disable(mocker): - """ - Ensure disable. - """ - spb = SvgPushButton(normal='mock1', disabled='mock2', active='mock3', selected='mock4') - spb.disable() - assert spb.isEnabled() is False - - def test_SvgLabel_init(mocker): """ Ensure SvgLabel calls the expected methods correctly to set the icon and size. diff --git a/tests/gui/test_widgets.py b/tests/gui/test_widgets.py index 0d243d4e0..22dcd8864 100644 --- a/tests/gui/test_widgets.py +++ b/tests/gui/test_widgets.py @@ -1162,65 +1162,158 @@ def test_SourceWidget_uses_SecureQLabel(mocker): def test_StarToggleButton_init_source_starred(mocker): controller = mocker.MagicMock() - source = factory.Source() - source.is_starred = True + source = factory.Source(is_starred=True) - stb = StarToggleButton(controller, source) + stb = StarToggleButton(controller, source.uuid, source.is_starred) - assert stb.source == source + assert stb.source_uuid == source.uuid assert stb.isChecked() is True def test_StarToggleButton_init_source_unstarred(mocker): controller = mocker.MagicMock() - source = factory.Source() - source.is_starred = False + source = factory.Source(is_starred=False) - stb = StarToggleButton(controller, source) + stb = StarToggleButton(controller, source.uuid, source.is_starred) - assert stb.source == source + assert stb.source_uuid == source.uuid assert stb.isChecked() is False -def test_StarToggleButton_eventFilter(mocker): +def test_StarToggleButton_eventFilter_when_checked(mocker): """ - Ensure the hover events are handled properly. + Ensure the hover events are handled properly when star is checked and online. """ controller = mocker.MagicMock() - stb = StarToggleButton(controller=controller, source=mocker.MagicMock()) + controller.is_authenticated = True + stb = StarToggleButton(controller, 'mock_uuid', True) + stb.pressed = mocker.MagicMock() stb.setIcon = mocker.MagicMock() stb.set_icon = mocker.MagicMock() + load_icon_fn = mocker.patch("securedrop_client.gui.widgets.load_icon") + # Hover enter test_event = QEvent(QEvent.HoverEnter) stb.eventFilter(stb, test_event) assert stb.setIcon.call_count == 1 + load_icon_fn.assert_called_once_with('star_hover.svg') + # Hover leave test_event = QEvent(QEvent.HoverLeave) stb.eventFilter(stb, test_event) stb.set_icon.assert_called_once_with(on='star_on.svg', off='star_off.svg') - # Hover leave when disabled - stb.disable() + # Authentication change + stb.on_authentication_changed(authenticated=True) + assert stb.isCheckable() is True + stb.pressed.disconnect.assert_called_once_with() + stb.pressed.connect.assert_called_once_with(stb.on_pressed) + + +def test_StarToggleButton_eventFilter_when_not_checked(mocker): + """ + Ensure the hover events are handled properly when star is checked and online. + """ + controller = mocker.MagicMock() + controller.is_authenticated = True + stb = StarToggleButton(controller, 'mock_uuid', False) + stb.pressed = mocker.MagicMock() + stb.setIcon = mocker.MagicMock() + stb.set_icon = mocker.MagicMock() + load_icon_fn = mocker.patch("securedrop_client.gui.widgets.load_icon") + + # Hover enter + test_event = QEvent(QEvent.HoverEnter) + stb.eventFilter(stb, test_event) + assert stb.setIcon.call_count == 1 + load_icon_fn.assert_called_once_with('star_hover.svg') + + # Hover leave test_event = QEvent(QEvent.HoverLeave) stb.eventFilter(stb, test_event) + stb.set_icon.assert_called_once_with(on='star_on.svg', off='star_off.svg') + + # Authentication change + stb.on_authentication_changed(authenticated=True) + assert stb.isCheckable() is True + stb.pressed.disconnect.assert_called_once_with() + stb.pressed.connect.assert_called_once_with(stb.on_pressed) + + +def test_StarToggleButton_eventFilter_when_checked_and_offline(mocker): + """ + Ensure the hover events do not change the icon when offline and that the star icon is set to + off='star_on.svg' when checked and offline. + """ + controller = mocker.MagicMock() + stb = StarToggleButton(controller, 'mock_uuid', True) + stb.pressed = mocker.MagicMock() + stb.setIcon = mocker.MagicMock() + stb.set_icon = mocker.MagicMock() + load_icon_fn = mocker.patch("securedrop_client.gui.widgets.load_icon") + + # Authentication change + stb.on_authentication_changed(authenticated=False) + assert stb.isCheckable() is False stb.set_icon.assert_called_with(on='star_on.svg', off='star_on.svg') + stb.pressed.disconnect.assert_called_once_with() + stb.pressed.connect.assert_called_once_with(stb.on_pressed_offline) + + # Hover enter + test_event = QEvent(QEvent.HoverEnter) + stb.eventFilter(stb, test_event) + stb.setIcon.assert_not_called() + load_icon_fn.assert_not_called() + + # Hover leave + test_event = QEvent(QEvent.HoverLeave) + stb.eventFilter(stb, test_event) + stb.setIcon.assert_not_called() + + +def test_StarToggleButton_eventFilter_when_not_checked_and_offline(mocker): + """ + Ensure the hover events do not change the icon when offline and that the star icon is set to + off='star_on.svg' when unchecked and offline. + """ + controller = mocker.MagicMock() + stb = StarToggleButton(controller, 'mock_uuid', False) + stb.pressed = mocker.MagicMock() + stb.setIcon = mocker.MagicMock() + stb.set_icon = mocker.MagicMock() + load_icon_fn = mocker.patch("securedrop_client.gui.widgets.load_icon") + + # Authentication change + stb.on_authentication_changed(authenticated=False) + assert stb.isCheckable() is False + stb.pressed.disconnect.assert_called_once_with() + stb.pressed.connect.assert_called_once_with(stb.on_pressed_offline) + + # Hover enter + test_event = QEvent(QEvent.HoverEnter) + stb.eventFilter(stb, test_event) + stb.setIcon.assert_not_called() + load_icon_fn.assert_not_called() + + # Hover leave + test_event = QEvent(QEvent.HoverLeave) + stb.eventFilter(stb, test_event) + stb.setIcon.assert_not_called() def test_StarToggleButton_on_authentication_changed_while_authenticated_and_checked(mocker): """ - If on_authentication_changed is set up correctly, then calling toggle on a checked button should - result in the button being unchecked. + If on_authentication_changed is set up correctly, then toggling a checked button should result + in the button being unchecked. """ controller = mocker.MagicMock() - source = mocker.MagicMock() - stb = StarToggleButton(controller, source=source) - stb.setChecked(True) - stb.on_toggle = mocker.MagicMock() + stb = StarToggleButton(controller, 'mock_uuid', True) + stb.on_pressed = mocker.MagicMock() stb.on_authentication_changed(authenticated=True) - stb.toggle() + stb.click() - assert stb.on_toggle.called is True + stb.on_pressed.assert_called_once_with() assert stb.isChecked() is False @@ -1230,97 +1323,231 @@ def test_StarToggleButton_on_authentication_changed_while_authenticated_and_not_ should result in the button being unchecked. """ controller = mocker.MagicMock() - source = mocker.MagicMock() - source.is_starred = False - stb = StarToggleButton(controller, source=source) - stb.setChecked(False) - stb.on_toggle = mocker.MagicMock() - assert stb.isChecked() is False - + stb = StarToggleButton(controller, 'mock_uuid', False) + stb.on_pressed = mocker.MagicMock() stb.on_authentication_changed(authenticated=True) - assert stb.isChecked() is False - stb.toggle() + stb.click() - assert stb.on_toggle.called is True + stb.on_pressed.assert_called_once_with() assert stb.isChecked() is True -def test_StarToggleButton_on_authentication_changed_while_offline_mode(mocker): +def test_StarToggleButton_on_authentication_changed_while_offline_mode_and_not_checked(mocker): """ Ensure on_authentication_changed is set up correctly for offline mode. """ controller = mocker.MagicMock() - source = mocker.MagicMock() - stb = StarToggleButton(controller, source=source) - stb.on_toggle_offline = mocker.MagicMock() - stb.on_toggle = mocker.MagicMock() + stb = StarToggleButton(controller, 'mock_uuid', False) + stb.on_pressed_offline = mocker.MagicMock() + stb.on_pressed = mocker.MagicMock() + stb.on_authentication_changed(authenticated=False) + + stb.click() + + stb.on_pressed_offline.assert_called_once_with() + stb.on_pressed.assert_not_called() + assert stb.isChecked() is False + +def test_StarToggleButton_on_authentication_changed_while_offline_mode_and_checked(mocker): + """ + Ensure on_authentication_changed is set up correctly for offline mode. + """ + controller = mocker.MagicMock() + stb = StarToggleButton(controller, 'mock_uuid', True) + stb.on_pressed_offline = mocker.MagicMock() + stb.on_pressed = mocker.MagicMock() stb.on_authentication_changed(authenticated=False) + stb.click() - assert stb.on_toggle_offline.called is True - assert stb.on_toggle.called is False + stb.on_pressed_offline.assert_called_once_with() + stb.on_pressed.assert_not_called() + assert stb.isCheckable() is False + assert stb.is_starred is True -def test_StarToggleButton_on_toggle(mocker): +def test_StarToggleButton_on_pressed_toggles_to_starred(mocker): """ - Ensure correct star icon images are loaded for the enabled button. + Ensure pressing the star button toggles from unstarred to starred. """ controller = mocker.MagicMock() - source = mocker.MagicMock() - stb = StarToggleButton(controller, source) + stb = StarToggleButton(controller, 'mock_uuid', False) - stb.on_toggle() + stb.click() - stb.controller.update_star.assert_called_once_with(source, stb.on_update) + stb.controller.update_star.assert_called_once_with('mock_uuid', False) + assert stb.isChecked() -def test_StarToggleButton_on_toggle_offline(mocker): +def test_StarToggleButton_on_pressed_toggles_to_unstarred(mocker): + """ + Ensure pressing the star button toggles from starred to unstarred. + """ + controller = mocker.MagicMock() + stb = StarToggleButton(controller, 'mock_uuid', True) + + stb.click() + + stb.controller.update_star.assert_called_once_with('mock_uuid', True) + assert not stb.isChecked() + + +def test_StarToggleButton_on_pressed_offline(mocker): """ Ensure toggle is disabled when offline. """ controller = mocker.MagicMock() - source = mocker.MagicMock() - stb = StarToggleButton(controller, source) + controller.is_authenticated = False + stb = StarToggleButton(controller, 'mock_uuid', True) + + stb.click() - stb.on_toggle_offline() stb.controller.on_action_requiring_login.assert_called_once_with() -def test_StarToggleButton_on_toggle_offline_when_checked(mocker): +def test_StarToggleButton_on_pressed_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(controller, source) + controller.is_authenticated = False + source = factory.Source(is_starred=True) + stb = StarToggleButton(controller, source.uuid, source.is_starred) set_icon_fn = mocker.patch('securedrop_client.gui.SvgToggleButton.set_icon') - # go offline stb.on_authentication_changed(False) assert stb.isCheckable() is False set_icon_fn.assert_called_with(on='star_on.svg', off='star_on.svg') - stb.on_toggle_offline() + stb.click() stb.controller.on_action_requiring_login.assert_called_once_with() -def test_StarToggleButton_on_update(mocker): +def test_StarToggleButton_update(mocker): """ - Ensure the on_update callback updates the state of the source and UI - element to the current "enabled" state. + Ensure update syncs the star state with the server if there are no pending jobs and we're not + waiting until the next sync (in order to avoid the "ghost" issue where update is called with an + outdated state between a star job finishing and a sync). """ controller = mocker.MagicMock() - source = mocker.MagicMock() - source.is_starred = True - stb = StarToggleButton(controller, source) - stb.setChecked = mocker.MagicMock() - stb.on_update(("uuid", False)) - assert source.is_starred is False - stb.controller.update_sources.assert_called_once_with() - stb.setChecked.assert_called_once_with(False) + controller.is_authenticated = True + stb = StarToggleButton(controller, 'mock_uuid', True) + + # Should not change because we wait until next sync + stb.pending_count = 0 + stb.wait_until_next_sync = True + stb.update(False) + assert stb.isChecked() is True + stb.update(True) + assert stb.isChecked() is True + + # Should update to match value provided by update because there are no pending star jobs and + # wait_until_next_sync is False, meaning a sync already occurred after the star job finished + stb.setChecked(True) + stb.pending_count = 0 + stb.wait_until_next_sync = False + stb.update(False) + assert stb.isChecked() is False + stb.update(True) + assert stb.isChecked() is True + + # Should not change because there are pending star jobs + stb.setChecked(True) + stb.pending_count = 1 + stb.wait_until_next_sync = True + stb.update(False) + assert stb.isChecked() is True + stb.update(True) + assert stb.isChecked() is True + # Still should not change because there are pending star jobs + stb.wait_until_next_sync = False + stb.update(False) + assert stb.isChecked() is True + stb.update(True) + assert stb.isChecked() is True + + +def test_StarToggleButton_update_when_not_authenticated(mocker): + """ + Ensure the button state does not change if the user is not authenticated. + """ + controller = mocker.MagicMock() + controller.is_authenticated = False + source = factory.Source(is_starred=True) + stb = StarToggleButton(controller, source.uuid, source.is_starred) + + # Button stays checked + stb.update(False) + assert stb.is_starred is True + + # Button stays unchecked + stb.is_starred = False + stb.update(True) + assert stb.is_starred is False + + +def test_StarToggleButton_on_star_update_failed(mocker): + ''' + Ensure the button is toggled to the state provided in the failure handler and that the pending + count is decremented if the source uuid matches. + ''' + controller = mocker.MagicMock() + controller.is_authenticated = True + stb = StarToggleButton(controller, 'mock_uuid', False) + + stb.click() + assert stb.is_starred is True + assert stb.pending_count == 1 + stb.on_star_update_failed('mock_uuid', is_starred=False) + assert stb.is_starred is False + assert stb.pending_count == 0 + + +def test_StarToggleButton_on_star_update_failed_for_non_matching_source_uuid(mocker): + ''' + Ensure the button is not toggled and that the pending count stays the same if the source uuid + does not match. + ''' + controller = mocker.MagicMock() + controller.is_authenticated = True + stb = StarToggleButton(controller, 'mock_uuid', False) + + stb.click() + assert stb.is_starred is True + assert stb.pending_count == 1 + stb.on_star_update_failed('some_other_uuid', is_starred=False) + assert stb.is_starred is True + assert stb.pending_count == 1 + + +def test_StarToggleButton_on_star_update_successful(mocker): + ''' + Ensure that the pending count is decremented if the source uuid matches. + ''' + controller = mocker.MagicMock() + controller.is_authenticated = True + stb = StarToggleButton(controller, 'mock_uuid', True) + + stb.click() + assert stb.pending_count == 1 + stb.on_star_update_successful('mock_uuid') + assert stb.pending_count == 0 + + +def test_StarToggleButton_on_star_update_successful_for_non_matching_source_uuid(mocker): + ''' + Ensure that the pending count is not decremented if the source uuid does not match. + ''' + controller = mocker.MagicMock() + controller.is_authenticated = True + stb = StarToggleButton(controller, 'mock_uuid', True) + + stb.click() + assert stb.pending_count == 1 + stb.on_star_update_successful('some_other_uuid') + assert stb.pending_count == 1 def test_LoginDialog_setup(mocker, i18n): diff --git a/tests/test_logic.py b/tests/test_logic.py index ebd0a9088..5cf045cf2 100644 --- a/tests/test_logic.py +++ b/tests/test_logic.py @@ -13,9 +13,9 @@ from securedrop_client import db from securedrop_client.logic import APICallRunner, Controller, TIME_BETWEEN_SHOWING_LAST_SYNC_MS from securedrop_client.api_jobs.base import ApiInaccessibleError -from securedrop_client.api_jobs.downloads import ( - DownloadChecksumMismatchException, DownloadDecryptionException, DownloadException -) +from securedrop_client.api_jobs.downloads import DownloadChecksumMismatchException, \ + DownloadDecryptionException, DownloadException +from securedrop_client.api_jobs.updatestar import UpdateStarJobError, UpdateStarJobTimeoutError from securedrop_client.api_jobs.uploads import SendReplyJobError, SendReplyJobTimeoutError with open(os.path.join(os.path.dirname(__file__), 'files', 'test-key.gpg.pub.asc')) as f: @@ -571,23 +571,41 @@ def test_Controller_on_update_star_success(homedir, config, mocker, session_make """ mock_gui = mocker.MagicMock() co = Controller('http://localhost', mock_gui, session_maker, homedir) - result = True - co.call_reset = mocker.MagicMock() - co.on_update_star_success(result) + co.star_update_failed = mocker.MagicMock() + co.on_update_star_success('mock_uuid') -def test_Controller_on_update_star_failed(homedir, config, mocker, session_maker): + +def test_Controller_on_update_star_failed(homedir, config, mocker): """ - If the starring does not occur properly, then an error should appear - on the error status sidebar, and a sync will not occur. - Using the `config` fixture to ensure the config is written to disk. + Check that if starring fails then the failure signal is emitted and the error bar is updated + with a failure message. """ - mock_gui = mocker.MagicMock() - co = Controller('http://localhost', mock_gui, session_maker, homedir) - result = Exception('boom') - co.call_reset = mocker.MagicMock() - co.on_update_star_failure(result) - mock_gui.update_error_status.assert_called_once_with('Failed to update star.') + gui = mocker.MagicMock() + co = Controller('http://localhost', gui, mocker.MagicMock(), homedir) + co.star_update_failed = mocker.MagicMock() + + error = UpdateStarJobError('mock_message', 'mock_uuid', True) + co.on_update_star_failure(error) + + co.star_update_failed.emit.assert_called_once_with(error.source_uuid, error.is_starred) + gui.update_error_status.assert_called_once_with('Failed to update star.') + + +def test_Controller_on_update_star_failed_due_to_timeout(homedir, config, mocker): + """ + Ensure the failure signal is not emitted and the error bar is not updated if the star fails due + to a timeout (regression test). + """ + gui = mocker.MagicMock() + co = Controller('http://localhost', gui, mocker.MagicMock(), homedir) + co.star_update_failed = mocker.MagicMock() + + error = UpdateStarJobTimeoutError('mock_message', 'mock_uuid', True) + co.on_update_star_failure(error) + + co.star_update_failed.emit.assert_not_called() + gui.update_error_status.assert_not_called() def test_Controller_invalidate_token(mocker, homedir, session_maker): @@ -1559,36 +1577,26 @@ def test_Controller_call_update_star_success(homedir, config, mocker, session_ma co.call_api = mocker.MagicMock() co.api = mocker.MagicMock() - mock_success_signal = mocker.MagicMock() - mock_failure_signal = mocker.MagicMock() - mock_job = mocker.MagicMock(success_signal=mock_success_signal, - failure_signal=mock_failure_signal) - mock_job_cls = mocker.patch( - "securedrop_client.logic.UpdateStarJob", return_value=mock_job) + star_update_successful = mocker.MagicMock() + star_update_failed = mocker.MagicMock() + mock_job = mocker.MagicMock(success_signal=star_update_successful, + failure_signal=star_update_failed) + mock_job_cls = mocker.patch("securedrop_client.logic.UpdateStarJob", return_value=mock_job) mock_queue = mocker.patch.object(co, 'api_job_queue') source = factory.Source() session.add(source) session.commit() - mock_callback = mocker.MagicMock() - - co.update_star(source, mock_callback) - - mock_job_cls.assert_called_once_with( - source.uuid, - source.is_starred - ) + co.update_star(source.uuid, source.is_starred) + mock_job_cls.assert_called_once_with(source.uuid, source.is_starred) mock_queue.enqueue.assert_called_once_with(mock_job) - assert mock_success_signal.connect.call_count == 2 - cal = mock_success_signal.connect.call_args_list - assert cal[0][0][0] == co.on_update_star_success - assert cal[0][1]['type'] == Qt.QueuedConnection - assert cal[1][0][0] == mock_callback - assert cal[1][1]['type'] == Qt.QueuedConnection - mock_failure_signal.connect.assert_called_once_with( + assert star_update_successful.connect.call_count == 1 + star_update_failed.connect.assert_called_once_with( co.on_update_star_failure, type=Qt.QueuedConnection) + star_update_successful.connect.assert_called_once_with( + co.on_update_star_success, type=Qt.QueuedConnection) def test_Controller_run_printer_preflight_checks(homedir, mocker, session, source): From 14047732113ab137b2a97aa97434b33089454d0c Mon Sep 17 00:00:00 2001 From: Allie Crevier Date: Fri, 20 Mar 2020 00:00:03 -0700 Subject: [PATCH 09/10] update star functional test --- securedrop_client/api_jobs/updatestar.py | 10 ++++------ securedrop_client/gui/widgets.py | 4 ++-- securedrop_client/logic.py | 6 ++++-- tests/functional/test_star_source.py | 2 +- tests/functional/test_unstar_source.py | 2 +- tests/test_logic.py | 8 +++++--- 6 files changed, 17 insertions(+), 15 deletions(-) diff --git a/securedrop_client/api_jobs/updatestar.py b/securedrop_client/api_jobs/updatestar.py index e2473a686..ce9f2fac3 100644 --- a/securedrop_client/api_jobs/updatestar.py +++ b/securedrop_client/api_jobs/updatestar.py @@ -36,25 +36,23 @@ def call_api(self, api_client: API, session: Session) -> str: return self.source_uuid except (RequestTimeoutError, ServerConnectionError) as e: error_message = f'Failed to update star on source {self.source_uuid} due to error: {e}' - raise UpdateStarJobTimeoutError(error_message, self.source_uuid, self.is_starred) + raise UpdateStarJobTimeoutError(error_message, self.source_uuid) except Exception as e: error_message = f'Failed to update star on source {self.source_uuid} due to {e}' - raise UpdateStarJobError(error_message, self.source_uuid, self.is_starred) + raise UpdateStarJobError(error_message, self.source_uuid) class UpdateStarJobError(Exception): - def __init__(self, message: str, source_uuid: str, is_starred: bool) -> None: + def __init__(self, message: str, source_uuid: str) -> None: super().__init__(message) self.source_uuid = source_uuid - self.is_starred = is_starred class UpdateStarJobTimeoutError(RequestTimeoutError): - def __init__(self, message: str, source_uuid: str, is_starred: bool) -> None: + def __init__(self, message: str, source_uuid: str) -> None: super().__init__() self.message = message self.source_uuid = source_uuid - self.is_starred = is_starred def __str__(self) -> str: return self.message diff --git a/securedrop_client/gui/widgets.py b/securedrop_client/gui/widgets.py index 49f278291..f8d1fd7f8 100644 --- a/securedrop_client/gui/widgets.py +++ b/securedrop_client/gui/widgets.py @@ -1352,14 +1352,14 @@ def update(self, is_starred: bool) -> None: self.is_starred = is_starred self.setChecked(self.is_starred) - @pyqtSlot(str) + @pyqtSlot(str, bool) def on_star_update_failed(self, source_uuid: str, is_starred: bool) -> None: """ If the star update failed to update on the server, toggle back to previous state. """ if self.source_uuid == source_uuid: self.is_starred = is_starred - self.setChecked(is_starred) + self.setChecked(self.is_starred) self.pending_count = self.pending_count - 1 @pyqtSlot(str) diff --git a/securedrop_client/logic.py b/securedrop_client/logic.py index e578fc8ac..a8bcb04e0 100644 --- a/securedrop_client/logic.py +++ b/securedrop_client/logic.py @@ -202,8 +202,9 @@ class Controller(QObject): Emits: str: the source UUID + bool: is_starred """ - star_update_failed = pyqtSignal(str) + star_update_failed = pyqtSignal(str, bool) def __init__(self, hostname: str, gui, session_maker: sessionmaker, home: str, proxy: bool = True, qubes: bool = True) -> None: @@ -522,7 +523,8 @@ def on_update_star_failure( ) -> None: if isinstance(error, UpdateStarJobError): self.gui.update_error_status(_('Failed to update star.')) - self.star_update_failed.emit(error.source_uuid, error.is_starred) + source = self.session.query(db.Source).filter_by(uuid=error.source_uuid).one() + self.star_update_failed.emit(error.source_uuid, source.is_starred) @login_required def update_star(self, source_uuid: str, is_starred: bool): diff --git a/tests/functional/test_star_source.py b/tests/functional/test_star_source.py index f48b9ae9f..001b06994 100644 --- a/tests/functional/test_star_source.py +++ b/tests/functional/test_star_source.py @@ -34,4 +34,4 @@ def check_for_sources(): qtbot.mouseClick(first_source_widget.star, Qt.LeftButton) qtbot.wait(1000) # Check the source is now checked. - assert first_source_widget.star.source.is_starred is True + assert first_source_widget.star.is_starred is True diff --git a/tests/functional/test_unstar_source.py b/tests/functional/test_unstar_source.py index a44e78af8..a9e4f9a54 100644 --- a/tests/functional/test_unstar_source.py +++ b/tests/functional/test_unstar_source.py @@ -34,4 +34,4 @@ def check_for_sources(): qtbot.mouseClick(first_source_widget.star, Qt.LeftButton) qtbot.wait(1000) # Check the source isn't checked once more. - assert first_source_widget.star.source.is_starred is False + assert first_source_widget.star.is_starred is False diff --git a/tests/test_logic.py b/tests/test_logic.py index 5cf045cf2..ff1beb9f4 100644 --- a/tests/test_logic.py +++ b/tests/test_logic.py @@ -584,11 +584,13 @@ def test_Controller_on_update_star_failed(homedir, config, mocker): gui = mocker.MagicMock() co = Controller('http://localhost', gui, mocker.MagicMock(), homedir) co.star_update_failed = mocker.MagicMock() + source = factory.Source() + co.session.query().filter_by().one.return_value = source - error = UpdateStarJobError('mock_message', 'mock_uuid', True) + error = UpdateStarJobError('mock_message', source.uuid) co.on_update_star_failure(error) - co.star_update_failed.emit.assert_called_once_with(error.source_uuid, error.is_starred) + co.star_update_failed.emit.assert_called_once_with(source.uuid, source.is_starred) gui.update_error_status.assert_called_once_with('Failed to update star.') @@ -601,7 +603,7 @@ def test_Controller_on_update_star_failed_due_to_timeout(homedir, config, mocker co = Controller('http://localhost', gui, mocker.MagicMock(), homedir) co.star_update_failed = mocker.MagicMock() - error = UpdateStarJobTimeoutError('mock_message', 'mock_uuid', True) + error = UpdateStarJobTimeoutError('mock_message', 'mock_uuid') co.on_update_star_failure(error) co.star_update_failed.emit.assert_not_called() From e26c538819357f6d11686c7756723eb5f64e8efe Mon Sep 17 00:00:00 2001 From: Allie Crevier Date: Mon, 23 Mar 2020 11:11:58 -0700 Subject: [PATCH 10/10] schedule undo --- securedrop_client/gui/widgets.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/securedrop_client/gui/widgets.py b/securedrop_client/gui/widgets.py index f8d1fd7f8..bada5f167 100644 --- a/securedrop_client/gui/widgets.py +++ b/securedrop_client/gui/widgets.py @@ -1359,8 +1359,8 @@ def on_star_update_failed(self, source_uuid: str, is_starred: bool) -> None: """ if self.source_uuid == source_uuid: self.is_starred = is_starred - self.setChecked(self.is_starred) self.pending_count = self.pending_count - 1 + QTimer.singleShot(250, lambda: self.setChecked(self.is_starred)) @pyqtSlot(str) def on_star_update_successful(self, source_uuid: str) -> None: