From 91be1ba650aceb6fd5e2c066d9f73a1b4d58250b Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Wed, 10 Nov 2021 08:50:07 +0100 Subject: [PATCH 1/3] PICARD-2325: Show error message on login errors --- picard/oauth.py | 17 +++++++++++++++-- picard/tagger.py | 10 +++++----- picard/ui/mainwindow.py | 11 ++++++++--- picard/ui/options/__init__.py | 2 +- picard/ui/options/general.py | 16 +++++++++++++--- picard/ui/ui_options_general.py | 18 +++++++++++------- ui/options_general.ui | 15 +++++++++++---- 7 files changed, 64 insertions(+), 25 deletions(-) diff --git a/picard/oauth.py b/picard/oauth.py index 048736f13..eb57e6591 100644 --- a/picard/oauth.py +++ b/picard/oauth.py @@ -223,17 +223,20 @@ class OAuthManager(object): def on_exchange_authorization_code_finished(self, scopes, callback, data, http, error): successful = False + error_msg = None try: if error: log.error("OAuth: authorization_code exchange failed: %s", data) + error_msg = self._extract_error_description(http, data) else: self.set_refresh_token(data["refresh_token"], scopes) self.set_access_token(data["access_token"], data["expires_in"]) successful = True except Exception as e: log.error('OAuth: Unexpected error handling authorization code response: %r', e) + error_msg = _('Unexpected authentication error') finally: - callback(successful=successful) + callback(successful=successful, error_msg=error_msg) def fetch_username(self, callback): log.debug("OAuth: fetching username") @@ -244,14 +247,24 @@ class OAuthManager(object): def on_fetch_username_finished(self, callback, data, http, error): successful = False + error_msg = None try: if error: log.error("OAuth: username fetching failed: %s", data) + error_msg = self._extract_error_description(http, data) else: self.username = data["sub"] log.debug("OAuth: got username %s", self.username) successful = True except Exception as e: log.error('OAuth: Unexpected error handling username fetch response: %r', e) + error_msg = _('Unexpected authentication error') finally: - callback(successful) + callback(successful=successful, error_msg=error_msg) + + def _extract_error_description(self, http, data): + if self.webservice.http_response_code(http) == 400: + response = load_json(data) + if 'error_description' in response: + return response['error_description'] + return None diff --git a/picard/tagger.py b/picard/tagger.py index 4beba698c..f63ca2832 100644 --- a/picard/tagger.py +++ b/picard/tagger.py @@ -325,20 +325,20 @@ class Tagger(QtWidgets.QApplication): authorization_code, scopes, partial(self.on_mb_authorization_finished, callback)) else: - callback(False) + callback(False, None) - def on_mb_authorization_finished(self, callback, successful=False): + def on_mb_authorization_finished(self, callback, successful=False, error_msg=None): if successful: self.webservice.oauth_manager.fetch_username( partial(self.on_mb_login_finished, callback)) else: - callback(False) + callback(False, error_msg) @classmethod - def on_mb_login_finished(self, callback, successful): + def on_mb_login_finished(self, callback, successful, error_msg): if successful: load_user_collections() - callback(successful) + callback(successful, error_msg) def mb_logout(self): self.webservice.oauth_manager.revoke_tokens() diff --git a/picard/ui/mainwindow.py b/picard/ui/mainwindow.py index 4d5670077..19a82034b 100644 --- a/picard/ui/mainwindow.py +++ b/picard/ui/mainwindow.py @@ -1501,9 +1501,14 @@ class MainWindow(QtWidgets.QMainWindow, PreserveGeometry): dialog = PasswordDialog(authenticator, reply, parent=self) dialog.exec_() - @classmethod - def on_mb_login_finished(self, successful): - log.debug('MusicBrainz authentication finished: %s', successful) + def on_mb_login_finished(self, successful, error_msg): + if successful: + log.debug('MusicBrainz authentication finished successfully') + else: + log.info('MusicBrainz authentication failed: %s', error_msg) + QtWidgets.QMessageBox.critical(self, + _("Authentication failed"), + _('Login failed: %s') % error_msg) def show_proxy_dialog(self, proxy, authenticator): dialog = ProxyDialog(authenticator, proxy, parent=self) diff --git a/picard/ui/options/__init__.py b/picard/ui/options/__init__.py index f89abb971..ae429d76d 100644 --- a/picard/ui/options/__init__.py +++ b/picard/ui/options/__init__.py @@ -44,7 +44,7 @@ class OptionsPage(QtWidgets.QWidget): SORT_ORDER = 1000 ACTIVE = True HELP_URL = None - STYLESHEET_ERROR = "QWidget { background-color: #f55; color: white; font-weight:bold }" + STYLESHEET_ERROR = "QWidget { background-color: #f55; color: white; font-weight:bold; padding: 2px; }" STYLESHEET = "QLabel { qproperty-wordWrap: true; }" def __init__(self, *args, **kwargs): diff --git a/picard/ui/options/general.py b/picard/ui/options/general.py index cff14a0cc..b29c1af5d 100644 --- a/picard/ui/options/general.py +++ b/picard/ui/options/general.py @@ -86,6 +86,8 @@ class GeneralOptionsPage(OptionsPage): self.ui.logout.clicked.connect(self.logout) self.ui.analyze_new_files.toggled.connect(self._update_cluster_new_files) self.ui.cluster_new_files.toggled.connect(self._update_analyze_new_files) + self.ui.login_error.setStyleSheet(self.STYLESHEET_ERROR) + self.ui.login_error.hide() self.update_login_logout() def load(self): @@ -129,15 +131,23 @@ class GeneralOptionsPage(OptionsPage): else: self.ui.server_host_primary_warning.show() - def update_login_logout(self): + def update_login_logout(self, error_msg=None): if self.tagger.webservice.oauth_manager.is_logged_in(): config = get_config() self.ui.logged_in.setText(_("Logged in as %s.") % config.persist["oauth_username"]) self.ui.logged_in.show() + self.ui.login_error.hide() self.ui.login.hide() self.ui.logout.show() + elif error_msg: + self.ui.logged_in.hide() + self.ui.login_error.setText(_('Login failed: %s') % error_msg) + self.ui.login_error.show() + self.ui.login.show() + self.ui.logout.hide() else: self.ui.logged_in.hide() + self.ui.login_error.hide() self.ui.login.show() self.ui.logout.hide() # Workaround for Qt not repainting the view on macOS after the changes. @@ -151,8 +161,8 @@ class GeneralOptionsPage(OptionsPage): super().restore_defaults() self.logout() - def on_login_finished(self, successful): - self.update_login_logout() + def on_login_finished(self, successful, error_msg=None): + self.update_login_logout(error_msg) def logout(self): self.tagger.mb_logout() diff --git a/picard/ui/ui_options_general.py b/picard/ui/ui_options_general.py index ad10dfd79..7c6657b6b 100644 --- a/picard/ui/ui_options_general.py +++ b/picard/ui/ui_options_general.py @@ -64,16 +64,20 @@ class Ui_GeneralOptionsPage(object): self.gridlayout1.setObjectName("gridlayout1") self.login = QtWidgets.QPushButton(self.rename_files_2) self.login.setObjectName("login") - self.gridlayout1.addWidget(self.login, 1, 0, 1, 1) - spacerItem1 = QtWidgets.QSpacerItem(40, 20, QtWidgets.QSizePolicy.Expanding, QtWidgets.QSizePolicy.Minimum) - self.gridlayout1.addItem(spacerItem1, 1, 2, 1, 1) - self.logout = QtWidgets.QPushButton(self.rename_files_2) - self.logout.setObjectName("logout") - self.gridlayout1.addWidget(self.logout, 1, 1, 1, 1) + self.gridlayout1.addWidget(self.login, 3, 0, 1, 1) self.logged_in = QtWidgets.QLabel(self.rename_files_2) self.logged_in.setText("") self.logged_in.setObjectName("logged_in") - self.gridlayout1.addWidget(self.logged_in, 0, 0, 1, 3) + self.gridlayout1.addWidget(self.logged_in, 1, 0, 1, 3) + spacerItem1 = QtWidgets.QSpacerItem(40, 20, QtWidgets.QSizePolicy.Expanding, QtWidgets.QSizePolicy.Minimum) + self.gridlayout1.addItem(spacerItem1, 3, 2, 1, 1) + self.logout = QtWidgets.QPushButton(self.rename_files_2) + self.logout.setObjectName("logout") + self.gridlayout1.addWidget(self.logout, 3, 1, 1, 1) + self.login_error = QtWidgets.QLabel(self.rename_files_2) + self.login_error.setText("") + self.login_error.setObjectName("login_error") + self.gridlayout1.addWidget(self.login_error, 0, 0, 1, 3) self.vboxlayout.addWidget(self.rename_files_2) self.groupBox_2 = QtWidgets.QGroupBox(GeneralOptionsPage) self.groupBox_2.setObjectName("groupBox_2") diff --git a/ui/options_general.ui b/ui/options_general.ui index b910edb89..e374f70db 100644 --- a/ui/options_general.ui +++ b/ui/options_general.ui @@ -118,14 +118,21 @@ QCheckBox { color: black } 2 - + Log in - + + + + + + + + Qt::Horizontal @@ -138,7 +145,7 @@ QCheckBox { color: black } - + Log out @@ -146,7 +153,7 @@ QCheckBox { color: black } - + From fa73b7d3e00909f67a40f520fb2efded113bb637 Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Sat, 20 Nov 2021 18:26:10 +0100 Subject: [PATCH 2/3] For login view use a vertical layout instead of grid layout. May solve PICARD-2320 and likely is a better solution for PICARD-1654 --- picard/ui/options/general.py | 3 -- picard/ui/ui_options_general.py | 40 ++++++++++--------- ui/options_general.ui | 69 ++++++++++++++++++--------------- 3 files changed, 58 insertions(+), 54 deletions(-) diff --git a/picard/ui/options/general.py b/picard/ui/options/general.py index b29c1af5d..7e6074d5c 100644 --- a/picard/ui/options/general.py +++ b/picard/ui/options/general.py @@ -150,9 +150,6 @@ class GeneralOptionsPage(OptionsPage): self.ui.login_error.hide() self.ui.login.show() self.ui.logout.hide() - # Workaround for Qt not repainting the view on macOS after the changes. - # See https://tickets.metabrainz.org/browse/PICARD-1654 - self.ui.vboxlayout.parentWidget().repaint() def login(self): self.tagger.mb_login(self.on_login_finished, self) diff --git a/picard/ui/ui_options_general.py b/picard/ui/ui_options_general.py index 7c6657b6b..6d7819c50 100644 --- a/picard/ui/ui_options_general.py +++ b/picard/ui/ui_options_general.py @@ -59,25 +59,29 @@ class Ui_GeneralOptionsPage(object): self.vboxlayout.addWidget(self.groupBox) self.rename_files_2 = QtWidgets.QGroupBox(GeneralOptionsPage) self.rename_files_2.setObjectName("rename_files_2") - self.gridlayout1 = QtWidgets.QGridLayout(self.rename_files_2) - self.gridlayout1.setSpacing(2) - self.gridlayout1.setObjectName("gridlayout1") - self.login = QtWidgets.QPushButton(self.rename_files_2) - self.login.setObjectName("login") - self.gridlayout1.addWidget(self.login, 3, 0, 1, 1) - self.logged_in = QtWidgets.QLabel(self.rename_files_2) - self.logged_in.setText("") - self.logged_in.setObjectName("logged_in") - self.gridlayout1.addWidget(self.logged_in, 1, 0, 1, 3) - spacerItem1 = QtWidgets.QSpacerItem(40, 20, QtWidgets.QSizePolicy.Expanding, QtWidgets.QSizePolicy.Minimum) - self.gridlayout1.addItem(spacerItem1, 3, 2, 1, 1) - self.logout = QtWidgets.QPushButton(self.rename_files_2) - self.logout.setObjectName("logout") - self.gridlayout1.addWidget(self.logout, 3, 1, 1, 1) + self.verticalLayout_3 = QtWidgets.QVBoxLayout(self.rename_files_2) + self.verticalLayout_3.setSpacing(2) + self.verticalLayout_3.setObjectName("verticalLayout_3") self.login_error = QtWidgets.QLabel(self.rename_files_2) self.login_error.setText("") self.login_error.setObjectName("login_error") - self.gridlayout1.addWidget(self.login_error, 0, 0, 1, 3) + self.verticalLayout_3.addWidget(self.login_error) + self.logged_in = QtWidgets.QLabel(self.rename_files_2) + self.logged_in.setText("") + self.logged_in.setObjectName("logged_in") + self.verticalLayout_3.addWidget(self.logged_in) + self.horizontalLayout = QtWidgets.QHBoxLayout() + self.horizontalLayout.setSpacing(6) + self.horizontalLayout.setObjectName("horizontalLayout") + self.login = QtWidgets.QPushButton(self.rename_files_2) + self.login.setObjectName("login") + self.horizontalLayout.addWidget(self.login) + self.logout = QtWidgets.QPushButton(self.rename_files_2) + self.logout.setObjectName("logout") + self.horizontalLayout.addWidget(self.logout) + spacerItem1 = QtWidgets.QSpacerItem(40, 20, QtWidgets.QSizePolicy.Expanding, QtWidgets.QSizePolicy.Minimum) + self.horizontalLayout.addItem(spacerItem1) + self.verticalLayout_3.addLayout(self.horizontalLayout) self.vboxlayout.addWidget(self.rename_files_2) self.groupBox_2 = QtWidgets.QGroupBox(GeneralOptionsPage) self.groupBox_2.setObjectName("groupBox_2") @@ -152,9 +156,7 @@ class Ui_GeneralOptionsPage(object): QtCore.QMetaObject.connectSlotsByName(GeneralOptionsPage) GeneralOptionsPage.setTabOrder(self.server_host, self.server_port) GeneralOptionsPage.setTabOrder(self.server_port, self.use_server_for_submission) - GeneralOptionsPage.setTabOrder(self.use_server_for_submission, self.login) - GeneralOptionsPage.setTabOrder(self.login, self.logout) - GeneralOptionsPage.setTabOrder(self.logout, self.analyze_new_files) + GeneralOptionsPage.setTabOrder(self.use_server_for_submission, self.analyze_new_files) GeneralOptionsPage.setTabOrder(self.analyze_new_files, self.ignore_file_mbids) GeneralOptionsPage.setTabOrder(self.ignore_file_mbids, self.check_for_updates) GeneralOptionsPage.setTabOrder(self.check_for_updates, self.update_check_days) diff --git a/ui/options_general.ui b/ui/options_general.ui index e374f70db..665f28c55 100644 --- a/ui/options_general.ui +++ b/ui/options_general.ui @@ -114,50 +114,57 @@ QCheckBox { color: black } MusicBrainz Account - + 2 - - + + - Log in + - + - - - - Qt::Horizontal + + + + 6 - - - 40 - 20 - - - - - - - - Log out - - - - - - - - - + + + + Log in + + + + + + + Log out + + + + + + + Qt::Horizontal + + + + 40 + 20 + + + + + @@ -299,8 +306,6 @@ QCheckBox { color: black } server_host server_port use_server_for_submission - login - logout analyze_new_files ignore_file_mbids check_for_updates From c03716a25b752365f5f0c206b5982246130c326f Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Sun, 21 Nov 2021 16:58:20 +0100 Subject: [PATCH 3/3] oauth: generate generic error message on unexpected errors --- picard/oauth.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/picard/oauth.py b/picard/oauth.py index eb57e6591..6e4335341 100644 --- a/picard/oauth.py +++ b/picard/oauth.py @@ -26,6 +26,7 @@ from functools import partial +from json.decoder import JSONDecodeError import time from PyQt5.QtCore import ( @@ -263,8 +264,8 @@ class OAuthManager(object): callback(successful=successful, error_msg=error_msg) def _extract_error_description(self, http, data): - if self.webservice.http_response_code(http) == 400: + try: response = load_json(data) - if 'error_description' in response: - return response['error_description'] - return None + return response['error_description'] + except (JSONDecodeError, KeyError, TypeError): + return _('Unexpected request error (HTTP code %s)') % self.webservice.http_response_code(http)