-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Allow user to change display name and password at the same time during signup #10892
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
Allow user to change display name and password at the same time during signup #10892
Conversation
… of account fields.
…fter the first update.
There was a problem hiding this 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:
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.
WordPress/src/main/java/org/wordpress/android/ui/accounts/signup/SignupEpilogueFragment.java
Outdated
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/ui/accounts/signup/SignupEpilogueFragment.java
Outdated
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/ui/accounts/signup/SignupEpilogueFragment.java
Outdated
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/ui/accounts/signup/SignupEpilogueFragment.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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 - thanks for taking care of that issue @renanferrari
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 theupdateAccountOrContinue()
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
Notes
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.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.