-
Notifications
You must be signed in to change notification settings - Fork 1.3k
UsernameChangerFullScreenDialogFragment made abstract for extensibility #10409
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
UsernameChangerFullScreenDialogFragment made abstract for extensibility #10409
Conversation
Generated by 🚫 dangerJS |
Note: I switched the base branch to |
…ality can be shared. The class now has the common functionality that will be needed in the sign-up and the account settings flow. The behaviour that differs with the header & analytics will be implemented in the respective classes that extend it.
…d Injector method since it makes it easier to test. The FragmentInjector can be replaced during tests. The older method requires modification to the TestComponents/TestModules etc. Also corrected how injections were taking place by moving injection to the subtype instead of the base type since the best practice is to inject subtypes.
Verifies that the suggestions functionality is still intact even though the implementation has been modified. Created an environment where the fragment could be tested in isolation utilizing Mockito,Dagger's Android Injection and a fragment container.
1d6d73f
to
e2544f4
Compare
Indeed. Just did a rebase with the feature branch, resolved the conflicts and now this is good to be merged in once you are fine with it 😄 |
As we agreed in chat, we're keeping these Espresso tests in the feature branch LGTM |
Fixes #8002
This is the second PR in the series that will be implementing the change username functionality.
Background
I made the
UsernameChangerFullScreenDialogFragment
abstract and added abstract methods so that the sign-up flow would have an implementation of it that retains the existing functionality while the new functionality for changing of the username in Account Settings would be able to create its own header behavior.The main difference between the two components is as follows.
Sign-up Username Changer
Account Settings Username Changer
Main Changes
Abstract methods
canHeaderTextLiveUpdate()
,getSuggestionsFailedStat()
,getHeaderText(username,display)
were created.UsernameChangerFullScreenDialogFragment
implemented the above methods by extendingBaseUsernameChangerFullScreenDialogFragment
Testing
WPAndroid
Since the sign-up flow can't be reached in debug mode, the easiest way to test based on @theck13 suggestion is to modify the
LoginActivity
to go to the signup screen.I did this modification at line 240
There's probably an easier way to do it, but I was hoping that doing it after login would give me a token so I could make changes.
A gif of the behaviour when tested :
UI Tests
I wrote an Espresso test that created an environment where the fragment could be spun up in isolation in an activity container with a mocked instance of the
dispatcher
. The test basically verifies that the functionality that has been modified is still intact and it will be instrumental in verifying the behavior of the followup settings username changer fragment.Main Changes
To accomplish this these were the steps:
FragmentActivity
element was added to the debug manifest so that a registered activity would be available as a container.@ContributesAndroidInjector
andDaggerFragment
was utilized for the fragment DI so the injector could be replaced easily and the mockeddispatcher
could be injected.Update release notes:
RELEASE-NOTES.txt
.