Revert "CRM-19626 - Event Registration page allows registration even …

[eileenmcnaughton]

…if no number (or the number 0) has been entered in any ticket quantity boxes"

This reverts commit 73dc8be.

My concern here is that this could be a breaking-feature-change/removal for some sites and I'm not convinced it is safe. I would prefer for it to go into next month - with someone taking the time to consult widely on it with the dev list / partners first.

[eileenmcnaughton]

@totten @colemanw @yashodha @monishdeb @jitendrapurohit @joshgowans @Stoob @seamuslee001 @xurizaemon @GinkgoFJG @agh1 @joannechester @petednz @JoeMurray @nganivet

Anyone want to talk me out of this? This change is giving me the heeby jeebies because my instincts say that someone will be using this as a feature - somewhat like the screen shot below.

I'm not wanting to say no to the change - I'm just wanting to slow it down and be sure rather than have it go out in a release which has critical bug fixes in it. I would see the proper process for a minor-feature change as being to invite comment from the partners list & dev-list, then to engage in a long round of discussion before giving up the will to live, followed by a decision of some sort based on people's input.

[yashodha]

@eileenmcnaughton As regards to CRM-19626 taking away the ability to by-pass paid event payment by entering 0 in text-box seemed like the logical thing to do. But I can see your point where someone may actually be using this as a feature in which case the same can be seen as an introduced bug.

We can address both the concerns by making it configurable and introducing a new column min_value in civicrm_price_field_value table which will take the call instead of the code deciding what the behavior should be.

Let me know if you agree on the same.

[agh1]

I agree with reverting the change. It would be nice to have an option (on the price set or the event) saying "require payment" or "require an option". It could basically apply the code here but only if that option is selected.

Anyway, if we go that direction, it shouldn't be done last-minute like this, so I'm favor of reverting for 4.7.14 and trying for 4.7.15.

[nganivet]

+1 on @Eileen, @yashodha and @agh1.

I checked with my customers and we have at least one use case for having the event free if no option is selected (membership org, only members can register and it is free for them, but they can bring a +1 and this would be paid. We cannot use 'register multiple participants' for that since the second participant would then be free as well as base pricing is the same for all participants in a register multiple).

[eileenmcnaughton]

OK - I'm going to reverse it and then we can keep discussing.

Re @agh1's comment - I would note the patch was not merged last minute. It's just when I read @agh1's release notes I spotted it and I was worried that I could see a use case not earlier though of - and from @nganivet's comments one that exists.

However, in the context of the release my preference is to revert & consider rather than push through to fix this. I've hit that a few times in this release, however the others have all been bugs related directly to a key fix ( #9390 ) and have undoubtably been bugs. I could have made the case not to fix them this release because they turned out to have been in all 4.7 releases but I wanted to reflect the energy & momentum of the people testing that process and felt that telling them to ignore particular bugs for now might derail them.

[eileenmcnaughton]

@yashodha I think min_value makes sense. What do others think of that proposal? My real preference is to be able to define 'or-sets' - this field OR that field is required - but that is a big scope change and I feel like even if we did that someone would come back and ask for a minimum.

I guess we would do something js-y in the UI so they don't have to click 'validate' to learn that it is below the min.

Also note - this PR has a unit test - we should rescue that for the replacement

[agh1]

@eileenmcnaugton sorry I wasn't clear: I meant a "real" fix like the min value shouldn't be last-minute. Reverting this now is great, though.

I don't think the price field min_value will help, though: the problem is in forcing someone to fill one of several price fields, none of which are required. Setting a minimum value on a price field would largely duplicate the functionality of requiring the field. Instead, the price set needs a setting.

[eileenmcnaughton]

Sorry - yes you are correct. I was thinking the field would go on the price set - but Yashi said price field. Am assuming you meant set @yashodha ?

[yashodha]

@agh1 @eileenmcnaughton : I meant price_field_value table only since text field is the one that depends on the user input. We have no qualms about the selects/radio/checkbox since the price can't be variant and are admin defined which is why I proposed on field value level and not on set.

min_value(more like min_entered_val) will not duplicate is_required field in the sense that min_value check will be done ONLY if the user enters anything in the textbox.

So for Eileen's scenario she can have min_value 0 and the user can get by without paying anything by just entering 0.

For the reporter's issue he can configure min_value 1 for all his text-boxes, and if the user chooses to enter any value it has to be > 0 if entered.

Let me know if that makes sense :)

[agh1]

@yashodha I'm looking back at the original description in CRM-19626:

The user creates an event page, using a price set that features a few 'Text / Numeric Quantity' boxes for users to specify how many of each ticket type they wish to purchase e.g. 3 student tickets, 4 standard tickets.

I interpret this as a price set with two text/numeric fields--student tickets and standard tickets--neither of which are required. A single student attending should be able to enter 1 student ticket and 0 standard tickets, and a single non-student should be able to enter 1 standard ticket and 0 student tickets.

If both fields were to have a min_value, he could set them both as required just as easily with no changes in the code, but he needs to accommodate the person who wants 0 student tickets.

We have gotten this sort of request a number of times, but being a development shop, we solve it by writing custom validation to ensure that at least one field is filled. However, a core fix should be basically the same: a setting at the price set (or event) level that either requires a non-empty form or requires a minimum total.

[universalhandle]

I think reverting was the right call.

To play devil's advocate a bit, CRM-19626 is kind of an edgy case. I'm not sure I completely understand them all, as it's easier to react to something visual vs. a write-up, but the proposed solutions seem to make the already elaborate event configuration even more involved. Perhaps custom code is in fact the best way to address this.

Not sure I entirely believe what I'm saying -- just throwing it out there to see if it sticks to the wall.

[nganivet]

After re-reading this thread twice I think I agree with Frank: the original author use case, my customer's, and what Yashi suggested are not quite the same. I am also sure we could find other uses cases that require different tests on the user input. So putting this logic in an extension makes a lot of sense.

I have wanted to compile an 'Extension Writing Guide' or extended chapter in the developer documentation for some time, recap'ing best practices for CiviCRM extension development as these are either oral or difficult to find. Having a 'library' of example extension is part of that guide, and this one certainly would have it's place there.

[eileenmcnaughton]

I think my general feeling is that we want core to have a logical offering and for the 'quick_config' pricesets to work the same as the non-quick-config. I think quick-config offers a min_amount? In which case we should consolidate that functionality in core with quick config and offer for both. So, I think @yashodha's suggestion about adding a min_amount to the price set is OK.

I agree about being careful about adding to core vs extension. But, the price set code and UI and the way it interacts with the concept of 'quick-config' are pretty bad at the moment so we should be working towards something more logical and with cleaner code and less wierd oddities. I don't mind adding a minor feature to core if it is working towards a bigger picture, as long as it does not regress other flows.

[eileenmcnaughton]

NB assuming @yashodha MEANT the min_value to go on the price set

[eileenmcnaughton]

Oh hang on - I just saw @yashodha's clarification - no by brain hurts that seems like a real edge case. I'll have to come back to that with a fresh brain. I understood the idea of enforcing a minimum total

[eileenmcnaughton]

Sorry - re-reading the ticket my understanding is their problem is that people can enter 0 in all 3 price field boxes and not pay anything. min_value on price set would seem to meet this requirement

[yashodha]

I agree with the idea of keeping it simple and having min_value at price set level to ensure minimum price is paid for one to be registered to the event.

I will add update the issue description in JIRA.

[eileenmcnaughton]

Thanks @yashodha - I feel bad reverting your fix - but I think it was the right thing to do.

[yashodha]

@eileenmcnaughton : No problem :)