Bug description:

When the user changes the payment method on the checkout page, an ajax callback is called, which refreshes the text inside the #payment-details div, but if the user changes the shipping method first and after that user changes the payment method, the ajax callback is not called anymore.

Steps to reproduce the bug:

- Install these modules:

  • Cart
  • Country
  • Order
  • Product
  • Store
  • Payment
  • Shipping Quotes
  • Ubercart Ajax Administration
  • Payment Method Pack
  • PayPal

- Create a product node and put it into the cart.
- Add two payment method which have different payment details text. (For example: PayPal Payments Standard, Cash on delivery)
- Add two shipping method (For example: Home delivery (Flat rate), Personal pick (Flat rate))
- Go to the checkout page.
- Change the payment method -> The payment detail is refreshed.
- Change the shipping method.
- Change again the payment method: -> The payment detail is not refreshed anymore.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

david.czinege created an issue. See original summary.

david.czinege’s picture

I solved this issue. The problem was the following:

In the AjaxAttachTrait.php file, where the ajax pane refresh is handled, the returning AjaxResponse object doesn't collect other AjaxResponse objects' attachments. So Drupal 8 doesn't know there are libraries or ajax handlers attached to specific form elements. Because of that, when user changes the shipping method, the payment method pane is refreshed by the ajax callback, but the payment method pane's ajax callback settings is not returned.

My sollution is:

- Now the ajaxReplaceCheckoutPane method return with an AjaxResponse object.
- The ajaxMultiplex method collects other AjaxResponse objects' attachments and set it the newly created AjaxResponse object and return with that.

david.czinege’s picture

Status: Active » Needs review
TR’s picture

Very nice, thank your for finding this and providing a fix.

Can you review the documentation text at the top of AjaxAttachTrait to make sure it's correct?

It would be nice to have a test case for this too, could you put a new case in AjaxTest to reproduce the problem so we can be sure this stays fixed?

david.czinege’s picture

This is the modified AjaxTest file which has a new test case: testCheckoutAjax

This patch should fail.

Status: Needs review » Needs work

The last submitted patch, 5: 2842968-ubercart-ajax-checkout-bug-test-only-5.patch, failed testing.

david.czinege’s picture

I reviewed the documentation text at the top of AjaxAttachTrait.
I think it's correct, because i don't modified that part of the code, which is mentioned there.
I have only updated the code, where the ajax response is set.

So this modification is backward compatible.

Now I upload a new patch which contains the test case, which was successfully failed in my previous comment, and it also contains the bug fix.

david.czinege’s picture

Status: Needs work » Needs review
TR’s picture

Thank you, your code looks great! I would like to test it on a clean site before I commit - that might take me a few days because I'm in the middle of some things here, but it looks like your patch is ready to be committed.

TR’s picture

Status: Needs review » Reviewed & tested by the community

I tested it and it works well. I just want to clean up some really minor code style issues then I'll commit it.

david.czinege’s picture

All right :)

I found 2 lines where I hard coded some text in the created test case in my last patch.
I forget to change them to random strings.

These are the two lines:
$policy1 = 'alma';//$this->randomString();
$policy2 = 'korte';//$this->randomString();

Should I upload another patch to fix this or can you do this?

david.czinege’s picture

Hi,

I don't want to disturb you, i am just curious, when will be this patch commited?

Can I help somehow?

Thanks in advance for the answer.

TR’s picture

Sorry, I just have very little time to devote to Ubercart recently. Committing this is on my to-do list, and hopefully I'll have a few hours in the near future to deal with this and other issues.

david.czinege’s picture

All right. :) Thanks for your quick response.

TR’s picture

I re-rolled the patch with some minor coding standards fixes. One last test from the testbot to make sure the patch applies ...

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 15: 2842968-ubercart-ajax-checkout-bug-6.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

  • TR committed 4594152 on 8.x-4.x authored by david.czinege
    Issue #2842968 by david.czinege: Ubercart Ajax Administration checkout...
TR’s picture

Status: Needs work » Fixed

(The 1 test failure is due to a Drupal core 8.5.x regression and is not related to this patch)

Thanks for your patience. After a few more coding standards changes I committed the patch.

TR’s picture

Followup - randomString() doesn't work properly over Ajax ...

  • TR committed 8403e97 on 8.x-4.x
    Issue #2842968 by TR: Followup correction to #2842968, use...

Status: Fixed » Closed (fixed)

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