Wp-calypso: Signup: Logging in mid flow

[scruffian]

@ranh noticed that when a user logs in during the signup flow they are redirected to the start of the flow again. When this feature was originally implemented the log in flow would redirect the user back the step they were at before rather than getting them to go through it all again.

We should try to fix this feature so that it behaves as it used to.

Sidenote: it might be worth considering changing the implementation as suggested here: https://github.com/Automattic/wp-calypso/issues/2963

[lsinger]

@scruffian can you elaborate a bit, as in, what are the steps to reproduce? I don't quite understand how a user would log in during signup, since I'd assume they don't have a user account yet. Or would it be someone with an existing account, logged out of it, then starting to create a new account, and during that logging in again with the old account?

[scruffian]

@lsinger If you enter an email address that already has an account on the user step, you see this:

The log in link takes you to https://wordpress.com/wp-login.php?redirect_to=https://wordpress.com/start/plans&email_address=xxx%40xxx.com, which should redirect you back to where you were in signup.

Does that make sense now? If not I can make a video!

[lsinger]

Ah, great @scruffian — that helps a lot. I was missing the "If you enter an email address that already has an account" bit. Thanks!

[Tug]

I was able to investigate this bug for a bit and I think a fix might be a bit more work than anticipated. Here is why:

The main reason the signup framework restarts the flow at the beginning is because it is designed to do so as we can see here https://github.com/Automattic/wp-calypso/blob/master/client/lib/signup/flow-controller.js#L61.

Simply removing that condition won't make it work magically since some flow are rewritten when the user is logged in (For instance https://github.com/Automattic/wp-calypso/blob/master/client/signup/config/flows.js#L333)
So we need to change the condition which detects the end of the flow (and assume that all previous steps are valid). The idea would be to simply check if the current step name is the last in the flow https://github.com/Automattic/wp-calypso/blob/master/client/signup/main.jsx#L383.

However it's not enough. While the progress store can be reuse to resume progress, the dependency store cannot since it was moved to the redux store and the latter is persisted using the user id as its key.
We would need to find a solution to persist that data from a logged out session. We could go back to a localStorage based solution for the dependency store or modify the way we persist or local store to allow some data to be available globally.

[bisko]

Here are some thoughts from my experience with the framework

The main reason the signup framework restarts the flow at the beginning is because it is designed to do so as we can see here https://github.com/Automattic/wp-calypso/blob/master/client/lib/signup/flow-controller.js#L61.

This can be improved and should be improved. While we worked on this, the main idea was to completely hide the login screen, but it seems we missed the case @scruffian reported. In these cases it shouldn't reset the store at all.

Simply removing that condition won't make it work magically since some flow are rewritten when the user is logged in (For instance https://github.com/Automattic/wp-calypso/blob/master/client/signup/config/flows.js#L333)

So we need to change the condition which detects the end of the flow (and assume that all previous steps are valid). The idea would be to simply check if the current step name is the last in the flow https://github.com/Automattic/wp-calypso/blob/master/client/signup/main.jsx#L383.

These two require a bit of rethinking how Signup works as a whole. The state should be a lot more consistent between user sessions, i.e. if the user logs in, it doesn't mean they should see a completely new signup flow, just because they're logged in instead of not logged in.

Having a good extensibility of the framework and hard checks like that is not the best way. I think it would be awesome if we had mechanisms for each step to "report" if it was submitted or not, instead of relying on the dependency store as a whole.

The whole idea behind moving SignupDependencyStore to Redux was this - so each step will have its own state saved in a centralized place and there won't be a need to have an extra store to store that data.

Having more logic stored inside the steps themselves by following a pattern that exposes methods to do all the validation/checks would be a good step forward. This way we can reduce the overhead inside FlowController and ProgressStore and reduce the complexity of the code. Added bonus is that the code for each step will be kept up in a single place with just only a thin layer on top of that to wrangle the flow.

However it's not enough. While the progress store can be reuse to resume progress, the dependency store cannot since it was moved to the redux store and the latter is persisted using the user id as its key.

I don't remember the redux store being saved with the user ID as its key. Can you point me to that? Possibly something changed since we last worked with the code. IIRC we had to explicitly reset the dependency store at the end of signup, because it persisted after that and the signup was filled with leftover data if the user tried to create a new site in the same session.

We would need to find a solution to persist that data from a logged out session. We could go back to a localStorage based solution for the dependency store or modify the way we persist or local store to allow some data to be available globally.

I'd vote with two hands against reverting back to localStorage. We specifically wanted to move away from localStorage and flux as dependencies of the Signup code. It would be better to find a way to make that with the persisted Redux state. This would, of course, require some refactoring of the current code, but this is going to be a good thing in the end as it will migrate more of the signup code to the latest coding standards! :)

[Tug]

I don't remember the redux store being saved with the user ID as its key. Can you point me to that?

It seems to have been added recently: https://github.com/Automattic/wp-calypso/blob/master/client/state/initial-state.js#L92

IIRC we had to explicitly reset the dependency store at the end of signup, because it persisted after that and the signup was filled with leftover data if the user tried to create a new site in the same session.

That's a valid concern. When should we reset the dependency store? My first idea would be if we end the flow OR if we switch to a new flow OR if the data is too old.

I'd vote with two hands against reverting back to localStorage.

I'm not a fan either but that's the simplest solution, making it work with the current redux store is going to be much more work imo.

I think it would be awesome if we had mechanisms for each step to "report" if it was submitted or not, instead of relying on the dependency store as a whole.

The progress store actually has this information already, we could always load the flow from it but I'm afraid it's going to be prone to breakage.

These two require a bit of rethinking how Signup works as a whole.

In any case I agree with this ;)

[alisterscott]

Confirming this is still an issue as of 6 March 2018

Steps

1. As a non-logged-in customer visit wordpress.com/start
2. Proceed to the account screen
3. Enter email of existing account
4. Use the log in now link
5. Customer is redirected back to the start of the sign up flow