[REF] Clean up handling of financial_type_id override #18032
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)
|
eileenmcnaughton
force-pushed
the
single
branch
from
633c647 to
dbd0abb
Compare
|
test this please |
eileenmcnaughton
force-pushed
the
single
branch
from
dbd0abb to
fb513de
Compare
mattwire
reviewed
Aug 2, 2020
mattwire
reviewed
Aug 2, 2020
| ); | ||
| 'contribution_status_id' => 'Pending', | ||
| ], $generalParams); | ||
| $contributionParams['api.Payment.creaate'] = $contributionParams['total_amount']; |
There was a problem hiding this comment.
argh - fixed
eileenmcnaughton
force-pushed
the
single
branch
from
fb513de to
e427daf
Compare
|
@eileenmcnaughton test fail may relate here |
eileenmcnaughton
force-pushed
the
single
branch
from
e427daf to
6ecf323
Compare
|
test fail related to this line https://github.com/civicrm/civicrm-core/pull/18032/files#diff-40e2e0f106ba620465acf3a9a81f2498R3051 being missing. It has have been the beneficiary of some leakage before this patch |
Currently we use the primaryContributionID to determine whether the templateContribution will have more than one line item & unset the parameter. It makes much more sense to do evaluations on how many line items the template contribution will have when we have loaded it's line items. The logic is that the financial_type_id can be overriden either by editing the recurring contribution (which you are permitted to do if only one line item exists) or by passing it in via input (which is ignored if there is more than one line item. This change ensures the input parameter reaches repeatTrannsaction - which is the appropriate place to determine whether to apply it. It removes one of 2 places that refers to primaryContributionID. The other is also inappropriate as it is basically used to look up the line items to see the number of terms for the membership. However, we already know that for any repeat transactions as we have already loaded the line items. In completeOrder the line items are that of the contribution being saved. We could pass the saved contribution id into this function as a quick way to eliminate the primaryContributionID or we could just determine num_terms rather than primaryContributionID earlier on
eileenmcnaughton
force-pushed
the
single
branch
from
6ecf323 to
fa839a6
Compare
eileenmcnaughton
commented
Aug 3, 2020
| @@ -4486,7 +4485,6 @@ public static function completeOrder($input, &$ids, $objects, $transaction = NUL | |||
| $changeDate = CRM_Utils_Array::value('trxn_date', $input, date('YmdHis')); | |||
|
|
|||
| $contributionResult = self::repeatTransaction($contribution, $input, $contributionParams); | |||
| $contributionParams['financial_type_id'] = $contribution->financial_type_id; | |||
There was a problem hiding this comment.
@mattwire with the other merged I'm just removing the setting & unsetting of $contributionParams['financial_type_id']
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
Consolidates handling of calculation as to whether to permit the financial_type_id to be overriden when repeating the transactionn
Before
This is calculated in getTemplateContribution - however there is an additional attempt to calculate it in the main completeOrder function. The calculation is only relevant for repeattransaction & code (which I added comments to) later ensures it is is unset for the complete flow
After
The $input['financial_type_id'] (if any) reaches repeattransaction and all relevant calculations as to whether to permit the override are done in getTemplateContribution
Technical Details
Currently we use the primaryContributionID to determine whether the templateContribution will
have more than one line item & unset the parameter. It makes much more sense to do evaluations
on how many line items the template contribution will have when we have loaded it's line items.
The logic is that the financial_type_id can be overriden either by editing the recurring contribution
(which you are permitted to do if only one line item exists) or by passing it in via input (which
is ignored if there is more than one line item.
This change ensures the input parameter reaches repeatTrannsaction - which is the appropriate
place to determine whether to apply it.
It removes one of 2 places that refers to primaryContributionID. The other is also inappropriate
as it is basically used to look up the line items to see the number of terms for the membership. However,
we already know that for any repeat transactions as we have already loaded the line items. In
completeOrder the line items are that of the contribution being saved.
We could pass the saved contribution id into this function as a quick way to eliminate the primaryContributionID
or we could just determine num_terms rather than primaryContributionID earlier on
Comments