vcs.fsf.org Git - civicrm-core.git/log

eileen [Wed, 12 Feb 2020 13:23:25 +0000 (02:23 +1300)]

dev/financial#84 Move sequential credit notes from 'deeply  embedeed in BAO functions' to usin a listener structure

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.