We need to introduce a way for a form element to refresh the entire form.
This is more complex than just returning $form from an ajax callback because of core bugs:
- #2675688: Status messages from AjaxRenderer not prepended correctly when adding field items per ajax
- #2897377: $form['#attributes']['data-drupal-selector'] is set to a random value so can't be used for its intended purpose
The refresh logic needs to take into account the status messages, which need to be inserted above the nearest grouping element.
For example, in a checkout context, that means inserting them above the checkout pane.
The patch needs to convert the CouponRedemption pane, which currently refreshes the order summary pane manually, but doesn't refresh the payment & shipping information panes. However, the pane can reduce the order total to 0, changing the output of these panes (PaymentInformation would switch output to just a billing address, for example). That's what we need to cover in a test.
We also need to take into account the need to do the same outside of checkout panes. For example, the CouponRedemption inline form can be used as a part of the cart form (/carts).
Original definition
Some of our panes have dependencies.
Examples:
1) When the shipping address changes, update the shipping services, payment, cart summary (in the US the shipping address controls taxes)
2) When the shipping service changes, update the payment and cart summary (due to the updated order total)
3) When the billing address changes, update the cart summary (in the EU the billing address controls taxes)
We need to figure out the best way to approach this.
Each pane will need a way to signal that a refresh is needed. We then need to figure out which parts of the form to refresh.
A simple but brute force option would be to refresh the entire form. We considered this in 1.x, abandoned it due to flickering, need to revisit.
A more complex solution would be for each pane to specify exactly on which other panes it depends, then use that info to refresh dependents.
Comment | File | Size | Author |
---|---|---|---|
#21 | 2710999-21.patch | 15.36 KB | bojanz |
| |||
#19 | 2710999-19.patch | 14.3 KB | bojanz |
#10 | interdiff-commerce-ajax-pane-refreshing-2710999-7-9.patch | 867 bytes | gmem |
#10 | commerce-ajax-pane-refreshing-2710999-9.patch | 11.53 KB | gmem |
#7 | commerce-ajax-pane-refreshing-2710999-7.patch | 11.52 KB | zengenuity |
Comments
Comment #2
zengenuity CreditAttribution: zengenuity at DrupalTutor / Zengenuity commentedI need this feature, because I have a very complex checkout form, with several ajax-enabled panes. I've attached a patch of what I have working so far.
I wanted to make sure that panes didn't need to know about each other in order for this work. This is important because I have a custom pane that needs to refresh the Payment Information and Order Summary panes, and I have another custom pane that needs to be refreshed after a coupon is added with the Coupon Redemption pane.
My implementation adds an ajaxRefreshPanes() method to CheckoutFlowWithPanesBase. This may not be the best place to put this functionality, but it could be moved into another class or it's own class pretty easily.
There is a new interface for checkout pane classes: SupportsAjaxRefresh. Panes that implement this interface have to provide
public static function ajaxRefreshPaneForm(array $form, FormStateInterface $form_state)
. In that method, they do whatever they need to do to refresh themselves by returning an array of ajax Command objects. They can return an empty array if they don't want refresh in a given situation.Panes or forms that want to trigger a global refresh should have #element_ajax or #ajax callbacks like this:
[CheckoutFlowWithPanesBase::class, 'ajaxRefreshPanes']
I like this approach because the logic for when to refresh a pane is in that pane's own class, and a pane that is triggering a refresh doesn't have to know anything about other panes on the same page. It doesn't even have to know which panes are active on the current page. I think this approach should work for most use cases and most combinations of core/contrib/custom panes, but if you have an edge case related to when to do the ajax updates, you can create your own checkout flow and override the ajaxRefreshPanes() method.
In addition to building the system to refresh the panes, I re-implemented the CouponRedemption pane's ajax refresh of itself and OrderSummary using the new approach. I also enabled refreshing of the OrderSummary and PaymentInformation panes, though neither of those trigger page-wide refreshes themselves.
Finally, I included two patches from other issues in this patch, because the PaymentInformation pane can't be refreshed with ajax without them. The issues are: #2916366: PaymentInformation pane throws an error when rebuilt with AJAX and default payment method is removed and #2916546: The PaymentInformation pane can't be replaced with an Ajax command
Comment #4
zengenuity CreditAttribution: zengenuity at DrupalTutor / Zengenuity commentedRe-roll.
Comment #5
zengenuity CreditAttribution: zengenuity at DrupalTutor / Zengenuity commentedRe-roll.
Comment #6
zengenuity CreditAttribution: zengenuity at DrupalTutor / Zengenuity commentedMissed one change to PaymentInformation pane in last re-roll.
Comment #7
zengenuity CreditAttribution: zengenuity at DrupalTutor / Zengenuity commentedRe-roll.
Comment #8
tancI'm no commerce expert but this is working nicely for me. I have a custom pane which has a simple radios element on it which now happily triggers an ajax update of the panes. In a validation callback I'm modifying the order based on the radio button selected and this changes the total value of the order. With this patch I'm now able to dynamically update the order status which is fantastic.
I don't suppose you know how I'd go about modifying the country field on the address to also trigger all panes to update (rather than just the postcode within the address widget)?
Comment #9
tancJust a note on what I did to get the country widget to update the panes (using this patch):
Implement hook_element_info_alter() and add a new process function:
Then in my custom module's Country class implement the processCountry method:
Comment #10
gmem CreditAttribution: gmem as a volunteer and at Acro Commerce commentedManaged to track down the failing tests, the first parent isn't always 'coupon_redemption' so I changed that to use in_array, which seems to have fixed the issue. Works as advertised, although haven't been able to replicate the ajax error locally.
Comment #12
Sophie.SKThe patch in #10 works well for us against Commerce 2.11.
I spy some coding standards issues that can be improved. If I get some time I'll come back to fix them. In the meantime, leaving this as "needs work".
Comment #13
p4trizio CreditAttribution: p4trizio at Elicos commentedThis patch is conflicting with the one at https://www.drupal.org/project/commerce/issues/2872455 (Allow coupons to be redeemed on the cart page): throws ajax errors
Comment #14
bojanz CreditAttribution: bojanz at Centarro commentedI'm working on this.
The key preparation work was figuring out a generic solution for #2916546: The PaymentInformation pane can't be replaced with an Ajax command.
Ideally we wouldn't need a wrapper, we'd use the pane div directly.
This is made difficult by this core bug: #2897377: $form['#attributes']['data-drupal-selector'] is set to a random value so can't be used for its intended purpose.
The core workaround is to set the #id manually.
From core's BlockEntitySettingTrayForm:
So we're going to do a similar thing in the checkout flow base class, on $form and each $pane_form.
The second important detail is making sure to use ajax commands for refreshing the form, instead of just returning a $form array, to avoid this core bug: #2675688: Status messages from AjaxRenderer not prepended correctly when adding field items per ajax. The current patch already does this, but it's good to clarify why.
Comment #16
bojanz CreditAttribution: bojanz at Centarro commentedCrediting steveoliver, for #2915942: Update the payment information pane if it exists with coupon pane.
Comment #17
bojanz CreditAttribution: bojanz at Centarro for Liip commentedUpdating issue summary and sponsorship (Liip).
Comment #18
zengenuity CreditAttribution: zengenuity at DrupalTutor / Zengenuity commented"A more complex solution would be for each pane to specify exactly on which other panes it depends, then use that info to refresh dependents."
Is this solution possible if multiple contrib/custom modules are adding panes? It seems like there is no way they could possibly know which other panes might be displaying related information. That's why in the original patch I had each pane figure out for itself if it needed to update.
Comment #19
bojanz CreditAttribution: bojanz at Centarro for Liip commented@zengenuity
You're quoting the original summary text from 2017.
Here's a first version of the patch.
It contains the #id fixes and wrapper simplifications described in #14.
It converts the CouponRedemptionForm checkout pane/inline form.
It replaces the entire form. I played with the idea of iterating over the panes and replacing them 1by1, allowing some to skip updating, but 95% of the panes will want to be replaced, making it simpler and easier to just replace everything at once.
Let's see what the existing tests say, before adding new ones.
EDIT: Left some logging in AjaxFormTrait, to be removed in the next reroll.
Comment #21
bojanz CreditAttribution: bojanz at Centarro for Liip commentedThe patch is now fully functional and ready for review.
Remember that you need Commerce -dev, this won't work with 2.11.
I have also confirmed that the trick in #9 still works. You need to specify [AjaxFormTrait::class, 'ajaxRefreshForm'] as the ajax callback.
Comment #22
FiNeX CreditAttribution: FiNeX as a volunteer commentedHi, what about refreshing the summary after shipping service has been selected? (see: https://www.drupal.org/project/commerce_shipping/issues/2898118 )
Comment #23
bojanz CreditAttribution: bojanz at Centarro for Liip commented@FiNeX
This issue was needed for that, but the main discussion remains in #2898118: Update the order summary via AJAX when a shipping rate is selected (deciding whether it is okay to resave the shipments every time shipping information changes).
Comment #24
FiNeX CreditAttribution: FiNeX as a volunteer commentedOk, thanks @bojanz :-)
Comment #25
Sophie.SKThis patch seems to work really nicely. Thank you for the work here @bojanz :)
It was a bit fiddly to get it working but I think that's my unique checkout setup. Now I just need to get it to update the item count in the basket block and we're golden!
I won't change the status as you mentioned tests - not sure if you wanted to add those first? - but it is working very cleanly.
Comment #26
bojanz CreditAttribution: bojanz at Centarro for Liip commentedIt's complete test wise, we already have tests that confirm that the order summary pane works when moved outside of the sidebar, since we no longer hardcode the order summary logic, it proves that any pane would work.
Comment #27
Sophie.SKCool, must have misunderstood an earlier comment then. Marking as RTBC :)
Comment #28
bojanz CreditAttribution: bojanz at Centarro for Liip commentedThank you for testing, appreciated.
I would love a +1 from @zengenuity before committing this.
Comment #32
zengenuity CreditAttribution: zengenuity at DrupalTutor / Zengenuity commentedI converted five custom/contrib panes that were using my previous patch to this new patch. All of them are working, and this approach is much simpler. I was able to remove probably 10-30 lines of code from each pane class.
I agree with @Sophie.SK that this is RTBC.
Comment #34
bojanz CreditAttribution: bojanz at Centarro for Liip commentedNow that both Sophie.SK and zengenuity have tested, I feel comfortable proceeding with this patch.
Awarding the commit to zengenuity, who proved the need for this API, and wrote the initial patch that satisfied user needs during 2018.
Thanks, everyone!