dev/core#1558 [REF] Remove unnecessary query, clean up silly function #16412
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)
|
getInvoiceNumber returns NULL if the 'invoicing' setting is disabled, by checking slightly earlier we can skip a query if it is not Upgrade fix: Copy code to upgrade script rather than call a function. The function will change as we fix the setting but we want the upgrade function to work off the old setting Test fix: Use hard coded invoicePrefix We know what it is - if it were not set right then the test would not pick it up because it is comparing based on the assumption it is - using the string is more reliable
69b4488 to
5d288dc
Compare
| @@ -132,8 +132,7 @@ public function tearDown() { | |||
| * @throws \CRM_Core_Exception | |||
| */ | |||
| public function testGetContribution() { | |||
| $contributionSettings = $this->enableTaxAndInvoicing(); | |||
| $invoice_prefix = CRM_Contribute_BAO_Contribution::checkContributeSettings('invoice_prefix', TRUE); | |||
There was a problem hiding this comment.
If this incorrectly returned empty then we would be incorrectly checking it was empty - better to test the known value that we set in enableTaxAndInvoicing
|
@eileenmcnaughton looks good to me. |
|
Merging based on @yashodha review |
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
This simplifies the checkContributeSettings function.
Before
$name parameter is optional & there is handling for it to be null - but it is never not passed in.
$checkInvoicing parameter means a result is only returned if the invoicing setting is also true. However in no case is it a good choice:
from getInvoiceNumber - but it makes more sense to check before calling this function if invoicing is enabled as this saves us calculating $nextContributionID if we don't need it. The other place getInvoiceNumber is called from is the invoice code so we can be comfortable invoicing is enabled & no check is required.
a unit test - where the test is more reliable if we hard code our expected result
an upgrade clause - we are better here to copy the function into the upgrade code because the upgrade wants to deal with the setting as it was for 4.7x users & not 'adapt' as we use a more up to date version of how the setting is stored.
After
checkContributeSettings has one required parameter (name). We check if invoicing is enabled before calling getInvoiceNumber. Upgrade code has it's own copy.
Technical Details
Comments