Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Event Cart: convert to use standard payment forms #17886

Closed
wants to merge 3 commits into from
Closed

Event Cart: convert to use standard payment forms #17886

wants to merge 3 commits into from

Conversation

mattwire
Copy link
Contributor

@mattwire mattwire commented Jul 18, 2020 •
edited
Loading

Overview

Convert the event cart to use standard payment forms and supported interfaces. Builds on #17339.

Before

Very old, unsupported code. Only a single payment processor supported and some don't work (eg. Stripe). Lot's of custom code to create contribution/participants/lineitems and payment made before contribution created.

After

Checkout:
image
(The pay now button is a sneak-preview of the upcoming google/apple/payment request support in stripe).

Settings:
image

Technical Details

This is a major refactor of the event cart checkout following it's move to an extension. We switch to using standard billingblock functionality and support an independent set of payment processors via settings. (Previously this was guessed from the events in the cart and only one processor could be used).
This switches event cart to use Order API + payment propertybag making this probably some of the most up to date code in CiviCRM!

Comments

@eileenmcnaughton @artfulrobot This is still a work in progress because I've not gone through and verified that receipting is working properly (and may switch to Payment.sendconfirmation for this). But you'll be pleased to see it uses Order API to create the contribution, participants and lineitems. And then uses payment propertybag before calling doPayment.

@civibot
Copy link

civibot bot commented Jul 18, 2020

(Standard links)

@civibot civibot bot added the master label Jul 18, 2020
@mattwire mattwire added the needs-work-not-review-ready Submitter required to take action, will be closed after 1-2 weeks in this state label Jul 18, 2020
@eileenmcnaughton
Copy link
Contributor

I'm working on trying to get the review queue under control so I'm closing out these PRs that acheive the same function closed as open.

@eileenmcnaughton
Copy link
Contributor

(obviously it might make sense to rebase the branch intermittently to see what might still be worth bringing over from here - )

@eileenmcnaughton
Copy link
Contributor

I should note there are 2 significant blockers on this work based on a quick skim

  1. the DAO move - there are technical problems to solve there for new installs before we can look at moving the DAO
  2. the Use of the property bag - I think we have figured out that we are quite far apart on our vision for what we want to achieve for the property bag & that needs resolving before we do more PRs on it (although I don't have much appetite for getting back into that at the moment)

@mattwire mattwire reopened this Aug 5, 2020
@mattwire mattwire force-pushed the eventcartextpayment branch 9 times, most recently from 58d283f to aa83d32 Compare August 7, 2020 17:27
@mattwire mattwire mentioned this pull request Aug 7, 2020
Merged
@mattwire mattwire changed the title WIP Event Cart convert to use standard payment forms Event Cart convert to use standard payment forms Aug 7, 2020
@mattwire mattwire marked this pull request as ready for review August 7, 2020 17:39
@mattwire mattwire force-pushed the eventcartextpayment branch 3 times, most recently from c009de0 to 9bf74bc Compare August 21, 2020 16:42
@mattwire mattwire marked this pull request as draft August 27, 2020 08:44
@mattwire
Copy link
Contributor Author

Can be tested by via https://github.com/mjwconsult/civicrm-eventcart

@mattwire mattwire force-pushed the eventcartextpayment branch 3 times, most recently from 3cae12e to 9b0cdca Compare April 12, 2021 22:41
@mattwire
Copy link
Contributor Author

mattwire commented May 9, 2021 

CRM_Financial_Form_PaymentFormsTest::testEventPaymentForms
Deprecated function Legacy property name 'is_recur', use Canonical property name 'isRecur'.

/home/jenkins/bknix-dfl/build/core-17886-1eqpf/web/sites/all/modules/civicrm/CRM/Core/Error.php:1053
/home/jenkins/bknix-dfl/build/core-17886-1eqpf/web/sites/all/modules/civicrm/CRM/Core/Error.php:1042
/home/jenkins/bknix-dfl/build/core-17886-1eqpf/web/sites/all/modules/civicrm/Civi/Payment/PropertyBag.php:269
/home/jenkins/bknix-dfl/build/core-17886-1eqpf/web/sites/all/modules/civicrm/Civi/Payment/PropertyBag.php:138
/home/jenkins/bknix-dfl/build/core-17886-1eqpf/web/sites/all/modules/civicrm/CRM/Core/Payment/AuthorizeNet.php:127
/home/jenkins/bknix-dfl/build/core-17886-1eqpf/web/sites/all/modules/civicrm/CRM/Core/Payment.php:1388
/home/jenkins/bknix-dfl/build/core-17886-1eqpf/web/sites/all/modules/civicrm/ext/eventcart/CRM/Event/Cart/Form/Checkout/Payment.php:340
/home/jenkins/bknix-dfl/build/core-17886-1eqpf/web/sites/all/modules/civicrm/tests/phpunit/CRM/Financial/Form/PaymentFormsTest.php:93
/home/jenkins/bknix-dfl/build/core-17886-1eqpf/web/sites/all/modules/civicrm/tests/phpunit/CiviTest/CiviUnitTestCase.php:226
/home/jenkins/bknix-dfl/extern/phpunit7/phpunit7.phar:615

@mattwire mattwire force-pushed the eventcartextpayment branch 3 times, most recently from 7358ba4 to 07bc45d Compare May 17, 2021 12:00
@mattwire mattwire mentioned this pull request May 17, 2021
Merged
eileenmcnaughton
eileenmcnaughton reviewed May 17, 2021
View reviewed changes
CRM/Core/Payment/AuthorizeNet.php Outdated
@@ -113,6 +113,10 @@ protected function supportsRecurContributionsForPledges() {
* @throws \Civi\Payment\Exception\PaymentProcessorException
*/
public function doPayment(&$params, $component = 'contribute') {
if ($params instanceof \Civi\Payment\PropertyBag) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is your work in progress PR & we are not supposed to be looking at it but I stumbled here from your other PR & saw this - we can't actually have something that requires the payment processors to add something like this in what we actually merge

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've put it there for now but we can also put it in the eventcart code just before calling doPayment() which would be a nice way of allowing us to pass a propertyBag to a paymentprocessor without requiring any modifications on the paymentprocessor side. Additionally a paymentprocessor could explicitly choose to switch ON legacy warnings once it's suitably converted.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep - not requiring any modifications on the processor side is the pre-requisite for us to start passing out propertyBag from any core functions

@mattwire mattwire force-pushed the eventcartextpayment branch 7 times, most recently from 3a98040 to 2474469 Compare May 25, 2021 16:23
@mattwire mattwire force-pushed the eventcartextpayment branch 3 times, most recently from 1910f1a to 6b3624d Compare June 1, 2021 15:45
@demeritcowboy
Copy link
Contributor

This has merge conflicts and has been sitting for several months with a needs-work tag so going to close but feel free to reopen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eileenmcnaughton eileenmcnaughton eileenmcnaughton left review comments

Assignees
No one assigned
Labels
master needs-work-not-review-ready Submitter required to take action, will be closed after 1-2 weeks in this state
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mattwire @eileenmcnaughton @demeritcowboy