Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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-upDecide if fields attached to the user should be displayed on the form.follow-upDecide 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.
Comment | File | Size | Author |
---|---|---|---|
#125 | 2857157-125.patch | 35.27 KB | bojanz |
| |||
#124 | 2857157-dev.patch | 41.11 KB | FiNeX |
| |||
#115 | 2857157-115.patch | 40.58 KB | mglaman |
| |||
#112 | 2857157-112.patch | 40.42 KB | mglaman |
#111 | 2857157-111.patch | 41.13 KB | mglaman |
Comments
Comment #2
bmcclure CreditAttribution: bmcclure as a volunteer and at Top Floor commentedPR 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!
Comment #3
mglamanThere'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.
Comment #4
Lukas von BlarerThe patch works great! Thank you!
Comment #5
vasikeCreated "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
Comment #6
mglamanThanks vasike, for linking!
Comment #7
vasikeFirst 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:
4. Create some tests
So i think we're on "Needs work" :(
Comment #8
bmcclure CreditAttribution: bmcclure as a volunteer and at Top Floor commentedOne 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?
Comment #9
bmcclure CreditAttribution: bmcclure as a volunteer and at Top Floor commentedAlso 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?
Comment #10
bmcclure CreditAttribution: bmcclure as a volunteer and at Top Floor commentedAnd 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?
Comment #11
mglamanMaybe a configurable message and destination. How about "stay on same page" or "redirect to viewing order" as an option for the pane when saved.
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.
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.
Comment #12
bmcclure CreditAttribution: bmcclure as a volunteer and at Top Floor commentedOn 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.
Comment #13
bmcclure CreditAttribution: bmcclure as a volunteer and at Top Floor commentedUpdated PR to include fix for recent constructor change in the base class:
https://patch-diff.githubusercontent.com/raw/drupalcommerce/commerce/pul...
Patch file: https://patch-diff.githubusercontent.com/raw/drupalcommerce/commerce/pul...
Comment #14
Alluuu CreditAttribution: Alluuu commentedI 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..
Comment #15
itamair CreditAttribution: itamair as a volunteer commentedTnx 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.
Comment #16
steveoliver CreditAttribution: steveoliver at Circatree commented@Alluuu see related issue #2808813: Create a recipe for assigning anon orders on user registration
Comment #17
josephdpurcell CreditAttribution: josephdpurcell at Digital Bridge Solutions commentedLots 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:
Comment #18
josephdpurcell CreditAttribution: josephdpurcell at Digital Bridge Solutions commentedThe screenshots for comment #17.
Comment #19
Sophie.SKWhile 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.
Comment #20
Sophie.SKAfter trying the patch... I realised that there were a few bugs with it. Attached is a new patch:
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" :)
Comment #21
Sophie.SKAck, 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!
Comment #22
Lukas von BlarerThe patch works for me for the basic functionality. I discovered a few bugs:
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.
Comment #23
Lukas von BlarerI am sorry, it seems that I didn't test properly. Right now I have all three options on the login checkout step.
Comment #24
Bram Linssen CreditAttribution: Bram Linssen as a volunteer commentedThe 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.
Comment #25
Bram Linssen CreditAttribution: Bram Linssen as a volunteer commentedLet's add the Billingprofile to the user's addressbook as well.
Comment #26
Maikel CreditAttribution: Maikel commentedI 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?
Comment #27
Sophie.SKThis 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.
Comment #28
Maikel CreditAttribution: Maikel commentedHmm, 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.
Comment #29
Sophie.SKHmm, 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?
Comment #30
Sophie.SKAdding 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.
Comment #31
Sophie.SKChanging status.
Comment #32
AndyD328Works 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.
Comment #33
Sophie.SKHaha 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.
Comment #34
drugan CreditAttribution: drugan as a volunteer commentedEdited 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.
Comment #35
drugan CreditAttribution: drugan as a volunteer commentedI think this change is required by most of the sites. Others can disable the pane if they don't need it.
Comment #36
mglamanThere 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.
We have an injected entity type manager, let's use it here.
We have an entity type manager injected, let's use it to have it create user object from its storage.
These need to be wrapped in
{%trans%}{%endtrans%}
Comment #37
MegaChriz CreditAttribution: MegaChriz as a volunteer commentedI'll look into creating an automated test for this today.
A quick review on the UI:
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:
Comment #38
MegaChriz CreditAttribution: MegaChriz at WebCoo commentedNew patch, with the following changes:
Registration::validatePaneForm()
:The property name should be 'pass' instead of 'password'.
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...
Comment #39
MegaChriz CreditAttribution: MegaChriz at WebCoo commentedChanges in this patch:
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]).
Comment #40
drugan CreditAttribution: drugan as a volunteer commentedAdded 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.
Comment #41
MegaChriz CreditAttribution: MegaChriz at WebCoo commented@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:
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.
Comment #42
drugan CreditAttribution: drugan as a volunteer commentedSomehow 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.
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.
Comment #43
mglamanNo. We're not providing roles.
Comment #44
drugan CreditAttribution: drugan as a volunteer commented@mglaman
Are there any reasons for not providing such a role?
Comment #45
MegaChriz CreditAttribution: MegaChriz at WebCoo commented@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.
Comment #46
drugan CreditAttribution: drugan as a volunteer commented@MegaChriz
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.
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.
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.
Comment #47
mglamandrugan, 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.
Comment #48
drugan CreditAttribution: drugan as a volunteer commentedOK, I've removed from display the patch with a role added. The #39 is the last visible which can be reviewed.
Comment #49
mglamandrugan, 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.
Comment #50
MegaChriz CreditAttribution: MegaChriz at WebCoo commented@mglaman
You can review #39, but I think the following changes should be made:
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.
Comment #51
mglamanNoted. 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.
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.
Comment #52
MegaChriz CreditAttribution: MegaChriz at WebCoo commented@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?
Comment #53
MegaChriz CreditAttribution: MegaChriz at WebCoo commentedThis patch builds upon the patch from #38.
Changes in this patch:
Still left to do:
Updated this also in the issue summary.
Comment #54
mglamanLet'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.
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.
What does the login pane do?
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.
Comment #55
drugan CreditAttribution: drugan as a volunteer commentedAdded:
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.
Comment #56
drugan CreditAttribution: drugan as a volunteer commentedThe above just as a suggestion, so removing the patch from display.
Comment #57
MegaChriz CreditAttribution: MegaChriz at WebCoo commented@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.
Comment #58
drugan CreditAttribution: drugan as a volunteer commented@MegaChriz
Great! Now we don't have irrelevant message on the login pane.
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.
Comment #59
MegaChriz CreditAttribution: MegaChriz at WebCoo commented@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.
Comment #60
MegaChriz CreditAttribution: MegaChriz at WebCoo commentedThis 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.Comment #61
mglamanOh, 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.
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!
Comment #62
mglamanReviewed! 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.
I wonder if we should name this CheckoutAccountCreateEvent
This is fine, concern is just event object name
s/check/is so isRegistrationEnabled? Either way, the naming feels awkward because the pane itself allows registration.
Maybe
isRegistrationPaneAvailable
?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.)
I know PhpStorm does this, but we don't commit constructors like this (for better or worse.) So let's follow pattern.
Do we need to set the redirect? Isn't it, by default, going to be the page from which the form was submitted?
Comment #63
MegaChriz CreditAttribution: MegaChriz at WebCoo commented@mglaman
Yeah, I wasn't really sure yet about the naming.
isRegistrationPaneAvailable()
sounds good to me.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.We could add more test coverage here. There is no test coverage yet for:
And to come back on your questions from #54:
The login pane doesn't display fields attached to the user, so this feature should be a follow-up then.
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.
Comment #64
drugan CreditAttribution: drugan as a volunteer commentedI 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.
Comment #65
heddnThis should address all the concerns in #62.
Comment #66
ChandeepKhosa CreditAttribution: ChandeepKhosa at 2Toucans commentedThanks @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.
Comment #67
fotograafinge CreditAttribution: fotograafinge commentedWorks 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?
Comment #68
bmcclure CreditAttribution: bmcclure as a volunteer and at Top Floor commentedThis 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:
I'm guessing we just need a conditional surrounding that block that checks to ensure there actually is a billing profile?
Comment #69
heddnIt also needed a re-roll. It was trivial so also added the fix for #68 in the same patch.
Comment #71
heddnThis needed a test change to accommodate the wording change in #2856583: Allow free orders (checkout without payment).
Comment #73
heddnAnother reference.
Comment #74
heddnUndefined index when clicking the continue to next step button.
Comment #75
bojanz CreditAttribution: bojanz at Centarro commentedThis 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?
This feels sensible. +1.
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.
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"?
Comment #76
MegaChriz CreditAttribution: MegaChriz at WebCoo commented@bojanz
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.
"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?
Comment #77
mglamanQuestion 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.
Comment #78
bojanz CreditAttribution: bojanz at Centarro commentedWe should just whether the order is placed in the isVisible() method. That way the pane won't work on non-complete steps.
Comment #79
MegaChriz CreditAttribution: MegaChriz at WebCoo commentedThere'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?Comment #80
MegaChriz CreditAttribution: MegaChriz at WebCoo commentedThe following patch changes the following:
CompletionRegistration::submitPaneForm()
, if the event has a redirect url on it. If so, it passes that to the form state.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.
Comment #81
MegaChriz CreditAttribution: MegaChriz at WebCoo commentedSpotted a few references to the registration checkout pane that I had not yet renamed.
Comment #84
MegaChriz CreditAttribution: MegaChriz at WebCoo commentedFixed the following:
Comment #86
MegaChriz CreditAttribution: MegaChriz at WebCoo commentedHopefully 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.
Comment #88
MegaChriz CreditAttribution: MegaChriz at WebCoo commentedApparently,
assertUrl('user/3/edit')
fails testing when Drupal is installed in a subdirectory. To me, it looks likeassertUrl()
is broken.Let's see if the workaround
Url::fromRoute()
helps to pass tests.Comment #90
MegaChriz CreditAttribution: MegaChriz at WebCoo commentedAccording 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.Comment #91
Sut3kh CreditAttribution: Sut3kh as a volunteer commentedI 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').Comment #92
FiNeX CreditAttribution: FiNeX as a volunteer commentedHi, 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?
Comment #93
Aerzas CreditAttribution: Aerzas as a volunteer commentedI proposed a patch on commerce_shipping (#2999013) to also bind the shipping profile(s) to the newly created user.
Comment #94
Aerzas CreditAttribution: Aerzas as a volunteer commentedHere 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 paneComment #96
MrPaulDriver CreditAttribution: MrPaulDriver commentedFurther the suggestion made by @MegaChriz at #50
I agree with this idea, but think it would make more sense to take the user to the order page at /user/*/orders.
Comment #97
aumcara CreditAttribution: aumcara commentedI 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.
Comment #98
johnpicozzi@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).
Comment #99
johnpicozziI 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.Comment #100
mglamanNeeds review for tests
Comment #102
FiNeX CreditAttribution: FiNeX as a volunteer commentedHi, currently this module show the registration form after completing the order. Would be possible to create automatically the account?
Comment #103
mglamanBeyond the error, why aren't we using the form display to apply the values?
Comment #104
mglamanThat'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.
Comment #105
mglamanForgot to make an interdiff, bad on my fault. Here's the important change: using form display to extract and validate values.
I'm going to take some time to review this patch in full, now.
Comment #106
mglamanWe may as well just put user storage as a class property.
Ensure the messenger service is injected instead of the whacky trait method.
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.
Comment #108
mglamanThis should fix the test failure and address my own nits.
However, I feel like this is awkward:
It also looks like we don't have tests which cover a user having a custom exposed field.
This manual validation can be removed.
Comment #109
HitchShockIt's working for me. An unauthorized user can now register in one of checkout flow steps
Comment #110
mglamanIt's not done, HitchShock. Very close though! Thanks for the manual test.
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?
Comment #111
mglamanThis tests custom user fields. Validation appears to work, but the actual attachment of values to the user does not. This patch will fail testing.
Comment #112
mglamanThis 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.
Comment #113
mglaman🤦♂️I forgot to rename the class and file to CompletionRegister
We should assert the account is active.
Comment #115
mglamanThis fixes my nits from #113 which also addresses the failure.
Comment #117
mglamanComment #118
Anybody#115 works correct for me! RTBC+1 for this important feature!
HitchShock and FiNeX, could you also test it please?
Comment #119
AnybodyOk after final testings I can confirm RTBC.... if someone disagrees, please reset. For me it works great and without any further issues.
Comment #120
bojanz CreditAttribution: bojanz at Centarro commentedTime 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.
Let's not use a fieldset, it wrestles away control from the Twig template.
Code style issues:
1.
We no longer inject the messenger, because it's available on the base plugin class via the MessengerTrait.
2.
It would be better to have this as separate conditions. It is cleaner, and allows the condition list to grow over time.
3.
This is odd.
Comment #121
FiNeX CreditAttribution: FiNeX as a volunteer commentedHi, 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).
Comment #122
FiNeX CreditAttribution: FiNeX as a volunteer commentedThe patch works quite well, except it doesn't send any welcome mail to the user but that is a minor issue.
Comment #123
FiNeX CreditAttribution: FiNeX as a volunteer commentedEDIT: misunderstanding :-) ignore this comment.
Comment #124
FiNeX CreditAttribution: FiNeX as a volunteer commentedUpdated patch against latest -dev version.
Comment #125
bojanz CreditAttribution: bojanz at Centarro commentedHopefully 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
Comment #126
bojanz CreditAttribution: bojanz at Centarro commentedComment #128
bojanz CreditAttribution: bojanz at Centarro commentedCommitted #125. Thanks, everyone!
Comment #129
FiNeX CreditAttribution: FiNeX as a volunteer commentedThanks everybody :-)