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
Followup to #2844917: Create a commerce_profile_select form element and field widget.
If the profile belongs to a user, the form element should provide a dropdown of profiles to choose from, along with Edit and Add new functionality.
We want the Commerce Addressbook 3.x UX here: https://www.drupal.org/files/project-images/Capture%20d%E2%80%99e%CC%81c...
Proposed resolution
Remaining tasks
#106 had a partially working patch.
<snip>
However, I ran into form issues that I could not solve in time.
Apply the patch, enable commerce_order_test, and go to "/commerce_order_test/profile_select_test_form". Click "+ Enter a new address". You will see that clicking the "Return to address selection" does nothing the first time. Under the hook, Drupal assigns the clicks to completely wrong elements. It treats selecting "Enter a new address" as a click to the Cancel button, and the click to the Cancel button as a click to the Edit button. This tells me that the process callback is rebuilding the form too early, but I don't know how to avoid that yet.
</snip>
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#308 | checkbox-same-as-billing-2852207-69-2.6.patch | 20.3 KB | h1nds1ght |
#308 | 57c11175d279fe55db9454b4274608195211aeb8-2.9.patch | 41.93 KB | h1nds1ght |
#308 | 760-2.10.patch | 37.16 KB | h1nds1ght |
#298 | Schermafbeelding 2018-11-19 om 20.46.33.png | 193.44 KB | nickvanboven |
#294 | interdiff-285-294.txt | 1.19 KB | andyg5000 |
Comments
Comment #2
jkuma CreditAttribution: jkuma as a volunteer commentedLet me work on it !
Comment #3
drugan CreditAttribution: drugan as a volunteer commented@jkuma Check the bojanz latest commit. He works on this right now.
Not sure if it is my specific issue or it is somehow related with the commit. What is interesting (see screenshot's second attempt to update) after manually adding "drupal/address": "1.x-dev" to my composer.json "require-dev" block it reports this:
The requested package drupal/address (locked at 1.0.0-rc3, required as 1.x-dev) is satisfiable by drupal/address[1.0.0-rc3] but these conflict with your requirements or minimum-stability.
And that is despite the fact 1.x-dev version is available for composer to install. (composer info -a drupal/address)
Comment #4
bojanz CreditAttribution: bojanz at Centarro commented@jkuma
Good luck :)
@drugan
There's no need for you to add address to your composer.json file. "composer update drupal/commerce --with-dependencies" would switch you to -dev automatically.
Comment #5
jkuma CreditAttribution: jkuma as a volunteer commented@bojan
Sorry for the delay in my reply. Last week-end, I spent quality time to study the way to fetch customer profiles for the current logged user. The problem is, as you may know, there are no such methods to get all customer profiles from an UID. So, we can create a plugin or add a new method in order to do so, but I think it's nasty.
From my perspective, we need to create a customer entity type and gather all related data (customer profiles, customer preferences) on this entity type. We can keep the uid property on order, but each time an order is created, we create or udpate a related customer entity. Using the customer entity, it'll be more easier to fetch profiles, but also to build a "my account" page.
Comment #6
czigor CreditAttribution: czigor at Centarro commentedAssigning.
Comment #7
czigor CreditAttribution: czigor at Centarro commentedThe attached patch kind of works. TODO:
1. Cleanup, docs, comments.
2. Fix buggy '+ Enter a new address'
3. Tests
4. PR
Comment #8
drugan CreditAttribution: drugan as a volunteer commentedAs for the functionality it looks good and quite similar to the one suggested by bojanz here but I'd like to see this solution rewritten like this:
That way we can do all the actions with one button. No +Enter new address and Edit and Return to address selection buttons. When clicked at one of the Select an address options you'd get all the appropriate fields filled in with the information for this address. When clicked at the Continue to review button the new profile is created if some of the existing profile field values were changed. If user wants just to edit existing profile without creating new one, they can go to Address Book in a user profile and do this. It is better because the history of all the address profiles and their ever used revisions are kept and managed in one place.
Comment #9
czigor CreditAttribution: czigor at Centarro commented@drugan Discussed this with @bojanz and the problem with omitting the buttons is that it's not clear for the user what's going to happen when they edit a profile: is it going to create a new profile or update the old one? So we decided to stick with the buttons.
There's one difference from the 7.x-3.x implementation: when editing a profile the profile will be updated in 8.x, and not a new one created as in 7.x-3.x (which might be a bug there).
My PR is here, still without tests but everything else is fixed:
https://github.com/drupalcommerce/commerce/pull/631
Comment #10
czigor CreditAttribution: czigor at Centarro commentedCheckout as anonymous should not offer anonymous profiles.
Comment #11
czigor CreditAttribution: czigor at Centarro commentedFixed the anonymous checkout case, tests are green on the PR.
Comment #12
chishah92 CreditAttribution: chishah92 at Valuebound commented@czigor : I applied the patch, but when i click to +Enter new address , Add new address and click on proceed to review it gives me WSOD
Comment #13
chishah92 CreditAttribution: chishah92 at Valuebound commentedThe patch is not working , as when i click to +Enter new address , Add new address and click on proceed to review it gives me WSOD
Comment #14
czigor CreditAttribution: czigor at Centarro commented@chishah92 For me it works on a clean drupal commerce 2 install. Can you provide some extra information that helps me to reproduce it?
Comment #15
chishah92 CreditAttribution: chishah92 at Valuebound commented@czigor :
I too have a clean drupal commerce 2
1) It works fine when i place the order for the first time where there is no address in the profile
2) But for the second time when you go to checkout and select Enter new address and click to proceed to review it gives WSOD
3) Also if you select the used one's and edit it and click to proceed to review, it gives WSOD
Please Advice,
Let me know if you want more information
Comment #16
chishah92 CreditAttribution: chishah92 at Valuebound commentedAlso to add on, I get below error from error.log
[Mon Feb 20 13:10:03.244281 2017] [php7:notice] [pid 2094] [client 127.0.0.1:40598] PHP 191. Drupal\\Core\\Form\\FormErrorHandler->setElementErrorsFromFormState() from core/lib/Drupal/Core/Form/FormErrorHandler.php:60, referer:
Comment #17
czigor CreditAttribution: czigor at Centarro commented@chishah92 What versions of drupal and commerce are you using?
Comment #18
Safallia Joseph CreditAttribution: Safallia Joseph at Valuebound commentedI also face the same issue as @chishah92.
I'm using Drupal core 8.2.4 and commerce 8.x-2.x
Comment #19
chishah92 CreditAttribution: chishah92 commentedDrupal core 8.2.4 and commerce 8.x-2.x
Comment #20
czigor CreditAttribution: czigor at Centarro commentedStill can't reproduce.
I did the following:
1. Downloaded the 8.2.4. tarball
2. I added the drupal repository to the core composer.json.
3. Ran
composer require drupal/commerce
.3a. EDIT: Applied the PR https://github.com/drupalcommerce/commerce/pull/631.
4. Installed drupal through browser using a standard install profile.
5. Enabled commerce, commerce_cart, commerce_checkout, commerce_order, commerce_price, commerce_product, commerce_store.
6. Imported US dollar as currency at /admin/commerce/config/currency.
7. Added a store with random values at /admin/commerce/stores.
8. Added a product with one variation at /product/add/default.
9. Did 2 checkouts with uid1, both times created a new profile.
Is there anything you did differently?
Comment #21
vasikei also can't reproduce.
Comment #22
chishah92 CreditAttribution: chishah92 commentedMy version was drupal/commerce 2.x-dev
Comment #23
czigor CreditAttribution: czigor at Centarro commented@chishah92 Mine as well. Added step 3a to the steps above.
Comment #24
Safallia Joseph CreditAttribution: Safallia Joseph at Valuebound commented@czigor
It worked. I tested using the wrong patch. Previously I tested by applying commerce-profile_select-2844920-7.patch patch. After applying https://patch-diff.githubusercontent.com/raw/drupalcommerce/commerce/pul... it works!
Comment #25
Safallia Joseph CreditAttribution: Safallia Joseph at Valuebound commentedComment #26
Safallia Joseph CreditAttribution: Safallia Joseph at Valuebound commentedThe profile selector is not working for the shipping information pane provided with Commerce Shipping Module. Maybe it must be an issue with the commerce shipping module.
Comment #27
czigor CreditAttribution: czigor at Centarro commented@Safallia Joseph For me it works. Can you describe what you expect and what happens instead?
Comment #28
Safallia Joseph CreditAttribution: Safallia Joseph at Valuebound commentedI enabled commerce shipping module and added the Shipping information pane. On checkout, the profile picker dropdown is displayed in the Shipping profile pane. But Selecting a different profile does not replace the current address summary with the new one. I used commerce shipping module version 8.x-2.x-dev.
Comment #29
czigor CreditAttribution: czigor at Centarro commentedThanks, I have fixed it. Can you test it?
Comment #30
czigor CreditAttribution: czigor at Centarro commentedComment #31
Safallia Joseph CreditAttribution: Safallia Joseph at Valuebound commentedSame PR? Was that issue related with profileSelect?
Comment #32
Safallia Joseph CreditAttribution: Safallia Joseph at Valuebound commentedYep! Its working now.
Thanks for the fix
Comment #33
Safallia Joseph CreditAttribution: Safallia Joseph at Valuebound commentedComment #34
chishah92 CreditAttribution: chishah92 commented@czigor : It works fine for both billing and shipping. But when i updated the 8.x-2.x-dev version of commerce_shipping released on feb 20. The profile selection feature doesnt appear in shipping info and also after add the new address and edit on review, the user has to fill up the shipping info form once again.
Comment #35
czigor CreditAttribution: czigor at Centarro commented@chishah92 This is because of #2852719: Temporarily stop storing shipping profiles in the address book. This has to be fixed in commerce_shipping.
Comment #36
chishah92 CreditAttribution: chishah92 commentedCool no issues. Thanks for the patch.
Comment #37
chishah92 CreditAttribution: chishah92 commented@czigor : I had created issue On editing the shipping info on review page , user has to fill up the form once again. for shipping module but it seems the issue is due to this patch
Comment #38
chishah92 CreditAttribution: chishah92 commented@czigor : also we are getting same issue for billing address, the once we save an address on review, when we click on edit we get the first address instead of the saved one.
Comment #39
czigor CreditAttribution: czigor at Centarro commented@chishah92: Not sure what you mean. Do you need shipping to reproduce this? I have several billing profiles, I select one of them, click Edit and in the form I see the selected profile's values correctly.
Comment #40
padma28 CreditAttribution: padma28 at Valuebound commented@czigor. I also got a same issue. When i click Edit link on review page, the billing and shipping information shows default first address not that we were selected the address.
This is the review page with updated address. I am clicking Edit link here, it goes to order information page
After i got the order information page, my selected address is not showing.
Comment #41
czigor CreditAttribution: czigor at Centarro commented@chishah92 @padma28 Added a fix, can you review it?
Comment #42
padma28 CreditAttribution: padma28 at Valuebound commentedThank You @czigor. The patch is working fine.
Comment #43
chishah92 CreditAttribution: chishah92 at Valuebound commented@czigor : Patch works fine. Thankyou so much for a quick update, Appreciated.
Comment #44
czigor CreditAttribution: czigor at Centarro commentedThis still needs tests.
Comment #45
csedax90 CreditAttribution: csedax90 commentedpatch works like a charm with latest dev
Comment #46
bmcclure CreditAttribution: bmcclure as a volunteer and at Top Floor commentedThis patch is working well for me too (from the GitHub PR).
For anyone using this patch and commerce_shipping, you can revert the recent temporary commerce_shipping change that stopped this from working using the patch I'm attaching here. I'm going to hide it since it's not the real patch for this issue, but it works well with the real patch for anyone using commerce_shipping's latest versions.
Comment #47
bmcclure CreditAttribution: bmcclure as a volunteer and at Top Floor commentedHere's a reroll of the patch against the current commerce release with one fix (setting $default_profile_id to NULL by default so it's not unset in case it doesn't get overridden later).
I commented in the PR so hopefully it'll be fixed there, too, but I needed somewhere to patch from in the meantime.
Comment #48
bmcclure CreditAttribution: bmcclure as a volunteer and at Top Floor commentedOne thing I've found, this may be a known issue already, but this patch assumes that "address" is the only field that's a part of the profile. For instance, I have a field_phone field which should ideally be shown/hidden along with the address field since it's a part of the profile being reused, however the field is still present when the address is hidden.
Comment #49
czigor CreditAttribution: czigor at Centarro commentedThanks @bmcclure, fixed these issues as well. They are in the PR. Tests are still needed.
Comment #50
chishah92 CreditAttribution: chishah92 at Valuebound commented@czigor : After applying the latest patch from git , when the user enters the address for the first time , The review button get's is disabled
ScreenShot :
Comment #51
bmcclure CreditAttribution: bmcclure as a volunteer and at Top Floor commentedCurrently we have a Quote checkout flow that uses commerce_shipping, but does not have billing information. The shipping profiles are using the profile select element via this patch.
As an anonymous user, when I fill out the profile form, calculate shipping, and then continue to the next step, it works fine, but I also get:
"Notice: Undefined index: pane_shipping_information[shipping_profile] in Drupal\commerce_order\Element\ProfileSelect::submitForm() (line 312 of modules/commerce/modules/order/src/Element/ProfileSelect.php)."
I'm not certain offhand if something is different on my installation causing this, or if this is an issue with the patch. Anyone else experiencing this?
Comment #52
bmcclure CreditAttribution: bmcclure as a volunteer and at Top Floor commentedSimply modifying the conditional from
(in_array($storage['pane_' . $pane_id]['mode'], ['new', 'edit']))
to(isset($storage['pane_' . $pane_id]) && in_array($storage['pane_' . $pane_id]['mode'], ['new', 'edit']))
fixes the issue for me. But I don't know why, because it seems like the profile isn't being saved then. However considering it works fine, I'm assuming something else is saving the profile in this anonymous / new profile case.Comment #53
chishah92 CreditAttribution: chishah92 at Valuebound commented@bmcclure : I tried to apply the patch #47 , but when i create new profiles , it overrides the previous one instead it should create new ones
Please Advice
Comment #54
bmcclure CreditAttribution: bmcclure as a volunteer and at Top Floor commented@chichah92 I would recommend using the patch in the GitHub PR instead of mine, as it's been updated since I posted my version. Do you still experience that issue with the latest patch from czigor's PR?
Comment #55
chishah92 CreditAttribution: chishah92 at Valuebound commented@bmcclure : Yes i have used the patch from czigor's PR , and i facing this issue as stated in #53
Comment #56
chishah92 CreditAttribution: chishah92 at Valuebound commented@czigor : The Patch Works Fine if i don't have a shipping profile, if i have the same and suppose i edit or create a new profile in my shipping information, it disables the review button.
Comment #57
itamair CreditAttribution: itamair as a volunteer commentedThis issue / task sounds really interesting, and needed in its correct implementation, but once applied the latest patch (from GitHub PR) I experience the following bad use cases:
Comment #58
bmcclure CreditAttribution: bmcclure as a volunteer and at Top Floor commentedIt's worth noting that I don't experience a couple of the issues you mention.
Comment #59
chishah92 CreditAttribution: chishah92 at Valuebound commentedThe issues that i face after the new patch is applied :
1) Initially when there is no profile, the checkout page does not show the Add new profile select list.
2) Adding two addresses for payment and shipping creates two profiles. But when we go in checkout page once again and add select add new profile for both payment and shipping information, the continue to review button is unset
3) Also when we select the same previous profiles and click to "Continue review" button in review page it shows only payment information with the first profile and When we click on Edit, one more duplicate profile get's created.
Summary : The profile save and edit are not working accordingly, needs rewrite.
Comment #60
itamair CreditAttribution: itamair as a volunteer commented@bmcclure sorry ... you are right, but I have to still confirm the following issue:
1) In the Order edit page/form, the Select & the Edit button on the Profile / Billing Information fieldset don’t work at all.
and add this, that really seems a mayor bug to me:
2) If I remove an Address / Customer Profile previously associated to a placed Order, that Order is not editable anymore and the site breaks on that attempt: exception thrown (and not catched)
Comment #61
bmcclure CreditAttribution: bmcclure as a volunteer and at Top Floor commentedThis patch is actually working pretty great on a production site for us. I'm attempting to figure out what the difference is between our setup and yours. Are you using the latest dev version of commerce and commerce_shipping?
1. For me, when there is no existing profile created, it shows a profile creation form on the checkout page as it should, which then creates a new profile.
2. I'm not experiencing this one either, but that might be because I'm also using a patch for commerce_shipping which auto-calculates shipping and controls the Continue button.
3. I'm not sure I understand what you mean about this one. For me, when I get to the Review page, it shows whichever profiles I have selected for payment and shipping, and clicking Edit next to either of them goes back to the Order Information page with the same profiles still selected. Could you explain a little more how the behavior differs for you?
Note that currently, commerce_shipping forces the shipping profile to be owned by the anonymous user, which prevents a profile select field from ever showing up for shipping (unless you revert that change with another patch).
Comment #62
bmcclure CreditAttribution: bmcclure as a volunteer and at Top Floor commented@itamair:
#1 seems like a bug that this patch may need to handle.
#2 is a known issue in Commerce unrelated to this patch, that's being handled elsewhere.
Comment #63
bmcclure CreditAttribution: bmcclure as a volunteer and at Top Floor commentedI'm actually not sure where the issue is regarding the fact that profiles should not be deletable when they're in use, but there should be an issue like that for every entity type associated with orders, as mentioned in this issue: https://www.drupal.org/node/2842579
Comment #64
itamair CreditAttribution: itamair as a volunteer commentedtnx @bmcclure ... I confirm that bug is unrelated to this patch, as it happens with the actual core/committed functionalities too
Comment #65
chishah92 CreditAttribution: chishah92 at Valuebound commented@bmcclure :
2) Even i have added the auto-calculate shipping patch(https://www.drupal.org/node/2849756), yet i face the issue
3) The senario is i have already two profiles in my address-book. When i place my order again, in checkout page i get to select any of the profiles. But if i select new profile for payment and select the old profile in shipping , the review page shows only payment information. And when i click to edit, it goes back to order information, but here one more duplicate profile gets created.
Can you guide me for which patch needs to be applied to revert it ?
Comment #66
bmcclure CreditAttribution: bmcclure as a volunteer and at Top Floor commentedJust had a quick chat with @chishah92 in Slack and realized that I'm experiencing the same issue as his scenario #3 above. I just visited my address book and there's a ton of duplicate addresses existing--I just didn't notice because all seems fine while going through checkout.
Comment #67
chishah92 CreditAttribution: chishah92 at Valuebound commentedAny updates on this one ?
Comment #68
bmcclure CreditAttribution: bmcclure as a volunteer and at Top Floor commentedI've incorporated all of the changes in the previous patch from czigor here, along with #reuse_profile options added for the ability to show a checkbox that copies another profile, and have been committing a few small changes to this functionality there.
The credit for this profile select functionality still goes to czigor, but as this is an alternate way to get this same patch if you also want to use the "Billing address same as shipping" functionality, and I may continue to make modifications/fixes to it there, I figured I would post it here as well:
https://github.com/drupalcommerce/commerce/pull/715
Comment #69
bmcclure CreditAttribution: bmcclure as a volunteer and at Top Floor commentedIn reference to my previous message, just wanted to note that I'm in the process of revamping some of how my code in PR 715 works to try and eliminate a couple issues I've been running into (duplicate profiles that keep getting created being the main one, among a couple of logic bugs I introduced when changing things around to support profile reuse). I'll have the PR updated today if anyone else happens to be using it and experiencing any issues.
Comment #70
bmcclure CreditAttribution: bmcclure as a volunteer and at Top Floor commentedI've updated PR 715, including refactoring and modifying some of the logic behind the functionality of selecting existing profiles, and reorganizing a lot of code in the element.
https://github.com/drupalcommerce/commerce/pull/715
It still works very similarly to czigor's patch, but supports the latest changes to the commerce 2.x dev branch, and I think it resolves the profile duplication issues.
Looking for feedback on this. Does it work well for others? If so, is there anything more needed in the element for it to be fully functional?
Note: This PR also includes code to resolve a related commerce_shipping issue (https://www.drupal.org/node/2852207)
Comment #71
itamair CreditAttribution: itamair as a volunteer commented#bmcclure I am not able to apply https://github.com/drupalcommerce/commerce/pull/715.patch to any actual version of Drupal commerce (both 2.0-beta6 and
2.x-dev).
Comment #72
bmcclure CreditAttribution: bmcclure as a volunteer and at Top Floor commented@itamair: GitHub seems to have some sort of really long term caching or something on PR .patch links, sadly. Several days after I updated the PR, that patch link is still outdated.
I found that using .diff instead of .patch showed me an updated link, but that might just be because that wasn't cached yet so your mileage may vary.
The PR itself applies to the latest version of commerce, it's just that particular patch link that doesn't.
Comment #73
bmcclure CreditAttribution: bmcclure as a volunteer and at Top Floor commentedNote to anyone using this patch: https://www.drupal.org/node/2872823 is related and might be affecting this functionality within the payment gateway form profile select element.
Comment #74
bmcclure CreditAttribution: bmcclure as a volunteer and at Top Floor commentedI've also updated the PR with a couple of fixes and some refactored code again. Here's a patch, since there seems to be an issue with GitHub returning the latest patch files.
Comment #75
BerdirNote that .patch and .diff is not the same thing. .patch is actually a patch-per-commit thingy that can't really be applied and results in strange merge conflicts and so on. .diff always works fine for me and I never have caching issues.
Comment #76
bmcclure CreditAttribution: bmcclure as a volunteer and at Top Floor commentedThanks for clarifying that! That definitely explains my confusion.
Comment #77
itamair CreditAttribution: itamair as a volunteer commented@Berdir @bmcclure tnx to both of you ... ;-)
Comment #78
itamair CreditAttribution: itamair as a volunteer commentedOk @bmcclure ... tnx!
the "https://github.com/drupalcommerce/commerce/pull/715.diff seem to apply correctly into the "drupal/commerce": "2.x-dev",
BUT ...
I face the following error/exception when I try to edit an existing Order (also if still in the Cart):
InvalidArgumentException: The commerce_profile_select element requires the #default_value property. in Drupal\commerce_order\Element\ProfileSelect::processForm() (line 99 of modules/contrib/commerce/modules/order/src/Element/ProfileSelect.php).
That might happen in this 2 possibile user/administrator scenarios:
A - (quite improbable, but anyway possibile) I try to edit the Order when it is still in the Cart and the User didn't attach any billing info to it yet (at Payment Info Panel);
B - I remove an existing Billing Profile and I try to edit the Order that was previously filled with that profile ... (a real possibile scenario). I understand that Drupal Commerce itself is keeping into the Order the reference to the Customer profile address, but I think this is really risky: Order info (once placed) should be independent by Customer Address Book info, and anyway persistent;
Further more I still experience this other bug (already by me mentioned in some previous comment):
C - Billing Info / Customer Address is not editable from the Order edit back-end (the edit submit button don't work).
Comment #79
bmcclure CreditAttribution: bmcclure as a volunteer and at Top Floor commented@itamair:
Regarding the exception, did you perhaps experience that same exception before using this patch? The #default_value validation was always done in the ProfileSelect element, this patch doesn't change that I don't believe.
Regarding point B above, Commerce should be disallowing the deletion of addresses, especially if they're in use anywhere. Commerce needs to implement a "soft delete" feature that simply hides the address once it's removed but allows it to still be referenced.
Regarding C, I haven't looked into that issue at all yet, have been focused on the customer UX so far. What should the actual behavior be for editing from an admin? Should an admin be able to edit a customer's profile from within the order, affecting all other orders that profile is used, when editing the order? Seems like we either need to fix the behavior of the button on that form, or remove it if it shouldn't be there.
Comment #80
itamair CreditAttribution: itamair as a volunteer commented@bmcclure
A - yes ... may be. I remember @Boianz already said there is not a logic to block the deletion of used entities. And this affects all the Drupal Commerce areas (and is a very major vulnerability and problem of the system)
B - ... but IMO Commerce shouldn't even directly reference the user profiles entities into the Order, but just add a detached copy of that (the profile address), so that it might be changed/edited in the Order backend independently (might be needed, after a Customer request ... or whatever), without the consequent update of the original profile, and its instances in different/other Orders ...
C - The Customer address / Billing Info profile Edit button don't work (in my case) in all its recurrences: both the Order edit backend (where it is present) and also in the Order information checkout pane itself ... (just placing the Cart Order)
Comment #81
bmcclure CreditAttribution: bmcclure as a volunteer and at Top Floor commentedA - True, it's pretty major in my opinion. Nothing we can do about it in this patch, however I hope that it gets some attention soon.
B - I see what you mean. I can definitely see how it could be useful, but I think it's a larger issue that extends beyond this patch to how orders are structured and processed overall. Perhaps a separate issue should be created to open this up for further discussion. I think the ability to select an existing profile is going to be useful either way, as that copying of the profile to the order could could still happen either way.
C - You're right, I'm noticing the same issue as well. It works fine in the Shipping Information pane for me but fails in the payment information pane. I'm working on some more fixes yet again for this and trying to simplify how complex it's gotten as much as possible, hoping the edit button will work in all contexts once I'm done with that.
Comment #82
bmcclure CreditAttribution: bmcclure as a volunteer and at Top Floor commentedI've been playing around with the patch code a lot again today, and I've discovered that, with the recent fix by Berdir, there might be some unnecessary code that was previously used to avoid the issues with the ProfileSelect field.
Moving back to Needs Work, because the new version is going to be simpler and work better, I think. Refactoring, tweaking, and testing, and I'll have a new patch up ASAP tomorrow.
Comment #83
itamair CreditAttribution: itamair as a volunteer commented@bmcclure tnx ... yes, I agree that on the B matter we would need a new issue open.
And I just opened it (Protecting Customer/Billing Profiles against changes)
I am minding may be entity revisions, instead of entity reference, would be a good way to go ...
Comment #84
bmcclure CreditAttribution: bmcclure as a volunteer and at Top Floor commentedAlright, I think this one is working much better:
https://github.com/drupalcommerce/commerce/pull/715
https://github.com/drupalcommerce/commerce/pull/715.diff
Comment #85
czigor CreditAttribution: czigor at Centarro commented@bmcclure Thanks for working on this!
We will need tests.
Comment #86
Eric HeydrichI installed the Patch from #85 and get the following error. On the checkout form there is no form for adding billing information.
TypeError: Argument 1 passed to Drupal\commerce_payment\Entity\PaymentMethod::setBillingProfile() must implement interface Drupal\profile\Entity\ProfileInterface, null given
Comment #87
bmcclure CreditAttribution: bmcclure as a volunteer and at Top Floor commented@eric-haydrich: What version of commerce? Can you possibly post a full stack trace from that error? Thanks!
Comment #88
Eric Heydrich@bmcclure Here's the stack trace. Current commerce dev-Version with patch 715 from #84 on Drupal 8.3.2.
Comment #89
czigor CreditAttribution: czigor at Centarro commentedI started working on writing just the tests, here they are. I think we need tests for the following:
I. Anonymous
I.1. The profile select should not appear, only the address.
I.2. Submitting the form creates a uid=0 profile.
II. Authenticated without profile
II. 1. No select, only address
II. 2. Submitting the form creates a profile for the user.
III. Authenticated user having 2 profiles already
III. 1. Select appears.
III. 2. Selecting another profile refreshes the displayed profile summary.
III. 2. a. Submitting the form does not change the selected profile.
III. 3. Clicking Edit works properly: the profile summary disappears, a prefilled form appears instead with a 'Return to profile selection without saving' button
III. 3. a. Submitting the form updates the existing profile, does not create a new one.
III. 4. Selecting '+ Enter a new profile' displays an empty profile form. No 'Return to profile selection without saving' button.
III. 4.a. Submitting the form creates a new profile.
We also need to test the same when there are two ProfileSelect form elements in the same form.
The attached patch contains I. and II. and the very beginning of III.
Comment #90
czigor CreditAttribution: czigor at Centarro commentedUpdated my PR at https://github.com/drupalcommerce/commerce/pull/631/files.
Improvements:
- Has tests
- Uses proper form API (no form_state storage and not depending on pane ids)
Known bugs:
- Clicking "Edit" does not populate form with profile field values.
- Does not create new profiles, always overwrites the last profile.
- When validation fails (because of a wrong zip code for example) the form goes back to the state where it's first loaded.
I have not run tests.
Comment #91
bmcclure CreditAttribution: bmcclure as a volunteer and at Top Floor commentedCool! Test coverage is awesome. I'm anxious to try this out, but it seems like it's broken functionally at the moment. Using the form API more closely seems like a good way to go, though I believe all 3 current known bugs were introduced with this latest patch.
I'll keep using my previous patch for the time being so that the profile select functionality works properly on our site, but I'll be monitoring this issue and will be sure to test any further patches that are at least functionally equivalent to the previous ones.
Comment #92
mglamanI'm running with this for a bit. Since maintainer, I can push to czigor's fork on GitHub for the PR. Already found an issue when country code changes, the element mode reverts back to View and not New/Edit.
Comment #93
mglamanI've pushed some fixes. More are needed. When you save from edit, the mode gets kicked back to view and all form values are gone
Comment #94
bojanz CreditAttribution: bojanz at Centarro commentedThis functionality is almost ready. I made some tweaks to the labels and moved the code to https://github.com/drupalcommerce/commerce/pull/760
Unfortunately, I ran into incompatibilities with shipping, which prevented me from committing this in time for RC1.
There's always RC2 though :)
Comment #95
zenimagine CreditAttribution: zenimagine commentedSubscribe
Comment #96
JeroenTTried the code in PR posted in comment #94.
When I go to checkout/../order_information i immediately get the following error:
Comment #97
johnhuang0808 CreditAttribution: johnhuang0808 at Fliegen commentedThe error in comment #96 occurs by the commerce_shipping module. Because commerce_shipping uses
commerce_profile_select
form element, it should be updated to follow the new array structure.Current
commerce_profile_select
type in commerce_shipping module:commerce_profile_select
type in PR#760 and code snippet is here.Comment #98
johnhuang0808 CreditAttribution: johnhuang0808 at Fliegen commentedIf PR #760 will be accepted and release in RC2 #2895339: Release Commerce 2.0-rc2, a new issue for working on the shipping profile should also be added into RC1 #2902683: Create a 8.x-2.0-rc1 release.
I can try to work on it.
Comment #99
johnhuang0808 CreditAttribution: johnhuang0808 at Fliegen commentedI enhanced the PR#760 on the commit. And it is ready for review. The discussion is also on the Github PR#760.
Comment #100
ransomweaver CreditAttribution: ransomweaver commented@JeroenT and others, johnHuang has a commit on his fork of drupalcommerce/commerce_shipping that makes this work. my composer.json looks like this:
@bojanz, can we bump this issue to major? It seems to me that profile reuse is a requirement of a full release, and therefore an RC, and there's getting to be too much code scattered in different folks' git branches. I think it needs to be reviewed and brought into the master branches so that people can get onto the RCs.
Comment #101
johnhuang0808 CreditAttribution: johnhuang0808 at Fliegen commentedHi @ransomweaver, thanks for your testing. The commerce_shipping doesn't complete, because the multiple shipment testing in commerce_shipping module doesn't pass.
Comment #102
ransomweaver CreditAttribution: ransomweaver commentedI have added a patch to this issue: https://www.drupal.org/node/2852207#comment-12250322 "Billing same as Shipping" which gets it working with PR 760 instead of the obsolete PRs 714/715
Comment #103
ransomweaver CreditAttribution: ransomweaver commentedRelated issue for Commerce Shipping: https://www.drupal.org/node/2908683
Comment #104
bojanz CreditAttribution: bojanz at Centarro commentedTook me a crazy amount of time to finish the related #2890858: Default Country for the Payment Information form not inherited which incorporated 5 different bugs.
Now to finish and merge this one.
Comment #105
JeroenTComment #106
bojanz CreditAttribution: bojanz at Centarro commentedUnfortunately, I was unable to get this code functional in time for 2.0, despite spending almost 2 days on it.
Attach my current progress.
The main problem with the PR we started with is that it changes ProfileSelect to be a FormElement, and uses a valueCallback() to process input. This causes a number of issues with the embedded entity form, so it had to be changed. However, changing that requires rewriting the entire flow.
Also, the UX of the PR did not match the UX of commerce_addressbook 3.x, opening the edit/add mode needed to hide the dropdown (otherwise we get an Address dropdown right above the Country dropdown, which looks confusing), and we needed a "Return to address selection" cancel button below it.
As others noticed, we had duplicate #name keys which didn't take $element['#parents'] into account, and clashes in $form_state itself for the form flag.
Finally, we had no revision handling that would protect entities against changes.
I fixed all of these problems, reviewed the UX with the Commerce Guys team, and the internal implementation with several core contributors. However, I ran into form issues that I could not solve in time.
Apply the patch, enable commerce_order_test, and go to "/commerce_order_test/profile_select_test_form". Click "+ Enter a new address". You will see that clicking the "Return to address selection" does nothing the first time. Under the hook, Drupal assigns the clicks to completely wrong elements. It treats selecting "Enter a new address" as a click to the Cancel button, and the click to the Cancel button as a click to the Edit button. This tells me that the process callback is rebuilding the form too early, but I don't know how to avoid that yet.
P.S. Opened the missing followup for profile copy functionality: #2910193: Allow reusing profile values from another inline form ("Billing same as shipping").
Comment #107
Jons CreditAttribution: Jons commentedIt would be great if we could add into the mix the ability to override the list of billing and shipping addresses, so as to create a set of addresses for specific users - hook_xxx_alter().
In our case - we're supporting some B2B - where a designated coordinator can set a range of addresses for linked purchasers (eg central office to deliver to a range of their shops, where someone in the shop can do the ordering, but billing goes to central office and goods go to only a pre-set address/list of addresses).
We sort out the registration and approval of the various parties and want to be able to set the appropriate addresses at checkout.
Comment #108
jackbravo CreditAttribution: jackbravo commentedThere is a PR in github by @jigish.addweb that solves this issue, and doesn't have the issue that @bojanz is describing. https://github.com/drupalcommerce/commerce/pull/796
This is the .diff pf that PR.
Comment #109
johnhuang0808 CreditAttribution: johnhuang0808 at Fliegen commentedHi @bojanz, I have no idea how to enable commerce_order_test module, since the drush and Drupal modules page don't find the commerce_order_test module. Could you help me to enable it?
Comment #110
ptmkenny CreditAttribution: ptmkenny commented@johnhuang0808 You need to enable the
extension_discovery_scan_tests
in settings.local.php on your local dev environment to see the test modules.Comment #111
ransomweaver CreditAttribution: ransomweaver commentedFor anyone who would like to get to 2.0 or the current dev and also have a functioning profile reuse with shipping/billing, here's my solution until this issue has been resolved.
The obstacle to applying PR 760 as a patch is merely that there was a small change made to ProfileSelect to check that the specified default country is actually available. The attached patch reverses that commit so that PR 760 can apply. If this check on the default country is important to you, I suggest making your own patch and putting it on gist to apply after PR 760 has been applied.
Here is some composer.json that puts this all together, including checkbox same as billing and auto-recalculate shipping:
Comment #112
Lukas von Blarer@ransomweaver wow, this comment is a life saver! Thank you!
Comment #113
Anonymous (not verified) CreditAttribution: Anonymous commented@ransomweaver Thank you for your composer patches collection.
Comment #114
Lukas von Blarer@ransomweaver Those patches combined have one issue: If I adnavce in the checkout until the review step and then navigate back to the step where my shipping information pane is, I get the following error:
TypeError: Argument 1 passed to Drupal\commerce_tax\TaxZone::match() must implement interface CommerceGuys\Addressing\AddressInterface, null given, called in /var/www/drupal/public_html/web/modules/contrib/commerce/modules/tax/src/Plugin/Commerce/TaxType/LocalTaxTypeBase.php on line 278 in Drupal\commerce_tax\TaxZone->match() (line 138 of /var/www/drupal/public_html/web/modules/contrib/commerce/modules/tax/src/TaxZone.php) #0 /var/www/drupal/public_html/web/modules/contrib/commerce/modules/tax/src/Plugin/Commerce/TaxType/LocalTaxTypeBase.php(278)
After that the shipping address fields are cleared. The user has to edit the shipping profile and re-enter all information.
I'm still trying to find out what is causing this. Can anyone reproduce this behavior?
Comment #115
bmcclure CreditAttribution: bmcclure as a volunteer and at Top Floor commentedI've been struggling with an issue using this patch combination where the shipping fields don't seem to have been saved to the profile by the time shipping is calculated. So, for example, in FedEx, the code
$shipment->getShippingProfile()->address->isEmpty()
returnsTRUE
and no rate request is attempted.Is anyone else running into this? It must be some sort of conflict between either patch versions or commerce and commerce_shipping versions it seems like, but I don't yet know why. I'm on the latest dev version of both currently.
Comment #116
Lukas von BlarerYes, I have the same issue when moving back in the checkout process. In my case this happens when moving to the login step and choosing the guest checkout option. After that the shipping profile fields are empty and I get the error I mentioned in #114.
Which patch is causing this to happen?
Comment #117
jeetmail72Hi
Please use this Drupal 8 module. I think it will full fill your requirement. This module allows reusing the existing address in the checkout form.
https://www.drupal.org/project/address_checkout
Comment #118
ransomweaver CreditAttribution: ransomweaver commentedComment #119
ptmkenny CreditAttribution: ptmkenny commentedI'm using the patches suggested in #111.
With these patches, "New address" appears to be selected by default.
I think it makes more sense to select the default customer profile address (if the customer has already made an order and input an address) since in many cases, the person probably is using the same billing address/having goods sent to the same place.
Comment #120
ransomweaver CreditAttribution: ransomweaver commented@ptmkenney #119 I'm not seeing that. I have the following:
1 - new user creates account prior to entering checkout info page
2 - user enters shipping info and cc card info, billing same as shipping or enters separate billing address
3 - completes order
4 - adds another item to cart and goes to checkout
5 - user lands on checkout info page with a) previous shipping address selected in the "select a profile" dropdown b) previous payment method selected by radio button "visa ending in ...."
I can't make any more fixes to this, as its now a dead end. See @bojanz in #106 for the way forward. This issue is at the top of the commerce roadmap issue list: https://www.drupal.org/project/commerce/issues/2913801
Comment #121
ptmkenny CreditAttribution: ptmkenny commented@ransomweaver Ok, it's an issue with my configuration then. I didn't realize your patch was a dead end because the issue was set to "Needs review". I'm adding the new issue as a related issue to make it easier for people who are searching for this to find.
Comment #122
flocondetoileFor info, the patches collection from #111 still applied well on commerce 2.2 :-)
Comment #123
Joao Sausen CreditAttribution: Joao Sausen at Dexa commented#111 looks fine, but everything here breaks the hook_commerce_shipping_methods_alter() since the $shipment in the hook will be a blank shipment, so you can't compare it to the methods in order to custom show/hide it.
Comment #124
flocondetoileExperienced similar issue as described in #123. In a custom shipping method, the shipping profile was not set on the shipment, preventing to make comparisons. Something must be wrong in the ajax request. Didn't find time to explore this issue. I finally removed patchs #111.
Comment #125
dionsj@ransomweaver: The patches you mentioned does indeed make it all work for me, with one major problem:
If I add a new field to the profile (CVR field, just a plain text field) it does not get saved properly in the form during checkout if I make modifications to it there, whereas the default profile fields do get saved (when editing an existing profile).
I would like to have a look at solving this myself, and am currently looking into how to do that, but if you (or anyone) know where the problem is likely occuring in code, that would save me some time.
Comment #126
heddnUpdating IS and seeing what the difference is between the PR in #108 and the patch in #106.
Comment #127
heddn106 applies, but 108 needs a re-roll. It also looks like the PR in #108 won't work with the current code base based on https://github.com/drupalcommerce/commerce/pull/796#issuecomment-333904322.
So that means we need to figure out the click assignment issue from the early process callback.
Comment #128
heddnI spent a little time looking into this. I was hoping I would be able to move this along, but alas, I cannot. The ajaxRefresh call returns the form and state using the fall through of element state mode = form. Because the form is built prior to calling ajax. We need to return the form as if the element state mode = view. But we don't have a good way to trigger on that because the FormBuilder (around lines 1095) defaults to setting the trigger_element to the first button in the form, even on first paint of the form. When there isn't any button pressing. I tried adding a (hidden) dummy button on the page so the first button is something else so we can trigger on a valid pressed button. But that didn't work either for some reason. I couldn't even get the form to print on refresh.
TLDR; No further progress. But I'm happy to review anyone's work that is able to fix the problem. The patch in #106 works pretty well otherwise.
Comment #129
heddnI wonder if we can use the same approach that is used by commerce_shipping's re-calculate shipping button on the shipping info checkout pane.
Comment #130
Lukas von BlarerThe patches still work great for me on Commerce 2.3, except for the issue I described in #114. Somehow the existing profile is lost if an anonymous user navigates back and forth in the checkout process. Can anyone else reproduce this?
Here is an issue that tries fixing the WSOD, but the issue remains: #2925470: Address variable should be optional in TaxZone::match().
Comment #131
JeroenTMarking as major since the follow-up issue #2910193: Allow reusing profile values from another inline form ("Billing same as shipping") is also major.
Comment #132
s.messaris CreditAttribution: s.messaris commentedThe method in #111 no longer works for commerce 8.x-2.4.
Comment #133
ericchew CreditAttribution: ericchew commentedHere is the patch from #106 applied to commerce 8.x-2.4
Comment #134
heddnGoing to start plugging away at this with my comments from #129 as the start.
Comment #135
heddnThere's still an issue with form state getting stale. But otherwise, this is much further along.
Comment #137
heddnThis resolved the issue in #135. So now we have a fully functional widget. Next is to fix up tests. If someone gets to that first, great. Un-assigning for now.
Comment #139
heddnJust fixing phpcs findings before I forget about it.
Comment #140
heddnComment #141
StijnStroobantsThanks for this great patch!
Working perfectly for the Payment information.
I tried to use it in combination with commerce_shipping, but the following error occurs:
Maybe this is a commerce_shipping issue. This evening I will try to debug further.
commerce 8.x-2.4
commerce_shipping 8.x-2.0-beta4
Comment #142
heddnI cannot seem to get phantomjs to run and store my user session, so local running of tests are falling on their face. Hopefully the testbot is more merciful. Here's an attempt at getting the first anonymous test to pass.
Comment #143
heddnComment #146
StijnStroobantsI fixed the error in #141 by patching the commerce_shipping module. (https://www.drupal.org/project/commerce_shipping/issues/2948954)
Now I can use the functionality for both fields. Only issue I have now is that the AJAX-functionality for editing the profile is only working on the first widget on that page.
Comment #147
heddnComment #149
heddnSo, after some actual attempts to implement the above patch. I think it won't work. See https://twitter.com/lucashedding/status/971083165895970817
Let's see if we can do this another way.
Comment #150
heddnNot sure how helpful the interdiff is going to be. This is an entire new approach by loading the entity add and edit forms in a modal. The UX is mostly OK. Tests are probably not passing yet. But things work and we get around the previous buggy problems.
Comment #152
ericchew CreditAttribution: ericchew commentedNice work Heddn. I've made a few additions
One issue I have found is when editing a profile, the user can choose to delete the profile. After deleting, the redirect breaks and you are shown AJAX command info instead of going back to the checkout page.One issue I have found is that in checkout, when you already have an existing credit card, then click create new credit card, the add and edit buttons destination parameter is messed up:?destination=/checkout/1/order_information%3Fajax_form%3D1%26_wrapper_format%3Ddrupal_ajaxComment #153
JeroenTComment #155
ericchew CreditAttribution: ericchew commentedChanged the AJAX wrapper to wrap the entire element so that the edit button is updated properly when switching profiles. I modified the destination parameter to get the path a different way so that AJAX parameters aren't included.
Comment #156
heddnHere's some more refactoring and accounts for anonymous cart checkout. For anonymous, we cannot really use a select widget as we cannot load a list of previous profiles.
Comment #157
heddnAnd here's the patch.
Comment #159
Eric HeydrichThe last patch didn't work. I get the following error:
InvalidArgumentException: The commerce_profile_select #profile_type property must be provided. in Drupal\commerce_order\Element\ProfileSelect::validateElementProperties() (Line 90 in root/web/modules/contrib/commerce/modules/order/src/Element/ProfileSelect.php).
Comment #160
ericchew CreditAttribution: ericchew commented@Eric Heydrich -- The reason you are getting that error is because you have something that is depending upon the old ProfileSelect element. If you use the patch in #157, you'll have to patch everything that utilizes the ProfileSelect element...such as the shipping information CheckoutPane (Issue # 2948954)
Comment #161
Alluuu CreditAttribution: Alluuu commentedHi,
This is my proposed approach for doing the reusable profiles in a way that shouldn't breaking anything, even as a temporary solution until something better comes along.
Basically I'm just adding a select options field to the element with an ajax onChange callback and would see that the fields could be pre-populated with corresponding profiles values, if any of the values were to be changed, a new profile would be created anyway and just in case for easier UX understanding I'd also add a "add new" option to the select list options, which would empty the fields but it shouldn't matter.
The code isn't finished, I still need to implement the changes to the actual form fields. As there seems to be an increasing demand for some sort of solution to this, I'm hoping this could be of any help or a considerable viable temporary solution. If it's of any interest I could finish the rest tomorrow ( I hope )
Best,
Alari
Comment #162
Alluuu CreditAttribution: Alluuu commentedSorry, I seem to have forgotten to declare a correct root folder for the patch, I'll upload again.
EDIT:
Apparently I don't know how to create a decent patch file :(
Comment #163
drugan CreditAttribution: drugan as a volunteer commented@Aluuu
Do this from inside commerce folder:
git diff > my.patch
Comment #164
heddnRe: the continued work for #157, I wonder if we can drop the CRUD operations entirely to add or edit a profile. What I saw on amazon.com was only a select list. It was empty if there were no addresses. Then at the top of the page, there were links to the profile section of amazon to add/edit/delete (manage) profiles. We don't have to go that extreme, but we could copy it spirit.
What if we did the similar. Just print a select list of all profiles, then had another link widget or something to link folks to the profile collection route to manage profiles. This link wouldn't be apart of the select widget at all. But would be listed on the billing and shipping steps of a checkout. If the user is anonymous or doesn't have any profiles already created we could still give them the initial profile form directly on the page. But if they do have profiles, then just display them in the select list. And leave up to the user to find the link on the page to manage profiles if they want to add anything more than add the first/initial profile.
I arrived at this just now when working on a multiple destination shipping solution. The page looks UGLY when I give them a select list /and/ the buttons to manage their profiles. I'd much prefer just a simple select list and links at the top of the page to manage my profiles.
Comment #165
heddnHere's a screenshot of how messy it looks. I want to hide the edit/add buttons. And move them to the top of the page.
Comment #166
ChandeepKhosa CreditAttribution: ChandeepKhosa at 2Toucans commented@heddn #164 'I wonder if we can drop the CRUD operations entirely to add or edit a profile' I agree this would be a better user experience. Borrowing from Amazon is bound to be a good idea :)
I've added the 'Needs usability review' tag to this issue as I think it would be helpful.
In the meantime if anyone has a little time to move the add/edit buttons higher, and write a patch that would be awesome!
Comment #167
harings_rob CreditAttribution: harings_rob at Harings.be for maxplus commentedNot sure if dropping it would be a good approach. Maybe we can give 2 options?
Anyway a decision needs to be made at some point, but I often order something online, and I would not want to get redirected away from the checkout to modify my address.
As for the patches: 2844920-156.patch works great, but I found today that the profile type is hardcoded.
This should instead be taken from to profile type field.
I have rerolled te patch on top of latest dev. And I have also removed a hardcoded profile bundle from the widget.
Comment #168
harings_rob CreditAttribution: harings_rob at Harings.be for maxplus commentedThat interdiff doesnt look right.
Comment #169
heddnHere's a quick hide of the operation buttons for billing. It isn't intended for use anywhere yet. Just to get a feel how hard it would be to hide it. It seems pretty easy actually.
Comment #170
heddnComment #171
heddnHere we fix up the first of the tests. More to come later.
Comment #172
fotograafinge CreditAttribution: fotograafinge commentedI keep getting these errors:The website encountered an unexpected error. Please try again later.InvalidArgumentException: The commerce_profile_select element requires the #default_value property. in Drupal\commerce_order\Element\ProfileSelect::processForm() (line 80 of modules/contrib/commerce/modules/order/src/Element/ProfileSelect.php).Not sure if it's caused by this patch (#171) or by the related issue #2948954: Commerce_profile_select reusable profiles=> my mistake, solved
Comment #173
heddnAnd here we are a little closer on tests. I think they are pretty close now. If someone with more FunctionalJavascript experience wanted to look at things, this might be able to wrap up pretty easily.
Comment #175
heddnI'm almost able to get tests passing. But the modal button is off-screen and isn't clickable in phantomjs.
Comment #177
heddnComment #179
Joao Sausen CreditAttribution: Joao Sausen at Dexa commentedI'm still following this issue, thanks for all the work heddn.
Comment #180
heddnLooks like #2831506: Minimal profile disallows modal AJAX tests under JavascriptTestBase is what we are facing here.
Comment #181
heddnIt was green on local. Let's see if we can make it pass on the bots too.
Comment #183
heddnComment #185
heddnComment #186
heddnComment #189
heddnSomething in here is causing the tests to hang. I'm moving on to other stuff, perhaps someone else with a new set of eyes can figure out what is going on.
Comment #190
heddnHere's an option to hide the profile preview. Combined with hiding the operation buttons, it makes it possible to do the following:
No further improvements on tests. I'm blocked.
Comment #191
drugan CreditAttribution: drugan as a volunteer commented@heddn++
You are a real fighter! Great work!
the only notice is that I have a profile marked as default but when the checkout payment information pane is loaded I see another profile preselected which is not default but just the very first in the list of my profilesUPDATE: sorry it was my custom hack on profiles now it works as expected :)
Comment #192
F_D_E CreditAttribution: F_D_E commentedThere is an insecure direct object reference on selected profile which is not trapped by Drupal permission.
The access right to the selected profile id (=selected_option) is not checked at the server side.
Finally, a malicious user can easily access to personal data about others users.
Comment #193
alexpottThis needs to move to the FunctionalJavascript folder. And then the test runner should work again.
Comment #194
heddnLet's see how that fares.
Comment #196
heddnHmm, let's see if this fixes any failures. I know there will be some, just curious what is left.
Comment #198
heddnComment #200
rwanthIn order for this to be mobile responsible, we should not be using a modal for new profile entry. "Enter a new profile" should be part of the "Select a Profile" dropdown, which should trigger an inline form when selected.
Comment #201
heddnWhile doing an inline form would be ideal for many reasons, it isn't necessary for mobile responsive experience. For the last hundred or so comments, we've been basically saying that doing inline isn't really working.
Here's a recent screenshot of the modal on a simulated S5 display port. Doesn't look bad to me. Are you seeing it differently on your site?
Comment #202
drugan CreditAttribution: drugan as a volunteer commentedin my mind we should return to the solution suggested in the parent issue:
https://www.drupal.org/node/2841813#comment-11857677
How it works:
on a payment (billing, shiiping) information checkout pane user sees a regular profile form automatically populated with values from a profile which is marked as default OR, if there is no default profile, the very first profile in the list of a user profiles.
In the dropdown user may choose another profile OR, clicking at the - New information - option get an empty profile form with address fields for a default country. Obviously that changing a country or a profile with different country will change address fields which are specific for this country.
No any additional buttons are needed. Clicking at the Continue to review button user will either:
I've re-roled the patch from the above link and use it on my dev site right now and even have a thought to create a module for the feature if the current issue will not be fixed in a reasonable amount of time.
Comment #203
rlangille CreditAttribution: rlangille commentedIMHO modals are not the way to go with this. Mobile + modals are always a bad combination, and I can't see them playing well with usability or accessibility.
What we have here essentially an entity reference field with an inline entity form and we're saying that's a dead end? I find that rather hard to believe considering all of the places these have been successfully used in the past and all the cases they cover.
If we need to move this up to a higher level and resolve this in the flow as Bojanz mentioned, or by changing how the data is used and referenced, then my vote is for that over introducing more complexity in the UI here.
Comment #204
enorby CreditAttribution: enorby commentedWhen going through checkout as an anonymous user, shipping rates are not being displayed when hitting the "Recalculate shipping" button. Rates are returned correctly when trying to submit the form without first fetching the rates, but attempting to fetch rates returns nothing and submitting the form gives the "An illegal choice has been detected" error.
Patch #162 worked fine. I installed patch #167, updated the commerce_profile_select form element in ShippingInformation to reflect the changes in ProfileSelect, and that is when this problem was introduced for me.
Comment #205
jonnyeom CreditAttribution: jonnyeom as a volunteer commentedFor patch #198, I am seeing a bug where when I change the Shipping Profile, the shipping rates are retrieved with the initial/default shipping_profile that was loaded when the page loaded.
Example.
1. Order_Information pane loads with profile 1.
2. Shipping rates retrieved for profile 1.
3. User chooses profile 2.
4. On the Ajax call, shipping rates retrieved for profile 1.
If I press "Go to Review", the rates for profile 1 is used.
However, If I then go back to the order_information pane, the rates are now correctly loaded with the profile 2 rates.
Thoughts guys?
Maybe, the form_state is saved in a strange way for these multi pane forms?
Comment #206
heddnLet see if chromedriver is any better than phantom.
Comment #208
heddnAttaching an animated gif of the usage of the form element in use. Hopefully that will make it easier for driveby reviews.
There's lots of place we use profile_select, so instead of getting all the tests to pass green again, let's be sure we are doing the right thing...
Comment #209
heddnThis passed tests on local against 8.6. Let's see how the testbot likes things.
Comment #210
heddnre #204, I think that is related to #2971164: Shipping method with condition (address) not visible for anonymous user.
Comment #212
heddnComment #214
heddnSeems we lost some element validation. Not sure where that got dropped. Let's see what happens when we re-add it.
Comment #216
rgpublicHm. I'm using patch #214 and the shipping bugfix mentioned in #100. I have a profile selector for billing. But I don't see any profile selector for shipping. It used to work back then, and now that I've come back on a project and updated Drupal commerce, got new patches etc, the shipping selector just disappeared :-( Any hints on how to disentangle this maze of interdependent patches is much appreciated...
Comment #217
Lukas von BlarerDoes anyone have a patch compatible with #2852207: Billing same as shipping?
Comment #218
s.messaris CreditAttribution: s.messaris commentedThe patch in #214 does not get the default billing profile correctly, because of an error in the sorting logic. Here is a fix.
Comment #219
s.messaris CreditAttribution: s.messaris commentedChanging all uses of
self::
tostatic::
in ProfileSelect class to allow classes extending it to override specific functions without problem.Comment #220
Lukas von BlarerLets run the tests.
Comment #223
mattjones86The latest patch doesn't seem to apply to commerce 2.8, is it easy re-roll the patch for that?
Comment #224
mattjones86Nevermind, checked it myself and it wasn't a major change. 8.x-2.x-dev compatible patch attached.
Comment #225
mattjones86Another go... hopefully tests pass on this one.
Gah, somehow I missed the AdminOrderTest file.
Comment #226
mattjones863rd time lucky... if this doesn't work I'll wait for someone who knows a bit more about the patch to look at it!
Comment #227
m.ioannidis CreditAttribution: m.ioannidis commentedPlease, see revised #219 patch.
It properly loads the profile using revision id instead of profile id.
Comment #228
KittenDestroyer CreditAttribution: KittenDestroyer as a volunteer commentedTaking as base #219 patch I've added fix for using default value if no user input present. The problem was that when you go back from review to order information, there was other value selected.
Comment #229
KittenDestroyer CreditAttribution: KittenDestroyer as a volunteer commentedUpdating status
Comment #230
mglamanLet's pause this for a minute. Since #214 patches have been added but don't apply. No interdiffs. And no discussion on fixes usability problems (modals causing page refresh).
Comment #231
mglamanThe primary goals of this issue were to implement the user experience developed in the 3.x branch of the Addressbook module. See https://www.drupal.org/files/project-images/Capture%20d%E2%80%99e%CC%81c...
We ran into Form API issues due to the way AJAX is handled on buttons vs non-buttons, leading to weird states after AJAX operations. That's about where we ended up in #106.
This patch brings us back to #106. Amazon is not a good example of UX, and core's current support for modals and their styling is lacking. We cannot have a modal cause a page refresh during checkout. With this patch we have a select list which shows the profile, allows clicking "edit" to modify values, or changing to create a new profile.
Instead of relying on Form API to change from "view" to "edit" and "cancel", we can just use some client-side JavaScript to show and hide elements. In hindsight, I wish I would have stepped back enough previously to recognize this.
Here is a patch which builds on #106. Continues revision support, and the UX.
Remaining items:
- Verify testProfileSelectAuthenticatedEdit passes (local was fighting over it, wouldn't complete)
- Test revision support
- Test revision support in context of a placed order
- MOAR TESTS!
Here is a crude demo.
Comment #233
mglamanThis patch fixes some fatal errors which were causing the failures. To ease any breaking changes, I want to still support
#default_value
but also#profile_type
and#profile_uid
. This patch is still a blend, but more test fixes.I'd also like to use getValue() from the form state properly, but persist
#profile
to prevent breaking changes (ie: shipping.)If curious for interdiffs, see https://github.com/mglaman/commerce/commits/addressbook3x. I'm making various commits along the way and posting progress here.
Comment #235
mglamanThis should fix the order admin test.
Next things:
- Test revision support
- Test revision support in the context of a placed order
- Look into
@todo AJAX on the order item IEF shouldn't cause read mode on profile.
- Implement
profile_type
andprofile_uid
instead of just relying on default_value (and update commerce implementations) (and implement BC handling in validateElementProperties?)Comment #236
jonnyeom CreditAttribution: jonnyeom as a volunteer commentedThanks for the patch @mglaman.
Here are the issues I am currently seeing with the latest patch (235)
Side Note: If I proceed to the next Checkout step and come back to this step, The Shipping is recalculated with the proper address. Therefore, I am pretty sure that the Shipment object is not being properly updated before being sent to the Shipping Rate Plugins.
I feel like some update should be getting triggered between the following lines: web/modules/contrib/commerce_shipping/src/Plugin/Commerce/CheckoutPane/ShippingInformation.php:277 - 282
I have not been able to confirm.
Overall UI/UX wise, no problems.
Comment #237
mglamanjonnyeom thanks for testing! I have not tried testing it with shipping at all, yet. So I appreciate that.
I think this makes sense, user input is our problem here. The form rebuilds the state from user input.
Ugh, I'd wish it didn't save changes, but preserved them in the user input. So if we fix the first, we should fix this one as well.
🙌🙌🙌
Comment #238
Jons CreditAttribution: Jons as a volunteer commentedHi
I tried creating an order with an existing profile with address, then repeating but clicking Edit and changing the customer name and completing the order.
The new values get saved on the profile and this gets assigned to the order. The changes also appear on previous order.
Would it be better if orders reflect the address at the time of the order - rather than an updated version?
Could revisions be used internally to achieve this, or should we stop edits?
I can see that revisions are used in the code but dont seem to be in this scenario?
Comment #239
Jons CreditAttribution: Jons as a volunteer commentedWe have a use-case where only a pre-defined set of profile addresses should be used by some users which would be pre-assigned to user, so we would not want the _new option or the edit button.
I tried to influence via HOOK _field_widget_form_alter() or HOOK_field_widget_multivalue_form_alter() or HOOK_form_alter() but could only see single address widget data not 'commerce_profile_select'
Comment #240
rwanthMinor UX problem, the edit profile button always appears immediately after the first field. If a site config has additional fields on the profile, like a telephone number, it appears after the button. Still functions properly.
Comment #241
mglamanNot sure why you hid my patch in #235. Putting it back.
I thought there was a weight here! Specifying the weight to 100 should fix the reported UX issue.
Comment #242
mglamanWorking on this during downtime at Decoupled Days.
Comment #243
mglamanHere's an update.
Included is a shipping patch as well, we'll need this fix and this patch to address #236
Adds fix reported in #240
These still stand:
- Test revision support
- Test revision support in the context of a placed order
- Look into
@todo AJAX on the order item IEF shouldn't cause read mode on profile.
- Implement
profile_type
andprofile_uid
instead of just relying on default_value (and update commerce implementations) (and implement BC handling in validateElementProperties?)Comment #244
mglamanOkay, so this adds a lot of test coverage to the element, especially revisions.
Next step is
- Test revision support in the context of a placed order
- Look into @todo AJAX on the order item IEF shouldn't cause read mode on profile.
Comment #246
mglamanphpcs + functional javascript test
Comment #247
mglamanUpdated OrderAdminTest to test the profile select.
Comment #248
mglamanActually I want a test for #238
We do use revisions. I just realized what the problem is. If someone makes a change within the profile select element we need to flag it as a new revision on save. We're respecting the editing of old revisions, but nothing to ensure a new revision is created after edit and ensuring previous data.
EDIT: I looked at the patch and we have:
We need a test, though.
The profile select element isn't respecting permissions at this time. We can tie in permissions for the create and edit functionality.
Comment #249
s.messaris CreditAttribution: s.messaris commentedHi, thanks for your work @mglaman looks very nice. Only think I don't like is the getting of available and default profiles. If that happened in a different method, someone that needs custom logic for removing the option for new address or excluding some profiles could do that by overriding that function instead of the whole
processElement()
.Comment #250
Jons CreditAttribution: Jons commentedthanks
I'll test new patch in general
I did come across the setting for revisions to be switched on/off here: admin/config/people/profiles/types/manage/customer and mine was off (the default), now on
Comment #251
codedrill CreditAttribution: codedrill commentedIt works well but i get javascript error on form submit if i do not change the dropdown value. The previously saved address is selected by default.
It's because the address fields are required but there is no form to fill. So browser tries to highlight the required fields but it could not find them.
If i select Enter new address and then select back the saved address, it works fine.
Comment #252
mglamanOkay, #249 I moved some of that logic to a method. There are some things we should abstract to make customizations easier.
Huh! For #250 I thought we had this enabled. This patch will do that.
For the JavaScript console errors in #251. That's going to be tricky. QuickEdit always has random errors when rendering elements and logged in as an admin. But the required element and focusing will need a trick, possibly. The usual workaround is #ajax, but we can't do that here.
Comment #253
bojanz CreditAttribution: bojanz at Centarro commentedFor QuickEdit we just need to make sure it's disabled for products. We already applied a similar trick to product variations.
Comment #254
mglamanHere is a patch which addresses #249 and #250.
I need not debug #251, yet. That is just some normal JavaScript trickery we'll need to do.
This will fail testing because of revisions. ::testElementSubmission tests that editing an existing profile creates a new revision. However, at least in the kernel test, the revision IDs are the same. I don't know if this would work properly in a Functional test or not, I did not have time to write one. Kernel test was just faster. In \Drupal\Tests\commerce_order\FunctionalJavascript\ProfileSelectTest we should verify that anytime we edit a profile from profile_select a new revision is generated for those changes.
I'm going on vacation this week (and leaving the laptop behind!) so here is a summary:
Thank you everyone who has reviewed and tested the work thusfar. We're getting pretty close.
EDIT: For the hidden I am assuming we could do this in the JavaScript. I think the :hidden selector will grab them since the parent is hidden.
Comment #256
mglamanBack from vacation and will continue on this.
Comment #257
ayalon CreditAttribution: ayalon commentedI cannot apply the patch in #254 to Commerce 2.8.0, also not applicable to commerce 2.x-dev. For which version is this patch meant?
Comment #258
ayalon CreditAttribution: ayalon commentedOk. I was able to apply the patch to drupal 2.8.0. My bad.
My major question is: Did you take into account this issue?
https://www.drupal.org/node/2852207
At the moment I don't see a possibility, how these to implementations can work together.
But to have the same shipping address as the billing address is the most common usecase.
Do you think of solving this aswell?
Comment #259
mglamanCleaning up displayed files, so that the issue summary shows latest patch and required shipping patch.
Comment #260
josephdpurcell CreditAttribution: josephdpurcell at Digital Bridge Solutions commentedAttached takes patch 253 and changes the permission name from "create profile" to "create customer profile". The reason for this, is I was only seeing administrators as having the ability to add a new profile during checkout.
Comment #261
josephdpurcell CreditAttribution: josephdpurcell at Digital Bridge Solutions commentedWhoops! I didn't create the 260 patch correctly. Re-uploading as 261.
Comment #262
mglamanFixed the revision test. It was a written test error, which was caused the form to default to a new form each time instead of having a proper default value.
Copying my todos
We need to review why a new revision is not being created whenever we save. We flag it as a revision, but the revision IDs always matchComment #264
mglamanWhoops. bad patch before. This new patch adds the JavaScript trick for handling
required
fields, and ensuring fields modified then canceled are restored to their original value.When the form is hidden, the element's JavaScript should toggle the required filter, much like the core Form API state does for required.Comment #266
mglaman* Fixes tests broken by permission checks
* Tests that editing a profile, changing a value, and then cancelling does not cause unexpected data change.
Comment #268
mglamanAdd a guard against deleted user. Also, found some permission bugs when editing an order.
Remaining task is FunctionalJavascript of editing a user to test the edit/select functionality.
Comment #269
mglamanUpdated patch which tests the order create/edit within FunctionalJavaScript.
Comment #271
mglaman🤦🏼♂️ totally forgot to annotate my new test, hence the failure.
Comment #272
mglamanComment #274
s.messaris CreditAttribution: s.messaris commentedI see some leftover console.logs in profile-select.js.
Also, when having a profile field that references an old revision, I can't save any changes to the entity, I keep getting an error
I am not sure this is not a problem with my setup though, I will investigate it further and comment again if I find something.
Comment #275
s.messaris CreditAttribution: s.messaris commentedI was able to reproduce the problem I mentioned in #274 on a clean installation. This is a serious bug, since it completely breaks order administration when an address is an old revision.
Steps to reproduce:
Comment #276
mglamans.messaris thanks! We have some tests around this - but not in functional (I think only kernel). We'll need toupdate the tests!
Thank you for testing.
Comment #277
mglamanFound another bug where the Address module causes a loss of data on "edit"
This causes the address to clear our its values in the following scenario
* User has an existing payment method
* User opts to add a new payment method
* User's default billing profile displays in READ properly
* Click edit and the address is blank
* Change to "+ New" and then back to default address
* Address is populated (because #value is OK?)
Comment #278
mglamanThis patch is a quickly addresses #274 and #277.
I need to write a test for #275 and #277.
Comment #279
mglamanWow, that broke a lot! Removing.
Comment #280
s.messaris CreditAttribution: s.messaris commentedI found another issue here, if I go through an anonymous checkout, in a flow where there is a step after the one that contains the billing info, and go back I cannot edit the info, since anonymous users don't have the "edit own profiles" permission. Giving that permission to anonymous users really isn't an option, since with it they are able to edit any anonymous user's profiles.
Comment #281
mglamanre: #280. Shoot. If this is the case, I would rather remove any permissions on the element and try to figure that out later.
Comment #282
mattjones86Please ignore -- I thought I had another issue here but it was actually with #2948954: Commerce_profile_select reusable profiles #27
Comment #283
s.messaris CreditAttribution: s.messaris commentedHi there, any progress on #275 and/or #280?
Comment #284
s.messaris CreditAttribution: s.messaris commentedI think simply trying to save the order in the end of OrderAdminTest::testEditOrder() will brink up #275. Here is a patch that should fail on that.
Comment #285
s.messaris CreditAttribution: s.messaris commentedI don't know why #284 test passes, the method I described in #275 reproduces the problem. :/
I think the attached patch solves both #275 and #280, but we are still missing tests so I am leaving this as needs work.
Comment #286
andyg5000It looks like the last few patches hose shipping recalculation (even with unpatched commerce_shipping-2.xdev).
The rate collection methods fire and the shipment is empty.
Comment #287
heddnSee the latest patch in #2948954: Commerce_profile_select reusable profiles. That should fix things.
Comment #288
andyg5000@heddn was correct. Please disregard my comment in #286.
Comment #289
_dcre_ CreditAttribution: _dcre_ commentedThere seems to be a bug in the latest patch (#285)
It looks like, it clones the default (selected/saved) profile and clears its field values, or something similar.
This process has a flaw, at least on my implementation.
I have appended the default commerce 'customer' profile with a paragraph of mine.
This paragraph contains some extra fields i need to have on the profile.
Problem is that when i hit the '+ Enter a new address' select option, the new profile that is created somehow preserves the same target_id of my custom paragraph, as the already selected one.
This has as a result that although multiple profiles can be created, they all reference the same paragraph.
When a new profile is created, a new paragraph should also be created and its new target_id should be applied on the new profile.
Comment #290
trickfun CreditAttribution: trickfun commentedthe patch 285 cannot be apply to 2.x-dev current version.
why is so hard to complete this feature?
and why this form element doesn't work with ajax_callback?
Comment #291
andyg5000After reports of address values being overwritten unintentionally, I've determined that the patch in #285 doesn't work for the admin order interfaces and can lead to data loss. Looks like the JavaScript that fires to copy values from profile select option list to the actual form elements is only built to work on checkout forms. Will report back if I can figure out a fix for that.
Update:
The issue on admin forms appears to be in FAPI and related to #parents being set properly before the following is called.
If you expose the 'profile_form' element on the admin order edit form (remove visible-on classes), you'll see the form values don't update. No problem on checkout form.
Comment #292
andyg5000Not sure this is the best approach to fix the issue, but gets to the point and solves #291. Leaving as needs work.
Comment #293
andyg5000Comment #294
andyg50003rd time is a charm. Realized the previous patch only worked on `shipping_profile`
Comment #295
corentin.crouvisier CreditAttribution: corentin.crouvisier commented@andyg5000 #294 patch not working on last drupal commerce (dev-2.x f75e72f)
Thank you for your job.
Comment #296
andyg5000I just rolled it on 89416888f03e18063b4a665a9b2ae54f2d061745. Should be fine if you fetch latest.
Comment #297
corentin.crouvisier CreditAttribution: corentin.crouvisier commentedI confirm that patch is well applied on 8941688.
I have a required field form error for all my address fields in combination with commerce_stripe when I choose a new payment mode credit card (even though I already have saved cards that are offered) if I insert card informations and I submit form without changing default address.
That returns a required field form error message for all my address fields.
Edit:
The commerce_profile_select render element is empty for all address fields exept the country but extra fields from profile are completed.
If I change the billing_profile and after I select the default profile, address is entirely loaded.
Comment #298
nickvanboven CreditAttribution: nickvanboven as a volunteer commentedTested the patch and if everything is filled correct it goes well, The problem i have know i have 2 profiles that are already in use, if i chose to create a new 1 and forget the phone number field for example or postcode i get an error which is good but the form goes away.
the select input stays on: + Enter a new address and i get an overview of my default address, if i unhide the form i see the values and the errors
Looks like the code below could be the problem it never returns a selected profile not initial and not on form submit
Comment #299
Thomas CysThe latest patch does not work with commerce_shipping and when you registered a new account during checkout. You constantly get an "Illegal choice has been detected" error and cannot proceed.See https://www.drupal.org/project/commerce_shipping/issues/2948954#comment-...
Comment #300
bojanz CreditAttribution: bojanz at Centarro commentedThe many problems encountered around completing this patch forced us to rethink our general approach.
The result of that has been #3003121: Add an InlineForm API, stop using complex form elements, which just landed after a full week of work.
That means that we won't use a form element anymore, we'll use an inline form plugin.
I will be rebuilding the patch from this issue in the upcoming days.
For now, I suggest staying on a patch that works for you, coupled with Commerce 2.11 or an older -dev.
Two other related issues:
#3007444: Add a way to duplicate an entity bundle
#3007446: Introduce a helper for renaming an entity bundle
This will allow us to split the "customer" profile type into "billing" and "shipping" profile types, for more flexibility.
Comment #301
Joao Sausen CreditAttribution: Joao Sausen at Dexa commentedgreat to hear @bojanz, thats a good decision.
Comment #302
s.messaris CreditAttribution: s.messaris commentedWon't this make work on https://www.drupal.org/project/commerce_shipping/issues/2852207 harder? It is a much needed feature that has already been floating for quite some time.
Comment #303
rgpublic> This will allow us to split the "customer" profile type into "billing" and "shipping" profile types, for more flexibility.
I also stumbled upon this sentence and I wonder what consequences that might have. Apart from the "Use shipping as billing (or vice-versa)" functionality that s.messaris mentioned, there might e.g. also be issues with the customer area / address management. Currently, each customer can easily manage their stored addresses, delete some of them, add new ones. I think it would be very inconvenient to have completely separate management for billing and shipping addresses with lots of duplicated addresses, because they usually tend to overlap, right?
Comment #304
mglamanRE: #302: This actually allows us to follow the paradigms from 1.x, where this is implemented. So I do not think it makes it harder.
RE: #303: That is a valid question. But two types also solves the question of "How do I ask for a shipping phone number?" Most users seem to want it as a shipping profile field versus a shipment field.
Comment #305
rgpublicHm, although I understand why people like to attach a phone number in case of shipping problems etc, there are obviously other solutions to that (adding it, as you said, as a shipment field, or simply using the billing field, because AFAICT usually people tend to have one phone number for all questions etc.). OTOH, completely separating the profiles makes the address management a mess in terms of usability. Lots of duplicated addresses etc. To be honest, I guess the best ("gold standard") solution would be: Allow profile fields to be configurable (show only for billing, show only for shipping, show for both). This would bring together the best of both worlds...
Comment #306
drupalnesia CreditAttribution: drupalnesia commentedDoes this patch care about "Allow multiple profiles=No" in Profile settings?
If user set Single Profile then Commerce must automatically fill the form, no need to choose any Profile.
Comment #307
matthieuscarset CreditAttribution: matthieuscarset as a volunteer and commentedThanks everyone for the effort put into this issue.
I have gone throught the whole comment threads and I also try to read the attached patchs.
It is unfortunately not clear which patch(s) is(are) necessary as of today.
Could you please clarify?
Also, I'd like to understand why are we updating the Form values throught Javascript ?
This doesn't sounds a bullet-proof solution to me, as we can't rely on CSS class to populate the form... can we?
I think we need to consider that developers might need to use the `commerce_profile_select` in their own modules/own checkout panes with custom markup.
Comment #308
h1nds1ght CreditAttribution: h1nds1ght commentedHi,
I also used the old patches for reuse profile and the same as billing checkbox. Unfortunately they didn't apply anymore on commerce >= 2.4. Therefore I tried one of the new patches for both, but they didn't work - my site crashed. Since I have code which is dependend on these patches I had to made a decision. Either keep it or remove it. I decided to give rerolling a chance and finally I got these old patches to work with commerce >= 2.10. What I did was increasing the commerce version step by step and crossing fingers patches would apply. If not I took a look on the reject files and implemented the non applying parts again. In my first tests everything works as before with commerce 2.11. I have included them with composer this way:
I hope this helps someone else having the same problems.
Comment #309
s.messaris CreditAttribution: s.messaris commentedHi all, any news on this?
Comment #310
csedax90 CreditAttribution: csedax90 commentedis there any news about the implementation? we have to launch an ecommerce shortly and we would like to understand whether to apply the latest patches or wait for the final implementation...
Comment #311
themic8 CreditAttribution: themic8 commentedIssue: that after anonymous users check out and admin updates the order. The billing address disappears. Only for anonymous users/orders. Each time the order is saved a new billing profile is created. This does not occur with users that have an account or non-anonymous users.
Current commerce 8.x-2.11 version. Not sure which version the project started on.
Patches:
Looks like this might be a different issue. Each time I save the order the billing profile gets set:
$element['#value'] == '_new'
Comment #312
jwwj CreditAttribution: jwwj as a volunteer commentedI could not get the patches suggested in #308 to apply on commerce 8.x-2.12 or 2.x-dev. Tried applying them all at once and one by one, but none of them got applied. The combination of my other dependencies blocks me from installing 2.11 and trying to apply these patches to see if the change happened between 2.11 and 2.12 ...
Is there some command that can tell me more details about why a patch does not get applied, what the conflict was?
Comment #313
s.messaris CreditAttribution: s.messaris commented@bojan, is this issue being worked on, now that the issues you mentioned in #300 have all been solved?
Comment #314
Jons CreditAttribution: Jons commentedSee https://www.drupal.org/project/commerce/issues/3022850 for follow-up progress
Comment #315
h1nds1ght CreditAttribution: h1nds1ght commented@themic8: Not sure if these patches ever catched the case of reusing anonymous profiles, since a profile should be always connected with a uid. I always used it with authenticated users and this still works in 2.11
@jwwj: The patches in #308 wont work in commerce >= 2.12. Commerce implemented inline form API in 2.12. So I think this will be EOL for these patches and we have to wait until something similiar will be available on future releases of commerce (see also #300)
Comment #316
bgronek CreditAttribution: bgronek at Igility commented@bojanz How is this issue coming along? Is there anything we can help with?
Thank you so much for your work on this.
Comment #317
Martijn de WitI think all/partly work is done here: #3022850: [Addressbook, part 1] Rework the ownership model for customer profiles. (and it's follow up's)
Just as @jons already mentioned here above.
See also the issues that are referencing to this post.
Comment #318
bojanz CreditAttribution: bojanz at Centarro commentedAs most of you have seen by now, the work was split into multiple issues.
The main one landed on May 14th: #3022850: [Addressbook, part 1] Rework the ownership model for customer profiles.
We followed up with several API improvements:
#3055035: Finalize the AddressBookManager service
#3065400: Allow AddressBook::copy() to update address book profiles
We also tagged Address 1.7 (June 4th) and Profile 1.0-rc5 (June 7th) with needed bug fixes and improvements.
The next patch is now up at: #3053165: [Addressbook part 2] Complete the UI by allowing choice between multiple addressbook profiles. It represents weeks of effort. Please review the screenshots and test the code if you can.
We're also working on improved user pages in #3059633: Provide a better addressbook UI for the user pages. Both of these should land soon and be included in the 2.14 release.
Comment #319
bojanz CreditAttribution: bojanz at Centarro commentedI am marking this issue as fixed because all relevant issues have been fixed:
#3053165: [Addressbook part 2] Complete the UI by allowing choice between multiple addressbook profiles
#3072083: Allow any profile type to be used as a "customer" profile type
#3059633: Provide a better addressbook UI for the user pages
#3060433: Provide a way to collect all order profiles (billing, shipping, etc)
#3075282: Introduce alter hooks for inline forms
And unlike the D7 address book, this UI is fully available in the admin pages as well, which took a ton of additional time.
The 2.14 release will be out within 24h, along with a blog post. Last chance to find a bug!
Do note that you'll need to update Profile, Shipping, and possibly your payment gateway (e.g. Authorize.net / Stripe / Braintree).
The payment gateway update will also give you access to the per-gateway setting for not collecting billing information.
Since July 8th we've done numerous followups and fixes to the implementation.
The profile module has also seen an RC6 and finally a 1.0 release.
The Shipping module has been updated to support the address book, and now allows specifying a profile type per shipment type.
This means that you can continue to use the "customer" profile type for both billing and shipping (which is still the primary and more common use case), or you can go to admin/config/people/profile-types, click Duplicate and create a customer_shipping profile type, then select it for your shipment type. The "Address book" tab will automatically update to show separate fieldsets for billing and shipping information.
Thanks to everyone who helped this long and hard effort.
EDIT: Created a change record here: https://www.drupal.org/node/3078258
Comment #320
rgpublicAbsolutely amazing that this is finally fixed. IMHO literally one of the most important "bug" fixes in recent years. I lost track on how many gray hairs I got due to this. After every minor Drupal Commerce upgrade: patches, patches that didn't work, more patches for Commerce shipping which were incompatible with other patches. Nightmare on patch street (TM). Thank you so much @bojanz for working on this. *Fireworks*