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

Conversation

totten
Copy link
Member

@totten totten commented Apr 30, 2020

Backport #17188 from 5.25 RC to 5.24 stable.

Note: There is an upgrader step, updateContributeSettings(), which was previously defined. This step is invoked by both PRs (5.25 RC and 5.24 stable) . The upgrade logic in updateContributeSettings() functions as a "fill" operation (i.e. converting data from legacy format but not overwriting newer settings), so it should be safe to re-run in multiple upgrades.

eileenmcnaughton and others added 8 commits April 29, 2020 16:41
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.
…_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%';
```
Consider this use-case:

* Install 5.22
* Configure `CiviContribute Component Settings`
* Upgrade to 5.24.6

The settings are lost during the upgrade.
…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.
@civibot civibot bot added the 5.24 label Apr 30, 2020
@civibot
Copy link

civibot bot commented Apr 30, 2020

(Standard links)

@totten totten merged commit a1b4551 into civicrm:5.24 Apr 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants