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

Added setDefaultEmail for EmailBuilder #1866

Merged
merged 6 commits into from
Nov 18, 2020
Merged

Added setDefaultEmail for EmailBuilder #1866

merged 6 commits into from
Nov 18, 2020

Conversation

laurentiu-git
Copy link
Contributor

Hey @samtstern !

I have implemented the feature for #1670 .

Copy link
Contributor

@samtstern samtstern left a comment

Choose a reason for hiding this comment

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

Two small changes:

  1. Is there a test you could add for this to make sure that changing this setting results in the right flow?
  2. Could you add an entry to CHANGELOG.md?

@laurentiu-git
Copy link
Contributor Author

I will think about a test, and about CHANGELOG.md it came in my mind after I did the pull request :)

@samtstern samtstern added this to the 7.1.0 milestone Nov 17, 2020
@laurentiu-git
Copy link
Contributor Author

Besides checking in AuthUITest that the value is stored inside ExtraConstants.DEFAULT_EMAIL, should I think of another way to test the flow? Perhaps inside the EmailActivityTest ?

@samtstern
Copy link
Contributor

@laurentiu-git the ideal test would be one that checks the effect of the value, not just that the value is set. So for example checking when the value is set and the user exists, the flow heads to the password screen not the email screen.

@samtstern
Copy link
Contributor

@laurentiu-git these tests look great, thank you! I'm going to resolve the merge conflicts (hope you don't mind) and then merge when CI is green.

@samtstern samtstern merged commit 4c75c23 into firebase:version-7.1.0-dev Nov 18, 2020
@laurentiu-git
Copy link
Contributor Author

While you are at it, I saw this test testOnTroubleSigningIn_expectTroubleSigningInFragment being commented out and it appears that now it's working.

@laurentiu-git laurentiu-git deleted the rld-fix-1670 branch November 18, 2020 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants