Skip to content

Conversation

renanferrari
Copy link
Contributor

@renanferrari renanferrari commented Dec 3, 2019

Fixes #10835

The problem: it's impossible for the user to change their display name and password at the same time during email signup. When trying to do so, the user gets a dialog stating that "The password setting cannot be changed at the same time as any other setting".

The cause: this is an API limitation (thank you @hypest for confirming this) that the current implementation doesn't take into consideration when it tries to update the display name and password with a single network call.

The solution: updating each account field (display name, username and password) separately – each with their own network call.

The main target of this PR was the SignupEpilogueFragment class, where I added some extra logic to manage the chain of updates. I suggest looking into the updateAccountOrContinue() method, which is where most of this logic happens.

I also tried to keep in mind scenarios where it would be necessary to recover from failures that occurred in the middle of the chain. Even though I wasn't able to test this as much as I wanted, the order in which the updates occur (username -> display name -> password) were thought of with this in mind and I've also added some extra functionality that would allow for a more consistent behavior in those cases, which were the main reasons for the changes you can see here and here. Let me know if I need to go into more detail about this.

To test

  1. Make sure you have OAuth credentials with user creation capabilities.
  2. If you have logged in to the app, sign out.
  3. On the initial screen, tap Sign up for WordPress.com.
  4. Tap Sign up with email.
  5. Enter an email address that is not associated with a WordPress.com account.
  6. On the next screen, tap Open mail.
  7. On the signup email sent to the address used in step 5, tap Sign up to WordPress.com.
  8. On the epilogue screen, fill in the Display Name and Password fields and tap Continue.
  9. Confirm that no error dialog appeared and that the values were correctly updated.

Notes

  1. For this PR, I did the minimum necessary to ensure the expected behavior and because of that, the proposed implementation here is extremely simple and tries to make good use of existing structures in the current code. However, this part of the signup flow seems to be a good candidate for further refactoring. As an example, I believe that the readability and testability of the SignupEpilogueFragment class could greatly benefit from the separation of responsibilities that a ViewModel could bring.

  2. While working on this PR, I found another existing bug where the username, display name, and password fields don't seem to retain their values after configuration changes (screen rotation, for example). In an attempt to maintain the focus of this PR, I haven't looked into this problem any further. Let me know if I should open a specific issue for it.


PR submission checklist:

  • I have considered adding unit tests where possible.

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@maxme maxme requested review from maxme and theck13 December 3, 2019 18:00
@maxme maxme self-assigned this Dec 3, 2019
@maxme maxme added this to the 13.9 milestone Dec 3, 2019
@maxme maxme added the Signup label Dec 3, 2019
Copy link
Contributor

@maxme maxme left a comment

Choose a reason for hiding this comment

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

The overall solution (making 3 network calls instead of one) looks fine to me.

Even though I wasn't able to test this as much as I wanted, the order in which the updates occur (username -> display name -> password) were thought of with this in mind

That sounds right. I did some manual tests with the breakpoint feature of Charles proxy and aborted some requests to check what would happen. We get the revert/retry dialog when the display name or username network call fails, which is what I was expecting:

Screenshot 2019-12-04 at 10 34 53

While working on this PR, I found another existing bug where the username, display name, and password fields don't seem to retain their values after configuration changes (screen rotation, for example). In an attempt to maintain the focus of this PR, I haven't looked into this problem any further. Let me know if I should open a specific issue for it.

Yes please fill a new issue for it, you don't have to fix it here.

@maxme maxme self-requested a review December 6, 2019 15:26
Copy link
Contributor

@maxme maxme 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 to me :shipit: - thanks for taking care of that issue @renanferrari

@maxme maxme merged commit 4beb477 into wordpress-mobile:develop Dec 6, 2019
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.

Can't change display name during signup

3 participants