Implement PaymentProcessor and PaymentProcessorType APIv4 Entities #15624

seamuslee001 · 2019-10-27T02:12:51Z

Overview

Adds PaymentProcesor and PaymentProcessorType entities to APIv4

Before

Entities don't exist in APIv4

After

Entities exist in APIv4

Technical Details

There are some changes to the DAO to set some defaults and also make it clearer some fields are required. This was done after consulting the create specs in the APIv3 versions of these Entities

ping @eileenmcnaughton @mattwire @colemanw

civibot · 2019-10-27T02:12:54Z

(Standard links)

If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

mattwire · 2019-10-27T12:36:43Z

CRM/Financial/DAO/PaymentProcessorType.php

          'description' => ts('Payment Processor Name.'),
+          'required' => TRUE,


There's an obvious typo not caused by this PR but can we fix please - description "Payment Processor Title". In fact, shouldn't these be "Payment Processor Type Name" and "Payment Processor Type Title"?

mattwire · 2019-10-27T12:38:28Z

tests/phpunit/CRM/Financial/BAO/PaymentProcessorTypeTest.php

@@ -116,6 +119,7 @@ public function testDel() {
      'title' => 'Test Del Payment Processor',
      'billing_mode' => 1,
      'is_active' => 1,
+      'class_name' => 'CRM_Core_Payment_Dummy',


In the DB class_name is stored as Payment_Dummy - shouldn't it be here too?

Yes I think so

mattwire · 2019-10-27T12:39:19Z

@seamuslee001 Couple of minor clarifications. And, shouldn't we have a DB upgrader step as we are setting various fields to be required?

colemanw · 2019-10-27T12:51:44Z

Civi/Api4/Action/PaymentProcessor/PaymentProcessorSaveTrait.php

+          $item['payment_instrument_id'] = $paymentProcessorType['payment_instrument_id'];
+        }
+      }
+    }


I think setting these field defaults would be better addressed by a SpecProvider, and we should avoid overriding the writeObjects function if at all possible.

See for example
https://github.com/civicrm/civicrm-core/blob/5.19/Civi/Api4/Service/Spec/Provider/NavigationSpecProvider.php#L50-L50

@colemanw how would you handle the situation say in L58 where setting the payment instrument is dependant on if they have passed in one or if not then get it from the Payment Processor Type ?

Good question! I don't think that can be done with SpecProvider, you make a good point there.

However, as it's written this function is problematic because it assumes a complete record is being passed in but (in the case of update) that may not be the case.

My vote is to move this logic to the BAO (and only fire it if payment_processor_id is passed in) :)

Have rebased this now that the code has shifted to the BAO level

seamuslee001 · 2019-10-27T19:49:14Z

@colemanw in APIv4 is there a way to define a pseudo field? so in this case financial_account_id is not actually in the schema spec for PaymentProcessor but we use it to define the financial account relationship for the payment processor @eileenmcnaughton ?

seamuslee001 · 2019-10-27T20:13:55Z

Civi/Api4/Action/PaymentProcessor/PaymentProcessorSaveTrait.php

+  protected function writeObjects($items) {
+    foreach ($items as &$item) {
+      if (empty($item['financial_account_id'])) {
+        $item['financial_account_id'] = \CRM_Financial_BAO_PaymentProcessor::getDefaultFinancialAccountID();


@colemanw had to add here as financial_account_id isn't in the DAO spec and not sure how to add on new fields

seamuslee001 · 2019-10-27T20:16:05Z

Civi/Api4/Service/Spec/Provider/PaymentProcessorTypeCreationSpecProvider.php

+   * @inheritDoc
+   */
+  public function modifySpec(RequestSpec $spec) {
+    $spec->getFieldByName('payment_instrument_id')->setRequired(FALSE)->setDefaultValue(1);


@colemanw i tried using 'Credit Card' as the value but didn't seem to work right

colemanw · 2019-10-28T14:09:18Z

@colemanw in APIv4 is there a way to define a pseudo field? so in this case financial_account_id is not actually in the schema spec for PaymentProcessor but we use it to define the financial account relationship for the payment processor @eileenmcnaughton ?

Yes, there is, via a specProivider. Here's an example of one: https://github.com/civicrm/civicrm-core/blob/5.19/Civi/Api4/Service/Spec/Provider/EventCreationSpecProvider.php#L54-L58

But I would ask anyone contemplating adding pseudofields to the spec to be very judicious about it. Api3 IMO has way too many pseudofields that don't belong, so before adding one to api4 please ensure that you:

Ask yourself - would it be better represented as a param?
If it belongs to another entity, check if it can already be handled by api joins.
Limit it to only the action(s) it applies to.

eileenmcnaughton · 2019-10-29T20:39:25Z

"Ask yourself - would it be better represented as a param?
If it belongs to another entity, check if it can already be handled by api joins.
Limit it to only the action(s) it applies to."

I'm inclined to think this IS a legit pseudofield. I could make a pretty strong case for adding it as a column to the payment_processor table but probably it will remain as an additional field that is stored in a convoluted way

eileenmcnaughton · 2019-11-05T04:50:18Z

CRM/Financial/DAO/PaymentProcessorType.php

-          'description' => ts('Payment Processor Name.'),
+          'title' => ts('Payment Processor Type Title'),
+          'description' => ts('Payment Processor Type Title.'),
+          'required' => TRUE,


Is this required? For processor it isn't maybe it is for type?

Wel that is the DAO for the Type ;)

eileenmcnaughton · 2019-11-05T04:51:01Z

Civi/Api4/Action/PaymentProcessor/Save.php

+ *
+ * @package CRM
+ * @copyright CiviCRM LLC (c) 2004-2019
+ * $Id$


$ids$ should go

seamuslee001 · 2019-12-15T04:48:54Z

@eileenmcnaughton @colemanw what can we do to move this forward now?

tests/phpunit/api/v3/PaymentProcessorTest.php

tests/phpunit/api/v3/PaymentProcessorTypeTest.php

colemanw · 2020-01-11T16:50:30Z

Civi/Api4/Action/PaymentProcessor/PaymentProcessorSaveTrait.php

+          $item['payment_instrument_id'] = $paymentProcessorType['payment_instrument_id'];
+        }
+      }
+    }


Good question! I don't think that can be done with SpecProvider, you make a good point there.

However, as it's written this function is problematic because it assumes a complete record is being passed in but (in the case of update) that may not be the case.

My vote is to move this logic to the BAO (and only fire it if payment_processor_id is passed in) :)

seamuslee001 · 2020-01-11T21:23:10Z

hi @colemanw thanks for the pointers i have split off into a separate PR #16279 about the moving of the handling of the payment_instrument_field for PaymentProcessor to the Create BAO function. and have updated the tests

seamuslee001 · 2020-01-11T21:24:19Z

tests/phpunit/api/v3/PaymentProcessorTest.php

    ];
+    if ($version === 3) {
+      // In APIv3 If a field is default NULL it is not returned.


@colemanw not sure if this is deliberate but it seems a noticeable change between the API versions, i assume it is documented somewhere?

Yes it is deliberate.
civicrm/org.civicrm.api4#116

You could simplify this patch by deleting NULL values from the api result instead of adding them to the expected result :)

ok have updated based on your suggestion @colemanw

seamuslee001 · 2020-01-11T21:25:54Z

Civi/Api4/Service/Spec/Provider/PaymentProcessorCreationSpecProvider.php

+    // Billing mode is copied across from the payment processor type field in the BAO::create function.
+    $spec->getFieldByName('billing_mode')->setRequired(FALSE);
+
+    $financial_account_id = new FieldSpec('financial_account_id', 'PaymentProcessor', 'Integer');


@eileenmcnaughton this may or may not be useful for you in understanding how to create pseudo fields in APIv4

tests/phpunit/api/v3/PaymentProcessorTypeTest.php

Move default values to spec as per Coleman Fix class name in tests and fix default values on tables Mock financial_account_id field Fix headers and remove some unnessary code Update api v4 code following move of the handling to the BAO and update tests as per Coleman

seamuslee001 · 2020-01-12T02:15:29Z

Jenkins re test this please

colemanw · 2020-01-12T14:26:23Z

Great work @seamuslee001

civibot bot added the master label Oct 27, 2019

seamuslee001 force-pushed the payment_processor_api4 branch 2 times, most recently from 3c3333d to ceb8de8 Compare October 27, 2019 03:42

mattwire reviewed Oct 27, 2019

View reviewed changes

colemanw requested changes Oct 27, 2019

View reviewed changes

seamuslee001 commented Oct 27, 2019

View reviewed changes

seamuslee001 force-pushed the payment_processor_api4 branch 3 times, most recently from 0b08bf8 to b2893c0 Compare October 27, 2019 23:53

seamuslee001 force-pushed the payment_processor_api4 branch 2 times, most recently from ea5e4e9 to 8c91f60 Compare October 29, 2019 22:12

eileenmcnaughton reviewed Nov 5, 2019

View reviewed changes

seamuslee001 mentioned this pull request Nov 5, 2019

Schema changes for PaymentProcessor and PaymentProcessorType to suppo… #15733

Merged

seamuslee001 force-pushed the payment_processor_api4 branch from 8c91f60 to bb20a70 Compare November 9, 2019 20:20

seamuslee001 force-pushed the payment_processor_api4 branch from bb20a70 to 847f13a Compare December 15, 2019 04:48

seamuslee001 force-pushed the payment_processor_api4 branch 3 times, most recently from 2216c0b to 8685927 Compare December 15, 2019 20:16

seamuslee001 force-pushed the payment_processor_api4 branch from 8685927 to 985815f Compare January 10, 2020 23:41

colemanw reviewed Jan 11, 2020

View reviewed changes

seamuslee001 force-pushed the payment_processor_api4 branch from 985815f to 8fe21e5 Compare January 11, 2020 21:20

seamuslee001 commented Jan 11, 2020

View reviewed changes

seamuslee001 force-pushed the payment_processor_api4 branch from 8fe21e5 to 8617314 Compare January 11, 2020 23:04

colemanw reviewed Jan 11, 2020

View reviewed changes

tests/phpunit/api/v3/PaymentProcessorTypeTest.php Outdated Show resolved Hide resolved

colemanw reviewed Jan 11, 2020

View reviewed changes

tests/phpunit/api/v3/PaymentProcessorTypeTest.php Outdated Show resolved Hide resolved

seamuslee001 force-pushed the payment_processor_api4 branch from 8617314 to 74e272a Compare January 11, 2020 23:52

colemanw added the merge on pass label Jan 12, 2020

colemanw approved these changes Jan 12, 2020

View reviewed changes

seamuslee001 merged commit 0b28857 into civicrm:master Jan 12, 2020

seamuslee001 deleted the payment_processor_api4 branch January 12, 2020 03:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement PaymentProcessor and PaymentProcessorType APIv4 Entities #15624

Implement PaymentProcessor and PaymentProcessorType APIv4 Entities #15624

seamuslee001 commented Oct 27, 2019

civibot bot commented Oct 27, 2019

mattwire Oct 27, 2019

mattwire Oct 27, 2019

eileenmcnaughton Oct 27, 2019

mattwire commented Oct 27, 2019

colemanw Oct 27, 2019

seamuslee001 Oct 27, 2019

colemanw Jan 11, 2020

seamuslee001 Jan 11, 2020

seamuslee001 commented Oct 27, 2019

seamuslee001 Oct 27, 2019

seamuslee001 Oct 27, 2019

colemanw commented Oct 28, 2019

eileenmcnaughton commented Oct 29, 2019

eileenmcnaughton Nov 5, 2019

seamuslee001 Nov 5, 2019

eileenmcnaughton Nov 5, 2019

seamuslee001 commented Dec 15, 2019

colemanw Jan 11, 2020

seamuslee001 commented Jan 11, 2020

seamuslee001 Jan 11, 2020

colemanw Jan 11, 2020

seamuslee001 Jan 11, 2020

seamuslee001 Jan 11, 2020

seamuslee001 commented Jan 12, 2020

colemanw commented Jan 12, 2020

		'description' => ts('Payment Processor Name.'),
		'required' => TRUE,

Implement PaymentProcessor and PaymentProcessorType APIv4 Entities #15624

Implement PaymentProcessor and PaymentProcessorType APIv4 Entities #15624

Conversation

seamuslee001 commented Oct 27, 2019

Overview

Before

After

Technical Details

civibot bot commented Oct 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattwire commented Oct 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seamuslee001 commented Oct 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colemanw commented Oct 28, 2019

eileenmcnaughton commented Oct 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seamuslee001 commented Dec 15, 2019

Choose a reason for hiding this comment

seamuslee001 commented Jan 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seamuslee001 commented Jan 12, 2020

colemanw commented Jan 12, 2020