dev/core#860: discount not applied to line item: call buildAmount hook in CRM_Member_Form_Membership::submit().

[davejenx]

Overview

Fix for dev/core/issues/860 Incorrect line item created for back-end membership sign-up using price set and CiviDiscount.

This is an alternative approach to the one in PR #14994, which caused test failures. See Technical Details below.

Before

When doing a back-end membership sign-up using a price set, with an additional item e.g. donation and with a CiviDiscount code applying to the membership, the line item created for the membership does not reflect the discount but the contribution total does. The problem does not occur without the additional item.

Steps to replicate

1. Create a CiviDiscount code for e.g. 25% off General membership.
2. Create a membership price set with 2 fields: Membership with options General & Student; Donation text field.
3. Create a contribution page using the price set (we don't use this here but in my testing, the discount did not work on back-end sign-ups until I did this step.)
4. Go to a contact's summary -> Memberships tab and click Add Membership.
5. Enter the discount code & Apply.
6. Choose price set -> select the above price set.
7. You may need to click Apply again to get the discount to show for General membership.
8. Select "General (Includes applied discount: 25% Test for General membership) - $ 75.00".
9. Enter a donation amount, e.g. $5.
10. Leave "Record Membership Payment?" ticked, leave other fields as default.
11. Click Save.

Expected result
Contribution created with total amount $80 and 2 line items:

• entity_table: civicrm_membership, line total $75.00
• entity_table: civicrm_contribution, line total $5.00

Actual result
Contribution created with total amount $80 and 2 line items:

• entity_table: civicrm_membership, line total $100.00
• entity_table: civicrm_contribution, line total $5.00

Note that the membership line item has the wrong amount and the sum of the line items is not equal to the contribution amount.

After

Actual result = Expected result

Technical Details

I traced the misbehaviour in a debugger and found that the price set was being instantiated twice, once with the discount, once without, causing the line items to be calculated without the discount. However if there was only one line item, a different route through the code kicked in, overriding the line item amount. I have some detailed notes from debugger sessions if those would be of interest.

I tried a couple of different approaches to fixing:

1. PR [WIP] dev/core/issues/860: discount not applied to line item: don't overwrite _priceSet if populated. #14994: avoid instantiating the price set twice by checking, in CRM_Member_Form_Membership::submit(), whether $this->_priceSet is already set: if it is, we skip the call to $this->setPriceSetParameters(). This worked in my testing but led to unit test failures.

2. This PR: insert a call to the buildAmount hook when instantiating the price set for the second time. This also works in my testing. It
   (a) requires duplicating some code for setting up the fee block, including checking isACLFinancialTypeStatus() and
   (b) retains the second instantiation of the price set.

[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).

[davejenx]

Hopefully that fail on E2E_Cache_ArrayCacheTest::testSetTtl is unrelated? - I see all other builds over the last 11 hours have failed, so I assume it's unrelated.

I'm working on a unit test for this issue.

[seamuslee001]

Jenkins re test this please

[eileenmcnaughton]

@davejenx wow that's a scary function. Can we
a) extract the lines you re-use out of that buildPriceSet & share the call to those lines, rather than duplicating them
b) remove references to this class in buildPriceSet - e.g if ($className == 'CRM_Member_Form_Membership') {
c) lets get rid of the & here $feeBlock = &$form->_priceSet['fields']; - probably by not defining $feeBlock at all

[davejenx]

Hi @eileenmcnaughton,
Here's a unit test plus your refactoring suggestions (a) & (c). Re (b), CRM_Price_BAO_PriceSet::buildPriceSet() makes several uses of $className and 'CRM_Member_Form_Membership', did you have an alternative plan for those?

[agileware-fj]

@davejenx, I've applied your PR to 5.16 on our sandbox - applies cleanly but doesn't seem to have any effect for our scenario.
That said, I've just realised that my test path did not include using a price field. Would you expect this to work in that scenario?

[davejenx]

@agileware-fj I suspected your case described at civicrm/org.civicrm.module.cividiscount#224 was different when you said you can replicate without adding an additional amount. The issue described in dev/core/issues/860 & addressed in this PR only occurs when there are > 1 line items.

When I ran the issue 860 case but with a single line item through a debugger, I found:

In CRM_Price_BAO_PriceSet::processAmount(), line 703:

      if (count($priceFields) == 1) {
        $amount_override = CRM_Utils_Array::value('partial_payment_total', $params, CRM_Utils_Array::value('total_amount', $params));
      }

sets $amount_override to 75. This is passed to CRM_Price_BAO_LineItem::format() which uses it to adjust the line item amount - though not the label.

The backtrace there was:
CRM/Price/BAO/PriceSet.php.CRM_Price_BAO_PriceSet::processAmount:704
CRM/Member/Form/Membership.php.CRM_Member_Form_Membership->submit:1166
CRM/Member/Form/Membership.php.CRM_Member_Form_Membership->postProcess:927
CRM/Core/Form.php.CRM_Member_Form_Membership->mainProcess:490

I'd vote for creating a separate core issue for your case, as the steps are different from the specific scenario I described in issue 860 and the fix is evidently different too.

[agileware-fj]

Reviewing this today
Agileware internal ref is CIVICRM-1300

[agileware-fj]

Sorry, didn't get to finish reviewing on Friday, but after sorting out buildkit woes today I'm happy with the results.

Review

• General standards
  • Explain (r-explain)
    • PASS : The goal/problem/solution have been adequately explained in the PR and with a link (Gitlab dev/core#860).
  • User impact (r-user)
    • PASS: The change would be intuitive or unnoticeable for a majority of users who work with this feature.
  • Documentation (r-doc)
    • PASS: The changes do not require documentation.
  • Run it (r-run)
    • PASS: Ran up a clean master buildkit environment and followed the steps bother before and after applying the patch. No change to workflow, and the PR fixes the issue as described
• Developer standards
  • Technical impact (r-tech)
    • PASS: The change preserves compatibility with existing callers/code/downstream.
    • COMMENTS: No interface changes, just a bug fix.
  • Code quality (r-code)
    • PASS: The functionality, purpose, and style of the code seems clear & sensible.
    • COMMENTS: This PR adds atomicity to Financial Type ACL checking, which seems unrelated but is a direct result of needing to reuse this in the membership form. As a result, this check is now reusable in other places.
  • Maintainability (r-maint)
    • PASS: The change sufficiently improves test coverage, or the change is trivial enough that it does not require tests.
  • Test results (r-test)
    • PASS: The test results are all-clear.

[eileenmcnaughton]

Thanks @agileware-fj & thanks @davejenx for your work on this - merging per review