Add call to validateAllContributions & fix getTotalAmount #20391
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)
|
monishdeb
reviewed
May 24, 2021
| $contribution1 = $this->callAPISuccess('contribution', 'get', ['id' => $contribution['id'], 'return' => 'tax_amount', 'sequential' => 1]); | ||
| $this->callAPISuccess('contribution', 'completetransaction', [ | ||
| 'id' => $contribution['id'], | ||
| $params = array_merge($this->_params, ['contribution_status_id' => 2, 'sequential' => 1, 'financial_type_id' => $financialTypeId]); |
There was a problem hiding this comment.
@eileenmcnaughton can you please change the 2 to 'Pending' !!
There was a problem hiding this comment.
I just moved it down - see it's a few lines above - - but it should be Pending if I'm changing it
There was a problem hiding this comment.
oops I mean changing 2 to 'Pending'.
There was a problem hiding this comment.
OK - updated - I actually flicked it over to Order.create which no longer requires contribution_status_id (forces it to pending)
I've updated 2 tests for readability and extended one to call validateAllContributions. The call failed and it turned out to be the check was wrong. Fixing that caused a test to fail. I have fixed the test by fixing getTotalAmount on the internal helper BAO_Order class to include the tax_amount for each line in the total. I think this would most affect tests - but it might affect the Membership_Form due to changes made recently to that form so I'm on the fence about going for the rc with this one
ff79dd8 to
53c8b1b
Compare
|
@monishdeb well at least this one has passed! |
|
Looks good, tested on local. r-run passed. |
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
Add call to validateAllContributions & fix getTotalAmount
Before
The check in validateAllContributions is checking that the contribution.total_amount = SUM(line_item.line_total) but in fact it should be SUM(line_item.line_total) + SUM(line_item.tax_amount). On fixing that (to allow the check to be added to testCompleteTransactionFeeAmount & pass I found that testSubmitContributionPageWithPriceSetQuantity was failing - however the failure is in the part that is used by the unit tests & the (unused out side of tests ? ) ContributionPage.submit api.
After
$order->getTotalAmount()fixed to be SUM(line_item.line_total) + SUM(line_item.tax_amount)Technical Details
I found this trying to extend test cover to allow us to deal with #20357 - it's possible this could be a regression in the back office membership class which does use this function - but I'm unclear about that and am on the fence about putting this against master or the rc
Comments