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

Convert IATS Settings to be standard practice #437

Merged
merged 1 commit into from
Nov 14, 2023

Conversation

seamuslee001
Copy link
Contributor

Iats Settings are currently done in a non standard way in a couple of different ways

  1. There is currently no metadata e.g. title etc for the settings
  2. All the settings are stored in a single array rather than as individual settings in the database as is the standard way for CiviCRM settings to be

This aims to address this by adding metadata for each individual setting and split up the array into individual settings rather than in the single array.

This function adds in helper function CRM_Iats_Utils::getSettings to handle the two different formats. This also has an upgrade step to perform the conversion.

The other thing this PR aims to do as per https://docs.civicrm.org/dev/en/latest/framework/setting/extension/ switches to using the Generic form for settings rather than a custom one.

@seamuslee001
Copy link
Contributor Author

@adixon thoughts on this?

@adixon
Copy link
Contributor

adixon commented Nov 14, 2023

Thank you, this looks excellent. Nice when other people clean up my technical debt!

I'll merge into master so it has some time to get tested before the next release.

@adixon adixon merged commit f2d6eec into iATSPayments:master Nov 14, 2023
@KarinG
Copy link
Contributor

KarinG commented Nov 17, 2023

And tested! We use iATS master in the WFC tests:

image

Backtrace is:

The website encountered an unexpected error. Please try again later.

TypeError: max(): Argument #1 ($value) must be of type array, string given in max() (line 256 of sites/default/files/civicrm/ext/com.iatspayments.civicrm/CRM/Core/Payment/iATSService.php).

CRM_Core_Payment_iATSService->doPayment() (Line: 116)
civicrm_api3_payment_processor_pay() (Line: 89)
Civi\API\Provider\MagicFunctionProvider->invoke() (Line: 156)
Civi\API\Kernel->runRequest() (Line: 79)

@adixon
Copy link
Contributor

adixon commented Nov 17, 2023

Thanks - that's a php8.? error unrelated to this pull, but I've just fixed it in master.

85f0a9d

Handy testing!

@KarinG
Copy link
Contributor

KarinG commented Nov 17, 2023

Test matrix runs a couple of time per week -> and 4 days ago all was green.

It used PHP 8.1.25 for both for this failed run as well as 4 days ago - so you're right it may not be this particular PR but it's a very recent one.

Will report back after the next run (if I re-run now - it really just re-runs - it has cached the code it pulls in - and don't think we need to run a one off)

PS - since this WFC testing is where we have useful coverage for the iATS extension - I'd like to remove our CiviCRM core tests -> PR = #432

image

@KarinG
Copy link
Contributor

KarinG commented Nov 19, 2023

Re: thanks - that's a php8.? error unrelated to this pull, but I've just fixed it in master

We still see the same error in the WFC test matrix (using iATS master) ->

image

@seamuslee001 - has another PR in the queue for review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants