Use correct...er flow to create partial payments on events
authoreileen <emcnaughton@wikimedia.org>
Fri, 31 Jan 2020 08:27:18 +0000 (21:27 +1300)
committereileen <emcnaughton@wikimedia.org>
Mon, 3 Feb 2020 21:13:11 +0000 (10:13 +1300)
commitd3e6e9a47496ff046056c589b18f0279e70264e3
treeae020bed4c1d646cb6bb426d5fcbc78dc003de7b
parent67b7cfa19f3cd398d3b2538681cb7311a9c6fa79
Use correct...er flow to create partial payments on events

This fixes the methodology for recording partial payments on the backoffice contribution form to create a pending contribution (order)
and then add a payment. The previous method was our first cut at supporting partial payments (before the Order+Payment flow) and
relies on some handling in the BAO which is crufty & has proven unreliable. This allows us to deprecate that handling & remove the
now unused addPayments function.

Prior to this PR I wrote a test for current behaviour & I had to alter that test for this as follows

1) prior to this change net_amount was clearly miscalculated & hence this fixes & I updated the test
2) prior to this change a financial transaction was created for the contribution. Now only the payment financial transaction is made.
3) prior to this change the financial item was set to partially paid, not it is still 'unpaid'

Items 1 is clearly a bug fix whereas 2 & 3 are a bit borderline.

Item 2 is the fact that for pending contributions (in general) we don't create financial items when creating them. We do for (some) other
statuses. This was  discussed here https://github.com/civicrm/civicrm-dev-docs/issues/712#issuecomment-545164618 & the summary was
'it would be better to change this but this is what was the scope /how it is'

Item 3 is discussed in https://docs.civicrm.org/dev/en/latest/financial/financialentities/ with the summary  being that in any other path
to creating a partial payment the financial item would be left at 'Unpaid' - hopefully until it's fully paid.

Item 1 is a clear bug fix & I think the fact no-one picked it up is material because it gives us a clue as to how many
people use this rather obscure flow that requires an admin user to alter the status to 'partially  paid' and alter the amount.

Given the above we have a choice
1) Make this flow behave like other flows, despite this flow being arguably correct or
2) Hack away to keep the current quirks

I've come down on the side of 1 as a standardisation. I think changing it so items 2 & 3 behave differently is worth pursuing but it's better
for this edge case flow to do the same as our approved flow & if we want to iterate on that we can
CRM/Contribute/BAO/Contribution.php
CRM/Event/Form/Participant.php
CRM/Financial/BAO/Payment.php
tests/phpunit/CRM/Contribute/BAO/ContributionTest.php
tests/phpunit/CRM/Event/Form/ParticipantTest.php