dev/financial#54 Fix mishandling of deferred revenue settings #14267
Conversation
|
(Standard links)
|
fe5e055 to
6c5f362
Compare
|
Jenkins re test this please |
6c5f362 to
72f7ab1
Compare
|
@eileenmcnaughton Jenkins doesn't like your style https://test.civicrm.org/job/CiviCRM-Core-PR/26200/checkstyleResult/new/ |
a1d782c to
4fa1e00
Compare
|
This is a needed step that gets rid of some more problematic, legacy code. If this could be tested by a couple of "financial savvy" users @monishdeb @kcristiano then I'm happy to merge |
| * @param string $rev | ||
| */ | ||
| public function upgrade_5_15_alpha1($rev) { | ||
| $this->addTask('Fix errant deferred revenue settings', 'updateContributeSettings'); |
There was a problem hiding this comment.
don't we need a $this->addTask(ts('Upgrade DB to %1: SQL', array(1 => $rev)), 'runSql', $rev); here
There was a problem hiding this comment.
Yes - good spotting. Added
| * Test checkContributeSettings. | ||
| */ | ||
| public function testCheckContributeSettings() { | ||
| $settings = CRM_Contribute_BAO_Contribution::checkContributeSettings('deferred_revenue_enabled'); |
There was a problem hiding this comment.
@eileenmcnaughton is there any handling here for storing the settings in the correct way if we get in the wrong format or saying its in the wrong format?
There was a problem hiding this comment.
@seamuslee001 how do you mean? It just copies the setting from the wrong place (nested in a key) to the right place (it actually leaves a duplicate but I think that's OK since we can clean that up when the rest of the settings are migrated from that key)
Currently we have a non-std store for invoicing settings that we are checking, yet the form saves it using our stds. This updates checks for these settings to actually check the right place
4fa1e00 to
504770b
Compare
|
Jenkins test this please |
|
I am happy with the final patch and agree with the changes. Tested on my local. @seamuslee001 @pradpnayak any last thoughts before merging it? |
|
I think not. Merging it now |
Overview
When invoicing was added to core the invoicing settings were added as an array rather than individual settings. This is against our standard but it was a long time ago & our review processes were poor then. However, it has had ongoing repercussions. This addresses one of them and also adds a process for converting them. We need this because switching to correctly referencing the deferred revenue settings might otherwise miss some where people have added the settings to follow the wrong format
Before
Saving the 2 settings
Always post to Accounts Receivable?
Enable Deferred Revenue?
Through the contribution settings form does not result in them being respected
After
The settings are respected
Technical Details
We should follow this up by changing the remaining settings in this array to being meta-data defined & then call the convert function
Comments
@mattwire @monishdeb @JoeMurray
https://lab.civicrm.org/dev/financial/issues/54#