Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Standardize connection errors #823

Merged
merged 3 commits into from
Mar 2, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions securedrop_client/gui/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -179,12 +179,12 @@ def update_activity_status(self, message: str, duration=0):
"""
self.top_pane.update_activity_status(message, duration)

def update_error_status(self, message: str, duration=10000, retry=False) -> None:
def update_error_status(self, message: str, duration=10000) -> None:
"""
Display an error status message to the user. Optionally, supply a duration
(in milliseconds), the default will continuously show the message.
"""
self.top_pane.update_error_status(message, duration, retry)
self.top_pane.update_error_status(message, duration)

def clear_error_status(self):
"""
Expand Down
32 changes: 3 additions & 29 deletions securedrop_client/gui/widgets.py
Original file line number Diff line number Diff line change
Expand Up @@ -124,8 +124,8 @@ def set_logged_out(self):
def update_activity_status(self, message: str, duration: int):
self.activity_status_bar.update_message(message, duration)

def update_error_status(self, message: str, duration: int, retry: bool):
self.error_status_bar.update_message(message, duration, retry)
def update_error_status(self, message: str, duration: int):
self.error_status_bar.update_message(message, duration)

def clear_error_status(self):
self.error_status_bar.clear_message()
Expand Down Expand Up @@ -313,15 +313,6 @@ class ErrorStatusBar(QWidget):
font-size: 14px;
color: #0c3e75;
}
QPushButton#retry_button {
border: none;
padding-right: 30px;
background-color: #fff;
color: #0065db;
font-family: 'Source Sans Pro';
font-weight: 600;
font-size: 12px;
}
'''

def __init__(self):
Expand Down Expand Up @@ -353,22 +344,15 @@ def __init__(self):
self.status_bar.setObjectName('error_status_bar') # Set css id
self.status_bar.setSizeGripEnabled(False)

# Retry button
self.retry_button = QPushButton('RETRY')
self.retry_button.setObjectName('retry_button')
self.retry_button.setFixedHeight(42)

# Add widgets to layout
layout.addWidget(self.vertical_bar)
layout.addWidget(self.label)
layout.addWidget(self.status_bar)
layout.addWidget(self.retry_button)

# Hide until a message needs to be displayed
self.vertical_bar.hide()
self.label.hide()
self.status_bar.hide()
self.retry_button.hide()

# Only show errors for a set duration
self.status_timer = QTimer()
Expand All @@ -378,7 +362,6 @@ def _hide(self):
self.vertical_bar.hide()
self.label.hide()
self.status_bar.hide()
self.retry_button.hide()

def _show(self):
self.vertical_bar.show()
Expand All @@ -390,21 +373,12 @@ def _on_status_timeout(self):

def setup(self, controller):
self.controller = controller
self.retry_button.clicked.connect(self._on_retry_clicked)

def _on_retry_clicked(self) -> None:
self.clear_message()
self._hide()
self.controller.resume_queues()

def update_message(self, message: str, duration: int, retry: bool) -> None:
def update_message(self, message: str, duration: int) -> None:
"""
Display a status message to the user for a given duration. If the duration is zero,
continuously show message.
"""
if retry:
self.retry_button.show()

self.status_bar.showMessage(message, duration)

if duration != 0:
Expand Down
13 changes: 6 additions & 7 deletions securedrop_client/logic.py
Original file line number Diff line number Diff line change
Expand Up @@ -322,17 +322,14 @@ def call_api(self,

def on_queue_paused(self) -> None:
self.gui.update_error_status(
_('The SecureDrop server cannot be reached.'),
duration=0,
retry=True)
_('The SecureDrop server cannot be reached. Trying to reconnect...'), duration=0)
self.show_last_sync_timer.start(TIME_BETWEEN_SHOWING_LAST_SYNC_MS)

def resume_queues(self) -> None:
self.api_job_queue.resume_queues()
self.show_last_sync_timer.stop()

# clear error status in case queue was paused resulting in a permanent error message with
# retry link
# clear error status in case queue was paused resulting in a permanent error message
self.gui.clear_error_status()

def completed_api_call(self, thread_id, user_callback):
Expand Down Expand Up @@ -465,6 +462,9 @@ def on_sync_failure(self, result: Exception) -> None:
self.invalidate_token()
self.logout()
self.gui.show_login(error=_('Your session expired. Please log in again.'))
elif isinstance(result, (RequestTimeoutError, ServerConnectionError)):
self.gui.update_error_status(
_('The SecureDrop server cannot be reached. Trying to reconnect...'), duration=0)

def show_last_sync(self):
"""
Expand Down Expand Up @@ -507,8 +507,7 @@ def logout(self):
out in the GUI.
"""

# clear error status in case queue was paused resulting in a permanent error message with
# retry link
# clear error status in case queue was paused resulting in a permanent error message
self.gui.clear_error_status()

if self.api is not None:
Expand Down
6 changes: 3 additions & 3 deletions tests/gui/test_main.py
Original file line number Diff line number Diff line change
Expand Up @@ -170,12 +170,12 @@ def test_show_sources(mocker):
def test_update_error_status_default(mocker):
"""
Ensure that the error to be shown in the error status bar will be passed to the top pane with a
default duration of 10 seconds and no retry link.
default duration of 10 seconds.
"""
w = Window()
w.top_pane = mocker.MagicMock()
w.update_error_status(message='test error message')
w.top_pane.update_error_status.assert_called_once_with('test error message', 10000, False)
w.top_pane.update_error_status.assert_called_once_with('test error message', 10000)


def test_update_error_status(mocker):
Expand All @@ -186,7 +186,7 @@ def test_update_error_status(mocker):
w = Window()
w.top_pane = mocker.MagicMock()
w.update_error_status(message='test error message', duration=123)
w.top_pane.update_error_status.assert_called_once_with('test error message', 123, False)
w.top_pane.update_error_status.assert_called_once_with('test error message', 123)


def test_update_activity_status_default(mocker):
Expand Down
17 changes: 3 additions & 14 deletions tests/gui/test_widgets.py
Original file line number Diff line number Diff line change
Expand Up @@ -93,9 +93,9 @@ def test_TopPane_update_error_status(mocker):
tp = TopPane()
tp.error_status_bar = mocker.MagicMock()

tp.update_error_status(message='test message', duration=5, retry=True)
tp.update_error_status(message='test message', duration=5)

tp.error_status_bar.update_message.assert_called_once_with('test message', 5, True)
tp.error_status_bar.update_message.assert_called_once_with('test message', 5)


def test_TopPane_clear_error_status(mocker):
Expand Down Expand Up @@ -283,7 +283,7 @@ def test_ErrorStatusBar_update_message(mocker):
esb.status_bar = mocker.MagicMock()
esb.status_timer = mocker.MagicMock()

esb.update_message(message='test message', duration=123, retry=True)
esb.update_message(message='test message', duration=123)

esb.status_bar.showMessage.assert_called_once_with('test message', 123)
esb.status_timer.start.assert_called_once_with(123)
Expand Down Expand Up @@ -321,17 +321,6 @@ def test_ErrorStatusBar_on_status_timeout(mocker):
assert esb.isHidden()


def test_ErrorStatusBar_on_retry_clicked(mocker):
controller = mocker.MagicMock()
esb = ErrorStatusBar()
esb.setup(controller)

esb._on_retry_clicked()

assert esb.isHidden()
controller.resume_queues.assert_called_once_with()


def test_ActivityStatusBar_update_message(mocker):
"""
Calling update_message updates the message of the QStatusBar.
Expand Down
21 changes: 19 additions & 2 deletions tests/test_logic.py
Original file line number Diff line number Diff line change
Expand Up @@ -427,6 +427,23 @@ def test_Controller_on_sync_failure_due_to_invalid_token(homedir, config, mocker
co.gui.show_login.assert_called_once_with(error='Your session expired. Please log in again.')


@pytest.mark.parametrize("exception", [RequestTimeoutError, ServerConnectionError])
def test_Controller_on_sync_failure_due_to_timeout(homedir, mocker, exception):
"""
If the sync fails because of a timeout, make sure to show an error message.
"""
gui = mocker.MagicMock()
co = Controller('http://localhost', gui, mocker.MagicMock(), homedir)
co.logout = mocker.MagicMock()
co.gui = mocker.MagicMock()
co.gui.update_error_status = mocker.MagicMock()

co.on_sync_failure(exception())

co.gui.update_error_status.assert_called_once_with(
'The SecureDrop server cannot be reached. Trying to reconnect...', duration=0)


def test_Controller_on_sync_success(homedir, config, mocker):
"""
If there's a result to syncing, then update local storage.
Expand Down Expand Up @@ -1482,7 +1499,7 @@ def test_APICallRunner_api_call_timeout(mocker, exception):

def test_Controller_on_queue_paused(homedir, config, mocker, session_maker):
'''
Check that a paused queue is communicated to the user via the error status bar with retry option
Check that a paused queue is communicated to the user via the error status bar
'''
mock_gui = mocker.MagicMock()
co = Controller('http://localhost', mock_gui, session_maker, homedir)
Expand All @@ -1491,7 +1508,7 @@ def test_Controller_on_queue_paused(homedir, config, mocker, session_maker):
co.show_last_sync_timer = mocker.MagicMock()
co.on_queue_paused()
mock_gui.update_error_status.assert_called_once_with(
'The SecureDrop server cannot be reached.', duration=0, retry=True)
'The SecureDrop server cannot be reached. Trying to reconnect...', duration=0)
co.show_last_sync_timer.start.assert_called_once_with(TIME_BETWEEN_SHOWING_LAST_SYNC_MS)


Expand Down