Merge pull request #1957 from phw/PICARD-2325-show-login-errors

PICARD-2325: Show error message on login errors
2026-01-07 09:04:29 +00:00 · 2021-11-21 17:56:28 +01:00
parent cfcadbe4c1 c03716a25b
commit 86de73d213
7 changed files with 104 additions and 60 deletions
--- 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 (
@@ -223,17 +224,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 +248,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):
+        try:
+            response = load_json(data)
+            return response['error_description']
+        except (JSONDecodeError, KeyError, TypeError):
+            return _('Unexpected request error (HTTP code %s)') % self.webservice.http_response_code(http)
--- 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()
--- 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)
--- 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):
--- 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,20 +131,25 @@ 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 <b>%s</b>.") % config.persist["oauth_username"])
            self.ui.logged_in.show()
+            self.ui.login_error.hide()
            self.ui.login.hide()
            self.ui.logout.show()
-        else:
+        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.
-        # 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)
@@ -151,8 +158,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()
--- a/picard/ui/ui_options_general.py
+++ b/picard/ui/ui_options_general.py
@@ -59,21 +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, 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.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.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.gridlayout1.addWidget(self.logged_in, 0, 0, 1, 3)
+        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")
@@ -148,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)
--- a/ui/options_general.ui
+++ b/ui/options_general.ui
@@ -114,44 +114,58 @@ QCheckBox { color: black }</string>
     <property name="title">
      <string>MusicBrainz Account</string>
     </property>
-     <layout class="QGridLayout">
+     <layout class="QVBoxLayout" name="verticalLayout_3">
      <property name="spacing">
       <number>2</number>
      </property>
-      <item row="1" column="0">
-       <widget class="QPushButton" name="login">
+      <item>
+       <widget class="QLabel" name="login_error">
        <property name="text">
-         <string>Log in</string>
+         <string/>
        </property>
       </widget>
      </item>
-      <item row="1" column="2">
-       <spacer name="horizontalSpacer">
-        <property name="orientation">
-         <enum>Qt::Horizontal</enum>
-        </property>
-        <property name="sizeHint" stdset="0">
-         <size>
-          <width>40</width>
-          <height>20</height>
-         </size>
-        </property>
-       </spacer>
-      </item>
-      <item row="1" column="1">
-       <widget class="QPushButton" name="logout">
-        <property name="text">
-         <string>Log out</string>
-        </property>
-       </widget>
-      </item>
-      <item row="0" column="0" colspan="3">
+      <item>
       <widget class="QLabel" name="logged_in">
        <property name="text">
         <string/>
        </property>
       </widget>
      </item>
+      <item>
+       <layout class="QHBoxLayout" name="horizontalLayout">
+        <property name="spacing">
+         <number>6</number>
+        </property>
+        <item>
+         <widget class="QPushButton" name="login">
+          <property name="text">
+           <string>Log in</string>
+          </property>
+         </widget>
+        </item>
+        <item>
+         <widget class="QPushButton" name="logout">
+          <property name="text">
+           <string>Log out</string>
+          </property>
+         </widget>
+        </item>
+        <item>
+         <spacer name="horizontalSpacer">
+          <property name="orientation">
+           <enum>Qt::Horizontal</enum>
+          </property>
+          <property name="sizeHint" stdset="0">
+           <size>
+            <width>40</width>
+            <height>20</height>
+           </size>
+          </property>
+         </spacer>
+        </item>
+       </layout>
+      </item>
     </layout>
    </widget>
   </item>
@@ -292,8 +306,6 @@ QCheckBox { color: black }</string>
  <tabstop>server_host</tabstop>
  <tabstop>server_port</tabstop>
  <tabstop>use_server_for_submission</tabstop>
-  <tabstop>login</tabstop>
-  <tabstop>logout</tabstop>
  <tabstop>analyze_new_files</tabstop>
  <tabstop>ignore_file_mbids</tabstop>
  <tabstop>check_for_updates</tabstop>