Signup: Properly suggest non-taken usernames when signing up - Redux #6596

Merged
merged 45 commits into from Jan 6, 2017

Projects

None yet

8 participants

@bisko
Contributor
bisko commented Jul 7, 2016 edited

This is a second try on Automattic/wp-calypso#5234 after a broken rebase.


Original PR message:

This PR tries to improve the problem described in Automattic/wp-calypso#3388

Testing instructions can be found below.

While trying to fix the issue, I stumbled on a problem that I am not sure how to solve:

In order to do the validity check, we need to ping the API to see if the user is available and to eventually get suggestions for free usernames. The call to the API can be a bit slow sometimes - 500ms to up to a few seconds, depending on network condition, etc.

I tried to work out a scenario where this wouldn't be a problem for the user and got four ideas:

  • Block the loading of the form and wait for the server to reply with a suggestion or with an OK message.
    • This would be very slow, compared to now, where it is instantaneous (there are no calls to the API made)
  • Do the call to the API when the form is already loaded and a suggestion is filled in the field.
    • The shortcoming here is that the user will see one name that will possibly change to something else after a bit of time (to make the API call) or will see an error if none of the suggestions were valid.
  • Do the call when the form is loaded and have a little loader on the field. The field will stay disabled/loading until the call is completed and it will get filled after that.
    • I am not sure if we have such a control (field with an inline loader)
    • Disabling the field for the user would not be very optimal
  • Get the suggestions when choosing the domain name. As the suggested username is based on the domain it would be logical to start the process to fetch the suggested names there.
    • This will allow for the call to finish before rendering the form.
    • Creates a dependency on another step. If the step is not done (for example if we decide to reorder the steps and choose a domain after that, or if we remove the domain choice entirely), the call will never be made. It is possible to verify if we already have the data and if not, to fetch it, but this is the same as solving the options above.

Currently I have implemented the second option above, to prove my code works.

Testing instructions:

  • Apply PR code
  • Apply D1691-code to your sandbox
  • Start signup
  • Fill up domain kwight (as in the issue)
  • Choose a domain, e.g. kwight.me
  • Choose no GAPPs integration
  • Wait a moment to get a username suggestion.

cc @michaeldcain @coreh @meremagee

Test live: https://calypso.live/?branch=fix/3388-signup-suggest-free-username

@bisko bisko self-assigned this Jul 7, 2016
@bisko
Contributor
bisko commented Jul 7, 2016

Rebased on #6124 to be able to use the Redux code for SignupDependencyStore and remove the reliance on localStorage.

This PR should only be merged after #6124 gets merged.

@mtias mtias and 1 other commented on an outdated diff Jul 7, 2016
client/components/signup-form/index.jsx
- const domainSteps = steps.filter( step => step.match( /^domain/ ) );
- let domainName = getValueFromProgressStore( {
- stepName: domainSteps[0] || null,
- fieldName: 'siteUrl',
- signupProgressStore: this.props.signupProgressStore
- } );
- const siteName = getValueFromProgressStore( {
- stepName: 'site',
- fieldName: 'site',
- signupProgressStore: this.props.signupProgressStore
- } );
- if ( domainName ) {
- domainName = domainName.split( '.' )[ 0 ];
- }
+ /**
+ * Fetch the suggested username from local storage
@mtias
mtias Jul 7, 2016 Member

Comment should be using //

@bisko
bisko Jul 8, 2016 Contributor

Updated.

@mtias mtias and 1 other commented on an outdated diff Jul 7, 2016
client/components/signup-form/index.jsx
@@ -10,6 +10,7 @@ import keys from 'lodash/keys';
import debugModule from 'debug';
import classNames from 'classnames';
import i18n from 'i18n-calypso';
+import store from 'store';
@mtias
mtias Jul 7, 2016 Member

Can we avoid localStorage if you are moving to redux?

@bisko
bisko Jul 8, 2016 Contributor

The rebase was just to rebase the code on #6124 and didn't include the new code. The need for localStorage has been removed in 9cdb59e, where suggestedUsername has been moved as a reducer in Redux.

@bisko bisko added this to the Signup: Reduxify Signup milestone Jul 12, 2016
bisko and others added some commits Jun 17, 2016
@bisko bisko Migrate `SignupDependencyStore` to redux. ddfb037
@bisko bisko Load Redux store from `context` in the Signup Component and remove pa…
…ssing the store as a property to the component.
9a0c3a1
@bisko bisko Properly utilize `createReducer` to implement the reducers. Little co…
…de improvements.
60f9592
@bisko bisko Migrate `require` calls to `import` e3e81ab
@bisko bisko Improve class property name to be more descriptive of what it does. 4854129
@bisko bisko Remove unnecessary code comment. 67cfccf
@bisko bisko Set a default state for the `SignupDependencyStore` store to be an em…
…pty object.
86ccfb7
@bisko bisko Add trailing commas to the reducers list and the reducer actions list…
… to provide cleaner diffs in the future if new reducers/actions get added.
668aa0c
@bisko bisko Fix unit tests to properly work after the introduction of the Redux s…
…tore to `SignupDependencyStore`
34f38e6
@bisko bisko Remove previous `rebase` leftovers. 65df132
@bisko bisko Add a README file for more detailed information on what the idea behi…
…nd the `signup` Redux state is.
a8cb3ff
@bisko bisko Improve import of `lodash` functionality to reflect latest changes. 66dc4bc
@bisko bisko Add selectors to the SignupDependencyStore b11d770
@bisko bisko Rename `reducers.js` to follow the proper naming scheme (not plural) d655c40
@bisko bisko Add unit tests for the reducers and selectors. 329caa0
@bisko bisko Check if the signup dependencies actually changed in the Redux state …
…before updating the component state.

This will save re-rendering of the component if the global state gets updated, but not the `signup.dependencyStore` subtree.
356ad33
@bisko bisko Pass `signupDependencies` as a property to the component.
`signupDependencies` are now passed as a property through the `connect` method, which connects the Redux state to the component.

This is a better way to make sure no additional re-renders are happening if the global state tree updates, but not the `signupDependencies` subtree.
6ef6fd2
@bisko bisko Add schema so the `dependencyStore` state can be persisted. c5a32f0
@bisko bisko Update reducer name to exclude redundant part of its name. f8bcea2
@bisko bisko Improve the way `SignupDependencyStore` is selected from the state.
Using `lodash/get` improves the readability of the code and saves a few long `if`s.
8f1cbc4
@bisko bisko Update Redux state schema to reflect the data saved in `SignupDepende…
…ncyStore` in a more descriptive way.
a75b2de
@bisko bisko [WIP] Query the API to get a valid username suggestion.
When a username is generated from the domain name, sometimes that username is already taken. A check must be done with the API to guarantee that the username is available.
1ced696
@bisko bisko [WIP] Move code around
Moved the `getUsernameSuggestion()` method to `step-actions.js` and converted the return value to a callback.
a009945
@bisko bisko [WIP] Fix linter errors 57a4e85
@bisko bisko [WIP] Start the username suggestion fetch right after the domain step.
Moved the username suggestion to start just after the domain step. This gives the process time to complete before finishing the other steps and have a suggestion ready on the user signup form.
fd450a2
@bisko bisko Remove TODO comments from code and enable disabled code d934859
@bisko bisko fix linter errors 8fd6e89
@bisko bisko Improve code formatting. c1467e2
@bisko bisko Suggested username is now saved in the Redux state as an optional dep…
…endency.
632eb7c
@bisko bisko Update README to include more detailed information about the newly in…
…troduced reducers.
4f5c8c1
@ryelle @bisko ryelle Jetpack Plans: Add a helper function to make sure we only send one tr…
…acks event when finished with plugins install
d6a692f
@ockham @bisko ockham Theme Details State: Update active field on THEME_ACTIVATED (#6703)
* Theme Sheet: Use isLoaded() helper

* Theme Details State: Update active field on THEME_ACTIVATED

* Theme Sheet: Drop obsolete isActive() method

* Theme Details Reducer: Cover THEME_ACTIVATED action with test

* Themes Reducer: Cover THEME_ACTIVATED action with test
b500ef2
@bisko bisko [WIP] Query the API to get a valid username suggestion.
When a username is generated from the domain name, sometimes that username is already taken. A check must be done with the API to guarantee that the username is available.
ffabcea
@bisko bisko [WIP] Move code around
Moved the `getUsernameSuggestion()` method to `step-actions.js` and converted the return value to a callback.
3f18db4
@bisko bisko [WIP] Start the username suggestion fetch right after the domain step.
Moved the username suggestion to start just after the domain step. This gives the process time to complete before finishing the other steps and have a suggestion ready on the user signup form.
5518b83
@bisko bisko Suggested username is now saved in the Redux state as an optional dep…
…endency.
ef5a32a
@bisko bisko merge from master 01f6674
@bisko bisko Fix file naming to respect the best practices 9d24fc0
@bisko bisko Remove unnecessary imports. e04b698
@bisko bisko Remove white space diffs 98b62b6
@lancewillett
Member

This came up in a search for older, stale pull requests. Still in progress, or is it abandoned now?

@bisko
Contributor
bisko commented Oct 11, 2016

This came up in a search for older, stale pull requests. Still in progress, or is it abandoned now?

It still in progress/reviewing. We will try to push it down the line as soon as possible.

@coreh

I followed the steps, and it is indeed working.

Left some minor comments on code style, but other than that, LGTM

client/lib/signup/step-actions.js
+ * Default the suggested username to `username` because if the validation succeeds would mean
+ * that the username is free
+ */
+ let resulting_username = username;
@coreh
coreh Dec 9, 2016 Member

resultingUsername?

@bisko
bisko Dec 14, 2016 Contributor

Yes, this is better. Updated.

client/lib/signup/step-actions.js
+ * - a valid suggested username
+ */
+ if ( messages.username && messages.username.taken && messages.suggested_username ) {
+ resulting_username = messages.suggested_username.suggested_username;
@coreh
coreh Dec 9, 2016 Member

What's up with this extra level of indirection here? Why messages.suggested_username.suggested_username?

@bisko
bisko Dec 14, 2016 Contributor

It's because of the way WP_Error handles adding additional data parameters to errors. I updated the API to return a not so redundant suggested_username.data instead.

client/state/action-types.js
@@ -422,6 +422,7 @@ export const SHORTCODE_REQUEST_FAILURE = 'SHORTCODE_REQUEST_FAILURE';
export const SHORTCODE_REQUEST_SUCCESS = 'SHORTCODE_REQUEST_SUCCESS';
export const SIGNUP_COMPLETE_RESET = 'SIGNUP_COMPLETE_RESET';
export const SIGNUP_DEPENDENCY_STORE_UPDATE = 'SIGNUP_DEPENDENCY_STORE_UPDATE';
+export const SIGNUP_OPTIONAL_DEPENDENCY_SUGGESTED_USERNAME = 'SIGNUP_OPTIONAL_DEPENDENCY_SUGGESTED_USERNAME';
@coreh
coreh Dec 9, 2016 Member

SIGNUP_OPTIONAL_DEPENDENCY_SUGGESTED_USERNAME_SET?

@bisko
bisko Dec 14, 2016 Contributor

Good catch! :) Updated.

@coreh
Member
coreh commented Dec 9, 2016

Tests seem to have failed due to a broken node 6.9 download, we should probably trigger a re-run of them.

@bisko
Contributor
bisko commented Dec 14, 2016

Tests seem to have failed due to a broken node 6.9 download, we should probably trigger a re-run of them.

You can trigger the rebuild in Circle CI's interface.

Tests just needed to rebuild.

@michaeldcain

Tested, LGTM. :shipit:

@bisko bisko merged commit eaa6ea0 into master Jan 6, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
@bisko bisko deleted the fix/3388-signup-suggest-free-username branch Jan 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment