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 loading, saving, and upgrade of CiviContribute settings #17193

Merged
merged 9 commits into from
Apr 30, 2020
5 changes: 5 additions & 0 deletions CRM/Upgrade/Incremental/php/FiveTwentyFour.php
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,11 @@ public function upgrade_5_24_alpha1($rev) {
);
}

public function upgrade_5_24_6($rev) {
$this->addTask(ts('Upgrade DB to %1: SQL', [1 => $rev]), 'runSql', $rev);
$this->addTask('Convert CiviContribute settings', 'updateContributeSettings');
}

/**
* Install sequentialCreditNotes extension.
*
Expand Down
1 change: 1 addition & 0 deletions CRM/Upgrade/Incremental/sql/5.24.6.mysql.tpl
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{* file to handle db changes in 5.24.6 during upgrade *}
47 changes: 33 additions & 14 deletions Civi/Core/SettingsBag.php
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,6 @@ 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;
Expand Down Expand Up @@ -167,6 +166,10 @@ public function all() {
$this->combined = $this->combine(
[$this->defaults, $this->values, $this->mandatory]
);
// computeVirtual() depends on completion of preceding pass.
$this->combined = $this->combine(
[$this->combined, $this->computeVirtual()]
);
}
return $this->combined;
}
Expand Down Expand Up @@ -254,8 +257,7 @@ public function revert($key) {
* @return SettingsBag
*/
public function set($key, $value) {
if ($key === 'contribution_invoice_settings') {
$this->setContributionSettings($value);
if ($this->updateVirtual($key, $value)) {
return $this;
}
$this->setDb($key, $value);
Expand All @@ -265,6 +267,8 @@ public function set($key, $value) {
}

/**
* Update a virtualized/deprecated setting.
*
* Temporary handling for phasing out contribution_invoice_settings.
*
* Until we have transitioned we need to handle setting & retrieving
Expand All @@ -274,29 +278,44 @@ public function set($key, $value) {
*
* https://lab.civicrm.org/dev/core/issues/1558
*
* @param string $key
* @param array $value
* @return bool
* TRUE if $key is a virtualized setting. FALSE if it is a normal setting.
*/
public function setContributionSettings($value) {
foreach (SettingsBag::getContributionInvoiceSettingKeys() as $possibleKeyName => $settingName) {
$keyValue = $value[$possibleKeyName] ?? '';
$this->set($settingName, $keyValue);
public function updateVirtual($key, $value) {
if ($key === 'contribution_invoice_settings') {
foreach (SettingsBag::getContributionInvoiceSettingKeys() as $possibleKeyName => $settingName) {
$keyValue = $value[$possibleKeyName] ?? '';
if ($possibleKeyName === 'invoicing' && is_array($keyValue)) {
$keyValue = $keyValue['invoicing'];
}
$this->set($settingName, $keyValue);
}
return TRUE;
}
$this->values['contribution_invoice_settings'] = $this->getContributionSettings();
return FALSE;
}

/**
* Temporary function to handle returning the contribution_settings key despite it being deprecated.
*
* See more in comment block on previous function.
* Determine the values of any virtual/computed settings.
*
* @return array
*/
public function getContributionSettings() {
public function computeVirtual() {
$contributionSettings = [];
foreach (SettingsBag::getContributionInvoiceSettingKeys() as $keyName => $settingName) {
$contributionSettings[$keyName] = $this->values[$settingName] ?? '';
switch ($keyName) {
case 'invoicing':
$contributionSettings[$keyName] = $this->get($settingName) ? [$keyName => 1] : 0;
break;

default:
$contributionSettings[$keyName] = $this->get($settingName);
break;
}
}
return $contributionSettings;
return ['contribution_invoice_settings' => $contributionSettings];
}

/**
Expand Down
9 changes: 9 additions & 0 deletions release-notes.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,15 @@ Other resources for identifying changes are:
* https://github.com/civicrm/civicrm-joomla
* https://github.com/civicrm/civicrm-wordpress

## CiviCRM 5.24.6

Released April 29, 2020

- **[Synopsis](release-notes/5.24.6.md#synopsis)**
- **[Bugs resolved](release-notes/5.24.6.md#bugs)**
- **[Credits](release-notes/5.24.6.md#credits)**
- **[Feedback](release-notes/5.24.6.md#feedback)**

## CiviCRM 5.24.5

Released April 22, 2020
Expand Down
41 changes: 41 additions & 0 deletions release-notes/5.24.6.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
# CiviCRM 5.24.6

Released April 29, 2020

- **[Synopsis](#synopsis)**
- **[Bugs resolved](#bugs)**
- **[Credits](#credits)**
- **[Feedback](#feedback)**

## <a name="synopsis"></a>Synopsis

| *Does this version...?* | |
|:--------------------------------------------------------------- |:-------:|
| Fix security vulnerabilities? | no |
| Change the database schema? | no |
| Alter the API? | no |
| Require attention to configuration options? | **yes** |
| Fix problems installing or upgrading to a previous version? | **yes** |
| Introduce features? | no |
| **Fix bugs?** | **yes** |

## <a name="bugs"></a>Bugs resolved

* **_CiviContribute_: The "Tax and Invoice" settings are not consistently saved/loaded/upgraded ([dev/core#1724](https://lab.civicrm.org/dev/core/-/issues/1724): [#17188](https://github.com/civicrm/civicrm-core/pull/17188))**

In the screen "Administer => CiviContribute => CiviContribute Component Settings", the settings for "Enable Tax and
Invoicing" (et al) may have been loaded/saved incorrectly. If you rely on the settings, and if you've previously
used an affected version (5.23.0 - 5.24.5), then you may wish to review the settings to ensure they are correct.

## <a name="credits"></a>Credits

This release was developed by the following authors and reviewers:

Wikimedia Foundation - Eileen McNaughton; Tadpole Collective - Kevin
Cristiano; JMA Consulting - Seamus Lee; CiviCRM - Tim Otten

## <a name="feedback"></a>Feedback

These release notes are edited by Tim Otten and Andrew Hunt. If you'd like to
provide feedback on them, please login to https://chat.civicrm.org/civicrm and
contact `@agh1`.
17 changes: 7 additions & 10 deletions settings/Contribute.setting.php
Original file line number Diff line number Diff line change
Expand Up @@ -41,17 +41,8 @@
'group' => 'contribute',
'name' => 'contribution_invoice_settings',
'type' => 'Array',
'default' => [
'invoice_prefix' => 'INV_',
'credit_notes_prefix' => 'CN_',
'due_date' => '10',
'due_date_period' => 'days',
'notes' => '',
'tax_term' => 'Sales Tax',
'tax_display_settings' => 'Inclusive',
],
'add' => '4.7',
'title' => ts('Deprecated setting'),
'title' => ts('Deprecated, virtualized setting'),
'is_domain' => 1,
'is_contact' => 0,
'help_text' => NULL,
Expand All @@ -74,6 +65,7 @@
'settings_pages' => ['contribute' => ['weight' => 90]],
],
'invoice_prefix' => [
'default' => 'INV_',
'html_type' => 'text',
'name' => 'invoice_prefix',
'add' => '5.23',
Expand All @@ -84,6 +76,7 @@
'is_contact' => 0,
],
'invoice_due_date' => [
'default' => '10',
'name' => 'invoice_due_date',
'html_type' => 'text',
'title' => ts('Due Date'),
Expand All @@ -93,6 +86,7 @@
'is_contact' => 0,
],
'invoice_due_date_period' => [
'default' => 'days',
'html_type' => 'select',
'name' => 'invoice_due_date_period',
'title' => ts('For transmission'),
Expand All @@ -110,6 +104,7 @@
],
],
'invoice_notes' => [
'default' => '',
'name' => 'invoice_notes',
'html_type' => 'wysiwyg',
'title' => ts('Notes or Standard Terms'),
Expand All @@ -131,6 +126,7 @@
'description' => ts('Should a pdf invoice be emailed automatically?'),
],
'tax_term' => [
'default' => 'Sales Tax',
'name' => 'tax_term',
'html_type' => 'text',
'add' => '5.23',
Expand All @@ -140,6 +136,7 @@
'is_contact' => 0,
],
'tax_display_settings' => [
'default' => 'Inclusive',
'html_type' => 'select',
'name' => 'tax_display_settings',
'type' => CRM_Utils_Type::T_STRING,
Expand Down
2 changes: 1 addition & 1 deletion sql/civicrm_generated.mysql
Original file line number Diff line number Diff line change
Expand Up @@ -398,7 +398,7 @@ UNLOCK TABLES;

LOCK TABLES `civicrm_domain` WRITE;
/*!40000 ALTER TABLE `civicrm_domain` DISABLE KEYS */;
INSERT INTO `civicrm_domain` (`id`, `name`, `description`, `version`, `contact_id`, `locales`, `locale_custom_strings`) VALUES (1,'Default Domain Name',NULL,'5.24.5',1,NULL,'a:1:{s:5:\"en_US\";a:0:{}}');
INSERT INTO `civicrm_domain` (`id`, `name`, `description`, `version`, `contact_id`, `locales`, `locale_custom_strings`) VALUES (1,'Default Domain Name',NULL,'5.24.6',1,NULL,'a:1:{s:5:\"en_US\";a:0:{}}');
/*!40000 ALTER TABLE `civicrm_domain` ENABLE KEYS */;
UNLOCK TABLES;

Expand Down
3 changes: 2 additions & 1 deletion tests/phpunit/CRM/Core/BAO/SettingTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ public function testHandlingOfContributionInvoiceSetting() {
'notes' => '<p>Give me money</p>',
'tax_term' => 'Extortion',
'tax_display_settings' => 'Exclusive',
// NOTE: This form of `invoicing` is accepted, but it may be normalized to the idiomatic form with a nested array.
'invoicing' => 1,
'is_email_pdf' => '1',
];
Expand All @@ -88,7 +89,7 @@ public function testHandlingOfContributionInvoiceSetting() {
$getVersion = $this->callAPISuccessGetValue('Setting', ['name' => 'contribution_invoice_settings']);
$this->assertEquals($settingsFromAPI, $settingsFromGet);
$this->assertAPIArrayComparison($getVersion, $settingsFromGet);
$this->assertEquals($contributionSettings, $settingsFromGet);
$this->assertEquals(['invoicing' => ['invoicing' => 1]] + $contributionSettings, $settingsFromGet);

// These are the preferred retrieval methods.
$this->assertEquals('G_', Civi::settings()->get('invoice_prefix'));
Expand Down
46 changes: 46 additions & 0 deletions tests/phpunit/Civi/Core/SettingsBagTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -30,4 +30,50 @@ public function testInnoDbFTS() {
$this->assertEquals(0, $settingsBag->get('enable_innodb_fts'));
}

/**
* The setting "contribution_invoice_settings" is actually a virtual value built on other settings.
* Check that various updates work as expected.
*/
public function testVirtualContributionSetting_explicit() {
$s = \Civi::settings();

$this->assertEquals(10, $s->get('contribution_invoice_settings')['due_date']);
$this->assertEquals(10, $s->get('invoice_due_date'));
$this->assertEquals(NULL, $s->getExplicit('invoice_due_date'));

$s->set('invoice_due_date', 20);
$this->assertEquals(20, $s->get('contribution_invoice_settings')['due_date']);
$this->assertEquals(20, $s->get('invoice_due_date'));
$this->assertEquals(20, $s->getExplicit('invoice_due_date'));

$s->set('contribution_invoice_settings', array_merge($s->get('contribution_invoice_settings'), [
'due_date' => 30,
]));
$this->assertEquals(30, $s->get('contribution_invoice_settings')['due_date']);
$this->assertEquals(30, $s->get('invoice_due_date'));
$this->assertEquals(30, $s->getExplicit('invoice_due_date'));

$s->revert('invoice_due_date');
$this->assertEquals(10, $s->get('contribution_invoice_settings')['due_date']);
$this->assertEquals(10, $s->get('invoice_due_date'));
$this->assertEquals(NULL, $s->getExplicit('invoice_due_date'));
}

/**
* The setting "contribution_invoice_settings" is actually a virtual value built on other settings.
* Check that mandatory values ($civicrm_settings) are respected.
*/
public function testVirtualContributionSetting_mandatory() {
$s = \Civi::settings();
$this->assertEquals(10, $s->get('contribution_invoice_settings')['due_date']);
$this->assertEquals(10, $s->get('invoice_due_date'));
$this->assertEquals(NULL, $s->getExplicit('invoice_due_date'));

$s->loadMandatory(['invoice_due_date' => 30]);

$this->assertEquals(30, $s->get('contribution_invoice_settings')['due_date']);
$this->assertEquals(30, $s->get('invoice_due_date'));
$this->assertEquals(NULL, $s->getExplicit('invoice_due_date'));
}

}
2 changes: 2 additions & 0 deletions tests/phpunit/api/v3/ContributionTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,8 @@ public function testCreateCheckContribution() {

/**
* Test the 'return' param works for all fields.
*
* @throws \CRM_Core_Exception
*/
public function testGetContributionReturnFunctionality() {
$params = $this->_params;
Expand Down
2 changes: 1 addition & 1 deletion xml/version.xml
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<?xml version="1.0" encoding="iso-8859-1" ?>
<version>
<version_no>5.24.5</version_no>
<version_no>5.24.6</version_no>
</version>