dev/financial#6 Use template contribution for Contribution.repeattransaction #22487
Conversation
|
(Standard links)
|
mattwire
force-pushed
the
repeattransactiontemplatecontribution
branch
from
47ea3c5
to
cc63dda
Compare
mattwire
force-pushed
the
repeattransactiontemplatecontribution
branch
from
cc63dda
to
9c15bfd
Compare
|
test this please |
| ]); | ||
| $templateContribution = CRM_Contribute_BAO_ContributionRecur::getTemplateContribution($params['contribution_recur_id']); | ||
| if (empty($templateContribution)) { | ||
| throw new CiviCRM_API3_Exception('Contribution.repeattransaction failed to get original_contribution_id for recur with ID: ' . $params['contribution_recur_id']); |
There was a problem hiding this comment.
can you change to API_Exception to make it APIv4 friendly too
There was a problem hiding this comment.
Actually I think within apiv3 this is correct.
API_Exception is a bit of a pain in a way - ideally we could catch 'all CiviCRM extensions' with one catch type but API_Exception doesn't extend any other CiviCRM exception types - I'm guessing that was before it was handed over to CT @colemanw ?
There was a problem hiding this comment.
@monishdeb This code is within an API3 function so it is unlikely to ever be called by API4? There is quite a bit within the function which would probably be cleaned up before creating a "repeattransaction" for API4 - see eg. #22488 for a start.
| $this->assertEquals(7, $contributions['values'][0]['contribution_status_id']); | ||
| $this->assertEquals(2, $contributions['values'][1]['contribution_status_id']); | ||
| } | ||
|
|
There was a problem hiding this comment.
@eileenmcnaughton @mattwire I am happy with the logic, but I am not sure moving forward, are we relying on APIv4 to do CRUD operations on new unit tests? if it doesn't matter unless the UT captures the fix/change accurately then I am ok here.
There was a problem hiding this comment.
@monishdeb I think you are saying 'would this be better if we were using apiv4 instead of 3 in the tests.
I guess we are moving that way - but we are not so far down the track I'm alarmed at adding new tests that use apiv3 - in this case, however, I think v4 would be better on the final get because it would make it easy to check contribution_status_id:name rather than the hard-coded '2' & '7'
There was a problem hiding this comment.
yes contribution_status_id:name is the other reason I felt like APIv4 would be appropriate here.
( I wish there's a emoticon for hi5 
)
There was a problem hiding this comment.
@eileenmcnaughton @monishdeb I agree with preferring to use API4 but I'm not sure I understand fully how we would implement it for the tests? We can set $this->_apiversion = 4; but in this case repeattransaction does not exist for API4 so we would have to change api version during the test which feels a bit messy?
There was a problem hiding this comment.
@mattwire generally I just call the v4 api direct in the tests (unless specifically testing apiv3 or both 3 & 4) - the callApiSuccess helpers are helpful for apiv3 but don't add much to api v4 - so it's really just that last Contribution::get where using apiv4 would make it easy to eliminate the checks on hard-coded status ids (which it the bit that raised the discussion)
|
@mattwire what is the status of this PR? It is a bit unclear to us whether it needs work or whether it is ready for review. And if it needs to work do you plan to work on this? If not we would advice to close this PR. |
|
@jaapjansma This is quite an important change that affects a few different payment processors (stripe, gocardless etc.). I think that it is only stuck on whether we should start writing tests using api4 or not and only @eileenmcnaughton can answer that - but in my opinion it should not block review/merge of this PR. In my opinion it is ready for review. |
|
I agree using apiv4 is better, but not blockimg |
|
Thanks @eileenmcnaughton @artfulrobot this should solve one of your workarounds in gocardless (will be in 5.51 I think) |
|
Cheers all. @mattwire it will only solve my problem when GC starts using template contributions! 😆 |
|
@artfulrobot Despite the name of this PR it's not actually specific to template contributions and should fix eg. https://lab.civicrm.org/extensions/gocardless/-/blob/main/CRM/Core/Payment/GoCardlessIPN.php#L568 |
Overview
Calling contribution.repeattransaction without specifying
original_contribution_idcauses it to try to get the "previous" contribution inCompletedstatus. That doesn't work if the previous contribution is in any other status (eg.Refunded,Failed).We should be using the generic function to get the template contribution as this allows for us to get the values from the template contribution if available or the latest contribution otherwise.
Before
Contribution.repeattransactioncrashes if first contribution is not "Completed". Template contribution is ignored.After
Template contribution is retrieved and used.
Technical Details
See https://lab.civicrm.org/dev/financial/-/issues/6
Comments