Extract isMembershipPriceSet (useForMember)

[eileenmcnaughton]

Overview

Extract isMembershipPriceSet (useForMember)

Before

I've spent hours trying to figure out what useForMember means & how it is set & used

After

Hopefully the meaning & how it is derieved is clear-ish. As for used - still figuring that out - see technical details....

Technical Details

@demeritcowboy I'm hoping you can help me here. I think I got the if for ccid right but it broke my brain a bit

In the online receipt we see

{if $membership_assign && !$useForMember}

After working through this I think that effectively means {if $membership_assign && !$isQuickConfig} and if true then present basic membership data & no line items

{if !$useForMember and isset($membership_amount) and !empty($is_quick_config)}

• I think $useFormMember is redundant here & just checking quickConfig is enough

Ditto in this line

{elseif empty($useForMember) && !empty($lineItem) and $priceSetID and empty($is_quick_config)}

and

My eventual theory is that useForMember wasn't robust enough and is_quick_config was added, without removing useForMember.

If you agree then I will remove all the usages of useForMember in the online_membership_receipt template

I'm trying to get rid of the issets around membership_amount and amount_level - but kept getting confused on what useForMember means

Comments

[civibot]

(Standard links)

• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[eileenmcnaughton]

priceset would only be empty for no amounts section at all

[eileenmcnaughton]

I added in the order object here - we are only using it to get the cached price set metadata but it has a lot of helpers that can, in future fixes, remove a lot of the code in these classes. In general I want things that can be calculated to not be passed around / only to be set as properties within functions that expose how they would be calculated - so using the cached price set metadata is the direction I want to go. The order class functions will replace init whic is where _priceSet is otherwise retrieved from

[demeritcowboy]

The test run went bananas but it looks like there is a real fail: not ok 921 - Error: CRM_Contribute_Form_ContributionTest::testContributionBasePreProcess. I don't see a stack trace just that message.

[eileenmcnaughton]

@demeritcowboy yeah - it is an awful test - I think I fixed it. The test is awful because

1. it tests the parent class 'as if' it is a form - which it isn't & it's preProcess is not supposed to stand alone and
2. it says it is checking for something that it doesn't check for. It does give a php error when run against the right class but I wound up commenting out that part of the test rather than entering the rabbit hole (well OK I'm already in the rabbit hole but.... there are degrees)

[eileenmcnaughton]

It is passing now

[demeritcowboy]

Right, I meant to take look-see. Will do.

[eileenmcnaughton]

@demeritcowboy yeah - I don't want to put pressure on you - but it would be nice if this was on your list somewhere cos I am pretty confused with the code & variables!!! And your Sherlock Holmes senses would be very helpful

I'm hoping to make the changes proposed above the the template & now we have other changes in that template this release it would be nice to hit the same release.

[mattwire]

I've looked through some of this before and it's a pit of doom. The code changes you are proposing look sane but it will almost certainly break some random scenario in a filing cabinet at the end of the universe. When it does break we can clean up the code a bit more and bring sanity to the universe.

[demeritcowboy]

Thanks for looking at it. I had taken another look recently but other than it seeming to at least at one time be about using price sets for memberships it is really difficult to get into.

[brienneK]

I ran into a validation error involving Memberships on a Contribution page; I did a git bisect and it seems like this PR (specifically commit e499272) may have introduced the issue.

[eileenmcnaughton]

thanks @briennekordis I will check it out!