Skip to content

Conversation

jd-alexander
Copy link
Contributor

@jd-alexander jd-alexander commented Aug 19, 2019

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

  • The header text should live update as the options are selected.
  • The header text should contain both the username and display name.

Account Settings Username Changer

  • The header text shouldn't update as the options are selected.
  • The header text template will be similar to that of iOS.

Main Changes

  • Abstract methods canHeaderTextLiveUpdate(), getSuggestionsFailedStat(), getHeaderText(username,display) were created.

  • UsernameChangerFullScreenDialogFragment implemented the above methods by extending BaseUsernameChangerFullScreenDialogFragment

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

private void loggedInAndFinish(ArrayList<Integer> oldSitesIds, boolean doLoginUpdate) {
    ActivityLauncher.showMainActivityAndSignupEpilogue(this, "John", "John Doe", "fake_url","johndoe876");
  ... rest of function commented out.

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 :

ezgif com-video-to-gif (3)

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:

  • The fragment injector on the application class was given a setter so that it could be replaced with a test variant.
  • A FragmentActivity element was added to the debug manifest so that a registered activity would be available as a container.
  • @ContributesAndroidInjector and DaggerFragment was utilized for the fragment DI so the injector could be replaced easily and the mocked dispatcher could be injected.

Update release notes:

  • If there are user-facing changes, I have added an item to RELEASE-NOTES.txt.

@maxme maxme changed the base branch from develop to issue/8002-change-username August 21, 2019 11:26
@peril-wordpress-mobile
Copy link

Warnings
⚠️ PR is not assigned to a milestone.

Generated by 🚫 dangerJS

@maxme
Copy link
Contributor

maxme commented Aug 21, 2019

Note: I switched the base branch to issue/8002-change-username that we'll use for this and for followup PR related to that feature.

…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.
@jd-alexander jd-alexander force-pushed the 8002_make_suggestions_fragment_abstract branch from 1d6d73f to e2544f4 Compare August 22, 2019 03:02
@jd-alexander
Copy link
Contributor Author

Note: I switched the base branch to issue/8002-change-username that we'll use for this and for followup PR related to that feature.

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 😄

@maxme
Copy link
Contributor

maxme commented Aug 22, 2019

As we agreed in chat, we're keeping these Espresso tests in the feature branch issue/8002-change-username and we'll drop them before merging it to develop.

LGTM :shipit: (to the feature branch)

@maxme maxme merged commit 744c00b into wordpress-mobile:issue/8002-change-username Aug 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants