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

Remove unused parameters from cancel #18997

Merged
merged 1 commit into from
Nov 22, 2020

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Nov 19, 2020

Overview

Follow up cleanup on BAO_Contribution::fail - remove always FALSE value and unused / always FALSE return values

Before

  protected static function cancel($processContributionObject, $memberships, $contributionId, $membershipStatuses, $updateResult, $participant, $oldStatus, $pledgePayment, $pledgeID, $pledgePaymentIDs, $contributionStatusId) {

Screenshot from 2020-11-20 10-39-44

After

poof

  protected static function cancel($memberships, $contributionId, $membershipStatuses, $participant, $oldStatus, $pledgePayment, $pledgeID, $pledgePaymentIDs, $contributionStatusId) {

Technical Details

cancel as a protected function is only called from one place and the processContribution value is always false.
This means that the returned processContribution value is always false - so we don't need to return
it as it is set to false already. In addition the currently returned updateResult is no longer
used so we don't need to return that either.

I will do the same changes in a separate PR for processFail

Comments

@civibot
Copy link

civibot bot commented Nov 19, 2020

(Standard links)

@civibot civibot bot added the master label Nov 19, 2020
cancel as a protected function is only called from one place and the processContribution value is always false.
This means that the returned processContribution value is always false - so we don't need to return
it as it is set to false already. In addition the currently returned updateResult is no longer
used so we don't need to return that either.

I will do the same changes in a separate PR for processFail
@eileenmcnaughton
Copy link
Contributor Author

@seamuslee001 this & the related are the next 2 minor cleanups on this code - basically we removed some functionality (onlinepending, details pop ups) from functions that called this so just working through all the stuff that is no longer needed

eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Nov 21, 2020
Once civicrm#18997 & civicrm#18998 is merged it becomes clear that processContribution can never be true & hence this code is unreachable
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Nov 21, 2020
Once civicrm#18997 & civicrm#18998 is merged it becomes clear that processContribution can never be true & hence this code is unreachable
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Nov 21, 2020
Once civicrm#18997 & civicrm#18998 is merged it becomes clear that processContribution can never be true & hence this code is unreachable
@eileenmcnaughton
Copy link
Contributor Author

@seamuslee001 @colemanw can I get a merge on this & #18998 - in the end both functions will go altogether

eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Nov 22, 2020
This test checks that 1 & only 1 activity is created when a membership is cancelled because the related contribution is cancelled.

It is currently failing as there is an extra 'status changed from cancelled to cancelled' activity created.

The fix is simple but not included as civicrm#18997 needs to be merged first
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Nov 22, 2020
This test checks that 1 & only 1 activity is created when a membership is cancelled because the related contribution is cancelled.

It is currently failing as there is an extra 'status changed from cancelled to cancelled' activity created.

The fix is simple but not included as civicrm#18997 needs to be merged first
@seamuslee001
Copy link
Contributor

This looks ok to me merging

@seamuslee001 seamuslee001 merged commit 3e0abe7 into civicrm:master Nov 22, 2020
@seamuslee001 seamuslee001 deleted the trans branch November 22, 2020 19:38
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Nov 22, 2020
This test checks that 1 & only 1 activity is created when a membership is cancelled because the related contribution is cancelled.

It is currently failing as there is an extra 'status changed from cancelled to cancelled' activity created.

The fix is simple but not included as civicrm#18997 needs to be merged first
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Nov 22, 2020
Once civicrm#18997 & civicrm#18998 is merged it becomes clear that processContribution can never be true & hence this code is unreachable
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Nov 22, 2020
Once civicrm#18997 & civicrm#18998 is merged it becomes clear that processContribution can never be true & hence this code is unreachable
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Nov 23, 2020
Once civicrm#18997 & civicrm#18998 is merged it becomes clear that processContribution can never be true & hence this code is unreachable
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Nov 23, 2020
This test checks that 1 & only 1 activity is created when a membership is cancelled because the related contribution is cancelled.

It is currently failing as there is an extra 'status changed from cancelled to cancelled' activity created.

The fix is simple but not included as civicrm#18997 needs to be merged first
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Nov 24, 2020
This test checks that 1 & only 1 activity is created when a membership is cancelled because the related contribution is cancelled.

It is currently failing as there is an extra 'status changed from cancelled to cancelled' activity created.

The fix is simple but not included as civicrm#18997 needs to be merged first
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.

2 participants