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.
Comment | File | Size | Author |
---|---|---|---|
#19 | 2842968-followup.patch | 811 bytes | TR |
| |||
#15 | 2842968-ubercart-ajax-checkout-bug-6.patch | 6.91 KB | TR |
#7 | 2842968-ubercart-ajax-checkout-bug-6.patch | 6.65 KB | david.czinege |
| |||
#5 | 2842968-ubercart-ajax-checkout-bug-test-only-5.patch | 4.41 KB | david.czinege |
#2 | 2842968-ubercart-ajax-checkout-bug.patch | 2.24 KB | david.czinege |
|
Comments
Comment #2
david.czinege CreditAttribution: david.czinege at Drupers commentedI 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.
Comment #3
david.czinege CreditAttribution: david.czinege at Drupers commentedComment #4
TR CreditAttribution: TR commentedVery 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?
Comment #5
david.czinege CreditAttribution: david.czinege at Drupers commentedThis is the modified AjaxTest file which has a new test case: testCheckoutAjax
This patch should fail.
Comment #7
david.czinege CreditAttribution: david.czinege at Drupers commentedI 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.
Comment #8
david.czinege CreditAttribution: david.czinege at Drupers commentedComment #9
TR CreditAttribution: TR commentedThank 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.
Comment #10
TR CreditAttribution: TR commentedI tested it and it works well. I just want to clean up some really minor code style issues then I'll commit it.
Comment #11
david.czinege CreditAttribution: david.czinege at Drupers commentedAll 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?
Comment #12
david.czinege CreditAttribution: david.czinege at Drupers commentedHi,
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.
Comment #13
TR CreditAttribution: TR commentedSorry, 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.
Comment #14
david.czinege CreditAttribution: david.czinege at Drupers commentedAll right. :) Thanks for your quick response.
Comment #15
TR CreditAttribution: TR commentedI re-rolled the patch with some minor coding standards fixes. One last test from the testbot to make sure the patch applies ...
Comment #18
TR CreditAttribution: TR commented(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.
Comment #19
TR CreditAttribution: TR commentedFollowup - randomString() doesn't work properly over Ajax ...