search

Automattic / wp-calypso

The JavaScript and API powered WordPress.com
https://developer.wordpress.com
GNU General Public License v2.0
12.42k stars 1.99k forks source link

Stepper: `calypso_signup_complete` event not firing in es onboarding flow #95047

Open skim1220 opened 2 weeks ago

skim1220 commented 2 weeks ago

Steps taken:

https://github.com/user-attachments/assets/f79a84ff-e9c9-4a7c-bc56-2ca84e90aa66

[Update on 2024-10-03]

I reported this issue on 2024-10-01. However, during my audit yesterday, 2024-10-02, this issue no longer occurred. calypso_signup_complete event is firing correctly when @ gmail address is used.

chriskmnds commented 2 weeks ago

Related discussion with findings in Slack: p1727704611670019-slack-C073776NJ66

I confirmed this only happens for ...@autom... addresses, which force a redirect to LOHP. Otherwise, the event gets recorded correctly after the processing step / before checkout. I confirmed with .blog domain and Personal plan, per instructions.

chriskmnds commented 2 weeks ago

This feels worth looking into, irrespective if being due to what may be an exception (the use of @auto... address). We had some discussion about calypso_signup_complete event being controlled by a particular step (processing step) here: https://github.com/Automattic/wp-calypso/pull/94706#discussion_r1768550003 This would be served better IMO if it were nearer to the framework (where we handle calypso_signup_start). Potentially, this wouldn't be an issue if a redirection happens before, but there is likely a lot more to consider.

chriskmnds commented 2 weeks ago

This feels worth looking into, irrespective if being due to what may be an exception (the use of @auto... address). We had some discussion about calypso_signup_complete event being controlled by a particular step (processing step) here: #94706 (comment) This would be served better IMO if it were nearer to the framework (where we handle calypso_signup_start). Potentially, this wouldn't be an issue if a redirection happens before, but there is likely a lot more to consider.

I did some digging around to understand this a bit more. I haven't gotten too deep though. The processing step is indeed the main control unit for running any "pending action" (singular, from the looks of it) before recording the signup-complete event.

I think that's all good since it works. There is some arbitrariness in how any step or flow (basically anything and from anywhere) can set a "pending action" into the data store, and that immediately becomes a criterion for the signup-complete event once in the processing step. My other concerns are (1) the singular form of this i.e. only the last set "pending action" is resolved by the processing step before recording the event, and (2) nothing clears the "pending action" array once called in the processing step. I sense something's a little shady here, but I cannot yet pinpoint an issue.

What may be worth looking into further:

  1. Decouple the logic from the processing step, make the step strictly presentational
  2. Enrich the pending actions with a "is signup complete candidate" prop - meaning, not every pending action should immediately become the condition for recording the signup complete event. Assuming these pending actions have any other value in the stack.
  3. Allow multiple pending actions and clear the array once depleted (this isn't the case currently, but not sure what if any ramifications)
  4. There could be more. Needs more investigation. I sense this is an area that needs clarity - if anything from the arbitrariness alone.

cc @escapemanuele @MicBosi

escapemanuele commented 2 weeks ago

Thank you for you analysis @chriskmnds.

I can see how the structure of the processing-step may be prone to shadiness and breakage.

  1. Cool, I'm totally ok with decoupling the logic from the processing screen. Would you move the "action" login to the framework level?
  2. Would this be useful only if point 3 is developed? Right now we record the signup complete only if the flow is a signup one, thus bringing to the creation of a site (is this correct? I should check). With your added prop this would bring extra safety, you're right.
  3. I remember I worked on something similar, to allow multiple pending actions, in the past, but I did not proceed since that added complexity was not required. It should be not too hard to implement, but do you think it would be an useful improvement, since a lot of flows are working fine without it? I think I delayed working on this until it was needed. Totally ok with a new removePendingAction or something similar, to be run aftter the action is called.
chriskmnds commented 2 weeks ago

Thank you for you analysis @chriskmnds.

I can see how the structure of the processing-step may be prone to shadiness and breakage.

  1. Cool, I'm totally ok with removing the decoupling the logic from the processing screen. Would you move the "action" login to the framework level?
  2. Would this be useful only if point 3 is developed? Right now we record the signup complete only if the flow is a signup one, thus bringing to the creation of a site (is this correct? I should check). With your added prop this would bring extra safety, you're right.
  3. I remember I worked on something similar, to allow multiple pending actions, in the past, but I did not proceed since that added complexity was not required. It should be not too hard to implement, but do you think it would be an useful improvement, since a lot of flows are working fine without it? I think I delayed working on this until it was needed. Totally ok with a new removePendingAction or something similar, to be run aftter the action is called.

@escapemanuele you can either create a new issue for this or update this one. Just anything to be logged and be available for grabs :-)

escapemanuele commented 1 week ago

@chriskmnds created these: