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

[Account Settings] Add Ability to Change Username #12234

Merged
merged 57 commits into from Aug 9, 2019

Conversation

@danielebogo
Copy link
Contributor

commented Jul 30, 2019

Refs. #9705

username-change-2

This PR adds the ability to change the username. The feature is based on the Flux + View Model pattern. The view controller is a subclass of SignupUsernameTableViewController in order to keep the same UI structure.

To test:

  • Select the Tab Me -> Account Settings
  • Tap on the username to open the new screen and change the username
  • If the user is an A8C user you shouldn't be able allowed to change the username

Update release notes:

  • If there are user facing changes, I have added an item to RELEASE-NOTES.txt.
@frosty
Copy link
Contributor

left a comment

I like the new suggestion approach! It seems to be working well. I also like using the 'type username to confirm' system. I left a few code comments, and I have a few other things I spotted:

  • Should we disable the Save button until a different (new) username is selected?

Screenshot 2019-08-08 at 11 18 20

  • When you've confirmed the change, it's still possible to select another username and tap Save again while the HUD is displayed.
enum Alert {
static let loading = NSLocalizedString("Loading usernames", comment: "Shown while the app waits for the username suggestions web service to return during the site creation process.")
static let title = NSLocalizedString("Careful!", comment: "Alert title.")
static let message = NSLocalizedString("You are changing your Username to %@. Changing your username will also affect your Gravatar profile and IntenseDebate profile addresses. \nConfirm your new Username to continue.", comment: "Alert message.")

This comment has been minimized.

Copy link
@frosty

frosty Aug 8, 2019

Contributor

Nitpick, but a couple of occurrences of 'username' in here have a capital U. I think they should all be lowercase?

import WordPressFlux

class ChangeUsernameViewModel {
typealias VoidListener = () -> Void

This comment has been minimized.

Copy link
@frosty

frosty Aug 8, 2019

Contributor

Perhaps just Listener?

@@ -18,6 +18,7 @@ protocol AccountSettingsRemoteInterface {
func getSettings(success: @escaping (AccountSettings) -> Void, failure: @escaping (Error) -> Void)
func updateSetting(_ change: AccountSettingsChange, success: @escaping () -> Void, failure: @escaping (Error) -> Void)
func changeUsername(to username: String, success: @escaping () -> Void, failure: @escaping () -> Void)
func validateUsername(to username: String, success: @escaping () -> Void, failure: @escaping (Error) -> Void)

This comment has been minimized.

Copy link
@frosty

frosty Aug 8, 2019

Contributor

Are we still using the validate methods now we changed to the suggestion model? If not, should we remove them?

@frosty

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

Should we disable the Save button until a different (new) username is selected?

I can't recreate this at the moment, so I'm not quite sure how I got into that state 🤔

One other thing I forgot to mention: on the confirmation alert, if you type the new username incorrectly we just dismiss the alert and fail silently. We should probably let the user know that their username wasn't changed.

@danielebogo danielebogo requested a review from frosty Aug 8, 2019

@danielebogo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

Hey @frosty thanks for your review! I pushed some change and it's ready for another round!
Here you can find some screenshot about the alert with the button disabled/enabled:

Simulator Screen Shot - iPhone X - 2019-08-08 at 15 47 02

Simulator Screen Shot - iPhone X - 2019-08-08 at 15 47 06

@frosty
Copy link
Contributor

left a comment

Changes look great 👍 I posted one more tiny code comment, and I have one other request:

  • When we pop the user back to the Account Settings screen, could we show a "Username changed to NEW_USERNAME" notice to confirm the action?
return
}
self?.changeUsernameAction?.isEnabled = false
textField.textColor = .black

This comment has been minimized.

Copy link
@frosty

frosty Aug 9, 2019

Contributor

We should maybe use .text here instead of .black?

@danielebogo danielebogo requested a review from frosty Aug 9, 2019

@danielebogo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

Thanks @frosty ! It's ready again 😊

@frosty
frosty approved these changes Aug 9, 2019
Copy link
Contributor

left a comment

Looks great! 😎 Nice job!

@danielebogo danielebogo merged commit 8a05ef1 into develop Aug 9, 2019

3 checks passed

Hound No violations found. Woof!
Peril Found some issues. Don't worry, everything is fixable.
Details
ci/circleci: build_and_test Your tests passed on CircleCI!
Details

@danielebogo danielebogo deleted the issues/9705-change-username branch Aug 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.