fix: 4766 - when logging in, show adequate error messages (#4783)

* fix: 4766 - when logging in, show adequate error messages. New file: * `login_result.dart`: Result of a log in attempt, more subtle than a `bool`. Impacted files: * `app_en.arb`: added 2 labels (network unreachable and server KO) * `app_fr.arb`: added 2 labels (network unreachable and server KO) * `background_task_manager.dart`: minor refactoring * `login_page.dart`: refactored using new class `LoginResult` in order to display more adequate error messages. * `pubspec.lock`: wtf * `smooth_app_data_importer.dart`: minor refactoring * `user_management_provider.dart`: refactored using new class `LoginResult` * Update packages/smooth_app/lib/pages/user_management/login_page.dart * Update packages/smooth_app/lib/pages/user_management/login_page.dart * Update packages/smooth_app/lib/pages/user_management/login_page.dart * Delete packages/smooth_app/lib/helpers/data_importer/smooth_app_data_importer.dart no more data_importer
openfoodfacts · Nov 17, 2023 · a6faac0 · a6faac0
1 parent 122aceb
commit a6faac0
Show file tree

Hide file tree

Showing 6 changed files with 127 additions and 75 deletions.
diff --git a/packages/smooth_app/lib/background/background_task_manager.dart b/packages/smooth_app/lib/background/background_task_manager.dart
@@ -6,6 +6,7 @@ import 'package:smooth_app/background/background_task.dart';
 import 'package:smooth_app/background/background_task_image.dart';
 import 'package:smooth_app/background/background_task_refresh_later.dart';
 import 'package:smooth_app/background/operation_type.dart';
+import 'package:smooth_app/data_models/login_result.dart';
 import 'package:smooth_app/database/dao_instant_string.dart';
 import 'package:smooth_app/database/dao_int.dart';
 import 'package:smooth_app/database/dao_string_list.dart';
@@ -177,7 +178,7 @@ class BackgroundTaskManager {
           }
         } catch (e) {
           // Most likely, no internet, no reason to go on.
-          if (e.toString().startsWith('Failed host lookup: ')) {
+          if (LoginResult.isNoNetworkException(e.toString())) {
             await _setTaskErrorStatus(taskId, taskStatusNoInternet);
             await _justFinished();
             return;

diff --git a/packages/smooth_app/lib/data_models/login_result.dart b/packages/smooth_app/lib/data_models/login_result.dart
@@ -0,0 +1,60 @@
+import 'package:flutter_gen/gen_l10n/app_localizations.dart';
+import 'package:openfoodfacts/openfoodfacts.dart';
+import 'package:smooth_app/query/product_query.dart';
+
+/// How did the login attempt work?
+enum LoginResultType {
+  successful,
+  unsuccessful,
+  serverIssue,
+  exception,
+}
+
+/// Result of a log in attempt, more subtle than a `bool`.
+class LoginResult {
+  const LoginResult(this.type, {this.user, this.text});
+
+  final LoginResultType type;
+  final User? user;
+  final String? text;
+
+  String getErrorMessage(final AppLocalizations appLocalizations) =>
+      switch (type) {
+        LoginResultType.successful => 'not supposed to happen',
+        LoginResultType.unsuccessful => appLocalizations.incorrect_credentials,
+        LoginResultType.serverIssue =>
+          appLocalizations.login_result_type_server_issue,
+        LoginResultType.exception => isNoNetworkException(text!)
+            ? appLocalizations.login_result_type_server_unreachable
+            : text!,
+      };
+
+  static bool isNoNetworkException(final String text) =>
+      text == 'Network is unreachable' ||
+      text.startsWith('Failed host lookup: ');
+
+  /// Checks credentials. Returns null if OK, or an error message.
+  static Future<LoginResult> getLoginResult(final User user) async {
+    try {
+      final LoginStatus? loginStatus = await OpenFoodAPIClient.login2(
+        user,
+        uriHelper: ProductQuery.uriProductHelper,
+      );
+      if (loginStatus == null) {
+        return const LoginResult(LoginResultType.serverIssue);
+      }
+      if (!loginStatus.successful) {
+        return const LoginResult(LoginResultType.unsuccessful);
+      }
+      return LoginResult(
+        LoginResultType.successful,
+        user: User(
+          userId: loginStatus.userId!,
+          password: user.password,
+        ),
+      );
+    } catch (e) {
+      return LoginResult(LoginResultType.exception, text: e.toString());
+    }
+  }
+}
diff --git a/packages/smooth_app/lib/data_models/user_management_provider.dart b/packages/smooth_app/lib/data_models/user_management_provider.dart
@@ -1,6 +1,7 @@
 import 'package:flutter/material.dart';
 import 'package:flutter/services.dart';
 import 'package:openfoodfacts/openfoodfacts.dart';
+import 'package:smooth_app/data_models/login_result.dart';
 import 'package:smooth_app/database/dao_secured_string.dart';
 import 'package:smooth_app/query/product_query.dart';
 import 'package:smooth_app/services/smooth_services.dart';
@@ -9,34 +10,15 @@ class UserManagementProvider with ChangeNotifier {
   static const String _USER_ID = 'user_id';
   static const String _PASSWORD = 'pasword';
 
-  // TODO(m123): Show why its failing
-  /// Checks credentials and conditionally saves them
-  Future<bool> login(User user) async {
-    final LoginStatus? loginStatus;
-    try {
-      loginStatus = await OpenFoodAPIClient.login2(
-        user,
-        uriHelper: ProductQuery.uriProductHelper,
-      );
-    } catch (e) {
-      throw Exception(e);
-    }
-
-    if (loginStatus == null) {
-      return false;
-    }
-
-    if (loginStatus.successful) {
-      await putUser(
-        User(
-          userId: loginStatus.userId!,
-          password: user.password,
-        ),
-      );
-      notifyListeners();
+  /// Checks credentials and conditionally saves them.
+  Future<LoginResult> login(final User user) async {
+    final LoginResult loginResult = await LoginResult.getLoginResult(user);
+    if (loginResult.type != LoginResultType.successful) {
+      return loginResult;
     }
-
-    return loginStatus.successful && await credentialsInStorage();
+    await putUser(loginResult.user!);
+    await credentialsInStorage();
+    return loginResult;
   }
 
   /// Deletes saved credentials from storage
@@ -102,32 +84,24 @@ class UserManagementProvider with ChangeNotifier {
   /// Check if the user is still logged in and the credentials are still valid
   /// If not, the user is logged out
   Future<void> checkUserLoginValidity() async {
-    try {
-      if (ProductQuery.isLoggedIn()) {
-        final User user = ProductQuery.getUser();
-        final LoginStatus? loginStatus = await OpenFoodAPIClient.login2(
-          User(
-            userId: user.userId,
-            password: user.password,
-          ),
-          uriHelper: ProductQuery.uriProductHelper,
-        );
-        if (loginStatus == null) {
-          // No internet or sever down
-          return;
-        }
-        if (loginStatus.successful) {
-          // Credentials are still valid so we just return
-          return;
-        } else {
-          // Credentials are not valid anymore so we log out
-          // TODO(m123): Notify the user
-          await logout();
-        }
-      }
-    } catch (e) {
-      // We don't want to crash the app if the login check fails
-      // So we do nothing here
+    if (!ProductQuery.isLoggedIn()) {
+      return;
+    }
+    final User user = ProductQuery.getUser();
+    final LoginResult loginResult = await LoginResult.getLoginResult(
+      User(
+        userId: user.userId,
+        password: user.password,
+      ),
+    );
+    switch (loginResult.type) {
+      case LoginResultType.successful:
+      case LoginResultType.serverIssue:
+      case LoginResultType.exception:
+        return;
+      case LoginResultType.unsuccessful:
+        // TODO(m123): Notify the user
+        await logout();
     }
   }
 }
diff --git a/packages/smooth_app/lib/l10n/app_en.arb b/packages/smooth_app/lib/l10n/app_en.arb
@@ -150,6 +150,14 @@
     "@login": {
         "description": "Text field hint: unified name for either username or e-mail address"
     },
+    "login_result_type_server_unreachable": "Network is unreachable",
+    "@login_result_type_server_unreachable": {
+        "description": "Error message when trying to log in without network"
+    },
+    "login_result_type_server_issue": "Problem on the server. Please try later.",
+    "@login_result_type_server_issue": {
+        "description": "Error message when trying to log in and the server does not answer correctly"
+    },
     "login_page_username_or_email": "Please enter username or e-mail",
     "login_page_password_error_empty": "Please enter a password",
     "create_account": "Create account",

diff --git a/packages/smooth_app/lib/l10n/app_fr.arb b/packages/smooth_app/lib/l10n/app_fr.arb
@@ -150,6 +150,14 @@
     "@login": {
         "description": "Text field hint: unified name for either username or e-mail address"
     },
+    "login_result_type_server_unreachable": "Impossible de se connecter au réseau",
+    "@login_result_type_server_unreachable": {
+        "description": "Error message when trying to log in without network"
+    },
+    "login_result_type_server_issue": "Problème serveur. Essayez plus tard.",
+    "@login_result_type_server_issue": {
+        "description": "Error message when trying to log in and the server does not answer correctly"
+    },
     "login_page_username_or_email": "Veuillez saisir un nom d'utilisateur ou un e-mail",
     "login_page_password_error_empty": "Veuillez saisir un mot de passe",
     "create_account": "Créer un compte",

diff --git a/packages/smooth_app/lib/pages/user_management/login_page.dart b/packages/smooth_app/lib/pages/user_management/login_page.dart
@@ -4,6 +4,7 @@ import 'package:flutter_svg/flutter_svg.dart';
 import 'package:matomo_tracker/matomo_tracker.dart';
 import 'package:openfoodfacts/openfoodfacts.dart';
 import 'package:provider/provider.dart';
+import 'package:smooth_app/data_models/login_result.dart';
 import 'package:smooth_app/data_models/preferences/user_preferences.dart';
 import 'package:smooth_app/data_models/user_management_provider.dart';
 import 'package:smooth_app/generic_lib/design_constants.dart';
@@ -29,7 +30,7 @@ class LoginPage extends StatefulWidget {
 
 class _LoginPageState extends State<LoginPage> with TraceableClientMixin {
   bool _runningQuery = false;
-  bool _wrongCredentials = false;
+  LoginResult? _loginResult;
 
   final GlobalKey<FormState> _formKey = GlobalKey<FormState>();
 
@@ -46,17 +47,17 @@ class _LoginPageState extends State<LoginPage> with TraceableClientMixin {
 
     setState(() {
       _runningQuery = true;
-      _wrongCredentials = false;
+      _loginResult = null;
     });
 
-    final bool login = await userManagementProvider.login(
+    _loginResult = await userManagementProvider.login(
       User(
         userId: userIdController.text,
         password: passwordController.text,
       ),
     );
 
-    if (login) {
+    if (_loginResult!.type == LoginResultType.successful) {
       AnalyticsHelper.trackEvent(AnalyticsEvent.loginAction);
       if (!mounted) {
         return;
@@ -69,10 +70,7 @@ class _LoginPageState extends State<LoginPage> with TraceableClientMixin {
       }
       Navigator.pop(context);
     } else {
-      setState(() {
-        _runningQuery = false;
-        _wrongCredentials = true;
-      });
+      setState(() => _runningQuery = false);
     }
   }
 
@@ -140,22 +138,25 @@ class _LoginPageState extends State<LoginPage> with TraceableClientMixin {
                         height: LARGE_SPACE * 3,
                       ),
 
-                      if (_wrongCredentials) ...<Widget>[
-                        SmoothCard(
-                          padding: const EdgeInsets.all(10.0),
-                          color: const Color(0xFFFF4446),
-                          child: Text(
-                            appLocalizations.incorrect_credentials,
-                            style: theme.textTheme.bodyMedium?.copyWith(
-                              fontSize: 18.0,
-                              color: const Color(0xFF000000),
+                      if (_loginResult != null &&
+                          _loginResult!.type != LoginResultType.successful)
+                        Padding(
+                          padding: const EdgeInsets.only(
+                            bottom: 10.0 + LARGE_SPACE * 2,
+                          ),
+                          child: SmoothCard(
+                            padding: const EdgeInsets.all(10.0),
+                            color: const Color(0xFFEB0004),
+                            child: Text(
+                              _loginResult!.getErrorMessage(appLocalizations),
+                              style: theme.textTheme.bodyMedium?.copyWith(
+                                fontSize: 18.0,
+                                color: const Color(0xFF000000),
+                              ),
+                              textAlign: TextAlign.center,
                             ),
                           ),
                         ),
-                        const SizedBox(
-                          height: LARGE_SPACE * 2,
-                        ),
-                      ],
                       //Login
                       SmoothTextFormField(
                         type: TextFieldTypes.PLAIN_TEXT,