From ff6f993e56bf4f87caa0317ea3d1f883956b74da Mon Sep 17 00:00:00 2001 From: eileen Date: Thu, 30 Jan 2020 10:49:16 +1300 Subject: [PATCH] dev/core#1558 Data conversion for non-standard setting. Per https://lab.civicrm.org/dev/core/issues/1558 the 'contribution_invoice_settings' 'setting' does not follow our standard of one setting per key and this has led to all sorts of hacks in various places. In trying to unravel this I eventually concluded we needed an interim step where the data would be stored as the correct settings but set and get would still work on the deprecated settinng. The reason for goinng this way is that I found more than one place where extension code is accessing these settings and the amount of work to fix up all the places in core that access it and the settings form were too big to deal with without a stepping stone. With this merged the settinngs are correctly defined and a definition for the deprecated setting still exists. Requesting the deprecated setting returns an array retrieved from the correct settings so is unchanged from calling functions point of view. Saving the deprecated setting saves to the correct places. However, it's now also possible to get & set the correct settings and once the core places are removed we will deprecate & remove the deprecated setting. Unit tests cover past & desired behaviour and the upgrade routine. --- CRM/Core/SelectValues.php | 21 +++++ CRM/Invoicing/Utils.php | 4 +- CRM/Upgrade/Incremental/Base.php | 17 +++- Civi/Core/SettingsBag.php | 58 ++++++++++++ settings/Contribute.setting.php | 93 ++++++++++++++++++- .../Contact/Page/View/UserDashBoardTest.php | 3 + tests/phpunit/CRM/Core/BAO/SettingTest.php | 66 +++++++++++-- .../CRM/Core/Payment/PayPalIPNTest.php | 4 +- .../Financial/BAO/FinancialAccountTest.php | 2 +- .../CRM/Upgrade/Incremental/BaseTest.php | 21 ++++- 10 files changed, 268 insertions(+), 21 deletions(-) diff --git a/CRM/Core/SelectValues.php b/CRM/Core/SelectValues.php index de5668a6e779..9ebe8db0ce6f 100644 --- a/CRM/Core/SelectValues.php +++ b/CRM/Core/SelectValues.php @@ -443,6 +443,27 @@ public static function geoProvider() { return $geo; } + /** + * Get options for displaying tax. + * + * @return array + * + * @throws \CRM_Core_Exception + */ + public function taxDisplayOptions() { + return [ + 'Do_not_show' => ts('Do not show breakdown, only show total - i.e %1', [ + 1 => CRM_Utils_Money::format(120), + ]), + 'Inclusive' => ts('Show [tax term] inclusive price - i.e. %1', [ + 1 => ts('%1 (includes [tax term] of %2)', [1 => CRM_Utils_Money::format(120), 2 => CRM_Utils_Money::format(20)]), + ]), + 'Exclusive' => ts('Show [tax term] exclusive price - i.e. %1', [ + 1 => ts('%1 + %2 [tax term]', [1 => CRM_Utils_Money::format(120), 2 => CRM_Utils_Money::format(20)]), + ]), + ]; + } + /** * Get the Address Standardization Providers from available plugins. * diff --git a/CRM/Invoicing/Utils.php b/CRM/Invoicing/Utils.php index 06b56fbab096..6f0bc50cf263 100644 --- a/CRM/Invoicing/Utils.php +++ b/CRM/Invoicing/Utils.php @@ -24,6 +24,8 @@ class CRM_Invoicing_Utils { * @param bool $oldValue * @param bool $newValue * @param array $metadata + * + * @throws \CiviCRM_API3_Exception */ public static function onToggle($oldValue, $newValue, $metadata) { if ($oldValue == $newValue) { @@ -32,7 +34,7 @@ public static function onToggle($oldValue, $newValue, $metadata) { $existingUserViewOptions = civicrm_api3('Setting', 'get', ['return' => 'user_dashboard_options'])['values'][CRM_Core_Config::domainID()]['user_dashboard_options']; $optionValues = civicrm_api3('Setting', 'getoptions', ['field' => 'user_dashboard_options'])['values']; $invoiceKey = array_search('Invoices / Credit Notes', $optionValues); - $existingIndex = in_array($invoiceKey, $existingUserViewOptions); + $existingIndex = array_search($invoiceKey, $existingUserViewOptions); if ($newValue && $existingIndex === FALSE) { $existingUserViewOptions[] = $invoiceKey; diff --git a/CRM/Upgrade/Incremental/Base.php b/CRM/Upgrade/Incremental/Base.php index bde6bb675bd4..0faa6012e410 100644 --- a/CRM/Upgrade/Incremental/Base.php +++ b/CRM/Upgrade/Incremental/Base.php @@ -9,6 +9,8 @@ +--------------------------------------------------------------------+ */ +use Civi\Core\SettingsBag; + /** * Base class for incremental upgrades */ @@ -195,11 +197,16 @@ public static function updateMessageTemplates($ctx, $version) { * @return bool */ public static function updateContributeSettings($ctx) { - $settings = Civi::settings()->get('contribution_invoice_settings'); - $metadata = \Civi\Core\SettingsMetadata::getMetadata(); - $conversions = array_intersect_key((array) $settings, $metadata); - foreach ($conversions as $key => $conversion) { - Civi::settings()->set($key, $conversion); + // Use a direct query as api now does some handling on this. + $settings = CRM_Core_DAO::executeQuery("SELECT value, domain_id FROM civicrm_setting WHERE name = 'contribution_invoice_settings'"); + + while ($settings->fetch()) { + $contributionSettings = (array) CRM_Utils_String::unserialize($settings->value); + foreach (array_merge(SettingsBag::getContributionInvoiceSettingKeys(), ['deferred_revenue_enabled' => 'deferred_revenue_enabled']) as $possibleKeyName => $settingName) { + if (!empty($contributionSettings[$possibleKeyName]) && empty(Civi::settings($settings->domain_id)->getExplicit($settingName))) { + Civi::settings($settings->domain_id)->set($settingName, $contributionSettings[$possibleKeyName]); + } + } } return TRUE; } diff --git a/Civi/Core/SettingsBag.php b/Civi/Core/SettingsBag.php index c94f3e7a89f6..779bc3a2302f 100644 --- a/Civi/Core/SettingsBag.php +++ b/Civi/Core/SettingsBag.php @@ -136,6 +136,7 @@ public function loadValues() { while ($dao->fetch()) { $this->values[$dao->name] = ($dao->value !== NULL) ? \CRM_Utils_String::unserialize($dao->value) : NULL; } + $dao->values['contribution_invoice_settings'] = $this->getContributionSettings(); } return $this; @@ -253,12 +254,51 @@ public function revert($key) { * @return SettingsBag */ public function set($key, $value) { + if ($key === 'contribution_invoice_settings') { + $this->setContributionSettings($value); + return $this; + } $this->setDb($key, $value); $this->values[$key] = $value; $this->combined = NULL; return $this; } + /** + * Temporary handling for phasing out contribution_invoice_settings. + * + * Until we have transitioned we need to handle setting & retrieving + * contribution_invoice_settings. + * + * Once removed from core we will add deprecation notices & then remove this. + * + * https://lab.civicrm.org/dev/core/issues/1558 + * + * @param array $value + */ + public function setContributionSettings($value) { + foreach (SettingsBag::getContributionInvoiceSettingKeys() as $possibleKeyName => $settingName) { + $keyValue = $value[$possibleKeyName] ?? ''; + $this->set($settingName, $keyValue); + } + $this->values['contribution_invoice_settings'] = $this->getContributionSettings(); + } + + /** + * Temporary function to handle returning the contribution_settings key despite it being deprecated. + * + * See more in comment block on previous function. + * + * @return array + */ + public function getContributionSettings() { + $contributionSettings = []; + foreach (SettingsBag::getContributionInvoiceSettingKeys() as $keyName => $settingName) { + $contributionSettings[$keyName] = $this->values[$settingName] ?? ''; + } + return $contributionSettings; + } + /** * @return \CRM_Utils_SQL_Select */ @@ -378,4 +418,22 @@ protected function setDb($name, $value) { } } + /** + * @return array + */ + public static function getContributionInvoiceSettingKeys(): array { + $convertedKeys = [ + 'credit_notes_prefix' => 'credit_notes_prefix', + 'invoice_prefix' => 'invoice_prefix', + 'due_date' => 'invoice_due_date', + 'due_date_period' => 'invoice_due_date_period', + 'notes' => 'invoice_notes', + 'is_email_pdf' => 'invoice_is_email_pdf', + 'tax_term' => 'tax_term', + 'tax_display_settings' => 'tax_display_settings', + 'invoicing' => 'invoicing', + ]; + return $convertedKeys; + } + } diff --git a/settings/Contribute.setting.php b/settings/Contribute.setting.php index c48a906a3556..fb2170cc6ff4 100644 --- a/settings/Contribute.setting.php +++ b/settings/Contribute.setting.php @@ -50,7 +50,7 @@ 'tax_display_settings' => 'Inclusive', ], 'add' => '4.7', - 'title' => ts('Contribution Invoice Settings'), + 'title' => ts('Deprecated setting'), 'is_domain' => 1, 'is_contact' => 0, 'help_text' => NULL, @@ -71,6 +71,97 @@ 'CRM_Invoicing_Utils::onToggle', ], ], + 'credit_notes_prefix' => [ + 'group_name' => 'Contribute Preferences', + 'group' => 'contribute', + 'name' => 'credit_notes_prefix', + 'html_type' => 'text', + 'quick_form_type' => 'Element', + 'add' => '5.23', + 'type' => CRM_Utils_Type::T_STRING, + 'title' => ts('Credit Notes Prefix'), + 'is_domain' => 1, + 'is_contact' => 0, + 'description' => ts('Prefix to be prepended to credit note ids'), + 'default' => 'CN_', + 'help_text' => ts('The credit note ID is generated when a contribution is set to Refunded, Cancelled or Chargeback. It is visible on invoices, if invoices are enabled'), + ], + 'invoice_prefix' => [ + 'html_type' => 'text', + 'name' => 'invoice_prefix', + 'add' => '5.23', + 'type' => CRM_Utils_Type::T_STRING, + 'title' => ts('Invoice Prefix'), + 'description' => ts('Enter prefix to be be preprended when creating an invoice number'), + 'is_domain' => 1, + 'is_contact' => 0, + ], + 'invoice_due_date' => [ + 'name' => 'invoice_due_date', + 'html_type' => 'text', + 'title' => ts('Due Date'), + 'add' => '5.23', + 'type' => CRM_Utils_Type::T_INT, + 'is_domain' => 1, + 'is_contact' => 0, + ], + 'invoice_due_date_period' => [ + 'html_type' => 'select', + 'name' => 'invoice_due_date_period', + 'title' => ts('For transmission'), + 'weight' => 4, + 'add' => '5.23', + 'type' => CRM_Utils_Type::T_STRING, + 'is_domain' => 1, + 'is_contact' => 0, + 'description' => ts('Select the interval for due date.'), + 'options' => [ + 'select' => ts('- select -'), + 'days' => ts('Days'), + 'months' => ts('Months'), + 'years' => ts('Years'), + ], + ], + 'invoice_notes' => [ + 'name' => 'invoice_notes', + 'html_type' => 'wysiwyg', + 'title' => ts('Notes or Standard Terms'), + 'type' => CRM_Utils_Type::T_STRING, + 'add' => '5.23', + 'is_domain' => 1, + 'is_contact' => 0, + 'description' => ts('Enter note or message to be displayed on PDF invoice or credit notes '), + 'attributes' => ['rows' => 2, 'cols' => 40], + ], + 'invoice_is_email_pdf' => [ + 'name' => 'invoice_is_email_pdf', + 'html_type' => 'checkbox', + 'add' => '5.23', + 'type' => CRM_Utils_Type::T_BOOLEAN, + 'is_domain' => 1, + 'is_contact' => 0, + 'title' => ts('Automatically email invoice when user purchases online'), + 'description' => ts('Should a pdf invoice be emailed automatically?'), + ], + 'tax_term' => [ + 'name' => 'tax_term', + 'html_type' => 'text', + 'add' => '5.23', + 'title' => ts('Tax Term'), + 'type' => CRM_Utils_Type::T_STRING, + 'is_domain' => 1, + 'is_contact' => 0, + ], + 'tax_display_settings' => [ + 'html_type' => 'select', + 'name' => 'tax_display_settings', + 'type' => CRM_Utils_Type::T_STRING, + 'add' => '5.23', + 'title' => ts('Tax Display Settings'), + 'is_domain' => 1, + 'is_contact' => 0, + 'pseudoconstant' => ['callback' => 'CRM_Core_SelectValues::taxDisplayOptions'], + ], 'acl_financial_type' => [ 'group_name' => 'Contribute Preferences', 'group' => 'contribute', diff --git a/tests/phpunit/CRM/Contact/Page/View/UserDashBoardTest.php b/tests/phpunit/CRM/Contact/Page/View/UserDashBoardTest.php index e7f1538df89d..9bf3c6f6a764 100644 --- a/tests/phpunit/CRM/Contact/Page/View/UserDashBoardTest.php +++ b/tests/phpunit/CRM/Contact/Page/View/UserDashBoardTest.php @@ -135,6 +135,9 @@ public function testDashboardContentContributionsWithInvoicingEnabled() { /** * Test the content of the dashboard. + * + * @throws \CRM_Core_Exception + * @throws \CiviCRM_API3_Exception */ public function testDashboardContentContributions() { $this->contributionCreate(['contact_id' => $this->contactID]); diff --git a/tests/phpunit/CRM/Core/BAO/SettingTest.php b/tests/phpunit/CRM/Core/BAO/SettingTest.php index c477fd041158..2901412599e2 100644 --- a/tests/phpunit/CRM/Core/BAO/SettingTest.php +++ b/tests/phpunit/CRM/Core/BAO/SettingTest.php @@ -29,31 +29,77 @@ public function tearDown() { parent::tearDown(); } + /** + * Test that enabling a valid component works. + */ public function testEnableComponentValid() { - $config = CRM_Core_Config::singleton(TRUE, TRUE); - + CRM_Core_Config::singleton(TRUE, TRUE); $result = CRM_Core_BAO_ConfigSetting::enableComponent('CiviCampaign'); - $this->assertTrue($result); } + /** + * Test that we get a success result if we try to enable an enabled component. + */ public function testEnableComponentAlreadyPresent() { - $config = CRM_Core_Config::singleton(TRUE, TRUE); - - $result = CRM_Core_BAO_ConfigSetting::enableComponent('CiviCampaign'); + CRM_Core_Config::singleton(TRUE, TRUE); + CRM_Core_BAO_ConfigSetting::enableComponent('CiviCampaign'); $result = CRM_Core_BAO_ConfigSetting::enableComponent('CiviCampaign'); - $this->assertTrue($result); } + /** + * Test that we get a false result if we try to enable an invalid component. + */ public function testEnableComponentInvalid() { - $config = CRM_Core_Config::singleton(TRUE, TRUE); - + CRM_Core_Config::singleton(TRUE, TRUE); $result = CRM_Core_BAO_ConfigSetting::enableComponent('CiviFake'); - $this->assertFalse($result); } + /** + * Test temporary retrieval & setting of converted settings. + * + * As a transitional measure we allow the settings that were munged into + * contribution_invoice_setting. This tests that the current method of getting via the 'old' key + * works. This will be deprecated & removed over the next few versions but + * 1) some extensions use these settings & + * 2) there is a lot of work to fix this mess in core so a transitional method makes sense. + * + * https://lab.civicrm.org/dev/core/issues/1558 + * + * @throws \CRM_Core_Exception + */ + public function testHandlingOfContributionInvoiceSetting() { + $contributionSettings = [ + 'invoice_prefix' => 'G_', + 'credit_notes_prefix' => 'XX_', + 'due_date' => '20', + 'due_date_period' => 'weeks', + 'notes' => '

Give me money

', + 'tax_term' => 'Extortion', + 'tax_display_settings' => 'Exclusive', + 'invoicing' => 1, + 'is_email_pdf' => '1', + ]; + Civi::settings()->set('contribution_invoice_settings', $contributionSettings); + $settingsFromGet = Civi::settings()->get('contribution_invoice_settings'); + $settingsFromAPI = $this->callAPISuccess('Setting', 'get', ['return' => 'contribution_invoice_settings'])['values'][CRM_Core_Config::domainID()]['contribution_invoice_settings']; + $getVersion = $this->callAPISuccessGetValue('Setting', ['name' => 'contribution_invoice_settings']); + $this->assertEquals($settingsFromAPI, $settingsFromGet); + $this->assertAPIArrayComparison($getVersion, $settingsFromGet); + $this->assertEquals($contributionSettings, $settingsFromGet); + + // These are the preferred retrieval methods. + $this->assertEquals('G_', Civi::settings()->get('invoice_prefix')); + $this->assertEquals('XX_', Civi::settings()->get('credit_notes_prefix')); + $this->assertEquals('20', Civi::settings()->get('invoice_due_date')); + $this->assertEquals('weeks', Civi::settings()->get('invoice_due_date_period')); + $this->assertEquals('

Give me money

', Civi::settings()->get('invoice_notes')); + $this->assertEquals('Extortion', Civi::settings()->get('tax_term')); + $this->assertEquals('Exclusive', Civi::settings()->get('tax_display_settings')); + } + /** * Ensure that overrides in $civicrm_setting apply when * using getItem($group,$name). diff --git a/tests/phpunit/CRM/Core/Payment/PayPalIPNTest.php b/tests/phpunit/CRM/Core/Payment/PayPalIPNTest.php index 6af63c183807..d51650131795 100644 --- a/tests/phpunit/CRM/Core/Payment/PayPalIPNTest.php +++ b/tests/phpunit/CRM/Core/Payment/PayPalIPNTest.php @@ -57,6 +57,8 @@ public function tearDown() { * * The scenario is that a pending contribution exists and the IPN call will update it to completed. * And also if Tax and Invoicing is enabled, this unit test ensure that invoice pdf is attached with email recipet + * + * @throws \CRM_Core_Exception */ public function testInvoiceSentOnIPNPaymentSuccess() { $this->enableTaxAndInvoicing(); @@ -81,7 +83,7 @@ public function testInvoiceSentOnIPNPaymentSuccess() { $_REQUEST = ['q' => CRM_Utils_System::url('civicrm/payment/ipn/' . $this->_paymentProcessorID)] + $this->getPaypalTransaction(); $mut = new CiviMailUtils($this, TRUE); - $paymentProcesors = civicrm_api3('PaymentProcessor', 'getsingle', ['id' => $this->_paymentProcessorID]); + $paymentProcesors = $this->callAPISuccessGetSingle('PaymentProcessor', ['id' => $this->_paymentProcessorID]); $payment = Civi\Payment\System::singleton()->getByProcessor($paymentProcesors); $payment->handlePaymentNotification(); diff --git a/tests/phpunit/CRM/Financial/BAO/FinancialAccountTest.php b/tests/phpunit/CRM/Financial/BAO/FinancialAccountTest.php index 773188695cec..699815bf2ac0 100644 --- a/tests/phpunit/CRM/Financial/BAO/FinancialAccountTest.php +++ b/tests/phpunit/CRM/Financial/BAO/FinancialAccountTest.php @@ -360,7 +360,7 @@ public function testBalanceDueIfDeferredRevenueEnabled() { ]); $balance = CRM_Contribute_BAO_Contribution::getContributionBalance($contribution['id'], $totalAmount); $this->assertEquals(0.0, $balance); - Civi::settings()->revert('contribution_invoice_settings'); + Civi::settings()->set('deferred_revenue_enabled', FALSE); } /** diff --git a/tests/phpunit/CRM/Upgrade/Incremental/BaseTest.php b/tests/phpunit/CRM/Upgrade/Incremental/BaseTest.php index 238816045d21..a08f02950f55 100644 --- a/tests/phpunit/CRM/Upgrade/Incremental/BaseTest.php +++ b/tests/phpunit/CRM/Upgrade/Incremental/BaseTest.php @@ -461,10 +461,27 @@ public function testRenameFields() { * 'proper' setting. */ public function testConvertUpgradeContributeSettings() { - Civi::settings()->set('contribution_invoice_settings', ['foo' => 'bar', 'deferred_revenue_enabled' => 1]); - $this->assertEquals(0, Civi::settings()->get('deferred_revenue_enabled')); + $setting = [ + 'deferred_revenue_enabled' => 1, + 'invoice_prefix' => 'G_', + 'credit_notes_prefix' => 'XX_', + 'due_date' => '20', + 'due_date_period' => 'weeks', + 'notes' => '

Give me money

', + 'tax_term' => 'Extortion', + 'tax_display_settings' => 'Exclusive', + ]; + CRM_Core_DAO::executeQuery("INSERT INTO civicrm_setting (name, domain_id, value) + VALUES ('contribution_invoice_settings', 1, '" . serialize($setting) . "')"); CRM_Upgrade_Incremental_Base::updateContributeSettings(NULL, 5.1); $this->assertEquals(1, Civi::settings()->get('deferred_revenue_enabled')); + $this->assertEquals('G_', Civi::settings()->get('invoice_prefix')); + $this->assertEquals('XX_', Civi::settings()->get('credit_notes_prefix')); + $this->assertEquals('20', Civi::settings()->get('invoice_due_date')); + $this->assertEquals('weeks', Civi::settings()->get('invoice_due_date_period')); + $this->assertEquals('

Give me money

', Civi::settings()->get('invoice_notes')); + $this->assertEquals('Extortion', Civi::settings()->get('tax_term')); + $this->assertEquals('Exclusive', Civi::settings()->get('tax_display_settings')); } /**