-
-
Notifications
You must be signed in to change notification settings - Fork 824
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
dev/core#927 Add shell extension to move contribution cancel actions into #18784
Conversation
(Standard links)
|
a4d1d4b
to
8fc39d3
Compare
8fc39d3
to
c00edb2
Compare
* | ||
* @link https://docs.civicrm.org/dev/en/latest/hooks/hook_civicrm_post | ||
*/ | ||
function contributioncancelactions_civicrm_post($op, $objectName, $objectId, $objectRef) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eileenmcnaughton We'll need to use civicrm_postCommit (https://docs.civicrm.org/dev/en/latest/hooks/hook_civicrm_postCommit/) to be safe here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm - you could make a case either way - currently it's within the transaction so if it fails the whole thing does - which would make the case that fixing the related entities is integral.
*/ | ||
function contributioncancelactions_civicrm_post($op, $objectName, $objectId, $objectRef) { | ||
if ($op === 'edit' && $objectName === 'Contribution') { | ||
if ($objectRef->contribution_status_id === CRM_Core_PseudoConstant::getKey('CRM_Contribute_BAO_Contribution', 'contribution_status_id', 'Cancelled')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eileenmcnaughton This might be a bit of a pain to resolve but this doesn't actually do what we need - this would fire every time a contribution that has status "Cancelled" is edited. But what we actually need is a "contribution status was changed to cancelled" event.
Maybe we should consider introducing a set of symfony events/hooks that explicitly notify on eg. contribution cancel. I've implemented a few (custom) things like this and civirules does similar things but the only way to do it is rather messy because you have to capture data (ie. the before state) in the pre hook, store it statically and then compare it in the post hook.
Potentially we could find a place deep down in the BAO/DAO (eg. writeObjects) that would trigger "specialised" events like this. I'm not sure what's best, maybe @totten @colemanw have some ideas.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mattwire I was assuming we would largely get around that by checking for related pending entities & cancelling them - so the lack of pending entities would prevent it from acting twice
@eileenmcnaughton You've got a better handle on how you think this will work for now and my comments may be more relevant for situations such as refunds rather than cancellations. I'm happy for this to be merged as-is and we can always work through issues as they come up. |
Ok - lets' merge this shell & move onto the tests |
Overview
This is the shell of the extension as discussed in https://lab.civicrm.org/dev/core/-/issues/927 - the intent is to move all the code from the various places that cancel memberships, participant payments and pledge payments when the corresponding contribution is cancelled into this extension.
The extension would be enabled by default but could be disabled to allow an extension or otherwise to configure different behaviour - or none at all
Before
Multiple parts of the code cancel related entities in a way that can not easily be altered by extensions
After
The related cancellations will be in an extension that can be disabled
Technical Details
This is only the extension shell so far
Comments