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

Fix test to use valid financials #20956

Merged
merged 3 commits into from
Jul 27, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions Civi/Test/ContactTestTrait.php
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,6 @@ public function createLoggedInUser(): int {
*
* @return int
* id of Organisation created
*
* @throws \CiviCRM_API3_Exception
*/
public function organizationCreate($params = [], $seq = 0): int {
if (!$params) {
Expand Down
11 changes: 10 additions & 1 deletion tests/phpunit/CRM/Contribute/BAO/ContributionTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,13 @@ class CRM_Contribute_BAO_ContributionTest extends CiviUnitTestCase {
use CRMTraits_Financial_FinancialACLTrait;
use CRMTraits_Financial_PriceSetTrait;

/**
* Should financials be checked after the test but before tear down.
*
* @var bool
*/
protected $isValidateFinancialsOnPostAssert = TRUE;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This opts in the whole class, and then we opt out the 1 test it doesn't work for


/**
* Clean up after tests.
*/
Expand Down Expand Up @@ -666,7 +673,9 @@ public function checkItemValues($contribution) {
* @throws \CRM_Core_Exception
* @throws \CiviCRM_API3_Exception
*/
public function testAssignProportionalLineItems() {
public function testAssignProportionalLineItems(): void {
// This test doesn't seem to manage financials properly, possibly by design
$this->isValidateFinancialsOnPostAssert = FALSE;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this get reset for the tests that come after it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seamuslee001 yep - I just put in a break point in the next test &

image

$contribution = $this->addParticipantWithContribution();
// Delete existing financial_trxns. This is because we are testing a code flow we
// want to deprecate & remove & the test relies on bad data asa starting point.
Expand Down
12 changes: 3 additions & 9 deletions tests/phpunit/CRM/Event/Form/ParticipantTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -22,17 +22,11 @@ public function setUp(): void {
}

/**
* CHeck that all tests that have created payments have created them with the right financial entities.
* Should financials be checked after the test but before tear down.
*
* Ideally this would be on CiviUnitTestCase but many classes would still fail. Also, it might
* be good if it only ran on tests that created at least one contribution.
*
* @throws \CRM_Core_Exception
* @var bool
*/
protected function assertPostConditions(): void {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These asserts are now done by the parent if $isValidateFinancialsOnPostAssert = TRUE; is true

$this->validateAllPayments();
$this->validateAllContributions();
}
protected $isValidateFinancialsOnPostAssert = TRUE;

/**
* Initial test of submit function.
Expand Down
15 changes: 15 additions & 0 deletions tests/phpunit/CRM/Event/Form/Registration/ConfirmTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,17 @@ class CRM_Event_Form_Registration_ConfirmTest extends CiviUnitTestCase {

use CRMTraits_Profile_ProfileTrait;


/**
* Should financials be checked after the test but before tear down.
*
* Ideally all tests (or at least all that call any financial api calls ) should do this but there
* are some test data issues and some real bugs currently blocking.
*
* @var bool
*/
protected $isValidateFinancialsOnPostAssert = TRUE;

public function setUp(): void {
$this->useTransaction(TRUE);
parent::setUp();
Expand Down Expand Up @@ -95,6 +106,8 @@ public function testSubmit(): void {
* @dataProvider getThousandSeparators
*/
public function testPaidSubmit($thousandSeparator) {
// @todo - figure out why this doesn't pass validate financials
$this->isValidateFinancialsOnPostAssert = FALSE;
$this->setCurrencySeparators($thousandSeparator);
$mut = new CiviMailUtils($this);
$paymentProcessorID = $this->processorCreate();
Expand Down Expand Up @@ -222,6 +235,8 @@ public function testPaidSubmit($thousandSeparator) {
* @throws \Exception
*/
public function testTaxMultipleParticipant() {
// @todo - figure out why this doesn't pass validate financials
$this->isValidateFinancialsOnPostAssert = FALSE;
$mut = new CiviMailUtils($this);
$params = ['is_monetary' => 1, 'financial_type_id' => 1];
$event = $this->eventCreate($params);
Expand Down
1 change: 0 additions & 1 deletion tests/phpunit/CiviTest/CiviUnitTestCase.php
Original file line number Diff line number Diff line change
Expand Up @@ -486,7 +486,6 @@ protected function loadXMLDataSet($fileName) {
* Create default domain contacts for the two domains added during test class.
* database population.
*
* @throws \CiviCRM_API3_Exception
* @throws \API_Exception
*/
public function createDomainContacts(): void {
Expand Down
10 changes: 10 additions & 0 deletions tests/phpunit/api/v3/ACLPermissionTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,16 @@ class api_v3_ACLPermissionTest extends CiviUnitTestCase {

use Civi\Test\ACLPermissionTrait;

/**
* Should financials be checked after the test but before tear down.
*
* The setup methodology in this class bypasses valid financial creation
* so we don't check.
*
* @var bool
*/
protected $isValidateFinancialsOnPostAssert = FALSE;

public $DBResetRequired = FALSE;
protected $_entity;

Expand Down
38 changes: 17 additions & 21 deletions tests/phpunit/api/v3/ContributionTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -3468,8 +3468,6 @@ public function testCompleteTransactionMembershipPriceSet() {
* @throws \CiviCRM_API3_Exception
*/
public function testPendingToCompleteContribution(): void {
// @todo - figure out why this test is not valid.
$this->isValidateFinancialsOnPostAssert = FALSE;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

works because of the fix below

$this->createPriceSetWithPage('membership');
$this->setUpPendingContribution($this->_ids['price_field_value'][0]);
$this->callAPISuccess('membership', 'getsingle', ['id' => $this->_ids['membership']]);
Expand Down Expand Up @@ -3729,8 +3727,6 @@ public function testSendMail(): void {
*/
public function testSendConfirmationPayLater(): void {
$mut = new CiviMailUtils($this, TRUE);
// This probably needs to call the order api in order to generate valid financial entities.
$this->isValidateFinancialsOnPostAssert = FALSE;
// Create contribution page
$pageParams = [
'title' => 'Webform Contributions',
Expand All @@ -3741,30 +3737,30 @@ public function testSendConfirmationPayLater(): void {
'pay_later_text' => 'I will send payment by cheque',
'pay_later_receipt' => 'Send your cheque payable to "CiviCRM LLC" to the office',
];
$contributionPage = $this->callAPISuccess('contribution_page', 'create', $pageParams);
$contributionPage = $this->callAPISuccess('ContributionPage', 'create', $pageParams);

// Create pay later contribution
$contribParams = [
$contribution = $this->callAPISuccess('Order', 'create', [
'contact_id' => $this->_individualId,
'financial_type_id' => 1,
'is_pay_later' => 1,
'contribution_status_id' => 2,
'contribution_page_id' => $contributionPage['id'],
'total_amount' => '10.00',
];
$contribution = $this->callAPISuccess('contribution', 'create', $contribParams);

// Create line item
$lineItemParams = [
'contribution_id' => $contribution['id'],
'entity_id' => $contribution['id'],
'entity_table' => 'civicrm_contribution',
'label' => 'My lineitem label',
'qty' => 1,
'unit_price' => '10.00',
'line_total' => '10.00',
];
$this->callAPISuccess('LineItem', 'create', $lineItemParams);
'line_items' => [
[
'line_item' => [
[
'entity_table' => 'civicrm_contribution',
'label' => 'My lineitem label',
'qty' => 1,
'unit_price' => '10.00',
'line_total' => '10.00',
],
],
],
],
]);

// Create email
try {
Expand All @@ -3785,7 +3781,7 @@ public function testSendConfirmationPayLater(): void {
// Retrieve mail & check it has the pay_later_receipt info
$mut->getMostRecentEmail('raw');
$mut->checkMailLog([
(string) $contribParams['total_amount'],
(string) 10,
$pageParams['pay_later_receipt'],
], [
'Event',
Expand Down
11 changes: 10 additions & 1 deletion tests/phpunit/api/v3/LineItemTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,13 @@ class api_v3_LineItemTest extends CiviUnitTestCase {
protected $params;
protected $_entity = 'line_item';

/**
* Should financials be checked after the test but before tear down.
*
* @var bool
*/
protected $isValidateFinancialsOnPostAssert = TRUE;

/**
* Prepare for test.
*
Expand Down Expand Up @@ -143,7 +150,9 @@ public function testGetBasicLineItem($version): void {
*
* @throws \CRM_Core_Exception
*/
public function testDeleteLineItem($version): void {
public function testDeleteLineItem(int $version): void {
// Deleting line items does not leave valid financial data.
$this->isValidateFinancialsOnPostAssert = FALSE;
$this->_apiversion = $version;
$getParams = [
'entity_table' => 'civicrm_contribution',
Expand Down
11 changes: 11 additions & 0 deletions tests/phpunit/api/v3/MembershipPaymentTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,17 @@ class api_v3_MembershipPaymentTest extends CiviUnitTestCase {
protected $_membershipStatusID;
protected $_contribution = [];

/**
* Should financials be checked after the test but before tear down.
*
* This test class is opted out as this method should not be called outside
* of the LineItem::create function and the test is artificial & not creating
* valid financials.
*
* @var bool
*/
protected $isValidateFinancialsOnPostAssert = FALSE;

public function setUp(): void {
parent::setUp();
$this->useTransaction(TRUE);
Expand Down
2 changes: 2 additions & 0 deletions tests/phpunit/api/v3/ProfileTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -542,6 +542,8 @@ public function testMembershipGetFieldsOrder(): void {
* @throws \CiviCRM_API3_Exception
*/
public function testProfileSubmitMembershipBatch(): void {
// @todo - figure out why this doesn't pass validate financials
$this->isValidateFinancialsOnPostAssert = FALSE;
$this->_contactID = $this->individualCreate();
$this->callAPISuccess('profile', 'submit', [
'profile_id' => 'membership_batch_entry',
Expand Down