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

[REF] Convert Elavon Payment Processor to be a core Extension #24183

Merged
merged 1 commit into from
Aug 19, 2022

Conversation

seamuslee001
Copy link
Contributor

Overview

This Converts Elavon to be a core extension and adds in a unit test on the Payment Processor

Before

Elavon Payment Processor shipped in core

After

Elavon Payment Processor shipped as a core extension and disabled by default

Note I did the upgrade script for 5.54.alpha1 because I figured we would do this post branching but it seems branching has been delayed a while

ping @eileenmcnaughton @totten @monishdeb

@civibot
Copy link

civibot bot commented Aug 8, 2022

(Standard links)

@civibot civibot bot added the master label Aug 8, 2022
@seamuslee001 seamuslee001 force-pushed the elavon_extension_convert branch 3 times, most recently from d488da1 to 993a327 Compare August 8, 2022 23:22
@eileenmcnaughton
Copy link
Contributor

This looks good from a code point of view - I'm going to r-run the install & rely on your test / your prod site testing ./ the fact the processor part is pretty well worn code for the Elavon functionality

A couple of points though

  • mixins - my understanding is we don't need to include the mixin folder for extension releases that don't support earlier CiviCRM versions (including core extensions`
  • mgd values for cleanup and update - I think the new install will wind up with unused for cleanup - which probabaly should be the default (even thought I personally mistrust it). I'm not sure about 'update` flip a coin

@seamuslee001
Copy link
Contributor Author

Re Mixins I just presumed we committed everything and the Mixin files were there just as a backup

@eileenmcnaughton
Copy link
Contributor

I checked a handful of other core extensions & they were 'mixin-free`

@colemanw
Copy link
Member

colemanw commented Aug 9, 2022

Correct, and if you are using the latest version of Civix, it should not have generated the mixin directory if your <ver> is set high enough in info.xml, which it is so I'm not sure what's going on there, but at any rate the directory can be removed.

@seamuslee001 seamuslee001 force-pushed the elavon_extension_convert branch from 993a327 to 2876a20 Compare August 11, 2022 05:27
@seamuslee001
Copy link
Contributor Author

ok I have removed the mixin folder and rebased to fix the conflict but I think this should be fine

@seamuslee001
Copy link
Contributor Author

Jenkins re test this please

@seamuslee001 seamuslee001 force-pushed the elavon_extension_convert branch 3 times, most recently from 3ae9448 to 36a2fd9 Compare August 16, 2022 23:58
@seamuslee001
Copy link
Contributor Author

@eileenmcnaughton is there any other issues with this PR now?

@seamuslee001 seamuslee001 force-pushed the elavon_extension_convert branch from 36a2fd9 to 7b322ca Compare August 17, 2022 00:02
@eileenmcnaughton
Copy link
Contributor

I'll wait & see if tests pass & then give it an r-run

@seamuslee001 seamuslee001 force-pushed the elavon_extension_convert branch 3 times, most recently from a85be08 to 615eeac Compare August 18, 2022 04:40
@seamuslee001 seamuslee001 force-pushed the elavon_extension_convert branch from 615eeac to 64b87c9 Compare August 18, 2022 21:25
@seamuslee001
Copy link
Contributor Author

@eileenmcnaughton this is now rebased and as mentioned in call the upgrade tests some how cover both ways i.e. when you have payment processors of type elavon and when you don't

@eileenmcnaughton
Copy link
Contributor

I gave this an r-run on the upgrade but am relying on the tests for the processor functionality as I don't have a way to test & the process @seamuslee001 went through to add tests on that first was solid

@seamuslee001 seamuslee001 merged commit 77b49e8 into civicrm:master Aug 19, 2022
@seamuslee001 seamuslee001 deleted the elavon_extension_convert branch August 19, 2022 00:09
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.

3 participants