[REF] Simplify getContributionStatuses #22280
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
(Standard links)
|
|
Jenkins re test this please |
CRM/Event/Form/Participant.php
Outdated
| @@ -1733,7 +1733,7 @@ public function buildEventFeeForm($form) { | |||
| } | |||
|
|
|||
| $form->add('select', 'contribution_status_id', | |||
| ts('Payment Status'), CRM_Contribute_BAO_Contribution_Utils::getContributionStatuses('participant') | |||
| ts('Payment Status'), CRM_Contribute_BAO_Contribution_Utils::getPendingAndCompleteStatuses('participant') | |||
There was a problem hiding this comment.
I'm still trying to get my head around the earlier function but this function doesn't take a parameter here.
There was a problem hiding this comment.
oh yeah - that was lazy of me
The effect of the altered calls to getContributionStatus is to get a small set of options returned. In the cases of participant & membership forms this is pending or completed. For the ufGroup option it seems cancelled & failed are also data entry options (this method should still cope if they don't exist). The existing function is really confusing - but all that confusion is designed for one form - back office contribution form - and augmented by overloading by other functions. This splits it out. Could sites be using 'weird and wonderful other statuses'? Our system really is set up around supporting specific statuses and doing financial entries for those so I think maybe we are right to not try to pander to any that could exist
eileenmcnaughton
force-pushed
the
statuses
branch
from
dac4e9d
to
a3d9eb3
Compare
|
Jenkins retest this please - |
|
Good job unravelling that function. Contribution status = "better". |
eileenmcnaughton
added a commit
to eileenmcnaughton/civicrm-core
that referenced
this pull request
Dec 30, 2021
Follow up to civicrm#22280
eileenmcnaughton
added a commit
to eileenmcnaughton/civicrm-core
that referenced
this pull request
Dec 30, 2021
It is no longer used in core but I found 2 universe places so I added back some handling for other entities & deprecated it and stopped using it entirely in core See civicrm#22280
|
Just noting I was going to remove the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
[REF] Simplify getContributionStatuses
The existing function is really confusing - but all that confusion is designed for one
form - back office contribution form - and augmented by overloading by other functions.
This splits it out. The same options are returned - however if sites have added their own options they would not be returned - we don't support that in the UI and our financial code really does expect a limited range of statuses so I don't know if that is a thing - maybe highlight in release notes?
Before
Function is super hard to read - being called to do different things makes that hard to unwravel. The function is used to get the drop down options for available statuses in 5 scenarions
After
The first 4 scenarios call simpler functions - leaving the last to be fixed up.
Technical Details
Could sites be using 'weird and wonderful other statuses'? Our system really is set up
around supporting specific statuses and doing financial entries for those so I think maybe
we are right to not try to pander to any that could exist
Comments