dev/financial#84 Move sequential credit notes from 'deeply embeded functions to separate structure

[eileenmcnaughton]

Overview

Restructures how sequnentialcreditnotes is implemented from an 'enmeshed' approach to a listener approach.

This introduces a new way of structuring our code - allowing us to implement features that are in core using listeners/hooks. The approach is basically an 'invisible extension' and allows us to separate all the things that implement one feature into one folder. This offers a path forwards for how to clean up the features in core that make it extremely hard for us to fix up the code.

Before

sequentialcreditnotes is implemented in the BAO - not only in the ContributionCreate BAO but also in the deeper financial functions that calls.

After

The functiionality is still in core and there is no apparent change to users. However, it is moved out to an invisible extension'. This cannot be seen in the UI and does not imply any future decision to remove it from core - but it does provide us with a path to clean up the code.

Note the hiding from the UI is done via a new key ui_visibility in the extension info.xml

Technical Details

https://lab.civicrm.org/dev/financial/issues/84

We have a significant problem in our code base that it is hard to cleanup our key processes and make them usable and Form-builder ready because of all the features deeply interwoven into them. It will be important that our Order->add payment flow works and can be utilsed by the form builder. The expectation is that adding a payment will update all the appropriate entities. We've been working pretty hard on this for a couple of years now (I think the time taken to clean up code is sadly greater than the time it takes to add it :-() and the Payment.create part is getting pretty solid now.

However, we are consistently hampered by code that has been 'enmeshed' into deep BAO code. In the case of the credit note creation something that can easily be implemented as a pre hook and kept entirely out of the Contribution.create function is not only embedded in there but also in updateFinancialAccountsOnContributionStatusChange - which is one of the functions we are some dozens of hours into adding tests & 'sorting out'. We need to find a way to

1. Declutter key functions where possible and
2. Model good ways of implementing things by hook (& work through limitations in this approach) as part of cleaning
   up core and pushing towards a LeXIM approach.

I originally agreed with the CiviDiet approach that we should try to move the myriad of things that are 'add ons' that someone thought was necessary & useful to others (or less charitably they thought that if they got it into core someone else would have to maintain it for them) moved out of core into extensions and eventually the maintenance of them moved out of the core team too.

However, it's clear from the discussion on moving credit notes out of core that this is too hard for the forseeable future. We can try to avoid more features being added to core but most features in core are used by at least one organization & the politics of moving things out are too hard. On the bright side we let a lot less new features into core these days - not none - the membership status override date springs to mind and the ongoing adding of fields & filters to reports - but less.

This leaves us with the question of whether we can still achieve the goal of making add-on features less of a developer burden. In addition to ensuring code is tested we can do this by getting it out of the 'deep functions' into a listener structure. I was able to covert the 'spaghetti-like-tendrils' in the credit note code to a simple 'pre' hook.

So it felt like we needed to create something new. Something like .. an extension .. but not visible on the extensions screen and in core. Something like ... an extension in a folder in the core codebase that is suppressed from the extensions UI....

So this adds it as an extension to core in a new location & ensures GUI users cannot see it. I don't know exactly where we draw the line around packaging things this way - Partial payments is clearly part of the core architecture, but there might be a world in which we move something like event cart to be a more distinct bundle. As we move to form builder forms we are going to
want forms being replaced to be able to be 'quarantined' for future amputation.... I would be looking to move financial type acls and invoicing to this structure - mostly because they are the biggest 'offenders' in terms of having code all over the place causing problems with other code initiatives & we've struggled to come up with a way to deal with that other than putting touching that code in the 'too hard basket'. But even things like PCPs & Premiums would be more maintainable if they were discrete & overcoming the challenges involved in that would solve a lot of extensibility issues.

Note this approach permits an incremental cleanup-up-to-separation process. For example we could create an extension for a feature and move some code to it and later move some other code. For example we could move the invoicingPostProcessHook to a shell invoicing extension and at some later point address other parts of the invoicing code (check LineItem::getLineItems
for the chunk of code that has been blocking other cleanup - moving this to a listener would help a lot

Note this PR does not address the significant performance issues associated with the creditnotes code on larger sites, only the impact it has on our code maintainability. If we can agree a way to start to address how to split out or code into listeners in an agreed structure then I can look at that as a follow up.

Comments

Tangentally part of me thinks the modernisation of our code base is the process of moving everything out of CRM into Civi, vendor, & I now I would say Core_Extensions

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[colemanw]

Wow. Good stuff.

I don't want to get hung up so this is a non-blocking question:
Is the reason for hiding the extension from the extension management screen a technical reason or a political one? Cause in general I'd be in favor of allowing admins to disable extensions if they want to (if doing so doesn't break anything).

Also, I believe there is a new extension UI in the works and I wonder if it will respect that flag.

[eileenmcnaughton]

@colemanw it's political - there is push back on it & I don't want that push back from preventing us moving forwards technically

[eileenmcnaughton]

Unrelated fail

[eileenmcnaughton]

test this please

[eileenmcnaughton]

Note the correct behaviour after testing is 'no change' - if you change a contribution status to 'Refunded' a creditnote_id should still be created as before

[totten]

1. May I suggest info.xml - Allow extensions to define a list of tags #16551 and something like in_array('hidden', $obj->tags)?

2. I like the idea of adding more metadata and using that to influence the extension-management. For example: which extensions are activated automatically during installation? Which can be disabled or uninstalled?

3. The phrase "ui visibility" feels odd to me. Does that convey something more than visible or hidden would convey?

I'd sort of like to see a list of extension-management-related tags that we can put into dev-docs. Spitballing:

• autoinstall: The extension is activated during Civi installation)
• hidden: The extension should omitted from the admininistrative GUI (but still be visible to APIs)
• mandatory: The extension cannot be disabled. (This may also imply that it's activated during Civi installation.)
• suggested: The extension should be presented as an option in the install UI (alongside the various components)

[eileenmcnaughton]

Ohh I like the look of that install proposal!

I'm gonna move to that PR to comment

[eileenmcnaughton]

Updated above^^

[eileenmcnaughton]

I added docs for the hidden feature here civicrm/civicrm-dev-docs#761

[colemanw]

Ok the new tags approach seems sensible and this can move forward.

[eileenmcnaughton]

Thanks @colemanw