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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bojanz created an issue. See original summary.

zengenuity’s picture

Status: Active » Needs review
FileSize
12.81 KB

I 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

Status: Needs review » Needs work

The last submitted patch, 2: commerce-ajax-pane-refreshing-2710999-2.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

zengenuity’s picture

zengenuity’s picture

zengenuity’s picture

Missed one change to PaymentInformation pane in last re-roll.

zengenuity’s picture

tanc’s picture

I'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)?

tanc’s picture

Just 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:

if (isset($info['address_country']['#process'])) {
  $info['address_country']['#process'][] = [
    'Drupal\my_module\Country',
    'processCountry',
  ];
}

Then in my custom module's Country class implement the processCountry method:

public static function processCountry(array &$element, FormStateInterface $form_state, array &$complete_form) {
  $element['country_code']['#ajax']['callback'] = [CheckoutFlowWithPanesBase::class, 'ajaxRefreshPanes'];
  return $element;
}
gmem’s picture

Managed 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.

The last submitted patch, 10: commerce-ajax-pane-refreshing-2710999-9.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Sophie.SK’s picture

Status: Needs review » Needs work

The 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".

p4trizio’s picture

This 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

bojanz’s picture

Assigned: Unassigned » bojanz

I'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:

    // static::ajaxSubmit() requires data-drupal-selector to be the same between
    // the various Ajax requests. A bug in \Drupal\Core\Form\FormBuilder
    // prevents that from happening unless $form['#id'] is also the same.
    // Normally, #id is set to a unique HTML ID via Html::getUniqueId(), but
    // here we bypass that in order to work around the data-drupal-selector bug.
    // This is okay so long as we assume that this form only ever occurs once on
    // a page.
    // @todo Remove this workaround once https://www.drupal.org/node/2897377 is
    //   fixed.
   $form['#id'] = Html::getId($form_state->getBuildInfo()['form_id']);

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.

bojanz’s picture

bojanz’s picture

Issue summary: View changes

Updating issue summary and sponsorship (Liip).

zengenuity’s picture

"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.

bojanz’s picture

Status: Needs work » Needs review
FileSize
14.3 KB

@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.

Status: Needs review » Needs work

The last submitted patch, 19: 2710999-19.patch, failed testing. View results

bojanz’s picture

Status: Needs work » Needs review
FileSize
15.36 KB

The 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.

FiNeX’s picture

Hi, what about refreshing the summary after shipping service has been selected? (see: https://www.drupal.org/project/commerce_shipping/issues/2898118 )

bojanz’s picture

@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).

FiNeX’s picture

Ok, thanks @bojanz :-)

Sophie.SK’s picture

This 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.

bojanz’s picture

It'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.

Sophie.SK’s picture

Status: Needs review » Reviewed & tested by the community

Cool, must have misunderstood an earlier comment then. Marking as RTBC :)

bojanz’s picture

Thank you for testing, appreciated.

I would love a +1 from @zengenuity before committing this.

The last submitted patch, 4: commerce-ajax-pane-refreshing-2710999-4.patch, failed testing. View results

The last submitted patch, 5: commerce-ajax-pane-refreshing-2710999-5.patch, failed testing. View results

The last submitted patch, 6: commerce-ajax-pane-refreshing-2710999-6.patch, failed testing. View results

zengenuity’s picture

I 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.

  • bojanz committed 1404b4a on 8.x-2.x authored by zengenuity
    Issue #2710999 by zengenuity, bojanz, gmem, steveoliver: Implement ajax...
bojanz’s picture

Status: Reviewed & tested by the community » Fixed

Now 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!

Status: Fixed » Closed (fixed)

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