Fix loading, saving, and upgrade of CiviContribute settings #17188
Conversation
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
…irtual 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.
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.
|
(Standard links)
|
|
Note: on my local, I've done some testing to make sure that "CiviContribute Component Settings" reads data from previous versions - and can save/load updates - when starting from:
|
|
jenkins, test this please |
b6fa397 to
2bb2705
Compare
|
I tested the upgraded and it worked correctly and the form loaded correctly, The code looks correct from my POV. Lets see what Jenkins makes of it |
|
Hmmblarneyblerg. So a previous test-run with the last commit ( I'm confused by how we're boxed in on the content of
The perplexing thing is that both of those seem older than the regression. (And this feels like an exercise in obscura... but we do want upgrades to work...) Maybe I need to step away and look at it again tomorrow. 😕 |
8388462 to
60e3ae8
Compare
|
OK, I think I got it. The data-type of
Which is... bonkers. 🤷 So I've pushed an updated form of the last commit which uses that format. Retested a few cases locally, and these seem good:
Additionally, |
60e3ae8 to
db5de31
Compare
Consider this use-case: * Install 5.22 * Configure `CiviContribute Component Settings` * Upgrade to 5.25.beta The settings are lost during the upgrade.
…_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%'; ```
db5de31 to
84d5298
Compare
…rtion 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.
|
Test fail unrelated |
Overview
Historically, preferences related to tax and invoice were stored in the setting
contribution_invoice_settings. Circa 5.23,contribution_invoice_settingswas deprecated and decomposed into several smaller settings (invoice_due_date,tax_term, etc) . There is a transitional mechanism to convert between the old-style/large setting and the new-style/small settings, but it has issues. This PR updates the transitional mechanism.This patch should fix transitional issues for most deployments. However, if you rely on the "CiviContribute Component Settings" to customize the "Tax and Invoicing" options, 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.
See also:
Before
contribution_invoice_settings, and upgrade... then the settings are not converted.invoice_due_date,tax_term, etc), but default-values are defined in old-stylecontribution_invoice_settings- leading to inconsistent behavior.contribution_invoice_settingsdo not reflect overrides ($civicrm_settings).After
contribution_invoice_settings, and upgrade... then the settings are converted.contribution_invoice_settings.Comments
Why does the "Overview" suggest that admins review the "CiviContribute Component Settings"? As a precaution. The main consideration would arise if someone attempted to reconfigure the options while using a version (5.23.0-5.24.5) where the form didn't save reliably. That could produce confusion. The clearest antidote is to simply check the final settings and see if they are correct.