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.

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

Update contribution transact code to use version from contributiontransactlegacy extension #437

Merged
merged 1 commit into from
Jan 23, 2021

Conversation

mattwire
Copy link
Collaborator

Overview

Follow up to #434

Before

Using same code as core which doesn't really follow "good practise" for payments.

After

Use code from contributiontransactlegacy extension which is updated to follow "good practise" for payments.

Technical Details

Comments

Note I didn't test this yet..

Comment on lines -677 to -690
/**
* Adjust Metadata for Transact action.
*
* The metadata is used for setting defaults, documentation & validation.
*
* @param array $params
* Array of parameters determined by getfields.
*/
function _wf_civicrm_api3_contribution_transact_spec(&$params) {
$fields = civicrm_api3('Contribution', 'getfields', ['action' => 'create']);
$params = array_merge($params, $fields['values']);
$params['receive_date']['api.default'] = 'now';
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@KarinG Why did you include this in the original PR? I don't think it is relevant?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh that was just a copy/paste. I noticed you had that in your replacement Extension as well and figured it may be useful.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It won't do anything except in the context of API3 so not needed here.

@KarinG
Copy link
Collaborator

KarinG commented Jan 17, 2021

Downloading the browser artifacts (hit Details on test that failed):
image

Then looking through the HTML output -> FunctionalJavascript_ContributionPageTest-8-11138688.html -> shows this:

The website encountered an unexpected error. Please try again later.
Error: Class 'Drupal\webform_civicrm\ReflectionClass' not found in Drupal\webform_civicrm\Utils->wf_civicrm_api3_contribution_transact() (line 715 of /home/runner/work/webform_civicrm/webform_civicrm/src/Utils.php).


$paymentProcessor = \CRM_Financial_BAO_PaymentProcessor::getPayment($params['payment_processor'], $params['payment_processor_mode']);
$paymentProcessor['object']->doPayment($params);

Copy link
Collaborator

Choose a reason for hiding this comment

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

No more doPayment?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, it's called via PaymentProcessor.Pay API.


$reflectionClass = new ReflectionClass($propertyBag);
$reflectionProperty = $reflectionClass->getProperty('props');
$reflectionProperty->setAccessible(TRUE);
Copy link
Collaborator

@KarinG KarinG Jan 17, 2021

Choose a reason for hiding this comment

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

What is ReflectionClass()?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Have now added a comment to the code. It's a way of accessing protected vars/methods in a class because civicrm/civicrm-core#17507 has not (yet) been merged... We need PropertyBag as an array and this gets it.

@KarinG
Copy link
Collaborator

KarinG commented Jan 17, 2021

Excellent @mattwire - looks like you have the ContributionPageTest ✅ working!

I'll have a look later today or tomorrow - specifically at invoiceIDs.

Do you have access to a 'live' test account for Stripe (like I have for iATS) so that you can add a Functional Javascript Test? Copy ContributionPageTest to ContributionPageStripeTest -> i.e. make a new one for Stripe specifically. It would be awesome to ensure we have iATS and Stripe coverage for D8 & D9!

@KarinG
Copy link
Collaborator

KarinG commented Jan 23, 2021

I did a test with iATS 1stPay (JS) - payment processor and that also worked well - documenting:

image

image

@KarinG
Copy link
Collaborator

KarinG commented Jan 23, 2021

Ok merging this - assuming that will make it easier for you to add a ContributionStripeTest.php 🙂

Summary - this PR passes automated tests for:

  • Dummy ✅
  • iATS Legacy ✅

And manual tests for:

  • Stripe (assuming Matt tested this) 👍
  • iATS 1stPay 👍

@KarinG KarinG merged commit 18ec447 into colemanw:8.x-5.x Jan 23, 2021
@mattwire mattwire deleted the 8legacytransact branch January 23, 2021 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants