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

dev/core#927 Update ContributionCancelActions to also handle 'failed' #19015

Merged
merged 2 commits into from
Nov 24, 2020

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Nov 22, 2020

Overview

This extends the contributioncancelactions extension to also work for Failed contributions

Before

Failed contributions still handled in the IPN

After

Failed contributions handled via the hook

Technical Details

The failed action has no discernable difference from the no-longer-used cancel action. The cancel action had some minor refactoring
done to work through the conclusion that the call to addRecurLineItems was never reached in a meaningful way
but I think we can skip all that & see that fail is not functionally different to cancel & just deprecate the function.

At this point we are very close to simply removing the last core handling for failed & cance & making the extension visible.

The only thing I can see that still needs checking in the 2 functions still to be removed is the handling of
is_override & override_date is tested - which I will do as a follow up

I removed the test CRM_Member_Form_MembershipTest::testFormWithFailedContribution as this is no longer possible in the UI & hence not valid. The test coverred adding / editing a membership and recording a failed contribution in the process. However the only exposed statuses are Pending & Completed

Comments

https://lab.civicrm.org/dev/core/-/issues/927

@civibot
Copy link

civibot bot commented Nov 22, 2020

(Standard links)

@civibot civibot bot added the master label Nov 22, 2020
@eileenmcnaughton eileenmcnaughton changed the title Update ContributionCancelActions to also handle 'failed' dev/core#927 Update ContributionCancelActions to also handle 'failed' Nov 22, 2020
'cancel_date' => 'now',
'contribution_status_id:name' => 'Cancelled',
])->addWhere('id', '=', $contributionID)->execute();
Civi::log()->debug("Setting contribution status to Failed");
Copy link
Contributor

Choose a reason for hiding this comment

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

@eileenmcnaughton The comment is not correct anymore - we are actually setting it to "Cancelled", unless that's the typo?

'cancel_date' => 'now',
'contribution_status_id:name' => 'Cancelled',
])->addWhere('id', '=', $contribution->id)->execute();
Civi::log()->debug("Setting contribution status to Failed");
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

The failed action has no discernable difference from the no-longer-used cancel action. The cancel action had some minor refactoring
done to work through the conclusion that the call to addRecurLineItems was never reached in a meaningful way
but I think we can skip all that & see that fail is not functionally different to cancel & just deprecate the function.

At this point we are very close to simply removing the last core handling for failed & cance & making the extension visible.

The only thing I can see that still needs checking in the 2 functions still to be removed is the handling of
is_override & override_date is tested - which I will do as a follow up
It is no longer possible to enter a contribution with a cancelled or failed status via the membership form so
this test is not valid. Only pending or completed are permitted
@eileenmcnaughton
Copy link
Contributor Author

Thanks @mattwire definitely a copy & paste fail (should that be cancel) - fixed now

@seamuslee001
Copy link
Contributor

This seems to be fine to me merging

@seamuslee001 seamuslee001 merged commit cb6b0ed into civicrm:master Nov 24, 2020
@seamuslee001 seamuslee001 deleted the fail branch November 24, 2020 00:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants