Problem/Motivation

The 'Login' checkout pane mentions the following when guest checkout is enabled:

Proceed to checkout. You can optionally create an account at the end.

While it says that an account can be created at the end, the registration pane in question isn't implemented yet.

Proposed resolution

Create a registration checkout pane, meant to be displayed after guest checkout completion.

Remaining tasks

  • The text 'You can optionally create an account at the end.' in the login checkout pane should only be displayed when the registration pane is enabled.
  • Implement an event to dispatch, so other modules can act on an account created through commerce.
  • Decide if user register settings should be followed, for example if email validation is required, the passwords fields shouldn't be shown. follow-up
  • Decide if fields attached to the user should be displayed on the form. follow-up
  • Decide whether the user should be redirected to their account page and/or make the redirect path configurable on the checkout pane.
    Skip for now.
  • Test coverage for the following?
    • Adding the billing profile to the user's address book.
    • The event being dispatched.

Original report by bmcclure

I'm going to submit a patch / PR to add a new checkout pane for registration after guest checkout completion, with text controlled via twig template per my conversation with bojanz in #commerce on Slack today.

CommentFileSizeAuthor
#125 2857157-125.patch35.27 KBbojanz
#124 2857157-dev.patch41.11 KBFiNeX
#115 2857157-115.patch40.58 KBmglaman
#112 2857157-112.patch40.42 KBmglaman
#111 2857157-111.patch41.13 KBmglaman
#108 2857157-108.patch35.1 KBmglaman
#108 interdiff-2857157-108-105.txt6.83 KBmglaman
#105 2857157-105.patch34.65 KBmglaman
#99 commerce-checkout-pane-guest-registration-2857157-99.patch34.83 KBjohnpicozzi
#94 interdiff-2857157-91-94.txt3.94 KBAerzas
#94 commerce-checkout-pane-guest-registration-2857157-94.patch34.71 KBAerzas
#91 interdiff-2857157-88-91.txt5.7 KBSut3kh
#91 commerce-checkout-pane-guest-registration-2857157-91.patch33.75 KBSut3kh
#88 interdiff-2857157-86-88.txt842 bytesMegaChriz
#88 commerce-checkout-pane-guest-registration-2857157-88.patch30.95 KBMegaChriz
#86 interdiff-2857157-84-86.txt2.26 KBMegaChriz
#86 commerce-checkout-pane-guest-registration-2857157-86.patch30.73 KBMegaChriz
#84 interdiff-2857157-81-84.txt4.92 KBMegaChriz
#84 commerce-checkout-pane-guest-registration-2857157-84.patch30.56 KBMegaChriz
#81 interdiff-2858157-80-81.txt2.01 KBMegaChriz
#81 commerce-checkout-pane-guest-registration-2858157-81.patch30.58 KBMegaChriz
#80 interdiff-2858157-74-80.txt36.06 KBMegaChriz
#80 commerce-checkout-pane-guest-registration-2858157-80.patch30.53 KBMegaChriz
#74 2857157-74.patch23.45 KBheddn
#74 interdiff_73-74.txt1.29 KBheddn
#73 2857157-73.patch22.45 KBheddn
#73 interdiff_71-73.txt847 bytesheddn
#71 interdiff_69-71.txt3.66 KBheddn
#71 2856583-71.patch22.46 KBheddn
#69 interdiff_65-69.txt850 bytesheddn
#69 2857157-69.patch22.43 KBheddn
#66 2857157-65-test-5.png254.36 KBChandeepKhosa
#66 2857157-65-test-4.png306.75 KBChandeepKhosa
#66 2857157-65-test-3.png318.49 KBChandeepKhosa
#66 2857157-65-test-2.png90.41 KBChandeepKhosa
#66 2857157-65-test-1.png162.37 KBChandeepKhosa
#65 interdiff_60-65.txt5.23 KBheddn
#65 2857157-65.patch22.34 KBheddn
#60 interdiff-2858157-57-60.txt7.3 KBMegaChriz
#60 commerce-checkout-pane-guest-registration-2858157-60.patch22.41 KBMegaChriz
#57 interdiff-2858157-55-57.txt2.79 KBMegaChriz
#57 commerce-checkout-pane-guest-registration-2858157-57.patch18.09 KBMegaChriz
#55 commerce-checkout-pane-guest-registration-2858157-55.patch18.61 KBdrugan
#55 interdiff-53-55.txt2.25 KBdrugan
#55 commerce-checkout-pane-guest-registration-2858157-55.patch18.61 KBdrugan
#53 interdiff-2858157-38-53.txt6.47 KBMegaChriz
#53 commerce-checkout-pane-guest-registration-2858157-53.patch16.8 KBMegaChriz
#42 interdiff-2857157-39-41.txt4.49 KBdrugan
#42 commerce-checkout-pane-guest-registration-2858157-41.patch26.4 KBdrugan
#40 interdiff-2857157-39-40.txt3.28 KBdrugan
#40 commerce-checkout-pane-guest-registration-2858157-40.patch26.21 KBdrugan
#39 interdiff-2858157-38-39.txt12.08 KBMegaChriz
#39 commerce-checkout-pane-guest-registration-2858157-39.patch23.51 KBMegaChriz
#38 interdiff-2858157-34-38.txt10.27 KBMegaChriz
#38 commerce-checkout-pane-guest-registration-2858157-38.patch15.84 KBMegaChriz
#34 commerce-checkout_pane_guest_registration-2857157-33.patch9.1 KBdrugan
#30 interdiff-commerce-checkout_pane_guest_registration-2857157-25-30.txt2.69 KBSophie.SK
#30 commerce-checkout_pane_guest_registration-2857157-30.patch14.52 KBSophie.SK
#25 commerce-checkout_pane_guest_registration-2857157-25.patch14.33 KBBram Linssen
#24 commerce-checkout_pane_guest_registration-2857157-22.patch14.16 KBBram Linssen
#21 commerce-checkout_pane_guest_registration-2857157-21.patch8.74 KBSophie.SK
#20 interdiff-commerce-checkout_pane_guest_registration-2857157-19.txt3.44 KBSophie.SK
#20 commerce-checkout_pane_guest_registration-2857157-19.patch8.84 KBSophie.SK
#19 interdiff-commerce-checkout_pane_guest_registration-2857157-18.txt218 bytesSophie.SK
#19 commerce-checkout_pane_guest_registration-2857157-18.patch8.85 KBSophie.SK
#18 screenshot-after-registration.png8.9 KBjosephdpurcell
#18 screenshot-before-registration.png35.62 KBjosephdpurcell
#2 commerce-checkout_pane_guest_registration-2857157-2.patch8.83 KBbmcclure
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bmcclure created an issue. See original summary.

bmcclure’s picture

Status: Active » Needs review
FileSize
8.83 KB

PR submitted: https://github.com/drupalcommerce/commerce/pull/657

Attaching patch here as well. Let me know here or in the PR if there is any feedback or changes requested. Thanks!

mglaman’s picture

There's a contrib in the 1.x that does this. We should find that and make sure we mark it as deprecated. I know I used it for a site to do this.

Lukas von Blarer’s picture

The patch works great! Thank you!

vasike’s picture

Created "Drupal 8 Contrib Porting Tracker" related issue: #2857620: [commerce_checkout_complete_registration] Commerce Checkout Complete Registration

I also updated the project page with info about the issue
https://www.drupal.org/project/commerce_checkout_complete_registration

mglaman’s picture

Thanks vasike, for linking!

vasike’s picture

First thing it seems it works.

However, let me share some issues and thoughts about current PR

1. I was able to create new account with an existing email.
On the old module, there was an issue about this
Commit for this : http://cgit.drupalcode.org/commerce_checkout_complete_registration/commi...
Do not show for existing email address.
Maybe this could be improved with some other option as login (for the same address) or create new account where the customer should change the email address.

2. It doesn't care about account settings (Should it???) like:
Require email verification when a visitor creates an account
New account email

3. Registration pane configuration:
There could be some configuration.
For example the old module has:

Set the password?
Notify user about account creation?
Register confirmation text. (with tokens/replacement)

4. Create some tests

So i think we're on "Needs work" :(

bmcclure’s picture

One thing I've noticed in testing the patch so far is that it's a little bit ambiguous that everything worked properly after you submit the form, other than the fact that you're logged in when the page reloads. Should a success message of some kind be added, or should that be something else's responsibility?

bmcclure’s picture

Also great comments on the PR vasike, thanks for reviewing it!

1. Could this somehow be handled earlier? Should a guest be able to complete checkout with a registered email address, without that order being associated with the account? Or should login be required if you try to use an existing email? If a guest should be able to checkout with an existing email address, should we offer a login form on the Complete step instead that would link the order with the account?

2. I think it should take those settings into account most likely, you're right. Just those two settings, or all settings? What about other fields people have added to the registration form?

3. Those 3 options seem like great additions to this based on the existing module. And the 3rd one would take care of my previous comment as well.

4. Agreed :)

Regarding #2 and #3, are those taken into account in the current Login pane's registration form? If not, should they be?

bmcclure’s picture

And also thinking about the login pane's registration form, if we're enhancing this one with these suggestions, is there a way we can abstract that and share some of the registration code between the Login and Registration panes, or should they remain completely separate?

mglaman’s picture

Status: Needs review » Needs work

One thing I've noticed in testing the patch so far is that it's a little bit ambiguous that everything worked properly after you submit the form, other than the fact that you're logged in when the page reloads. Should a success message of some kind be added, or should that be something else's responsibility?

Maybe a configurable message and destination. How about "stay on same page" or "redirect to viewing order" as an option for the pane when saved.

1. Could this somehow be handled earlier? Should a guest be able to complete checkout with a registered email address, without that order being associated with the account? Or should login be required if you try to use an existing email?

I believe in 2.x we do not automatically create users or associate orders to users if email exists. So I wonder if the email exists we hide the form and say "oh, looks like your are registered already!" or keep the pane hidden? I'd like to get bojanz's input here. I feel like most people implementing this won't have this kind of issue. Such as "continue as guest" and when email entered check if it exists, etc.

Regarding #2 and #3, are those taken into account in the current Login pane's registration form? If not, should be they be?

We'd need to review, and they probably should.

My concern, too, is that we now have a custom registration form in one pane, and now here. We'll need to see how we can de-duplicate code between the two.

EDIT: bmcclure had same thought :) We need to see how we can dedupe some of this code to make both better.

bmcclure’s picture

I believe in 2.x we do not automatically create users or associate orders to users if email exists. So I wonder if the email exists we hide the form and say "oh, looks like your are registered already!" or keep the pane hidden?

On the one hand, they've already elected not to log in at the beginning of checkout. But perhaps they didn't know they had an account at the time, and currently there isn't any indication that you're a guest checking out with an existing email address earlier in the process (unless perhaps there should be).

That's one possible reason for offering the option of a login on this pane if the email already exists (which would associate the order with the account and log you in). But maybe someone would want to hide the pane instead. I'm not sure what the more common expected approach here would be on other e-commerce sites to be honest.

bmcclure’s picture

Alluuu’s picture

I believe in 2.x we do not automatically create users or associate orders to users if email exists. So I wonder if the email exists we hide the form and say "oh, looks like your are registered already!" or keep the pane hidden? I'd like to get bojanz's input here. I feel like most people implementing this won't have this kind of issue. Such as "continue as guest" and when email entered check if it exists, etc.

I remember that in D7 Commerce Kickstart there was a possibility for when an order was created a user account was also created, so orders with same emails were associated in case the user later registers, he'll see all of his previous orders.

Also after the user completed an order as guest there was an option to send them an email with an account activation link if they wished so. I don't remember if the activation link took them to a password creating page or an automatic temporary password was created and sent.

I really like the UX of this, that there's no hassle registration, even when the user decides for it later on, and all prior orders are kept in one place. Can this not be achieved with Commerce 2.x somehow?

This is something I would possibly need for my project anyway and if pointed in the right direction I'd give creating it a go. However in D7 I believe it was done with rules, but we don't have a working rules module for D8..

itamair’s picture

Tnx for this patch ...

IMO this "Guest registration after checkout" Pane should be extended with settings to define an assigned role to the new register user (besides the authenticated_user one). In our Drupal commerce instance the registered customers should have a specific user Role.

steveoliver’s picture

josephdpurcell’s picture

Lots of good stuff here. I was able to apply this patch to latest commerce 8.x branch and I've attached a screenshot of what the user sees on checkout before and after registration.

I will attempt to summarize the conversation so far. It sounds like there are two scenarios that need addressed:

Separately, there is a question of how to handle association of anonymous orders to a custmer, which #2808813: Create a recipe for assigning anon orders on user registration is handling. That should address comments like #7.

I think this pane solves Scenario 2 well. I think this can be RTBC with the following issues addressed:

  1. Refactor to respect the site's account registration settings, or make the pane configurable with similar settings (this includes settings like whether to send confirmation email link)
  2. Consider consolidating with Login pane's registration (optional, imo)
  3. Either allow configuring what role is assigned on registration or create separate ticket
  4. A flash message indicating they successfully registered, assuming they are auto-logged in (optional, imo)
  5. Tests
josephdpurcell’s picture

The screenshots for comment #17.

Sophie.SK’s picture

While the patch applied cleanly, I had an error when trying to edit the checkout flow - one of the required parameters wasn't passed into CheckoutPaneBase::__construct() in the Registration pane.

Adding updated patch (mostly so I can add it to Composer ;)) and an interdiff.

Haven't applied any of the changes requested above (or considered them in any way) - just wanted to get this working - so leaving as "needs work" for now.

Sophie.SK’s picture

After trying the patch... I realised that there were a few bugs with it. Attached is a new patch:

  • Check to see if a user exists by email address before presenting the pane (not sure if this works - can someone sense-check?)
  • Remove manual validation of required fields in pane
  • Create new account during the submit step, rather than the validate step
  • Solved a few bugs too! Woo!

Attached is new patch and updated interdiff. Still haven't even looked at the list of requests above so not going to change this to "needs review" :)

Sophie.SK’s picture

Ack, wrong file path. (Caused a great deal of confusion.)

The interdiff is the same as before so not uploading a new one, just a different file path and I just realised that all of my patches are named wrong. Yay!

Lukas von Blarer’s picture

The patch works for me for the basic functionality. I discovered a few bugs:

  1. The user's email address is not being set to the one from the "Account information" pane
  2. The payment methods are not saved for the user account
  3. The address book is empty. I guess the profiles are not associated with the user?

Additionally the confirmation message is pretty important as requested in #17.

Currently I am a unable to get a setup where there are three options on the login step: Login, Register and Guest checkout. I have this setup on all my D7 commerce sites and would miss it if it wasn't in core for 2.x if we have all the rest of the login functionality.

Lukas von Blarer’s picture

I am sorry, it seems that I didn't test properly. Right now I have all three options on the login checkout step.

Bram Linssen’s picture

The previous patch didn't properly save the user account. The email was saved as the user name and no email was saved.
I have improved that in this patch.

Bram Linssen’s picture

Let's add the Billingprofile to the user's addressbook as well.

Maikel’s picture

I applied the patch, saw a nice registration form after anonymous checkout, but now when i apply for an account i keep getting a 403.

The user is not present yet, the emailadres isn't either.

Am i doing something wrong with my permissions? Visitors are permitted to register without approval. Tried with and without email verification. Am i missing something?

Sophie.SK’s picture

This works for me, by the way - we've got the patch applied and up on our UAT environment for more rigorous testing currently.

@Maikel - do you mean that you perform the steps as an anonymous user, create a password, and then try to register the account separately afterwards (eg go to /user/register)? If so, the user account gets saved after the username and password are saved. The account is created at this point and the user is logged in.

FOr me, if I try to go to /user/register, I get redirected to /user/edit. I guess if you've disabled "user can edit their own account" they might get a 403 permission denied.

Maikel’s picture

Hmm, that's strange. Can understand the reasoning but it breaks earlier.

What i did was complete checkout. And on the page with the 'Save your details' block enter a username and a password. After i click save i get a 403 and no user is created in the backend.

Sophie.SK’s picture

Hmm, have you definitely got the right version of the patch?

The user login only redirects the user when after the account is created, unless something is preventing your user account from being created correctly. Do you have any hooks acting on the form and/or user creation?

Sophie.SK’s picture

Adding a new patch that preserves some of the parent checkout pane functionality, and corrects a couple of DCS issues.

Seems to still work for me. Except that now I've broken my 3rd party integration. :D Diff also attached.

Sophie.SK’s picture

Status: Needs work » Needs review

Changing status.

AndyD328’s picture

Status: Needs review » Needs work

Works very well with one issue, the patch is missing the final closing brace from
modules/contrib/commerce/modules/checkout/src/Plugin/Commerce/CheckoutPane/Registration.php

and generates

ParseError: syntax error, unexpected end of file, expecting function (T_FUNCTION) or const (T_CONST) in Composer\Autoload\includeFile()
(line 222 of [...]/web/modules/contrib/commerce/modules/checkout/src/Plugin/Commerce/CheckoutPane/Registration.php)

With that fix it looks like it does exactly what it should, many thanks.

---EDIT---

I can see the brace in the patch file but it's not making it into the file system. Not sure what that's about but it's happened every time I've applied it.

Sophie.SK’s picture

Haha yeah @andyd I just pinged you in Slack about that same problem.

It's really odd that I can see it in the patch but I can't see it in the file. It's done the same every time I've applied it too, so I've been reinstating it manually every time I do a composer update. I'm not sure how best to resolve it.

drugan’s picture

Edited and rerolled the patch against the latest commit. Also, removed modules/checkout/commerce_checkout.module.orig from the patch as obviously it is an artefact after some merge conflict resolving.

Also, tested the patch. Works great. Thanks @Sophie.SK.

drugan’s picture

Status: Needs work » Reviewed & tested by the community

I think this change is required by most of the sites. Others can disable the pane if they don't need it.

mglaman’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

There are no tests added for this. We need to have test coverage for this. Also, if this pane is on by default, it should respect the site's global setting of allowing user registration (admin only) in the isVisible check.

  1. +++ b/modules/checkout/src/Plugin/Commerce/CheckoutPane/Registration.php
    @@ -0,0 +1,223 @@
    +    $existing_user = \Drupal::entityManager()->getStorage('user')->loadByProperties(['mail' => $this->order->getEmail()]);
    

    We have an injected entity type manager, let's use it here.

  2. +++ b/modules/checkout/src/Plugin/Commerce/CheckoutPane/Registration.php
    @@ -0,0 +1,223 @@
    +    $account = User::create([
    

    We have an entity type manager injected, let's use it to have it create user object from its storage.

  3. +++ b/modules/checkout/templates/commerce-checkout-registration.html.twig
    @@ -0,0 +1,20 @@
    +    <h2>Save Your Details</h2>
    +    <p>Complete your account registration now to be able to access your order information at any time.</p>
    

    These need to be wrapped in {%trans%}{%endtrans%}

MegaChriz’s picture

Assigned: bmcclure » MegaChriz

I'll look into creating an automated test for this today.

A quick review on the UI:

  • Why does each word in "Save Your Details" start with a capital?
  • Why is the button called "Save details". Wouldn't "Create my account" fit better? On the login step the text says "You can optionally create an account at the end.", so I think that "Create account" or "Create my account" would fit better as that matches with the text on the login step.
  • After creating the account, the following things happen:
    • You stay on the checkout complete page.
    • You get logged in.

    There is no indication that creating the account was successful. I think there should be a message that the account was created and that you are now logged in. I'm also not sure if it makes sense to stay on the checkout complete page. In my case it starts with the same message as before creating the account:

    Your order number is 24.
    You can view your order on your account page when logged in.

  • I'm not sure if this is due to my custom configured Drupal site, but it seems that the password that I chose at the end does not work when I try to login. I will investigate this on a clean install.
MegaChriz’s picture

New patch, with the following changes:

  1. I've added an automated test for creating an account after guest checkout!
  2. Fixed the password issue that I mentioned in #37. Not being able to login after creating an account also happened in the automated test. This was because the password was set on the wrong key when creating the account in Registration::validatePaneForm():
    +++ b/modules/checkout/src/Plugin/Commerce/CheckoutPane/Registration.php
    @@ -0,0 +1,223 @@
    +      'password' => $form_state->getValue(['registration', 'register', 'password']),
    

    The property name should be 'pass' instead of 'password'.

  3. Fixed concerns 1, 2 and 3 from #36.
  4. Bugfix: you could continue without creating a password. Fixed this by making the password field required.
  5. Bugfix: when entering wrong details causing validation errors, a PHP fatal error happened.

I also worked on respecting the user register setting: the registration form should not be shown if visitors are not allowed to create an account. As this resulted into quite a bit more code changes, I'll save these for the next patch.

More coming, hang on...

MegaChriz’s picture

Changes in this patch:

  1. The registration pane is not shown when users are not allowed to register (by checking the account setting 'Who can register accounts?').
  2. The text 'You can optionally create an account at the end.' is not displayed in the login checkout pane when users are not allowed to register.
  3. An automated test is added that assert that the registration pane is not shown when users are not allowed to register.
  4. Both the login checkout pane and registration checkout pane now have a dependency on the configuration factory service.

I plan to change some of the texts and I think that the user should be redirected to their account page after completing checkout. Also, I think that the user shouldn't be logged in if email verification is required (if configured on the account settings page [admin/config/people/accounts]).

drugan’s picture

Added drupal_commerce_customer role allowing customer immediately see their order and profile information on the newly created account.

@MegaChriz

I think that's not a good idea to take away from a customer the right to register and see their own orders they were paid for. Yes, a regular user just browsing commerce site should not be allowed to register an account but it is not the case with a customer. Whatever the setting is for account registration we have to override it. E-commerce functionality of the site is a different domain and should be separated from other activities. You can create a blog on a commerce site (why not) and that blog users should abide the global account registration settings.

MegaChriz’s picture

@drugan
User register setting check
I agree that E-commerce functionality of the site is a different domain. I added the user register setting check because @mglaman said that the checkout pane should follow the user register settings in #36. It was also mentioned by @josephdpurcell in #17. Perhaps it should be configurable on the checkout pane how it should work:

  • Allow customers to set their password vs require mail validation.
  • Include custom user fields.
  • And (but probably out of scope for this issue) an option to auto-generate the username. I saw this feature in WooCommerce.

Role assign
I personally think that adding a role to automatically assign to the guest is a bit out of scope of this issue. @josephdpurcell suggested in #17 that a role should be configurable, but also that it maybe should be addressed in a different issue.

drugan’s picture

Somehow forgotten about uninstalling drupal_commerce_customer role when uninstalling checkout module. Also, some coding standard errors are corrected.

@MegaChriz

Yes, I agree. Adding a new role a bit out of a scope of this issue but let it be just for testing purposes.

I agree that E-commerce functionality of the site is a different domain. I added the user register setting check because @mglaman said that the checkout pane should follow the user register settings in #36.

Again, we are both agree that it is a bad idea to abide global registration settings. May be @mglaman gives us more info why he does not like ignoring these settings and we change our mind too.

mglaman’s picture

+++ b/modules/checkout/commerce_checkout.info.yml
@@ -13,3 +13,4 @@ config_devel:
+    - user.role.drupal_commerce_customer

No. We're not providing roles.

drugan’s picture

@mglaman

Are there any reasons for not providing such a role?

MegaChriz’s picture

@drugan
A reason for not providing such a role in this issue is that it adds additional complexity from which I believe is not needed to solve this issue. An other reason is that this role feature could also be added as something configurable (as suggested in #17). Sites may have a customer like role already that they want to assign instead.

And, more subjective, I'm less in favor of modules adding roles. In my opinion, role management should be totally up to the site builder. I for example, would like to keep the amount of roles needed for a site to a minimum. What also plays a role (pun intended) is that I usually assign the permission to view own orders and manage own profile to the authenticated user already. So in my personal situation the new role is redundant.

drugan’s picture

@MegaChriz

What also plays a role (pun intended) is that I usually assign the permission to view own orders and manage own profile to the authenticated user already. So in my personal situation the new role is redundant.

We don't discuss here a situation of any particular person. The solution should be suitable for most DC users. We should remember the case when a site implements different activities not related with DC directly. So, not all registered users are customers. Yes, you may consider empty Orders tab on an account as a teaser to buy something on the site but what about Address Book tab? The permission to view this tab implies CRUD permissions on the Customer profile. Which we don't need at all for users who are not intended ever never to buy. Though give us a profit creating a noise around the site.

A reason for not providing such a role in this issue is that it adds additional complexity from which I believe is not needed to solve this issue.

Providing a role does not interfere in any part of this solution. It just allows a customer to see their own information after the solution has done its work. Also, I can't see any complexity added.

Sites may have a customer like role already that they want to assign instead.

The question is how to assign this role programmatically right after the first checkout was completed. Note that now more and more DC users are coming in who are site builders, not developers. There is no d7-like rule which could be created from the UI. The same with developers. On each new site they should think how to add an event subscriber or similar solution or just applying an interdiff from the current issue. Why not give them out of the box the Customer role to which they can add their own permissions if they need to. Again, if they need their own a customer-like role then later it could be added to a customer as one of the roles (filtering users by the Customer role).

Adding a default Customer role does not make any harm at all. Instead, it makes people's life a little easier.

mglaman’s picture

drugan, you're bloating the scope of the issue and patch by adding this. Which also means that it is harder to review. Which makes it harder to get in.

The purpose of this patch is to provide a registration conversion at the end of the checkout using a custom pane. If the concern is adding roles, then we should be invoking a specific event (CHECKOUT_USER_REGISTERED)

The only item for discussion should be about respecting the site's current registration settings if the pane is enabled by default. If the pane is default -> disabled, then I do not care.

I will not be reviewing the patch in its current state while it has the role in it. As a maintainer I'm saying this: we will not be adding a role in this patch. I am not sold on providing patches and find it out of scope for the module itself. However, that discussion is for a new issue because it also concerns the normal register option on the login pane.

drugan’s picture

OK, I've removed from display the patch with a role added. The #39 is the last visible which can be reviewed.

mglaman’s picture

drugan, thanks. I'll review #39.

I understand where the points are being made. Let's just address them once this lands. We should probably be firing events and standardizing the areas in which we register users.

MegaChriz’s picture

@mglaman
You can review #39, but I think the following changes should be made:

  • After registering on the checkout complete, I think there should be redirect to the user account page. It doesn't make sense to stay on the checkout complete page, in my opinion.
  • Textual changes as noted in #37.
  • Some small code style fixes that were corrected in #42.

I'm not completely sure yet if the user register settings should be fully followed in this process. @drugan has a point that registering on the site is not completely the same as registering after completing checkout. If we do follow the user register settings, we may also want to consider if we should follow other settings as well, as noted in #41.

mglaman’s picture

After registering on the checkout complete, I think there should be redirect to the user account page. It doesn't make sense to stay on the checkout complete page, in my opinion.

Noted. For now, I'd like to leave that as a follow-up because I'm not too sure. It makes sense but isn't something that should be forced. Falling back, again, to an event being fired so people can interject.

has a point that registering on the site is not completely the same as registering after completing checkout.

My concern here is this: in the Login pane the site admin has to explicitly allow registration. We're now adding a pane which is enabled by default and automatically allows registration. So we either 1) respect site settings 2) do not ship the pane enabled by default.

A community member has already documented this process, here https://docs.drupalcommerce.org/commerce2/user-guide/checkout/allow-regi.... I find disabled first more preferable. But I also have not yet had time to read through all the comments and cases made for default being enabled.

MegaChriz’s picture

@mglaman
I understand your concerns. If the pane is enabled by default, it may not be obvious to stores that don't want registrations on their site that registrations are possible via Commerce. So, having it disabled by default sounds good to me. There's only one thing that bugs me: the Login checkout pane currently says: 'You can optionally create an account at the end.', which will be false if the registration pane is disabled. How can we ensure that this text is only displayed when the registration pane is enabled? Or would it be better to just remove that text?

Anyway, I'm working on updating the interface texts. I will base my changes on the patch from #38 for now. That patch doesn't have the user register check.

@mglaman
Should the event dispatching code be added in this issue, or do you think it would be better to have that in a follow up too?

MegaChriz’s picture

This patch builds upon the patch from #38.

Changes in this patch:

  • The registration pane is disabled be default. Also adjusted the automated test to reflect this change. It was a bit a struggle to find out how to enable a checkout pane programmatically. Is there is an easier way of doing that?
  • Changed interface texts. Header of the registration pane is now "Create an account?" instead of "Save Your Details". The submit button now reads "Create my account" instead of "Save details".
  • Small code style fixes.

Still left to do:

  • The text 'You can optionally create an account at the end.' in the login checkout pane should only be displayed when the registration pane is enabled.
  • Implement an event to dispatch, so other modules can act on an account created through commerce. Decide whether that is needed to do in this issue or in a follow-up.
  • Decide if user register settings should be followed, for example if email validation is required, the passwords fields shouldn't be shown.
  • Decide if fields attached to the user should be displayed on the form.
  • Decide whether the user should be redirected to their account page and/or make the redirect path configurable on the checkout pane.

Updated this also in the issue summary.

mglaman’s picture

Should the event dispatching code be added in this issue, or do you think it would be better to have that in a follow up too?

Let's go ahead and add it in here. We'll need one for the Login pane but that can be a follow-up. We're here now.

Decide if user register settings should be followed, for example if email validation is required, the passwords fields shouldn't be shown.

I don't think we do this for the login pane's register form. We can skip and determine later if both forms should do so.

Decide if fields attached to the user should be displayed on the form.

What does the login pane do?

Decide whether the user should be redirected to their account page and/or make the redirect path configurable on the checkout pane.

Skip for now. Because I'm not sure if it should maybe go to their order? The event fire will allow people to control a redirect.

drugan’s picture

Added:

Do not show "You can optionally create an account at the end" text if the pane is disabled.

Forcibly save the pane on the "Complete" step. Emit a warning on an attempt to do otherwise.

drugan’s picture

The above just as a suggestion, so removing the patch from display.

MegaChriz’s picture

@drugan
Looks awesome! I moved the pane enabled check to a different method. Only concern is that if you want to override the registration pane with your own version, the login pane wouldn't detect that, so you would need to override the login pane as well in this case. At least with the registration pane enabled check moved to a different method, only that method needs to be overrided.

I'm not sure yet about forcing the checkout pane to only be set on the 'complete' step. I agree that it doesn't make sense to set the pane on a different step considering the default workflow, but since checkout workflows are flexible in which steps they have, the pane won't be usable in checkout workflows that do not have a 'complete' step if the pane is forced to be on the 'complete' step. For now, I removed this from the patch, even though I found the idea to be awesome.

drugan’s picture

@MegaChriz

Great! Now we don't have irrelevant message on the login pane.

... since checkout workflows are flexible in which steps they have, the pane won't be usable in checkout workflows that do not have a 'complete' step if the pane is forced to be on the 'complete' step.

I can't see any flexibility for this pane. Login step? No. Order information, Payment, Review, Sidebar? Hardly. No "complete" step - no pane as it does not has sense anywhere else.

MegaChriz’s picture

@drugan
True that the registration pane has no place during other steps (at least in the default checkout workflow), but by forcing it to be on the 'complete' step, site developers are forced to include a 'complete' step in their custom checkout workflow if they want to use this pane. Currently, a 'complete' step is not required to have in a checkout workflow. A custom checkout workflow can have any number of steps and any name for each step.

MegaChriz’s picture

Assigned: MegaChriz » Unassigned
Status: Needs work » Needs review
FileSize
22.41 KB
7.3 KB

This patch adds a 'account create' event, which is dispatched at the end of Registration::submitPaneForm(). It may seem odd at first that the form and form state are passed to the event, but since a potential redirect should be set on the form state, we have to pass that to the event.

mglaman’s picture

Assigned: Unassigned » mglaman

Currently, a 'complete' step is not required to have in a checkout workflow.

Oh, it is. As somoene who was required for a URL to not say "complete" but something else, it is required. It's a soft requirement that will need some work. So I'm fine with requiring it.

It may seem odd at first that the form and form state are passed to the event, but since a potential redirect should be set on the form state, we have to pass that to the event.

Good call, there. That may be better than having our event collect a redirect URL.

I'm assigning to myself to finally do a full review. Let's pause on and following patches until I reply.

Thanks everyone!

mglaman’s picture

Assigned: mglaman » Unassigned
Issue tags: -Needs tests

Reviewed! Looks good. Just some small things. Definitely, needs some bikeshedding of method names and event name. We'll want to get bojanz's input for sure. Also ++ on test coverage.

  1. +++ b/modules/checkout/src/Event/AccountCreateEvent.php
    @@ -0,0 +1,108 @@
    +class AccountCreateEvent extends Event {
    
    +++ b/modules/checkout/src/Plugin/Commerce/CheckoutPane/Registration.php
    @@ -0,0 +1,252 @@
    +    $event = new AccountCreateEvent($account, $this->order, $pane_form, $form_state);
    

    I wonder if we should name this CheckoutAccountCreateEvent

  2. +++ b/modules/checkout/src/Event/CheckoutEvents.php
    @@ -0,0 +1,19 @@
    +  const ACCOUNT_CREATE = 'commerce_checkout.account_create';
    

    This is fine, concern is just event object name

  3. +++ b/modules/checkout/src/Plugin/Commerce/CheckoutPane/Login.php
    @@ -233,7 +233,7 @@ class Login extends CheckoutPaneBase implements CheckoutPaneInterface, Container
    +      '#markup' =>  $this->checkRegistrationEnabled() ? $this->t('Proceed to checkout. You can optionally create an account at the end.') : $this->t('Proceed to checkout.'),
    
    @@ -397,4 +397,16 @@ class Login extends CheckoutPaneBase implements CheckoutPaneInterface, Container
    +  protected function checkRegistrationEnabled() {
    

    s/check/is so isRegistrationEnabled? Either way, the naming feels awkward because the pane itself allows registration.

    Maybe isRegistrationPaneAvailable?

  4. +++ b/modules/checkout/src/Plugin/Commerce/CheckoutPane/Login.php
    @@ -397,4 +397,16 @@ class Login extends CheckoutPaneBase implements CheckoutPaneInterface, Container
    +    $panes = $this->checkoutFlow->getPanes();
    +    return $panes['registration']->getConfiguration()['step'] != '_disabled';
    

    We should still make sure the array key is there (!empty()) just in case someone removed the definition. (Remove from UI so the client cannot mess with it.)

  5. +++ b/modules/checkout/src/Plugin/Commerce/CheckoutPane/Registration.php
    @@ -0,0 +1,252 @@
    +  public function __construct(
    +    array $configuration,
    +    $plugin_id,
    +    $plugin_definition,
    +    CheckoutFlowInterface $checkout_flow,
    +    CredentialsCheckFloodInterface $credentials_check_flood,
    +    AccountInterface $current_user,
    +    EntityTypeManagerInterface $entity_type_manager,
    +    UserAuthInterface $user_auth,
    +    RequestStack $request_stack,
    +    EventDispatcherInterface $event_dispatcher
    +  ) {
    

    I know PhpStorm does this, but we don't commit constructors like this (for better or worse.) So let's follow pattern.

  6. +++ b/modules/checkout/src/Plugin/Commerce/CheckoutPane/Registration.php
    @@ -0,0 +1,252 @@
    +    $form_state->setRedirect('commerce_checkout.form', [
    +      'commerce_order' => $this->order->id(),
    +    ]);
    

    Do we need to set the redirect? Isn't it, by default, going to be the page from which the form was submitted?

MegaChriz’s picture

Issue summary: View changes

@mglaman
Yeah, I wasn't really sure yet about the naming.

  1. CheckoutAccountCreateEvent sounds better indeed.
  2. OK
  3. isRegistrationPaneAvailable() sounds good to me.
  4. OK
  5. I find the code easier to read this way and you can easier see if it matches with the create() method. I searched for coding standards on this topic, but the only thing I found is a proposal to make it a coding standard in #1539712-88: [policy, no patch] Coding standards for breaking function calls and language constructs across lines. It is nowhere used in Drupal core nor in Drupal Commerce at the moment.
  6. Not sure if the redirect is needed. I assume it was copied from the login checkout pane:
    /**
     * {@inheritdoc}
     */
    public function submitPaneForm(array &$pane_form, FormStateInterface $form_state, array &$complete_form) {
      $triggering_element = $form_state->getTriggeringElement();
      switch ($triggering_element['#op']) {
        case 'continue':
          break;
    
        case 'login':
        case 'register':
          $storage = $this->entityTypeManager->getStorage('user');
          /** @var \Drupal\user\UserInterface $account */
          $account = $storage->load($form_state->get('logged_in_uid'));
          user_login_finalize($account);
          $this->order->setCustomer($account);
          $this->credentialsCheckFlood->clearAccount($this->clientIp, $account->getAccountName());
          break;
      }
    
      $form_state->setRedirect('commerce_checkout.form', [
        'commerce_order' => $this->order->id(),
        'step' => $this->checkoutFlow->getNextStepId($this->getStepId()),
      ]);
    }
    

We could add more test coverage here. There is no test coverage yet for:

  • Adding the billing profile to the user's address book.
  • The event being dispatched.

And to come back on your questions from #54:

Decide if fields attached to the user should be displayed on the form.

What does the login pane do?

The login pane doesn't display fields attached to the user, so this feature should be a follow-up then.

Decide whether the user should be redirected to their account page and/or make the redirect path configurable on the checkout pane.

Skip for now. Because I'm not sure if it should maybe go to their order? The event fire will allow people to control a redirect.

I thought going to the user page made more sense because Drupal core does this as well when you register and because it follows upon your latest action: the creation of your account. But going to the user's order also make sense as that follows upon the key action you were doing: placing an order (account creation is a side action). I agree to skip this decision for now.

Updated issue summary.

drugan’s picture

Do we need to set the redirect?

I also think that redirect is a bit of a redundant one. Think of it that a user is newcomer one and does not know yet where they are now when on the order view page. Instead, we need to change the checkout complete message to something like this:

Your order number is @number.

You can view your order on your account page.

ChandeepKhosa’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
162.37 KB
90.41 KB
318.49 KB
306.75 KB
254.36 KB

Thanks @heddn. I have reviewed your patch in #65 and confirm it's working well for me with Drupal 8.4.5 & Commerce 2.4

Once you add the patch, it won't work right away, you need to edit your Checkout Flow(s) (/admin/commerce/config/checkout-flows).
Move the `Guest registration after checkout` pane from the `disabled` section to the `Complete` section, underneath the `Completion message` pane, or wherever else you desire.

Save, then export your configuration and commit to git.

I'm marking this at RTBC.

I've attached screenshots to show how it looks for me, working very nicely.

fotograafinge’s picture

Works great ! Patch #65 als works with Drupal 8.5.1 and Commerce 2.6

Is it possible to show all required fields from de user registration form?

bmcclure’s picture

Status: Reviewed & tested by the community » Needs work

This patch doesn't work for our use case.

We don't use a billing profile for Quote-only orders, and so the following code from the Registration class errors out:

    // Add the billing profile to the user's address book.
    $profile = $this->order->getBillingProfile();
    $profile->setOwner($account);
    $profile->save();

I'm guessing we just need a conditional surrounding that block that checks to ensure there actually is a billing profile?

heddn’s picture

Status: Needs work » Needs review
FileSize
22.43 KB
850 bytes

It also needed a re-roll. It was trivial so also added the fix for #68 in the same patch.

Status: Needs review » Needs work

The last submitted patch, 69: 2857157-69.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

heddn’s picture

Status: Needs work » Needs review
FileSize
22.46 KB
3.66 KB

This needed a test change to accommodate the wording change in #2856583: Allow free orders (checkout without payment).

Status: Needs review » Needs work

The last submitted patch, 71: 2856583-71.patch, failed testing. View results

heddn’s picture

Status: Needs work » Needs review
FileSize
847 bytes
22.45 KB

Another reference.

heddn’s picture

FileSize
1.29 KB
23.45 KB

Undefined index when clicking the continue to next step button.

bojanz’s picture

Status: Needs review » Needs work

This is looking great, and is almost ready. Thanks, everyone.

I think it's fine to not support showing custom/required fields right now, cause we don't support it at step 1 of checkout either. We can expand both at the same time. This allows us to ship this feature ASAP.

Question: It feels odd that we're passing $form and $form_state to CheckoutAccountCreateEvent, but firing the event from the pane submit, which means its too late to do anything with those two parameters, other than redirect. Would it make sense to remove $form and $form_state, and confirm that throwing a NeedsRedirectException works?

+    // Ensure that there is no user on the site with the same email address.
+    $existing_user = $this->entityTypeManager->getStorage('user')->loadByProperties(['mail' => $this->order->getEmail()]);
+
+    if ($existing_user) {
+      return $pane_form;
+    }

This feels sensible. +1.

+    $this->order->setCustomer($account);
+
+    // Add the billing profile to the user's address book.
+    $profile = $this->order->getBillingProfile();
+    if ($profile) {
+      $profile->setOwner($account);
+      $profile->save();
+    }

You should inject the OrderAssignment class and call $orderAssignment->assign($order, $account), instead of doing this.
That allows other assignment operations to be done as well (assigning payment methods, shipping profiles, etc).
commerce_cart_user_login() already does that for regular logins, but at this stage the order is already placed, so it ignores it. That's why we need to do it ourself.

+ * @CommerceCheckoutPane(
+ *   id = "registration",
+ *   label = @Translation("Guest registration after checkout"),
+ * )

We should bikeshed a better pane ID for this pane, so that it's clear to people viewing an export that this pane belongs on checkout complete.
Perhaps "registration_on_complete"? Or "completion_registration" to match "completion_message"?

MegaChriz’s picture

@bojanz

Question: It feels odd that we're passing $form and $form_state to CheckoutAccountCreateEvent, but firing the event from the pane submit, which means its too late to do anything with those two parameters, other than redirect. Would it make sense to remove $form and $form_state, and confirm that throwing a NeedsRedirectException works?

These parameters are indeed passed to the CheckoutAccountCreateEvent to be able to set a redirect (see #60), but I thought it would also be useful to have as context.
I wasn't aware of the existence of a NeedsRedirectException. It sounds like that would form a good solution too. I'm not too sure yet about how useful $form and $form_state as context actually are.

We should bikeshed a better pane ID for this pane, so that it's clear to people viewing an export that this pane belongs on checkout complete.
Perhaps "registration_on_complete"? Or "completion_registration" to match "completion_message"?

"completion_registration" sounds fine to me. In this case, I think it would be good to rename the class as well to match the pane ID?

mglaman’s picture

Question about: completion_registration . Can we prevent someone from placing this earlier in the checkout flow? Like, I don't know why anyone would. But I suppose naming the plugin and class and label like this helps show people "please, don't shoot your self in the foot. You can, and it'll hurt, but don't."

NeedsRedirectException is the logic control exception that Drupal core uses so that you can force a redirect when a form is being built, and not solely on it being submitted. We discovered it uses when setting up offsite payment redirects. So I agree we should not pass the form and form state, to prevent manipulation of that array and object.

bojanz’s picture

Question about: completion_registration . Can we prevent someone from placing this earlier in the checkout flow? Like, I don't know why anyone would. But I suppose naming the plugin and class and label like this helps show people "please, don't shoot your self in the foot. You can, and it'll hurt, but don't."

We should just whether the order is placed in the isVisible() method. That way the pane won't work on non-complete steps.

MegaChriz’s picture

There's on caveat when using NeedsRedirectException here. If there are multiple event subscribers for the account create event, then an event subscriber that wants to do a redirect, unintentionally prevents other event subscribers from being called. Maybe it's better to add a method setRedirectPath() on the event instead?

MegaChriz’s picture

The following patch changes the following:

  • The checkout pane is renamed from "registration" to "completion_registration".
  • Passing form and form state to CheckoutAccountCreateEvent is removed.
  • Methods are added to CheckoutAccountCreateEvent to get and set a redirect.
  • The complete registration checkout pane checks after dispatching the account create event (in CompletionRegistration::submitPaneForm(), if the event has a redirect url on it. If so, it passes that to the form state.
  • A test is added to verify that the user gets redirected if an event subscriber set the redirect on the event. A module called "commerce_checkout_account_create_event_test" is added to implement such an event subscriber.

I've chosen for setting/getting methods for redirect urls on CheckoutAccountCreateEvent, because throwing a NeedsRedirectException would prevent any other event subscribers for the same event from being called.

I had some issues with running tests locally, so I'm not sure if the tests will pass.

MegaChriz’s picture

Spotted a few references to the registration checkout pane that I had not yet renamed.

The last submitted patch, 80: commerce-checkout-pane-guest-registration-2858157-80.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Status: Needs review » Needs work

The last submitted patch, 81: commerce-checkout-pane-guest-registration-2858157-81.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

MegaChriz’s picture

Fixed the following:

  1. Misnamed services yaml file for test module 'commerce_checkout_account_create_event_test'.
  2. Referenced a wrong class in \Drupal\commerce_checkout_account_create_event_test\EventSubscriber\AccountCreate.
  3. Coding standards.

Status: Needs review » Needs work
MegaChriz’s picture

Hopefully this fixes the test failure. Also added the suggestion by @bojanz from #78: when the order isn't placed yet, the pane is not shown, effectively making it only possible to be visible at the completion step.

Status: Needs review » Needs work
MegaChriz’s picture

Apparently, assertUrl('user/3/edit') fails testing when Drupal is installed in a subdirectory. To me, it looks like assertUrl() is broken.

Let's see if the workaround Url::fromRoute() helps to pass tests.

Status: Needs review » Needs work
MegaChriz’s picture

According to @mixologic, $this->assertUrl('user/3/edit'); should have worked to pass tests, so I'm not sure why it doesn't. Any help welcome.

Sut3kh’s picture

I believe I have 'fixed' the test by changing $this->drupalGet($this->product->toUrl()->toString()); to $this->drupalGet($this->product->toUrl()); (which I did to all instances for consistency).

I think @megachriz is correct and there is a bug around assertUrl()/drupalGet() and I have created a new issue on the topic at #2986962: BrowserTestBase::drupalGet() does not appear to be handling base url properly.

The other thing I have changed (the original reason I was working on this!) is a minor tweak to use $form_state->getValue($pane_form['#parents']); in CompletionRegistration::validatePaneForm() to make it easy to subclass the panel (i.e. for use with the email_registration module where I simply want to make the username a hidden field but there is no need to override/copy paste validatePaneForm() other than because the pane has an id other than 'completion_registration').

FiNeX’s picture

Hi, what about "https://www.drupal.org/project/commerce_guest_registration" module? It creates automatically the user profile (or match the order with an existing profile having the same email address). Would be possible to merge it with this feature?

Aerzas’s picture

I proposed a patch on commerce_shipping (#2999013) to also bind the shipping profile(s) to the newly created user.

Aerzas’s picture

Here is a proposed update:
- Make use of EntityFormDisplay to display user custom fields
- Check if email exists at the isVisible method level to avoid a blank pane

Status: Needs review » Needs work
MrPaulDriver’s picture

Further the suggestion made by @MegaChriz at #50

After registering on the checkout complete, I think there should be redirect to the user account page. It doesn't make sense to stay on the checkout complete page, in my opinion.

I agree with this idea, but think it would make more sense to take the user to the order page at /user/*/orders.

aumcara’s picture

I am taking the train here and also encounter the problem exposed at the top (new user of D8) and when I received my first order I realised that the custormers stays Anonymous !!

Can someone guide me what to do in order to have a similar behavior as D7 ?
It's quiet embarrassing to keep them "anonymous" after the order is complete.
Is there a quick workaround even temporary in order to achieve the same behavior as it was in D7 ?

thanks for replying

... Also, ... is there a way under D8 to avoid (during order processing) that customer need to enter 2 times the same informatio (shipping and Billing name and address) instead of clicking a check box to use the same data as the one filled in initialy ?

Thanks again.

johnpicozzi’s picture

@aumcara - It would appear that the module suggested in #92 would do what you are looking for.

I added the patch in #94 and got the following error after submitting the registration form.

Error: Unsupported operand types in Drupal\commerce_checkout\Plugin\Commerce\CheckoutPane\CompletionRegistration->validatePaneForm() (line 210 of modules/contrib/commerce/modules/checkout/src/Plugin/Commerce/CheckoutPane/CompletionRegistration.php).

johnpicozzi’s picture

I found that the following produced the above error if you didn't have any fields other than username and password in your registration form.

+ $values['fields']);

The attached patch adds a check to make sure $values['fields'] has a value before using it.

mglaman’s picture

Status: Needs work » Needs review

Needs review for tests

Status: Needs review » Needs work
FiNeX’s picture

Hi, currently this module show the registration form after completing the order. Would be possible to create automatically the account?

mglaman’s picture

+++ b/modules/checkout/src/Plugin/Commerce/CheckoutPane/CompletionRegistration.php
@@ -0,0 +1,265 @@
+    // Process additional fields. Because of the EntityFormDisplay, they are all
+    // considered at the root of the form values.
+    $fields = Element::children($pane_form['form']['register']['fields']);
+    foreach ($fields as $field) {
+      $values['fields'][$field] = $form_state->getValue($field);
+    }
...
+    if (!empty($values['fields'])) {
+      $accountValues += $values['fields'];
+    }

Beyond the error, why aren't we using the form display to apply the values?

mglaman’s picture

Hi, currently this module show the registration form after completing the order. Would be possible to create automatically the account?

That's a different issue -- let's discuss in a new issue, please. Let's try to keep this one on a registration form to convert the user at the end of checkout.

mglaman’s picture

Assigned: Unassigned » mglaman
Status: Needs work » Needs review
FileSize
34.65 KB

Forgot to make an interdiff, bad on my fault. Here's the important change: using form display to extract and validate values.

    $values = $form_state->getValue($pane_form['#parents']);
    $account = $this->entityTypeManager->getStorage('user')->create([
      'pass' => $values['register']['password'],
      'mail' => $this->order->getEmail(),
      'name' => $values['register']['name'],
    ]);

    $form_display = EntityFormDisplay::collectRenderDisplay($account, 'register');
    $form_display->extractFormValues($account, $pane_form['register']['fields'], $form_state);
    $form_display->validateFormValues($account, $pane_form['register']['fields'], $form_state);

I'm going to take some time to review this patch in full, now.

mglaman’s picture

  1. +++ b/modules/checkout/src/Plugin/Commerce/CheckoutPane/CompletionRegistration.php
    @@ -0,0 +1,260 @@
    +      && !$this->entityTypeManager->getStorage('user')->loadByProperties(['mail' => $this->order->getEmail()]);
    ...
    +    $account = $this->entityTypeManager->getStorage('user')->create([
    ...
    +    $account = $this->entityTypeManager->getStorage('user')->load($user->id());
    

    We may as well just put user storage as a class property.

  2. +++ b/modules/checkout/src/Plugin/Commerce/CheckoutPane/CompletionRegistration.php
    @@ -0,0 +1,260 @@
    +    $this->messenger()->addStatus($this->t('Registration successful. You are now logged in.'));
    

    Ensure the messenger service is injected instead of the whacky trait method.

  3. +++ b/modules/checkout/src/Plugin/Commerce/CheckoutPane/Login.php
    @@ -397,4 +399,16 @@ class Login extends CheckoutPaneBase implements CheckoutPaneInterface, Container
    +  protected function isRegistrationPaneAvailable() {
    +    $panes = $this->checkoutFlow->getPanes();
    +    return !empty($panes['completion_registration']->getConfiguration()['step']) && $panes['completion_registration']->getConfiguration()['step'] != '_disabled';
    +  }
    

    Does this need review against recent work done to the payment panes, in regards to checking availability? Might not.

I still need to review the tests, as well.

Status: Needs review » Needs work

The last submitted patch, 105: 2857157-105.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mglaman’s picture

Status: Needs work » Needs review
FileSize
6.83 KB
35.1 KB

This should fix the test failure and address my own nits.

However, I feel like this is awkward:


    if (!empty($pane_form['register']['fields'])) {
      $form_display = EntityFormDisplay::collectRenderDisplay($account, 'register');
      $form_display->extractFormValues($account, $pane_form['register']['fields'], $form_state);
      $form_display->validateFormValues($account, $pane_form['register']['fields'], $form_state);
    }

It also looks like we don't have tests which cover a user having a custom exposed field.

+++ b/modules/checkout/src/Plugin/Commerce/CheckoutPane/CompletionRegistration.php
@@ -0,0 +1,277 @@
+      $form_display->validateFormValues($account, $pane_form['register']['fields'], $form_state);
...
+      list($field_name) = explode('.', $violation->getPropertyPath(), 2);
+      if (isset($values['fields'][$field_name])) {
+        $form_state->setError($pane_form['form']['register']['fields'][$field_name], $violation->getMessage());
+      }

This manual validation can be removed.

HitchShock’s picture

Status: Needs review » Reviewed & tested by the community

It's working for me. An unauthorized user can now register in one of checkout flow steps

mglaman’s picture

Status: Reviewed & tested by the community » Needs work

It's not done, HitchShock. Very close though! Thanks for the manual test.

It also looks like we don't have tests which cover a user having a custom exposed field.

I need to work on a test for this, which is on my agenda today. Have you tested this scenario? When there is a custom field exposed on the register form and ensuring it is displayed and submitted properly?

mglaman’s picture

FileSize
41.13 KB

This tests custom user fields. Validation appears to work, but the actual attachment of values to the user does not. This patch will fail testing.

mglaman’s picture

Status: Needs work » Needs review
FileSize
40.42 KB

This patch fixes the tests and ensures custom user fields on the Register form display are respected. It also adds a comment link to how core's RegisterForm handles non-form display elements like name, email, and password for validation.

mglaman’s picture

  1. +++ b/modules/checkout/src/Event/CheckoutEvents.php
    --- /dev/null
    +++ b/modules/checkout/src/Plugin/Commerce/CheckoutPane/CompletionRegistration.php
    

    🤦‍♂️I forgot to rename the class and file to CompletionRegister

  2. +++ b/modules/checkout/tests/src/Functional/CheckoutOrderTest.php
    @@ -284,11 +289,304 @@ class CheckoutOrderTest extends CommerceBrowserTestBase {
    +    $account = reset($accounts);
    ...
    +    $account = reset($accounts);
    

    We should assert the account is active.

Status: Needs review » Needs work

The last submitted patch, 112: 2857157-112.patch, failed testing. View results

mglaman’s picture

Assigned: mglaman » Unassigned
Status: Needs work » Needs review
FileSize
40.58 KB

This fixes my nits from #113 which also addresses the failure.

Status: Needs review » Needs work

The last submitted patch, 115: 2857157-115.patch, failed testing. View results

mglaman’s picture

Status: Needs work » Needs review
Anybody’s picture

#115 works correct for me! RTBC+1 for this important feature!
HitchShock and FiNeX, could you also test it please?

Anybody’s picture

Status: Needs review » Reviewed & tested by the community

Ok after final testings I can confirm RTBC.... if someone disagrees, please reset. For me it works great and without any further issues.

bojanz’s picture

Assigned: Unassigned » bojanz
Status: Reviewed & tested by the community » Needs work

Time to get this done.

Unfortunately, the current patch is not committable yet.

General problems:
1. I do think that the pane should be enabled by default, because guest checkout is enabled by default, and this pane completes the desired UX in that case. It also allows us to remove the clunky pane enabling we are currently doing in CheckoutOrderTest.

2. CompletionRegister is still not using the order assignment class, which means that while the billing profile gets assigned, nothing else does (shipping profiles, payment methods, etc).

3. CheckoutAccountCreateEvent doesn't clarify that it's only fired on checkout complete, not when registering at the beginning of checkout. Might need a better name too.

4.

+ *   wrapper_element = "fieldset",

Let's not use a fieldset, it wrestles away control from the Twig template.

Code style issues:
1.

+    $this->messenger = $messenger;

We no longer inject the messenger, because it's available on the base plugin class via the MessengerTrait.

2.

+  /**
+   * {@inheritdoc}
+   */
+  public function isVisible() {
+    // Registering is only possible for anonymous users and this pane can only
+    // be shown at the end of checkout. Ensure that there is no user on the site
+    // with the same email address.
+    return $this->currentUser->isAnonymous()
+      && $this->order->getState()->value != 'draft'
+      && !$this->userStorage->loadByProperties(['mail' => $this->order->getEmail()]);
+  }

It would be better to have this as separate conditions. It is cleaner, and allows the condition list to grow over time.

3.

+    return [
+      '#theme' => 'commerce_checkout_completion_register',
+      'form' => $pane_form,
+    ];

This is odd.

FiNeX’s picture

Hi, the patch on issue #2999013: React to order assignment, and assign the shipping profile should partially fix the second problem highlighted by bojanz. Now I'm tryng Commerce -dev with both patches (#2857157-115: Implement registration after guest checkout and #2999013-7: React to order assignment, and assign the shipping profile).

FiNeX’s picture

The patch works quite well, except it doesn't send any welcome mail to the user but that is a minor issue.

FiNeX’s picture

EDIT: misunderstanding :-) ignore this comment.

FiNeX’s picture

Updated patch against latest -dev version.

bojanz’s picture

Hopefully the last patch. Thanks to Liip for sponsoring the final work.

This addresses all of my points from #120. Note that the event has been renamed and is now called CheckoutCompletionRegisterEvent.

Other changes:
- Removed the UserOrderViewController which was probably included by accident (it wasn't used)
- Gone through the UI with rszrama and mglaman, and we have tweaked all labels and help texts.
- Renamed the new test module to commerce_checkout_test, we tend to have one large test module per parent module, to avoid having a test module per test class.

Followups:
- #3024620: Simplify CheckoutOrderTest
- #3024622: Improve error handling on the competion register form

bojanz’s picture

Status: Needs work » Needs review

  • bojanz committed 954ff35 on 8.x-2.x
    Issue #2857157 by MegaChriz, heddn, drugan, mglaman, Sophie.SK, Bram...
bojanz’s picture

Status: Needs review » Fixed

Committed #125. Thanks, everyone!

FiNeX’s picture

Thanks everybody :-)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.