CRM-19608 - Show/hide auto-renew element on membership pages even whe…

[MegaphoneJon]

…n a non-quick price set is used

[eileenmcnaughton]

This is a good change from a code point of view in that the concept of quick config was originally supposed to only refer to the visuals for configuring the pricesets. The processing pages were not supposed to handle them differently. Unfortunately it didn't work out that way but we are trying to remove references & special handling for quick config as much as possible

[MegaphoneJon]

@eileenmcnaughton CRM-10117 suggests that there SHOULD be differences in how the processing pages handled the quick config price sets. E.g. the "Total Amount" (see screenshots below, second shot is NOT quick config).

IMO I think we should remove the differences between how processing pages render both types of price sets - it feels like needlessly added complexity. I'm concerned about making changes to existing sites' contribution pages though.

[eileenmcnaughton]

Yeah - that total amount thing is being based on 'is_quick_config' - but it could also be 'always on' or based on there being more than one option in the price set.

[seamuslee001]

@MegaphoneJon @eileenmcnaughton just looking through the PR queue, Where is this PR at. What should we be doing next?

[MegaphoneJon]

@seamuslee001 Thanks for your stewardship! IMO this needs someone from core team to decide whether using "quick_config" price sets should affect the display. @eileenmcnaughton and I say no; CRM-10117 and the source code suggest that Dave Greenberg thought otherwise.

Then @eileenmcnaughton can chime in whether she believes her earlier comment rises to the level of PR review. If yes: merge(?) If no, needs review.

[eileenmcnaughton]

I feel like we have a strong habit in our codebase of having one thing 'stand in' for another thing. In this case the thing is 'is_show_total_amount' - I think the 'right' fix would be to have a field on the civicrm_price_set table that determines whether total_amount shows. That's not to say it has to be 'this fix'

[eileenmcnaughton]

@monishdeb @jitendrapurohit @mattwire @colemanw @GinkgoFJG @seamuslee001 anyone have thoughts on this - it has been languishing for a long time for no strong reason.

It

1. adds the total amount to forms, regardless of the isQuickConfig flag. I think that is less confusing.
2. Adds membership autorenew, regardless of the isQuickConfig flag. Ditto, but I wonder if there are any possible implications on this one

PS this is now the oldest PR

[mattwire]

Well it makes sense to simplify it here. I hadn't realised there was a difference. It seems that something is slightly broken in current master anyway as it's supposed to be displaying "This membership will renew automatically" or show a checkbox to enable/disable auto-renew based on the auto-renew config on the membership. But it seems the javasscript to do this is only being called once on page load and not each time the radio is changed.

[eileenmcnaughton]

OK - that strengthens my believe this change should be made - have upped it to 'merge-ready' - will leave a bit longer for more feedback

[colemanw]

Makes sense to me.