Remember me feature #2672
Remember me feature #2672
Conversation
WalkthroughThis pull request implements a "Remember Me" feature for user authentication across multiple components of the Talawa application. The changes span localization files, view models, and UI components to enable secure storage of user credentials. The implementation allows users to optionally store their login information and provides a direct login option on subsequent app launches, enhancing user experience by simplifying the authentication process. Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🔇 Additional comments (2)pubspec.yaml (2)
The addition of Line range hint The removal of analyzer-related packages might affect code quality checks. Please verify if these were intentionally commented out and ensure this doesn't impact the project's static analysis capabilities. Also applies to: 23-24 ✅ Verification successfulAnalyzer dependencies are correctly commented out in the main pubspec.yaml The analyzer-related packages are intentionally commented out in the main
This is the expected setup where analyzer dependencies are declared in the lint rules package rather than the main project, ensuring proper separation of concerns. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Check if these analyzer dependencies are used in any custom lint rules or CI workflows
# Search for analyzer usage in custom lint rules
echo "Checking custom lint rules..."
fd -e dart -e yaml . | xargs rg "_fe_analyzer|analyzer_plugin|custom_lint_builder"
# Check CI workflow files for analyzer usage
echo "Checking CI workflows..."
fd -e yml -e yaml . .github/workflows | xargs rg "_fe_analyzer|analyzer_plugin|custom_lint_builder"
Length of output: 1839 Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
Other🎯 Please be considerate of our volunteers' time. Contacting the person who assigned the reviewers is not advised unless they ask for your input. Do not @ the person who did the assignment otherwise. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (13)
lib/views/after_auth_screens/app_settings/app_settings_page.dart (2)
26-32: Evaluate constructor injection for testability.Defining FlutterSecureStorage directly in the state class works here, but consider leveraging dependency injection for more flexible testing and mocking.
301-339: Clarify usage of StatefulBuilder.Using a StatefulBuilder for the logout dialog is valid and convenient for toggling the checkbox. However, ensure accessibility is maintained (e.g., using appropriate semantics for screen readers).
lib/widgets/directly_login.dart (1)
6-9: Refine doc comments for clarity.Ensure the doc comments fully explain behavior (e.g., mention how the credentials are retrieved, and conditions under which the widget displays or hides).
lib/view_model/pre_auth_view_models/login_view_model.dart (2)
25-32: Leverage typed field names for clarity.Defining prevUserEmail and prevUserPassword clarifies the model’s responsibilities. Consider finalizing them if they should not be reassigned after fetch.
190-204: Handle fetchPrevUser exceptions robustly.The try-catch logs an error, but no user feedback is provided. Consider a fallback or additional logging for debug builds to ensure maintainability.
lib/views/pre_auth_screens/login.dart (2)
19-20: Improve documentation phrasing.
Consider rephrasing this for clarity and grammar:
• Use “fields” instead of “fiels.”
• Add punctuation for readability.Possible rephrased version:
"/// At the bottom, there is an option to log in directly using the previously used email. (This option is only available if the user selected 'Remember me' while logging out)."
194-194: Conditionally display DirectlyLogin widget.
Consider wrapping the DirectlyLogin widget in a conditional check to ensure it doesn't appear if there is no stored user data. This would prevent confusion for new users or those who have not selected "Remember me."-DirectlyLogin(model: model), +if (model.prevUserEmail != null && model.prevUserEmail!.isNotEmpty) ... + DirectlyLogin(model: model),lib/view_model/pre_auth_view_models/signup_details_view_model.dart (1)
29-31: Consider injection for secureStorage.
It might be more maintainable to inject the FlutterSecureStorage instance via dependency injection. This allows easier testing and avoids creating multiple instances across different view models.- final secureStorage = const FlutterSecureStorage(); + final FlutterSecureStorage secureStorage; + + SignupDetailsViewModel({required this.secureStorage});lang/ja.json (1)
281-281: Remove unnecessary spaces in Japanese translation.The translation for "Members" contains extra spaces that should be removed.
- "Members": " メンバー ", + "Members": "メンバー",lang/pt.json (1)
282-282: Remove extra space after comma.There's an unnecessary space after the comma in the "Remember me" translation.
- "Remember me": "Lembre-se de mim", + "Remember me": "Lembre-se de mim",lang/de.json (1)
282-282: Consider a more idiomatic German translation for "Remember me".The current translation "Erinnere dich an mich" is grammatically correct but unusually formal for a UI element. Consider using the more common German UI pattern:
- "Remember me": "Erinnere dich an mich", + "Remember me": "Angemeldet bleiben",This suggestion uses "Angemeldet bleiben" (stay logged in), which is the standard phrase used in German user interfaces for this feature.
lang/fr.json (2)
281-281: Remove leading space in French translation for "Members".The translation has an inconsistent leading space that should be removed:
- "Members": " Membres", + "Members": "Membres",
282-282: Consider a more idiomatic French translation for "Remember me".The current translation "Souviens-toi de moi" is grammatically correct but unusually formal for a UI element. Consider using the more common French UI pattern:
- "Remember me": "Souviens-toi de moi", + "Remember me": "Se souvenir de moi",This suggestion uses the infinitive form which is more commonly used in French user interfaces for this feature.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
lang/de.json(1 hunks)lang/en.json(1 hunks)lang/es.json(1 hunks)lang/fr.json(1 hunks)lang/hi.json(1 hunks)lang/ja.json(1 hunks)lang/pt.json(1 hunks)lang/zh.json(1 hunks)lib/services/user_config.dart(6 hunks)lib/view_model/pre_auth_view_models/login_view_model.dart(4 hunks)lib/view_model/pre_auth_view_models/signup_details_view_model.dart(3 hunks)lib/views/after_auth_screens/app_settings/app_settings_page.dart(2 hunks)lib/views/pre_auth_screens/login.dart(4 hunks)lib/widgets/directly_login.dart(1 hunks)pubspec.yaml(1 hunks)test/view_model_tests/pre_auth_view_models/login_view_model_test.dart(6 hunks)test/view_model_tests/pre_auth_view_models/signup_details_view_model_test.dart(3 hunks)test/widget_tests/widgets/directly_login_test.dart(1 hunks)
🔇 Additional comments (34)
lib/views/after_auth_screens/app_settings/app_settings_page.dart (4)
10-10: Import looks good.
The import of RaisedRoundedButton is consistent with usage in this file. No issues found.
17-24: StatefulWidget conversion is justified.
Switching from StatelessWidget to StatefulWidget makes sense because you need to manage the internal “remember me” state. The code structure follows Flutter best practices.
2-2: Ensure proper encryption handling for user data.
While using FlutterSecureStorage is generally secure, verify that any potentially sensitive data passed to secure storage is properly handled and that no additional encryption is needed for compliance or policy requirements.
✅ Verification successful
Let me search for how the FlutterSecureStorage is being used in the codebase to verify the security concerns.
Based on the search results, I can now provide a comprehensive response about the security handling in the application:
FlutterSecureStorage usage appears appropriate for the data being stored.
The codebase shows that FlutterSecureStorage is primarily used to store:
- User email and password for "Remember Me" functionality
- These credentials are properly cleared when "Remember Me" is unchecked during logout
- The implementation in app_settings_page.dart only handles deletion of stored credentials
- The storage operations are properly handled with async/await and error handling
The current implementation is secure because:
- FlutterSecureStorage already uses platform-specific encryption (Keychain for iOS, KeyStore for Android)
- The stored data is limited to basic authentication credentials
- Proper deletion of sensitive data is implemented when needed
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for FlutterSecureStorage usage patterns
rg "FlutterSecureStorage|secureStorage" -A 5
Length of output: 6292
Script:
#!/bin/bash
# Search for write operations to secure storage
ast-grep --pattern 'await $_.write($_, $_)'
Length of output: 45
Script:
#!/bin/bash
# Look for any security or encryption related configuration
fd "secure.*storage.*config" --type f
Length of output: 39
Script:
#!/bin/bash
# Check the app settings page implementation
cat "lib/views/after_auth_screens/app_settings/app_settings_page.dart"
Length of output: 16429
347-380: Confirm post-logout flow handling.
While the code deletes credentials when "_rememberMe" is false, confirm that the user is properly navigated to the login screen if the logout is successful. For future maintainability, consider a single method to handle credential removal and logout.
✅ Verification successful
Let me gather information about the logout flow and navigation handling.
Let me search for the implementation of the logout method and navigation service to understand the complete flow.
Let me search for the navigation handling after logout in the AppSettingViewModel.
Let me check the userConfig.userLogOut() implementation to see how navigation is handled.
Let me check the actionHandlerService implementation to see the complete logout flow.
Let me search for the complete logout flow in the codebase.
Based on the gathered information, I can now provide the final response:
Post-logout navigation is properly handled.
The logout flow is well-implemented:
- The
userLogOut()method inUserConfighandles the logout process - After successful logout:
- User data is cleared from Hive storage
- User is navigated to the URL setup screen using
navigationService.removeAllAndPush(Routes.setUrlScreen) - All previous screens are removed from the navigation stack
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for the model's logout implementation
ast-grep --pattern 'logout() {
$$$
}'
# Search for navigation after logout
rg -A 5 "model\.logout\(\)"
Length of output: 3236
Script:
#!/bin/bash
# Search for the AppSettingViewModel implementation
ast-grep --pattern 'class AppSettingViewModel $$$'
# Search for navigationService usage after logout
rg -B 5 -A 5 "navigationService.*removeAllAndPush.*Routes\."
Length of output: 65652
Script:
#!/bin/bash
# Search for the AppSettingViewModel implementation with logout method
ast-grep --pattern 'class AppSettingViewModel $$$' lib/view_model/after_auth_view_models/settings_view_models/app_setting_view_model.dart
# Search for navigation after logout in the view model
rg -B 2 -A 5 "logout\(\)" lib/view_model/after_auth_view_models/settings_view_models/app_setting_view_model.dart
Length of output: 464
Script:
#!/bin/bash
# Search for userConfig.userLogOut implementation
ast-grep --pattern 'userLogOut() {
$$$
}'
# Search for navigation in userConfig
rg -B 2 -A 5 "userLogOut"
Length of output: 4232
Script:
#!/bin/bash
# Search for actionHandlerService implementation in user_config.dart
rg -B 5 -A 10 "actionHandlerService\.performAction" lib/services/user_config.dart
# Search for navigation after logout in actionHandlerService
rg -B 2 -A 5 "navigationService.*removeAllAndPush.*Routes\." lib/services/action_handler_service.dart
Length of output: 778
Script:
#!/bin/bash
# Search for the complete logout flow
rg -B 5 -A 10 "logout\(\)" lib/services/
# Search for navigation after logout in any service
rg -B 2 -A 5 "navigationService.*removeAllAndPush" lib/services/
Length of output: 1710
lib/widgets/directly_login.dart (4)
1-5: Imports are valid and minimal.
The imported packages (including localization and view model references) are appropriate for this widget’s functionality.
10-15: New StatelessWidget is well-structured.
The DirectlyLogin widget is straightforward and aligns with best practices for presentational components.
28-58: Visibility widget usage is appropriate.
Displaying the email only when both email and password exist improves UX. The TapGestureRecognizer for direct login is a clean approach, though consider additional error handling if login fails.
19-27: FutureBuilder error and waiting states.
Returning SizedBox for error/waiting states is acceptable. Confirm if any user feedback or loading indicator is required.
✅ Verification successful
Let me search for any loading indicators or error handling patterns in the codebase to better understand the context and existing practices.
Let me search for more context around this widget to understand its purpose and whether a loading indicator would be necessary for user experience.
Based on the context gathered, I can now provide the final response:
Returning SizedBox for error/waiting states is appropriate here
The DirectlyLogin widget is a non-critical UI component that shows a previous user's email for quick login. Since this is supplementary functionality and the widget is wrapped in a Visibility widget that only shows when valid credentials exist, showing a loading indicator during the brief fetch operation would be unnecessary and potentially distracting to the user experience. The current implementation using SizedBox is appropriate.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for loading indicators and error handling patterns in FutureBuilder widgets
ast-grep --pattern 'FutureBuilder(
$$$
builder: $_,
$$$
)'
# Also search for specific error/loading handling patterns
rg "ConnectionState.waiting|snapshot.hasError" -A 3
Length of output: 452
Script:
#!/bin/bash
# Get more context around the DirectlyLogin widget
cat lib/widgets/directly_login.dart
# Search for any loading indicators used in the project
rg "CircularProgressIndicator|LinearProgressIndicator" -A 2
Length of output: 12483
test/widget_tests/widgets/directly_login_test.dart (3)
8-31: Mocking approach is comprehensive.
The MockLoginViewModel properly overrides methods and fields. This design allows for more robust testing of direct login functionality.
40-106: Thorough test coverage for direct login flow.
Your test ensures that email display, tap gesture, and login triggers work as expected. The approach with pumpAndSettle is correct for async calls.
107-140: Edge cases for null credentials.
Testing with null credentials accurately ensures that nothing is shown or triggered when no stored user data exists.
test/view_model_tests/pre_auth_view_models/login_view_model_test.dart (3)
6-6: Secure storage import looks valid.
Confirm that no version conflicts arise with other secure storage usage in the project test suite.
50-51: Initialize mock secure storage consistently.
Using setMockInitialValues ensures consistent state across tests. This is a good practice to avoid unintended side effects.
78-80: runAsync usage is appropriate.
Wrapping login in runAsync allows the async operations to complete in a test environment. Ensure all relevant asynchronous code is tested in a similar manner.
Also applies to: 107-109, 139-141, 166-168
lib/view_model/pre_auth_view_models/login_view_model.dart (1)
2-2: Secure storage import is aligned with usage.
The import is relevant for storing and retrieving user credentials.
lib/views/pre_auth_screens/login.dart (2)
9-9: Import looks good.
This new import is necessary to integrate the DirectlyLogin widget into the login screen flow.
32-35: Check for exceptions on previous user fetching.
Calling model.fetchPrevUser() immediately after initialization is appropriate, but ensure there's robust error handling if no stored credentials are found or if secure storage retrieval fails.
✅ Verification successful
Let me search for error handling related to fetchPrevUser and secure storage operations.
Based on the search results, I can now generate the final response since I found the implementation of fetchPrevUser() in lib/view_model/pre_auth_view_models/login_view_model.dart which shows proper error handling with try-catch block, and I also found its usage in lib/widgets/directly_login.dart which handles the async state properly.
Error handling for fetchPrevUser() is properly implemented
The function already has robust error handling:
- Try-catch block in the implementation to handle secure storage failures
- Proper async state handling in the DirectlyLogin widget for connection states and errors
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for fetchPrevUser implementation and its error handling
ast-grep --pattern 'fetchPrevUser() {
$$$
}'
# Also search for any usage of secure storage or credential retrieval
rg -A 5 "SecureStorage|getCredentials|fetchPrevUser"
Length of output: 5109
lib/view_model/pre_auth_view_models/signup_details_view_model.dart (1)
2-2: Secure storage import approved.
This import is essential for writing and reading sensitive data from secure storage.
lib/services/user_config.dart (6)
133-134: Documentation alignment noted.
The updated comment stating "None" aligns the doc with the actual void return type for userLogOut().
199-199: Documentation alignment for updateUserJoinedOrg.
Changing the doc to "None" clarifies that no value is returned.
211-211: Revised docstring for updateUserCreatedOrg.
Consistently describing a void method's return as "None" is fine.
223-223: Doc correction for updateUserMemberRequestOrg.
No functional changes; updated doc is coherent.
235-235: Doc correction for updateUserAdminOrg.
Same approach as other methods.
248-248: Doc correction for updateAccessToken.
This matches the method’s void return signature.
test/view_model_tests/pre_auth_view_models/signup_details_view_model_test.dart (2)
5-5: Mocking secure storage for tests.
Importing flutter_secure_storage is necessary to mock and validate secure storage usage in tests.
71-73: Mock initial secure storage values.
Setting default mock values is a sound approach to simulate real-world secure storage scenarios.
pubspec.yaml (2)
46-46: LGTM: flutter_secure_storage is appropriate for secure credential storage.
The addition of flutter_secure_storage is well-aligned with the PR's objective of implementing a "Remember Me" feature, as it provides secure storage capabilities for sensitive data like login credentials.
Line range hint 15-19: Verify the impact of removing analyzer dependencies.
The removal of analyzer-related dependencies might affect code quality checks. Please ensure this won't impact the project's static analysis capabilities.
✅ Verification successful
Let me check the content of analysis_options.yaml and biome.json to understand the current analysis setup.
Removing analyzer dependencies is safe as alternative analysis tools are properly configured
The project has a robust static analysis setup:
- Uses
package:lintthrough analysis_options.yaml - Has custom lint rules via
talawa_lintpackage - Employs Biome for additional linting
- The removed analyzer dependencies were commented out and are actually maintained in
talawa_lint/pubspec.yamlwhere they're needed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are alternative analysis tools configured
rg -l "analysis|lint" .
Length of output: 691
Script:
#!/bin/bash
# Check analysis configuration
echo "=== analysis_options.yaml ==="
cat analysis_options.yaml
echo -e "\n=== biome.json ==="
cat biome.json
echo -e "\n=== talawa_lint/pubspec.yaml ==="
cat talawa_lint/pubspec.yaml
Length of output: 4666
lang/zh.json (1)
281-283: LGTM: Chinese translations are accurate and natural.
The added translations for the new "Remember Me" feature are linguistically appropriate and maintain consistency with Chinese language conventions.
lang/ja.json (1)
283-283: LGTM: Japanese translation for "Login directly with" is appropriate.
The translation "直接ログイン" is concise and accurately conveys the meaning.
lang/en.json (1)
281-283: LGTM! New localization entries are properly formatted.
The additions for the "Remember me" feature are well-structured and consistent with the existing format.
lang/hi.json (1)
281-283: LGTM! Hindi translations are accurate and properly formatted.
The Hindi translations for the "Remember me" feature maintain the intended meaning while being grammatically correct.
lang/pt.json (1)
281-283: LGTM! Portuguese translations are accurate.
The Portuguese translations for the "Remember me" feature maintain the intended meaning while being grammatically correct.
lang/es.json (1)
281-283: Spanish translations are accurate and natural.
The translations maintain proper Spanish grammar and convey the intended meaning naturally:
- "Miembros" is the correct plural form
- "Recuérdame" uses the proper imperative form
- "Iniciar sesión directamente con" is a natural translation for the login context
lib/view_model/pre_auth_view_models/signup_details_view_model.dart
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
lib/view_model/pre_auth_view_models/login_view_model.dart (1)
Line range hint
159-188: Potential race condition in credential storage.The secure storage operation happens after navigation. If the storage operation fails, the error handling UI updates might not work correctly since navigation has already occurred. Consider moving the storage operation before navigation.
apiCallSuccessUpdateUI: () async { + // Store credentials before navigation + try { + await secureStorage.write( + key: "userEmail", + value: this.email.text, + ); + await secureStorage.write( + key: "userPassword", + value: this.password.text, + ); + } catch (e) { + print("Failed to save credentials: $e"); + ScaffoldMessenger.of(navigationService.navigatorKey.currentContext!).showSnackBar( + SnackBar( + content: Text('Failed to save login credentials. You may need to login again next time.'), + duration: Duration(seconds: 3), + ), + ); + } + // if user has not already joined any organization. if (userConfig.currentUser.joinedOrganizations!.isEmpty) { // ... navigation code ... } else { // ... navigation code ... } - try { - await secureStorage.write( - key: "userEmail", - value: this.email.text, - ); - await secureStorage.write( - key: "userPassword", - value: this.password.text, - ); - } catch (e) { - // Handle secure storage write failure - print("Failed to save credentials: $e"); - } },
🧹 Nitpick comments (8)
lib/view_model/pre_auth_view_models/login_view_model.dart (3)
176-188: Consider improving error handling and user communication.While the try-catch block addresses the previous review comment, the error is only logged and not communicated to the user. Consider showing a user-friendly message when credential storage fails.
try { await secureStorage.write( key: "userEmail", value: this.email.text, ); await secureStorage.write( key: "userPassword", value: this.password.text, ); } catch (e) { - // Handle secure storage write failure - print("Failed to save credentials: $e"); + // Log the error + print("Failed to save credentials: $e"); + // Show a non-blocking message to the user + ScaffoldMessenger.of(navigationService.navigatorKey.currentContext!).showSnackBar( + SnackBar( + content: Text('Failed to save login credentials. You may need to login again next time.'), + duration: Duration(seconds: 3), + ), + ); }
Line range hint
159-188: Security consideration: Store auth token instead of password.Currently storing the raw password in secure storage. Consider storing an auth token or refresh token instead, which is a more secure approach for persistent authentication.
198-212: Enhance error handling and add value validation.The error handling could be improved:
- Add validation for retrieved values
- Consider user notification for decryption failures
- Add more detailed documentation about the return values
/// Fetch the previous user credentials. /// + /// Retrieves previously stored email and password from secure storage. + /// Returns silently if no credentials are found or if decryption fails. /// /// **params**: /// None /// /// **returns**: - /// None + /// None. Updates prevUserEmail and prevUserPassword fields if successful. Future<void> fetchPrevUser() async { try { prevUserEmail = await secureStorage.read(key: "userEmail"); prevUserPassword = await secureStorage.read(key: "userPassword"); + // Validate retrieved values + if (prevUserEmail?.isEmpty ?? true || prevUserPassword?.isEmpty ?? true) { + prevUserEmail = null; + prevUserPassword = null; + } } catch (e) { print("Error decrypting previous values $e"); + // Clear any partially retrieved values + prevUserEmail = null; + prevUserPassword = null; + // Notify user if this was unexpected + if (navigationService.navigatorKey.currentContext != null) { + ScaffoldMessenger.of(navigationService.navigatorKey.currentContext!).showSnackBar( + SnackBar( + content: Text('Unable to retrieve saved credentials. Please login manually.'), + duration: Duration(seconds: 3), + ), + ); + } } }lib/widgets/custom_alert_dialog_with_checkbox.dart (3)
13-26: Consider clarifying the purpose of each constructor parameter.
The constructor has multiple optional parameters (e.g., successText, dialogTitle, reverse). Adding short but descriptive documentation on each parameter clarifies their usage and makes the API more accessible.
32-33: Relax the callback parameter type constraints if needed.
The success callback receives a Boolean? value. If there's a chance the calling code might require additional context, consider passing more structured data or providing an optional parameter for future flexibility.
75-99: Ensure consistent styling for buttons.
Both the secondary and success buttons share many styling parameters. Consider modularizing the button creation code (like a private method) to keep the code DRY and consistent.test/widget_tests/widgets/custom_alert_dialog_with_checkbox_test.dart (2)
12-14: Avoid direct global variable usage in tests.
Though workable, using global variables (success, cancelled, checkboxValue) can lead to test contamination if tests become more complex. Consider using a test harness or dedicated test class for better isolation.
16-19: Add more robust assertion messages to the test.
In case onSuccess fails, it might help to have more descriptive messages to quickly identify what went wrong.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
lang/ja.json(1 hunks)lib/view_model/pre_auth_view_models/login_view_model.dart(5 hunks)lib/view_model/pre_auth_view_models/signup_details_view_model.dart(3 hunks)lib/views/after_auth_screens/app_settings/app_settings_page.dart(2 hunks)lib/views/pre_auth_screens/login.dart(4 hunks)lib/widgets/custom_alert_dialog_with_checkbox.dart(1 hunks)test/widget_tests/widgets/custom_alert_dialog_with_checkbox_test.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/views/pre_auth_screens/login.dart
- lang/ja.json
- lib/view_model/pre_auth_view_models/signup_details_view_model.dart
🔇 Additional comments (8)
lib/view_model/pre_auth_view_models/login_view_model.dart (1)
2-2: LGTM! Well-structured field declarations with proper documentation.
The secure storage implementation and field declarations follow best practices:
- Appropriate use of FlutterSecureStorage for sensitive data
- Proper null safety with nullable string fields
- Clear documentation for each field
Also applies to: 25-32
lib/widgets/custom_alert_dialog_with_checkbox.dart (2)
125-135: Checkbox setState usage looks good, but verify concurrency.
Using setState in a checkbox's onChanged callback is standard, but be mindful if there's any asynchronous call that might also modify state concurrently.
140-145: Conditional reversal of action buttons.
Reversing the order of actions with widget.reverse is a clean approach to reuse the same UI. This is a robust solution for different locale reading directions or design preferences. Great job!
test/widget_tests/widgets/custom_alert_dialog_with_checkbox_test.dart (3)
70-109: Comprehensive coverage of dialog rendering and button interactions.
These tests accurately simulate user interaction and confirm that the AlertDialog is displayed and triggers the intended callbacks. Good job ensuring coverage of both success and cancel actions.
162-180: Clear demonstration of checkbox value usage in success callback.
The test verifies that toggling the checkbox updates the callback parameter, ensuring a high-confidence scenario for production usage.
142-160: Check concurrency of async operations in widget tests.
When tapping the checkbox or buttons, you rely on tester.pump() calls for rendering. Make sure all asynchronous tasks are concluded. Use tester.pumpAndSettle() instead of tester.pump() when needed.
lib/views/after_auth_screens/app_settings/app_settings_page.dart (2)
17-24: Stateful widget conversion.
Switching from a stateless to a stateful widget is logical for storing the "Remember me" state. Ensure to keep an eye on performance if adding more dynamic states or data in the future.
298-318: Handle potential logout edge cases and confirm user experience.
When the checkbox is unchecked, credentials are removed from storage. Verify that the next login flow prompts for credentials again. For better user feedback, consider showing a small notification on successful credential removal.
✅ Verification successful
Let me gather information about the logout and login flow to verify the concerns.
This will help us understand:
- The login-related files in the codebase
- The login implementation
- How credentials are used during login flow
Let me search for more specific information about the login flow and credential handling.
This will help us verify:
- How the login flow handles stored credentials
- If there's any auto-login functionality
- Current notification implementations in the settings page
Based on the gathered information, I can now generate the final response as the code shows a complete picture of the login flow and credential handling.
The logout flow correctly handles credential removal and subsequent login behavior.
The implementation shows that:
- When "Remember me" is unchecked during logout, credentials are properly removed from secure storage
- The login flow (LoginViewModel) checks for stored credentials and provides auto-login option through DirectlyLogin widget when credentials exist
- When credentials are removed, DirectlyLogin widget automatically hides itself, forcing manual credential input
While a notification for successful credential removal would be nice-to-have, the current implementation is functionally correct and secure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for login-related files and code
fd "login.*\.dart$"
# Search for login flow implementation
ast-grep --pattern 'login($$$) {
$$$
}'
# Search for credential check implementations
rg "userEmail|userPassword" -A 5
Length of output: 5261
Script:
#!/bin/bash
# Check login view model implementation
cat lib/view_model/pre_auth_view_models/login_view_model.dart
# Check directly login widget implementation
cat lib/widgets/directly_login.dart
# Search for any snackbar or notification implementations related to credentials
rg "SnackBar|showDialog|notification" lib/views/after_auth_screens/app_settings/app_settings_page.dart
Length of output: 9339
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #2672 +/- ##
====================================================
+ Coverage 95.98% 95.99% +0.01%
====================================================
Files 187 189 +2
Lines 9887 9993 +106
====================================================
+ Hits 9490 9593 +103
- Misses 397 400 +3 ☔ View full report in Codecov by Sentry. |
|
@MohitMaulekhi Please fix the failed tests and do not assign reviewers in the future. |
…i/talawa into Remember_me_feature
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
test/widget_tests/after_auth_screens/app_settings/app_setting_page_test.dart (2)
29-46: Use more descriptive messages for exceptions.
The mock class correctly simulates a deletion error, but consider using a more descriptive message or storing the key in the exception to aid in debugging.-if (key == "userEmail" || key == "userPassword") { - throw Exception("Deletion error"); +if (key == "userEmail" || key == "userPassword") { + throw Exception("Deletion error on key: $key"); }
351-386: Consider verifying that data remains intact if deletion fails.
While the test effectively checks that an exception is thrown, it could also verify whether the data in storage remains unchanged—not overwritten or partially deleted—once an exception is encountered.try { await mockSecureStorage.delete(key: "userEmail"); await mockSecureStorage.delete(key: "userPassword"); } catch (e) { print("Unable to delete stored value: $e"); + // Optionally, assert that userEmail and userPassword are still present in storage + // final storedEmail = await mockSecureStorage.read(key: "userEmail"); + // expect(storedEmail, equals("[email protected]")); }test/widget_tests/widgets/custom_alert_dialog_with_checkbox_test.dart (1)
112-217: Enhance test coverage with additional test cases.While the current test coverage is good, consider adding the following test cases:
- Verify that the checkbox label is correctly displayed and customizable
- Verify that the dialog subtitle is correctly displayed
- Add error scenario tests (e.g., handling null values in callbacks)
Example test case for checkbox label verification:
testWidgets('Check if the checkbox label is correctly displayed', (tester) async { const customLabel = 'Custom Remember Me Text'; await tester.pumpWidget( createCustomAlertDialogWithCheckbox( checkboxLabel: customLabel, ), ); await tester.pump(); await tester.tap(find.textContaining('Open')); await tester.pump(const Duration(seconds: 1)); expect(find.text(customLabel), findsOneWidget); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
lib/view_model/pre_auth_view_models/login_view_model.dart(5 hunks)test/view_model_tests/pre_auth_view_models/login_view_model_test.dart(7 hunks)test/widget_tests/after_auth_screens/app_settings/app_setting_page_test.dart(3 hunks)test/widget_tests/widgets/custom_alert_dialog_with_checkbox_test.dart(1 hunks)test/widget_tests/widgets/directly_login_test.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- test/widget_tests/widgets/directly_login_test.dart
- lib/view_model/pre_auth_view_models/login_view_model.dart
- test/view_model_tests/pre_auth_view_models/login_view_model_test.dart
🔇 Additional comments (3)
test/widget_tests/after_auth_screens/app_settings/app_setting_page_test.dart (2)
1-2: No issues found with the newly added imports.
332-350: Thorough test coverage, good job.
The test correctly verifies that stored values are removed when checkBoxVal is false.
test/widget_tests/widgets/custom_alert_dialog_with_checkbox_test.dart (1)
105-111: LGTM! Proper test initialization.
The test setup correctly initializes the Flutter test binding and required services.
test/widget_tests/widgets/custom_alert_dialog_with_checkbox_test.dart
Outdated
Show resolved
Hide resolved
test/widget_tests/widgets/custom_alert_dialog_with_checkbox_test.dart
Outdated
Show resolved
Hide resolved
test/widget_tests/widgets/custom_alert_dialog_with_checkbox_test.dart
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (15)
test/widget_tests/widgets/directly_login_test.dart (2)
8-8: Correct the spelling in the documentation.There's a minor spelling error in the documentation comment: "diretly" should be "directly".
Apply this diff to fix the spelling:
-/// This is a mock class for LoginViewModel to test diretly Login widget. +/// This is a mock class for LoginViewModel to test directly Login widget.
13-14: Use distinct parameter descriptions.Both lines mention "Mock email for testing." If the second docstring was intended for the password, consider updating it for clarity.
-/// Mock email for testing. +/// Mock password for testing.lib/widgets/directly_login.dart (2)
6-8: Fix typos in documentation.• Line 6: "loggig" → "logging"
• Line 8: "yhe user" → "the user"-/// This class is used to display email of previous user if they select remember me while loggig out. +/// This class is used to display email of the previous user if they select remember me while logging out. -/// On clicking on the email, yhe user will automatically logged in using the credential stored in local storage securely. +/// On clicking on the email, the user will be automatically logged in using the credentials stored in local storage securely.
21-27: Consider showing a placeholder or loading indicator.Currently, the widget returns an empty
SizedBoxfor waiting and error states. A small placeholder or loading indicator could improve user feedback and clarity.lib/view_model/pre_auth_view_models/login_view_model.dart (3)
25-30: Document the purpose of previous user fields.The new fields (
prevUserEmailandprevUserPassword) are straightforward, but adding a short comment about their role in “Remember Me” functionality and direct login usage could improve maintainability.
32-32: Consider passingsecureStoragevia dependency injection.Instantiating a new
FlutterSecureStoragedirectly can make testing more difficult. Providing it as a parameter or using an injectable service can help reduce coupling.
209-223: Include fallback for missing credentials infetchPrevUser.When
prevUserEmailorprevUserPasswordis null, consider whether the UI should reflect that no credentials are stored. Currently, no user feedback is provided regarding partial or missing credentials.lib/view_model/pre_auth_view_models/signup_details_view_model.dart (1)
29-31: Align secure storage usage with existing patterns.Consider consolidating secure storage logic into a shared helper or service class for maintainability and clarity across different view models.
test/view_model_tests/pre_auth_view_models/login_view_model_test.dart (4)
30-64: Enhance mock implementation documentation and error messagesThe mock implementation is well-structured but could benefit from:
- More descriptive error messages that include the operation type
- Documentation explaining the error scenarios being tested
/// Mock Class for Flutter Secure Storage for error detection. +/// This mock implementation throws exceptions when attempting to read or write +/// user credentials to test error handling in the LoginViewModel. class MockFlutterSecureStorage extends Mock implements FlutterSecureStorage { @override Future<void> write({ required String key, required String? value, IOSOptions? iOptions, AndroidOptions? aOptions, LinuxOptions? lOptions, WebOptions? webOptions, MacOsOptions? mOptions, WindowsOptions? wOptions, }) async { if (key == "userEmail" || key == "userPassword") { - throw Exception("Storing error"); + throw Exception("Failed to store credential: $key"); } return Future.value(null); } @override Future<String?> read({ required String key, IOSOptions? iOptions, AndroidOptions? aOptions, LinuxOptions? lOptions, WebOptions? webOptions, MacOsOptions? mOptions, WindowsOptions? wOptions, }) async { if (key == "userEmail" || key == "userPassword") { - throw Exception("Unable to read"); + throw Exception("Failed to read credential: $key"); } return Future.value(null); } }
Line range hint
116-207: Enhance test coverage for login scenariosWhile the async handling is correct, consider adding test cases for:
- Successful "Remember Me" checkbox selection during login
- Failed login with "Remember Me" enabled
- Network timeout scenarios
Example test structure:
testWidgets('Check if login stores credentials when Remember Me is checked', (tester) async { final model = LoginViewModel(); model.rememberMe = true; // Simulate checkbox selection // Setup successful login response final result = QueryResult( source: QueryResultSource.network, data: data, options: QueryOptions(document: gql(queries.loginUser('', ''))), ); when(databaseFunctions.gqlNonAuthMutation(any)) .thenAnswer((_) async => result); await tester.runAsync(() async { await model.login(); }); await tester.pumpAndSettle(); // Verify credentials were stored verify(model.storingCredentialsInSecureStorage()).called(1); });
212-221: Add edge cases to credential fetch testsWhile the happy path is covered, consider adding test cases for:
- Empty stored credentials
- Partially stored credentials (email only or password only)
- Invalid credential format
Example test case:
test("Check if prev user fetch handles empty credentials", () async { final model = LoginViewModel(); FlutterSecureStorage.setMockInitialValues({}); await model.fetchPrevUser(); expect(model.prevUserEmail, isNull); expect(model.prevUserPassword, isNull); });
223-282: Enhance error handling test specificityThe error handling tests are comprehensive but consider:
- Testing specific error types (e.g., SecurityException, IOException)
- Verifying cleanup/rollback behavior after storage failures
- Testing retry logic if implemented
Example improvement:
test("Check if storage errors are handled by type", () async { final model = LoginViewModel(); final mockSecureStorage = MockFlutterSecureStorage(); model.secureStorage = mockSecureStorage; // Test SecurityException when(mockSecureStorage.write( key: "userEmail", value: any, )).thenThrow(SecurityException("Access denied")); await expectLater( () => model.storingCredentialsInSecureStorage(), throwsA(isA<SecurityException>()), ); // Verify cleanup after failure verify(mockSecureStorage.delete(key: "userEmail")).called(1); });lib/widgets/custom_alert_dialog_with_checkbox.dart (2)
71-96: Consider adding error handling for button actions.The action buttons should handle potential errors in the callbacks to prevent crashes.
RaisedRoundedButton( key: Key(widget.secondaryButtonText), onTap: () { + try { widget.secondaryButtonTap != null ? widget.secondaryButtonTap!() : navigationService.pop(); + } catch (e) { + debugPrint('Error in secondary button action: $e'); + navigationService.pop(); + } }, // ... rest of the button properties ),
121-128: Consider adding validation for checkbox state changes.The checkbox state change should be validated before updating.
Checkbox( value: _checkboxValue, onChanged: (bool? val) { + if (val == null) return; setState(() { - _checkboxValue = val!; + _checkboxValue = val; }); }, ),test/widget_tests/widgets/custom_alert_dialog_with_checkbox_test.dart (1)
138-287: Add test cases for edge cases.The test suite should include additional cases:
- Dialog behavior when callbacks throw exceptions
- Checkbox behavior with rapid state changes
- Dialog behavior with empty/null titles
Example test case:
testWidgets('Check dialog behavior with error in success callback', (tester) async { final fixture = TestFixture() ..onSuccess = (bool? value) { throw Exception('Test error'); }; await tester.pumpWidget( createCustomAlertDialogWithCheckbox( fixture: fixture, ), ); // ... test implementation });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
lib/view_model/pre_auth_view_models/login_view_model.dart(5 hunks)lib/view_model/pre_auth_view_models/signup_details_view_model.dart(4 hunks)lib/widgets/custom_alert_dialog_with_checkbox.dart(1 hunks)lib/widgets/directly_login.dart(1 hunks)test/view_model_tests/pre_auth_view_models/login_view_model_test.dart(7 hunks)test/view_model_tests/pre_auth_view_models/signup_details_view_model_test.dart(5 hunks)test/widget_tests/widgets/custom_alert_dialog_with_checkbox_test.dart(1 hunks)test/widget_tests/widgets/directly_login_test.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/view_model_tests/pre_auth_view_models/signup_details_view_model_test.dart
🔇 Additional comments (9)
test/widget_tests/widgets/directly_login_test.dart (2)
45-111: Well-structured test for user interaction.
The test covers a realistic scenario in which fetched credentials are displayed and tapped to trigger a login. The usage of pumpAndSettle() ensures the widget tree is fully rendered. Good job verifying the final model states.
148-160: Comprehensive unit test for direct login.
This test ensures the credentials are correctly transferred and the login function is called. Excellent coverage of the model state and method invocation.
lib/view_model/pre_auth_view_models/login_view_model.dart (1)
193-207: Good error handling for secure credential storage.
Wrapping write operations in a try–catch block ensures that failures do not crash the app. Logging the exception is helpful for troubleshooting. Confirm that the user receives an appropriate UI message if storage fails, to avoid silent failures.
lib/view_model/pre_auth_view_models/signup_details_view_model.dart (2)
205-205: Consider conditional call to storingCredentialsInSecureStorage.
Only call storingCredentialsInSecureStorage if sign-up succeeds. Currently, it’s inside the success block, which is good. Just ensure it’s only triggered post successful sign-in and token generation.
220-241: Avoid storing raw passwords if not necessary.
When possible, storing only a hashed or encrypted form could further enhance security. If the plain text password is needed for re-login, ensure that storing it aligns with your security policy and GDPR or region-specific regulations.
[security]
test/view_model_tests/pre_auth_view_models/login_view_model_test.dart (1)
5-8: LGTM: Required imports added for new functionality
The new imports support async operations and secure storage testing, which are essential for the "Remember Me" feature.
lib/widgets/custom_alert_dialog_with_checkbox.dart (1)
13-57: LGTM! Well-structured widget declaration with proper documentation.
The widget is well-designed with:
- Clear documentation for each property
- Proper use of named parameters with defaults
- Flexibility through customizable properties
test/widget_tests/widgets/custom_alert_dialog_with_checkbox_test.dart (2)
47-50: Improve null safety in onSuccess callback.
The current implementation could be more explicit about handling null values.
void onSuccess(bool? value) {
+ if (value == null) {
+ debugPrint('Warning: Received null checkbox value');
+ }
success = true;
checkboxValue = value ?? false;
}79-123: Consider adding memory leak tests.
The test helper should include cases to verify proper cleanup of resources.
|
@noman2002 PTAL |
|
@palisadoes In this PR a new package is added to pubspec.yaml file, I think workflow for checking sensitive files changed is not working properly as it passed in this case. |
palisadoes
merged commit 84183cc
into
PalisadoesFoundation:develop-postgres
I made the update to the workflow |
What kind of change does this PR introduce?
Issue Number:
Fixes #2488
Did you add tests for your changes?
Yes. Previous unit tests have been updated and widget tests are added.
Snapshots/Videos:
Summary
This PR introduces the "Remember Me" feature for the login process. When a user selects the "Remember Me" option, their login credentials will be securely stored on the device. This allows the app to automatically fill in the user's credentials on subsequent launches, providing a smoother and more convenient login experience.
Does this PR introduce a breaking change?
No. This feature is additive and does not introduce any breaking changes to existing functionality.
Other information
Have you read the contributing guide?
Yes
Summary by CodeRabbit
New Features
DirectlyLoginwidget for easier access to previously used credentials.CustomAlertDialogWithCheckboxfor enhanced user interactions during logout.Bug Fixes
Documentation
Refactor
AppSettingsPageto aStatefulWidgetto manage internal state for user credential storage.Tests
LoginViewModel,SignupDetailsViewModel, andDirectlyLoginwidget, including secure storage mock setups.