dev/financial#84 Remove sequential credit notes from core #16462
Conversation
|
(Standard links)
|
eileenmcnaughton
changed the title
eileenmcnaughton
force-pushed
the
creditnote_gone
branch
2 times, most recently
from
a504ac1
to
ad00405
Compare
This moves the code that allows settings to be added to forms by metadata from the Generic setting form to the setting trait. This means many more forms will support injecting settings by metadata. Specifically we switch across the Contribution preferences form, making credit_note_prefix a metadata-added field. This is preliminary to moving that to an extension & allows us to inject it in by simply defining the metadata in a hook
eileenmcnaughton
force-pushed
the
creditnote_gone
branch
from
ad00405
to
fd12526
Compare
…sion that ships with core. https://lab.civicrm.org/dev/financial/issues/84 The code that adds sequentialcredtit notes is a hangover from pre-Lexim days when people's various needs just got added to core. It would not have been added to core post Lexim for the following reasons 1) architectural - it assumes a one to one relationship between contributions & credit notes which is not reflective of our data model 2) performance - the code had significant performance impact on large sites 3) specificity - the code was for a specific localised use case of needing guaranteed sequential credit-notes 4) maintainability - we've had to put in a lot of work to get the code down to the few lines being removed. The goal is not to allow organisations to transfer maintenance burden onto the community unless there is a clear rationale. Medium term the intent is that this extension will not ship with core & will simply be available from the extensions directory as any other extension. In order to reach this point we need a transitional approach. In the first instance sites should need to take no action at all in order to experience no change of behaviour. In order to achieve this we continue to ship this functionality with core as an extension. We enable the extension on upgrade (but on new installs it is opt-in). At this point sites who do not want this functionality can disable the extension. The next step will be to get people who want the extension to install it to their extensions directory so we can remove this from core later. This will involve us fixing the extension load code to prefer the extension directory over core if the extension is in both places & adding checks & messaging to encourage people to install the extension if they continue to want it. At some point we will remove it from the core tarball. Ongoing maintenance will be provided by whoever wants to maintain it - if someone wants to be added as a maintainer of the extension repo the answer is yes.
eileenmcnaughton
force-pushed
the
creditnote_gone
branch
from
fd12526
to
05c1e0d
Compare
In the course of civicrm#16462 I concluded that there might be (untested scenarios) where this would still be hit. In civicrm#16462 I adopted a cleaner (tested) approach. However, I think it's unlikely that will be merged before 5.23 is cut so I propose reverting this change & re-reverting it once the rc is cut as a more conservative approach. We can focus on getting civicrm#16462 wrapped up for 5.24 This reverts commit d7943ca.
| 'deferred_revenue_enabled' => CRM_Core_BAO_Setting::CONTRIBUTE_PREFERENCES_NAME, | ||
| 'default_invoice_page' => CRM_Core_BAO_Setting::CONTRIBUTE_PREFERENCES_NAME, | ||
| 'invoicing' => CRM_Core_BAO_Setting::CONTRIBUTE_PREFERENCES_NAME, | ||
| ]; |
There was a problem hiding this comment.
Is this a tangential cleanup? (Not trying to criticize - just so that one can read/interpret better.)
| * @throws \CiviCRM_API3_Exception | ||
| */ | ||
| public static function installCreditNotes(CRM_Queue_TaskContext $ctx) { | ||
| civicrm_api3('Extension', 'install', ['sequentialcreditnotes']); |
There was a problem hiding this comment.
So if one is accepting the premise/goal and focused on technical aspects of implementation - would it be fair to say that this task (automatically enabling the extension during an upgrade) is perhaps the main "new" thing going on? e.g. for reviewing, it's important to do some r-run on this aspect?
There was a problem hiding this comment.
I separated it out #16513
|
On the Gitlab issue, I've pinged about the policy/conceptual level. Regarding review of the implementation... I think this code passes a basic skimming, and let me try to verbalize a bit about what a reviewer would be looking for (apologies if this is stating the obvious):
|
|
On gitlab there was push back - and to be honest i think the same push back would apply to all the code in core that we might ideally consider moving into non-core extensions (eg. event cart, financial type acls, Surveys, USPS parsing)
I have concluded it's in the too hard basket for the next few years to actually move things out of core (and that we should focus on only letting code in if it improves the maintainability of the code). However, in looking at how to deal with this code it was apparent
In discussion with @colemanw we realised that
By doing this we provide a path forwards for other cleanup - ie. it's always been tough to find a path towards tidying up code features that are badly implemented all over the place. I tried to extensionise some of the dedupe code at one point to deal with maintainability issues (there is a hook in the middle of it that makes it uncleanupable because the surface areas is 'everything' & the idea was to have a version 1 extension & then later a version 2 extension with a sensible hook structure) - but because it had to be neck or nothing I had to throw in the towel. If there was a path towards refactoring-towards-code-being-a-concise-feature then I would have been chipping away at that. Likewise the invoicing code uses around about 100 lines of code across 20 forms (although I have cleaned that up a bit) to do what could be done in a one-line in a preProcess hook (assign a variable as to whether the setting for invoicing is enabled)- having a way to register hooks by features would help. While I don't think we should be totally flip about designating every checkbox as a CoreExtension - the process of separating the features out is also the process of cleaning up the code & if we have a way to structure those features & use hooks (without being committed to making a maintenance decision of any sort) it unblocks us a lot in cleaning up some of the code that is plaguing us and encourages us to mature our hook infrastructure. |
|
Closing - this is still part of core - although it's restructured within core |
Overview
Moves sequential credit notes from core to an extension that ships with core.
https://lab.civicrm.org/dev/financial/issues/84
Extension is now
https://lab.civicrm.org/extensions/sequentialcreditnotes
https://civicrm.org/extensions/sequential-credit-notes
Review request https://lab.civicrm.org/extensions/extension-review-requests/issues/12
Before
When a contribution is created as refunded a credit note id is calculated. This calculation is slow and is done regardless of whether the site requires it.
After
The above is done in an extension - sites can disable the extension & eventually it will not ship with core
Technical Details
The code that adds sequentialcredtit notes is a hangover from pre-Lexim days when people's various needs just got added to core.
It would not have been added to core post Lexim for the following reasons
to allow organisations to transfer maintenance burden onto the community unless there is a clear rationale.
Medium term the intent is that this extension will not ship with core & will simply be available from the extensions
directory as any other extension. In order to reach this point we need a transitional approach. In the first instance sites
should need to take no action at all in order to experience no change of behaviour. In order to achieve this
we continue to ship this functionality with core as an extension. We enable the extension on upgrade (but on new
installs it is opt-in).
At this point sites who do not want this functionality can disable the extension.
The next step will be to get people who want the extension to install it to their extensions directory
so we can remove this from core later. This will involve us fixing the extension load code to prefer the extension
directory over core if the extension is in both places & adding checks & messaging to encourage people to
install the extension if they continue to want it.
At some point we will remove it from the core tarball. Ongoing maintenance will be provided by whoever wants to maintain it - if
someone wants to be added as a maintainer of the extension repo the answer is yes.
Comments
Previous attempts to fix performance issues
#13511
#15232 - makes it possible to opt out by extension but logically it should be opt in by extension
#15235