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.

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

33 firebase error handler #35

Merged
merged 4 commits into from
Aug 11, 2020
Merged

33 firebase error handler #35

merged 4 commits into from
Aug 11, 2020

Conversation

tamari-gray
Copy link
Owner

@tamari-gray tamari-gray commented Aug 11, 2020

fixes #33

Hi nick, went out of scope of ticket by implementing firebase error handling in signinscreen.
I did that so i could write tests for it.

i also had to touch the main,dart file, setting a navService in main(), and feeding the navigaorKey to myapp.
cant remember why we made the authservice in the main() function and not my app but i dont doubt theres a good reason haha.
and i updated the classes in your test files, if the ci tests pass, then im a lucky man.

Not sure if will work on ios

  • can you please test on ios for me, im not sure if the error message codes will be the same

@coveralls
Copy link

Pull Request Test Coverage Report for Build 204156602

  • 15 of 21 (71.43%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-3.03%) to 96.97%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/screens/sign_in.dart 4 6 66.67%
lib/services/auth/navigation_service.dart 11 15 73.33%
Totals Coverage Status
Change from base Build 198697977: -3.03%
Covered Lines: 224
Relevant Lines: 231

💛 - Coveralls

@nickmeinhold
Copy link
Collaborator

Hey mate! Just having a look through now.

cant remember why we made the authservice in the main() function and not my app

It's so we can inject mock services into the MyApp widget - either in a widget test where the widget under test is MyApp (so effectively the whole app) or in integration tests.

can you please test on ios for me, im not sure if the error message codes will be the same

What specifically do you want me to test? What error message code are you expecting?

Copy link
Collaborator

@nickmeinhold nickmeinhold left a comment

Choose a reason for hiding this comment

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

Looks great man, awesome work!!!

lib/screens/sign_in.dart Show resolved Hide resolved
lib/services/auth/firebase_auth_service.dart Show resolved Hide resolved
lib/services/auth/navigation_service.dart Show resolved Hide resolved
@nickmeinhold nickmeinhold merged commit 851993f into dev Aug 11, 2020
@nickmeinhold
Copy link
Collaborator

@tamari-gray just to follow up on the exceptions question, the exceptions are (as far as I understand) consistent across platforms since flutter/plugins#775 and apparently are being improved to use types: firebase/flutterfire#2653

@tamari-gray
Copy link
Owner Author

@nickmeinhold thanks so much for the detailed comments/ explanations & feedback!

@tamari-gray tamari-gray deleted the 33-firebase-error-handler branch August 28, 2020 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

create firebase error handler
3 participants