If Payment and the Donation pane are on the same checkout page, and the Payment validation fails, then the Donation line item is added to the cart, and the page is reloaded.

At this point the Donation is in cart, so when you correct payment validation problem, and then submit your checkout page form, it gets added again on form submission, resulting in duplicate items in the cart.

This doesn't happen with basic validation on the Payment pane (e.g. missing required fields) -- it only happens when the Card Number doesn't match the credit card type, or other more complex validations..

I'm using Paypal, maybe this is related.

Perhaps addressing #1819786: Hide donation checkout pane when cart already contains donation would make this problem go away...

CommentFileSizeAuthor
#12 issue-1851196.patch3.06 KBlsolesen
#9 issue-1851196.patch2.74 KBlsolesen
#6 issue-1851196.patch1.42 KBlsolesen
#5 commerce_donate-remove_pane_form_after_adding_donation-1851196-5.patch1.6 KBabasso
#4 commerce_donate-remove_pane_form_after_adding_donation-1851196-4.patch3.03 KBtmsimont
#3 commerce_donate-remove_pane_form_after_adding_donation-1851196-3.patch1.75 KBtmsimont

Comments

tmsimont’s picture

tmsimont commented

related: #1851312: Payment validation occurs after other panes execute submit callbacks rather than before, resulting in non-standard form behavior

Now I'm not sure if this problem is an issue with the payment module for commerce, or if it's just this module...

tmsimont’s picture

tmsimont commented

Apparently this issue should be resolved within this module, per rszrama's comments here:
#1380310: Optionally require all panes on a checkout page to validate before processing them and here #1851312: Payment validation occurs after other panes execute submit callbacks rather than before, resulting in non-standard form behavior

Because commerce_donate_checkout_pane_checkout_form_submit() fires before all of the pane's on the same page validate, it needs to consider the fact that it might be called more than once during checkout. Also, it seems to be called if the user clicks "Go Back" and then re-submits the page where the donation is added.

tmsimont’s picture

tmsimont commented
Status: Active » Needs review
StatusFileSize
new commerce_donate-remove_pane_form_after_adding_donation-1851196-3.patch1.75 KB

Attached is a patch that simply checks an order for the configured donation product ID before adding the donation line item fields to the checkout pane. If there's already a donation in the order, the pane does not display a form, but instead displays markup:

  '#markup' => t("A donation has already been added to your order.  You can remove this donation by editing your !cartlink.", array("!cartlink" => l("shopping cart","cart"))),

Perhaps the behavior could be instead configurable by the module, but this at least addresses the bug that I encountered where the Donation can be added numerous times to 1 order if there are validation errors on other panes on the same checkout page as the donation form.

tmsimont’s picture

tmsimont commented
StatusFileSize
new commerce_donate-remove_pane_form_after_adding_donation-1851196-4.patch3.03 KB

Last patch doesn't check for existence of property before calling on it.. this should fix it

abasso’s picture

abasso commented
Issue summary: View changes
StatusFileSize
new commerce_donate-remove_pane_form_after_adding_donation-1851196-5.patch1.6 KB

Could not apply patch above, created a new one.

lsolesen’s picture

lsolesen commented
StatusFileSize
new issue-1851196.patch1.42 KB

Added a proper patch made from within the module.

lsolesen’s picture

lsolesen commented

The proposed workaround needs work:

Notice: Undefined index: checkout_donate in commerce_donate_checkout_pane_checkout_form_validate() (line 154 of /var/www/site/docroot/fond/sites/all/modules/contrib/commerce_donate/includes/commerce_donate.checkout_pane.inc).
Notice: Undefined index: language in commerce_donate_checkout_pane_checkout_form_validate() (line 155 of /var/www/site/docroot/fond/sites/all/modules/contrib/commerce_donate/includes/commerce_donate.checkout_pane.inc).
Notice: Undefined index: language in commerce_donate_checkout_pane_checkout_form_submit() (line 181 of /var/www/site/docroot/fond/sites/all/modules/contrib/commerce_donate/includes/commerce_donate.checkout_pane.inc).
lsolesen’s picture

lsolesen commented
Status: Needs review » Needs work
lsolesen’s picture

lsolesen commented
StatusFileSize
new issue-1851196.patch2.74 KB
lsolesen’s picture

lsolesen commented
Status: Needs work » Needs review

Patch in #9 fixes notices and warnings after submitting the product.

robcarr’s picture

robcarr
Primary language English
Location Perthshire, Scotland
commented

#9 works for me. Thanks

lsolesen’s picture

lsolesen commented
StatusFileSize
new issue-1851196.patch3.06 KB

Minor adjustment to patch.

stella’s picture

stella commented

I'm not sure this is the correct approach. What if I wanted more than one donation product type in the cart?

stella’s picture

stella commented 
+        $pane_form[$pane_id]['already_added'] = array(
+          '#markup' => t("A donation has already been added to your order.  You can remove this donation by editing your !cartlink.", array("!cartlink" => l("shopping cart","cart"))),
+        );
+        // Return without additional fields.

That's also the wrong approach to embedding a link in a translatable string as you're loosing the context of the 'shopping cart' string when translating it. You should be doing something like:

+        $pane_form[$pane_id]['already_added'] = array(
+          '#markup' => t('A donation has already been added to your order.  You can remove this donation by editing your <a href="@url">shopping cart</a>.', array('@url' => url('cart'))),
+        );
+        // Return without additional fields.

  • stella committed 51af184 on 7.x-1.x authored by lsolesen 
    Issue #1851196 by lsolesen, tmsimont, abasso: Donation is added multiple...
stella’s picture

stella commented
Status: Needs review » Fixed

@lsolesen after closer review, I've committed the patch (with the translation fix described above) as I note that the code just prevents the checkout donation product from being added more than once, which is fine. It doesn't prevent other donation products from being added multiple times.

Thanks all!

Status: Fixed » Closed (fixed)

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