The current "Add payment" form approach reverses a normal merchant's logic. Where a merchant might say, "I'd like to charge $25 to this card," our form currently asks, "What card would you like to charge? How much? Ok, a full charge or just an authorization?"

We should fix the logic and also provide more helpful information on the form, such as the order items and the effect of any previous payments, including a textual representation of the current order balance (instead of just defaulting the current balance in the payment amount field).

This is an in-browser mock-up of the new approach:

Note that it shows the actual order items and also includes in the order totals area a line showing how much has already been paid and what the remaining balance is. We do need to ensure this is backwards compatible, so we should:

  1. Define a new controller for this new form approach.
  2. Default Drupal Commerce to using this new controller.
  3. Look for a setting variable that if set edits the route to use the old controller. Perhaps something as simple as a setting called use_two_step_add_payment_form.
  4. Mark the old controller as deprecated so it can be deleted in 4.x.
  5. Embed the order items table atop the form, along with the order total summary.
  6. Add lines to the summary for the total paid and outstanding balance.
  7. Put the payment details inside a fieldset to visually match the order comments form on the view page.
  8. Float the currency selector if present. This isn't a strict requirement, because it can be a pain in the neck to get the vertical alignment right in the abstract. But this would be nicer than vertical stacking.
  9. Note the changed label, from "Payment option" to "Payment method."
  10. If the merchant is reusing a payment method, no problem, we process payment on submit.
  11. If the merchant is entering a new payment method, update the form to include the necessary elements.
  12. Create the payment method on submit and charge the payment as before.

I'll make a separate ticket specific to supporting a return from an offsite payment solution. The scope of this ticket is primarily to make the form ready for that while also providing a much nicer UX to merchants.

CommentFileSizeAuthor
#11 3538209-11.patch31.07 KBadrianandres
#10 3538209-10.patch102.17 KBjsacksick
#9 3538209-commerce-2.40-9.patch32.46 KBjsacksick
#7 3538209-new-merchant-facing-payment-form-2.patch30.92 KBadrianandres
#4 3538209-new-merchant-facing-payment-form-1.patch29.52 KBadrianandres
add-payment-form-mockup.jpg240.36 KBrszrama

Issue fork commerce-3538209

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

rszrama created an issue. See original summary.

imbatman made their first commit to this issue’s fork.

imbatman opened merge request !482

adrianandres’s picture

adrianandres commented
StatusFileSize
new 3538209-new-merchant-facing-payment-form-1.patch29.52 KB
rszrama’s picture

rszrama commented
Status: Active » Needs work

Quick fix needed: the logic of the use_two_step_add_payment_form setting is backwards. If it's set to TRUE, then we should use the old style two step form, not if it's set to FALSE.

From an update standpoint, I think it's fine for the new version of the form to be the default. Howver, it is fair to question whether or not that breaks backend interfaces or test suites for existing sites. We'll certainly document the change in the release notes, but let's also add an update hook that doesn't actually change anything but displays the following message to users who update:

Your payment add form has been updated to a new single page format. If you depend on the old two step method, you will need to set a variable in settings.php to preserve it. See the release notes for Commerce Core x.y.z for more information.

Otherwise this looks and functions great! Still ways we can improve the styling of the form (e.g., fixing order total summaries, limiting the form width, etc.), but let's consider that out of scope for this ticket.

jsacksick’s picture

jsacksick commented
Issue tags: +Needs tests

@imbatman: Thank you for this.

I haven't tested this manually, but we have to fix the broken tests. I think we need to keep the tests coverage for the existing form, since we're retaining it, while also introducing tests for the new form.

Additionally, couple of possible improvements:

  1. We can leverage the AjaxFormTrait instead of redefining an ajax callback etc (We need to see if this works ofc).
  2. From the event subscriber, let's import the class so we can do: PaymentForm::class (this prevents mistakes).
  3. I'm suggesting we rename PaymentForm (which is too generic) to indicate that this form is for adding a payment for an order. So perhaps OrderPaymentAddForm or just OrderPaymentForm?
  4. We can probably remove the buildPaymentOptionLabels() helper method in favor of EntityHelper::extractLabels()

Overall, what's actually really good about the new approach is that the logic was broken down into separate methods, which allows a child implementation to override specific parts of the code.

adrianandres’s picture

adrianandres commented
StatusFileSize
new 3538209-new-merchant-facing-payment-form-2.patch30.92 KB
jsacksick’s picture

jsacksick commented

Note that no actions buttons are shown when the payment gateway isn't onsite, so no way to submit a payment for manual gateways etc...

jsacksick’s picture

jsacksick commented
StatusFileSize
new 3538209-commerce-2.40-9.patch32.46 KB

Ok I'm now exposing the "Add payment" button for all payment options (not just onsite gateways).
A "payment_received" checkbox was added (label "Mark as received") for manual gateways.

Finally, the payment state isn't set to early as most of the createPayment() implementation out there expect a payment with a "new" state passed.

jsacksick’s picture

jsacksick commented
StatusFileSize
new 3538209-10.patch102.17 KB

Attaching a static patch for Commerce 3.

adrianandres’s picture

adrianandres commented
StatusFileSize
new 3538209-11.patch31.07 KB
damienmckenna’s picture

damienmckenna
Location TN, USA
commented

Should the setting be namespaced, e.g. "commerce_use_two_step_add_payment_form"?

jsacksick’s picture

jsacksick commented

#12: definitely.

tbkot made their first commit to this issue’s fork.

tbkot’s picture

tbkot commented
Status: Needs work » Needs review

Conflicts in the MR are resolved, and a new test is added to check a new form to add payments

jsacksick’s picture

jsacksick commented
Status: Needs review » Needs work

We're close (I think) though I'm not sure if we kept coverage for the existing form? Or are we just testing the new form?

Let's rename the setting flag to:
commerce_payment_use_legacy_add_payment_form

Also commerce_payment_update_10303 has the following message:

  $message = t('Your payment add form has been updated to a new single page format. If you depend on the old two step method, you will need to set a variable in settings.php to preserve it. See the release notes for Commerce Core x.y.z for more information.');

So let's change x.y.z to 3.3.0. and let's actually update "you will need to set a variable in settings.php" to 'you will need to set the "commerce_payment_use_legacy_add_payment_form" variable in settings.php'.

tbkot’s picture

tbkot commented
Status: Needs work » Needs review

@jsacksick
Tests check only the new form. For the old implementation, we need to set the "commerce_payment_use_legacy_add_payment_form" setting to TRUE in the test. Do we need that?

  • jsacksick committed 1bb6ab4c on 3.x authored by imbatman 
    feat: #3538209 Provide a new "Add payment" form

By: rszrama
By:...
jsacksick’s picture

jsacksick commented
Status: Needs review » Fixed
Issue tags: -Needs tests

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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