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.

Sign up for GitHub

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

Jump to bottom

[#8] [UI] As a user, if I fail to log in, I see an error popup #55

Merged
merged 4 commits into from
Aug 17, 2023
Merged

[#8] [UI] As a user, if I fail to log in, I see an error popup #55

merged 4 commits into from
Aug 17, 2023

Conversation

Thieurom
Copy link
Collaborator

@Thieurom Thieurom commented Aug 14, 2023 •
edited
Loading

What happened 👀

Show the Alert dialog with an appropriate message when fail to log in.

  • Updated LoginForm:
    • Added email and password TextEditingControllers to keep track of inputs.
    • Implemented _submit().
  • Updated LoginViewModel:
    • Changed loginViewModelProvider from NotifierProvider to AsyncNotifierProvider.
    • Added internet_connection_manager library for checking Internet connection, created a wrapper named InternetConnectionManager.
    • Implemented partially login().
  • Added showAlertDialog() to show a native alert dialog for iOS and Android.

Insight 📝

  • The riverpod will be used for dependency injection (besides state management), so there's no need to use a separate library (i.e., get_it).
  • There are quite many ways for unit testing in Flutter. And when it comes to testing asynchronous events, it's even more complex. This PR follows this great article from Code with Andrea.

Proof Of Work 📹

Un-authorized error

iOS Android
un-authorized-ios.mp4
un-authorized-android.webm

No Internet connection error

iOS Android
no-internet-ios.mp4
no-internet-android.webm

@Thieurom Thieurom added this to the 0.3.0 milestone Aug 14, 2023
@Thieurom Thieurom self-assigned this Aug 14, 2023
@Thieurom Thieurom force-pushed the feature/8-ui-login-fail-alert branch 2 times, most recently from c0be120 to 6515054 Compare August 14, 2023 09:04
@codecov
Copy link

codecov bot commented Aug 14, 2023 •
edited
Loading

Codecov Report

Patch coverage: 40.90% and project coverage change: -5.28% ⚠️

Comparison is base (1434636) 52.47% compared to head (79981c7) 47.20%.
Report is 1 commits behind head on develop.

Additional details and impacted files 
@@             Coverage Diff             @@
##           develop      #55      +/-   ##
===========================================
- Coverage    52.47%   47.20%   -5.28%     
===========================================
  Files           11       14       +3     
  Lines          101      125      +24     
===========================================
+ Hits            53       59       +6     
- Misses          48       66      +18
Flag Coverage Δ
unittests 47.20% <40.90%> (-5.28%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
lib/uimodels/app_error.dart 0.00% <0.00%> (ø)
lib/utils/build_context_ext.dart 0.00% <0.00%> (ø)
lib/utils/internet_connection_manager.dart 40.00% <40.00%> (ø)
lib/screens/login/login_view_model.dart 92.30% <87.50%> (-7.70%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Thieurom Thieurom force-pushed the feature/8-ui-login-fail-alert branch 2 times, most recently from 6b4275d to 5fa7ea2 Compare August 15, 2023 07:33
@Thieurom Thieurom force-pushed the feature/8-ui-login-fail-alert branch from 5fa7ea2 to c8d6ea4 Compare August 15, 2023 08:21
@Thieurom Thieurom changed the title [wip] [#8] Show unauthenticated error dialog [#8] [UI] As a user, if I fail to log in, I see an error popup Aug 15, 2023
@Thieurom Thieurom marked this pull request as ready for review August 15, 2023 08:22
nkhanh44
nkhanh44 reviewed Aug 15, 2023
View reviewed changes
Copy link
Collaborator

@nkhanh44 nkhanh44 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

lib/widgets/alert_dialog.dart Outdated Show resolved Hide resolved
lib/utils/internet_connection_manager.dart Outdated Show resolved Hide resolved
nkhanh44
nkhanh44 reviewed Aug 16, 2023
View reviewed changes
lib/screens/login/login_view_model.dart
final isConnected = await internetConnectionManager.hasConnection();

if (!isConnected) {
state = const AsyncError(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May I ask if we need to write unit tests for these to increase the code coverage? as the Codevov just got failed 🙏
https://github.com/nimblehq/flutter-ic-khanh-thieu/pull/55/checks?check_run_id=15928008166

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed this too. However, the login() method is not integrated with API yet. I think we can supplement unit tests for LoginViewModel in the integrating task 🙏

doannimble
doannimble reviewed Aug 16, 2023
View reviewed changes
Copy link

@doannimble doannimble left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rest lgtm

lib/screens/login/login_form.dart Outdated Show resolved Hide resolved
lib/screens/login/login_screen.dart Show resolved Hide resolved
lib/screens/login/login_view_model.dart Outdated Show resolved Hide resolved
lib/screens/login/login_view_model.dart Show resolved Hide resolved
lib/widgets/alert_dialog.dart Outdated Show resolved Hide resolved
lib/widgets/alert_dialog.dart Outdated Show resolved Hide resolved
test/screens/login/login_view_model_test.dart Outdated
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better if the unit test should be written on the integration task

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replied here 🙏

doannimble
doannimble approved these changes Aug 17, 2023
View reviewed changes
Copy link

@doannimble doannimble left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to go 🏃‍♂️

@nkhanh44 nkhanh44 merged commit d0b4046 into develop Aug 17, 2023
2 of 4 checks passed
@nkhanh44 nkhanh44 deleted the feature/8-ui-login-fail-alert branch August 17, 2023 03:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nkhanh44 nkhanh44 nkhanh44 approved these changes

@doannimble doannimble doannimble approved these changes

@minhnimble minhnimble Awaiting requested review from minhnimble minhnimble is a code owner

Assignees

@Thieurom Thieurom

Labels
type : feature @0.3.0
Projects
None yet
Milestone
0.3.0
Development

Successfully merging this pull request may close these issues.

[UI] As a user, if I fail to log in, I see an error popup
3 participants
@Thieurom @nkhanh44 @doannimble