From 1807fca4fa8509b5ff4e25d7bf0a3084e9b97a55 Mon Sep 17 00:00:00 2001
From: eileen <emcnaughton@wikimedia.org>
Date: Tue, 28 Apr 2020 19:38:57 +1200
Subject: [PATCH 1/8] dev/core#1724 Fix Changes to CiviContribute Component
Settings not saved
It seems that, depending on the order in which settings are loaded, this is not always populated. Moving it here
addresses, although medium term we should address all references to it
---
Civi/Core/SettingsBag.php | 1 +
tests/phpunit/api/v3/ContributionTest.php | 2 ++
2 files changed, 3 insertions(+)
diff --git a/Civi/Core/SettingsBag.php b/Civi/Core/SettingsBag.php
index 28f3ef36869e..ca79d8603664 100644
--- a/Civi/Core/SettingsBag.php
+++ b/Civi/Core/SettingsBag.php
@@ -168,6 +168,7 @@ public function all() {
[$this->defaults, $this->values, $this->mandatory]
);
}
+ $this->combined['contribution_invoice_settings'] = $this->getContributionSettings();
return $this->combined;
}
diff --git a/tests/phpunit/api/v3/ContributionTest.php b/tests/phpunit/api/v3/ContributionTest.php
index b727d7cf6133..e391f12e4a6c 100644
--- a/tests/phpunit/api/v3/ContributionTest.php
+++ b/tests/phpunit/api/v3/ContributionTest.php
@@ -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;
From c84199671846e3ff492319fffedca9e0fc3c18f0 Mon Sep 17 00:00:00 2001
From: Tim Otten <totten@civicrm.org>
Date: Tue, 28 Apr 2020 14:24:34 -0700
Subject: [PATCH 2/8] dev/core#1724 - SettingsBagTest - Add coverage for
contribution_invoice_settings
---
tests/phpunit/Civi/Core/SettingsBagTest.php | 46 +++++++++++++++++++++
1 file changed, 46 insertions(+)
diff --git a/tests/phpunit/Civi/Core/SettingsBagTest.php b/tests/phpunit/Civi/Core/SettingsBagTest.php
index 54993ff03184..6271337b29c0 100644
--- a/tests/phpunit/Civi/Core/SettingsBagTest.php
+++ b/tests/phpunit/Civi/Core/SettingsBagTest.php
@@ -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'));
+ }
+
}
From 8f2a141a09e91f680ecd1f0a7d0a61777a2d2586 Mon Sep 17 00:00:00 2001
From: Tim Otten <totten@civicrm.org>
Date: Tue, 28 Apr 2020 14:14:45 -0700
Subject: [PATCH 3/8] (REF) dev/core#1724 - SettingsBag - Convert to
computeVirtual/updateVirtual
The `SettingsBag` uses the `$combined` property to (locally) cache a full
view of the active settings (based on combining different layers of data --
default-values, explicit-values, and mandatory-values). This design is
good for read-mostly data.
This patch changes the `__ContributionSettings` to be another layer in the
construction of `$combined` -- the virtual-values layer. It fixes issues
wherein:
1. The virtual value is recomputed during every call to `get($key)`.
2. The virtual value is only based on explicit-values -- it disregards
default-values and mandatory-values.
---
Civi/Core/SettingsBag.php | 37 ++++++++++++++++++++++---------------
1 file changed, 22 insertions(+), 15 deletions(-)
diff --git a/Civi/Core/SettingsBag.php b/Civi/Core/SettingsBag.php
index ca79d8603664..3435fd3114f7 100644
--- a/Civi/Core/SettingsBag.php
+++ b/Civi/Core/SettingsBag.php
@@ -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;
@@ -167,8 +166,11 @@ 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()]
+ );
}
- $this->combined['contribution_invoice_settings'] = $this->getContributionSettings();
return $this->combined;
}
@@ -255,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);
@@ -266,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
@@ -275,29 +278,33 @@ 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] ?? '';
+ $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] ?? '';
+ $contributionSettings[$keyName] = $this->get($settingName);
}
- return $contributionSettings;
+ return ['contribution_invoice_settings' => $contributionSettings];
}
/**
From be17727794da72b675ede4cf77ef23e199a665ab Mon Sep 17 00:00:00 2001
From: Tim Otten <totten@civicrm.org>
Date: Tue, 28 Apr 2020 15:33:33 -0700
Subject: [PATCH 4/8] dev/core#1724 - Fix defaults for invoice-related settings
The default values were previously stored in 'contribution_invoice_settings', but
the updates are stored in individual settings, which makes it difficult to reconcile
the default values and explicit values.
With this revision, 'contribution_invoice_settings' is strictly virtual, and
all the other settings (like 'invoice_due_date') are canonical.
---
settings/Contribute.setting.php | 17 +++++++----------
1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/settings/Contribute.setting.php b/settings/Contribute.setting.php
index b9bd3699f0a8..9e6e73f37f17 100644
--- a/settings/Contribute.setting.php
+++ b/settings/Contribute.setting.php
@@ -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,
@@ -74,6 +65,7 @@
'settings_pages' => ['contribute' => ['weight' => 90]],
],
'invoice_prefix' => [
+ 'default' => 'INV_',
'html_type' => 'text',
'name' => 'invoice_prefix',
'add' => '5.23',
@@ -84,6 +76,7 @@
'is_contact' => 0,
],
'invoice_due_date' => [
+ 'default' => '10',
'name' => 'invoice_due_date',
'html_type' => 'text',
'title' => ts('Due Date'),
@@ -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'),
@@ -110,6 +104,7 @@
],
],
'invoice_notes' => [
+ 'default' => '',
'name' => 'invoice_notes',
'html_type' => 'wysiwyg',
'title' => ts('Notes or Standard Terms'),
@@ -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',
@@ -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,
From 690000c71e704aa2248b5d4fd5a1cbd6d5fb5df0 Mon Sep 17 00:00:00 2001
From: Tim Otten <totten@civicrm.org>
Date: Wed, 29 Apr 2020 16:34:28 -0700
Subject: [PATCH 5/8] Set version to 5.25.beta3
---
CRM/Upgrade/Incremental/sql/5.25.beta3.mysql.tpl | 1 +
sql/civicrm_generated.mysql | 2 +-
xml/version.xml | 2 +-
3 files changed, 3 insertions(+), 2 deletions(-)
create mode 100644 CRM/Upgrade/Incremental/sql/5.25.beta3.mysql.tpl
diff --git a/CRM/Upgrade/Incremental/sql/5.25.beta3.mysql.tpl b/CRM/Upgrade/Incremental/sql/5.25.beta3.mysql.tpl
new file mode 100644
index 000000000000..1d9fd478e2d5
--- /dev/null
+++ b/CRM/Upgrade/Incremental/sql/5.25.beta3.mysql.tpl
@@ -0,0 +1 @@
+{* file to handle db changes in 5.25.beta3 during upgrade *}
diff --git a/sql/civicrm_generated.mysql b/sql/civicrm_generated.mysql
index 8732a785bdca..f7fcd718209b 100644
--- a/sql/civicrm_generated.mysql
+++ b/sql/civicrm_generated.mysql
@@ -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.25.beta2',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.25.beta3',1,NULL,'a:1:{s:5:\"en_US\";a:0:{}}');
/*!40000 ALTER TABLE `civicrm_domain` ENABLE KEYS */;
UNLOCK TABLES;
diff --git a/xml/version.xml b/xml/version.xml
index 0334d4c15c98..264bb2962c73 100644
--- a/xml/version.xml
+++ b/xml/version.xml
@@ -1,4 +1,4 @@
<?xml version="1.0" encoding="iso-8859-1" ?>
<version>
- <version_no>5.25.beta2</version_no>
+ <version_no>5.25.beta3</version_no>
</version>
From 6f306db38752ff81d5def481e3490b63a88d4c99 Mon Sep 17 00:00:00 2001
From: Tim Otten <totten@civicrm.org>
Date: Tue, 28 Apr 2020 17:40:03 -0700
Subject: [PATCH 6/8] FiveTwentyFive - Convert settings during upgrade
Consider this use-case:
* Install 5.22
* Configure `CiviContribute Component Settings`
* Upgrade to 5.25.beta
The settings are lost during the upgrade.
---
CRM/Upgrade/Incremental/php/FiveTwentyFive.php | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/CRM/Upgrade/Incremental/php/FiveTwentyFive.php b/CRM/Upgrade/Incremental/php/FiveTwentyFive.php
index 892053f1a6ae..e7f269ed06e9 100644
--- a/CRM/Upgrade/Incremental/php/FiveTwentyFive.php
+++ b/CRM/Upgrade/Incremental/php/FiveTwentyFive.php
@@ -79,6 +79,11 @@ public function upgrade_5_25_alpha1($rev) {
$this->addTask('Convert Report Form dates from jcalander to datepicker', 'convertReportsJcalendarToDatePicker');
}
+ public function upgrade_5_25_beta3($rev) {
+ $this->addTask(ts('Upgrade DB to %1: SQL', [1 => $rev]), 'runSql', $rev);
+ $this->addTask('Convert CiviContribute settings', 'updateContributeSettings');
+ }
+
/**
* Convert date fields stored in civicrm_report_instance to that format for datepicker
*/
From 84d52986c4ebd93b55741bc62dcabe2c8a4e610e Mon Sep 17 00:00:00 2001
From: Tim Otten <totten@civicrm.org>
Date: Tue, 28 Apr 2020 21:54:12 -0700
Subject: [PATCH 7/8] dev/core#1724 - Backward-compatible handling of
'contribution_invoice_settings.invoicing'
In the old setting `contribution_invoice_settings`, the `invoicing`
property (aka "CiviContribute Component Settings" => "Enable Tax and
Invoicing") could take one of two values:
1. If enabled, it's an array with trueish subkey: `['invoicing'=>1]`
2. If disabled, it's a numeric zero.
This encoding is weird, and `contribution_invoice_settings` is generally deprecated
and has been replaced by a virtual rendition which needs to be consistent with the
old representation -- so that a couple-dozen things continue working.
For testing, it helped me to try these commands at different points:
```
cv ev 'return CRM_Invoicing_Utils::isInvoicingEnabled();'
cv api setting.get return=invoicing
cv api setting.get return=contribution_invoice_settings
select * from civicrm_setting where name like 'contrib%' or name like 'invoic%';
```
---
Civi/Core/SettingsBag.php | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/Civi/Core/SettingsBag.php b/Civi/Core/SettingsBag.php
index 3435fd3114f7..be51cc6ae415 100644
--- a/Civi/Core/SettingsBag.php
+++ b/Civi/Core/SettingsBag.php
@@ -287,6 +287,9 @@ 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;
@@ -302,7 +305,15 @@ public function updateVirtual($key, $value) {
public function computeVirtual() {
$contributionSettings = [];
foreach (SettingsBag::getContributionInvoiceSettingKeys() as $keyName => $settingName) {
- $contributionSettings[$keyName] = $this->get($settingName);
+ switch ($keyName) {
+ case 'invoicing':
+ $contributionSettings[$keyName] = $this->get($settingName) ? [$keyName => 1] : 0;
+ break;
+
+ default:
+ $contributionSettings[$keyName] = $this->get($settingName);
+ break;
+ }
}
return ['contribution_invoice_settings' => $contributionSettings];
}
From ecda7f33f283066d56d2824671f3c6b43589a488 Mon Sep 17 00:00:00 2001
From: Tim Otten <totten@civicrm.org>
Date: Wed, 29 Apr 2020 18:23:40 -0700
Subject: [PATCH 8/8] testHandlingOfContributionInvoiceSetting() - More
realistic data+assertion
In practice, for the old setting `contribution_invoice_settings`, the
`invoicing` property could take one of two values:
1. If enabled, the value is an array with trueish subkey: `['invoicing'=>['invoicing'=>1]]`
2. If disabled, the value is a numeric `0`: `['invoicing'=>0]`
Following this means better compatibility.
The format is counter-intuitive, and the unit-test sets the value to enabled
using a more intuitive format (`['invoicing'=>1]`). It's currently valid to
`set()` this way, but `get()`ting the value produces the
more-compatible/historical format.
---
tests/phpunit/CRM/Core/BAO/SettingTest.php | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tests/phpunit/CRM/Core/BAO/SettingTest.php b/tests/phpunit/CRM/Core/BAO/SettingTest.php
index 2901412599e2..d4c4ddebb1f6 100644
--- a/tests/phpunit/CRM/Core/BAO/SettingTest.php
+++ b/tests/phpunit/CRM/Core/BAO/SettingTest.php
@@ -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',
];
@@ -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'));