[WIP] dev/core/issues/860: discount not applied to line item: don't overwrite _priceSet if populated.

[davejenx]

Overview

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

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. The one in this PR avoids instantiating the price set twice by checking, in CRM_Member_Form_Membership, whether $this->_priceSet is already set: if it is, we skip the call to $this->setPriceSetParameters(). This is simple and it works, in my testing. It begs the question why the existing code instantiates the price set twice and I didn't find a convincing rationale for that. If there's someone who understands this rationale, they may be able to suggest a more satisfying fix. However the fix presented here is simple, fairly contained and, in my testing, leads to correct line items where previously they were incorrect. Incorrect line items are a Bad Thing, so I'm hoping a simple, contained fix will be acceptable even if it doesn't answer the bigger question of "but why is it trying to instantiate the price set twice?"

The other approach I tried was to insert a call to the buildAmount hook when instantiating the price set for the second time. This also works but
(a) required duplicating some code for setting up the fee block, including checking isACLFinancialTypeStatus() and
(b) retains the second instantiation of the price set, which seems unnecessary.
So I preferred the first approach.

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

[seamuslee001]

Test fails look related

[davejenx]

Yup. Wonder if it's a consequence of the tests doing things that wouldn't happen within a real request, like "rebuild the price set form variable to include the tax information against each price options" in testLineItemAmountOnSalesTax(). Anyway we can't go introducing test fails, so I'll mark this as WIP and submit a separate PR with my other approach to fixing the issue...

Done: #15004.

[davejenx]

I'll close this one, as #15004 fixes the issue without introducing test fails.