CRM-20637 extend expired price fields in backend

[lcdservices]

see the explanation in Jira for rational. this extends access to expired price set fields in the backend. it also changes the variable name to be inline with it's purpose and term in the method declaration. the existing variable name is misleading.

---

• CRM-20637: display expired price set fields for backend regstration

[seancolsen]

I figured I'd review this to help reduce the PR queue, and this one is pretty old.

Review

(CiviCRM Review Template WORD-1.1)

• (r-explain) Pass
• (r-test) Pass
• (r-code) Pass
• (r-doc) ❌ Needs changes
• (r-maint) Pass
• (r-run) ❌ Needs changes
• (r-user) ❌ Needs opinions from other people
• (r-tech) Pass

Opinions from other people requested to resolve r-user

The big question here is this... the price_field entity has active_on and expired_on fields to control a period of time during which the price field is "active". During the inactive time period, should the price field be:

• (a) inactive for all users?

  — or —

• (b) inactive for public event registrations and active for back-office registrations?

The case for (a):

• It's how CiviCRM currently functions
• In looking at other commits to the related code, I noticed CRM-19918: Price field 'Active on' date ignored in Edit Event Registration > 'Change Selections' #9764 (reported by @twomice, committed by @monishdeb, and approved by @jitendrapurohit) which made the back office "Change Selections" interface more restrictive in order to hide price set fields with active_on dates in the future. But it could be that Allen's main priority was to bring the "Change Selections" screen to a point where it would function consistently with the "Add Event Registration" screen.

The case for (b):

• @lcdservices is advocating for (b) with this change
• Personally, I agree

For the people tagged above: What do you think about (a) vs (b)? If you're for (a), can you explain a little about why?

Docs changes requested to resolve r-doc

If we decide on (b) and go forward with this change, I think it would be helpful to better explain this functionality to users. A slight tweak to the help text should do it. Here we go:

diff --git a/templates/CRM/Price/Form/Field.tpl b/templates/CRM/Price/Form/Field.tpl
index 8ee57cec2f..bdad23b1ce 100644
--- a/templates/CRM/Price/Form/Field.tpl
+++ b/templates/CRM/Price/Form/Field.tpl
@@ -207,7 +207,7 @@
       <td class="label">{$form.active_on.label}</td>
       <td>{include file="CRM/common/jcalendar.tpl" elementName=active_on}
       {if $action neq 4}
-        <br /><span class="description">{ts}Date this field becomes effective (optional).  Used for price set fields that are made available starting on a specific date.{/ts}</span>
+        <br /><span class="description">{ts}Public forms will not display this price field before the set date. Back office forms will be unaffected by this date.{/ts}</span>
       {/if}
       </td>
     </tr>
@@ -216,7 +216,7 @@
       <td class="label">{$form.expire_on.label}</td>
       <td>{include file="CRM/common/jcalendar.tpl" elementName=expire_on}
       {if $action neq 4}
-        <br /><span class="description">{ts}Date this field expires (optional).  Used for price set fields that are no longer available after a specific date (e.g. early-bird pricing).{/ts}</span>
+        <br /><span class="description">{ts}Public forms will not display this price field after the set date. Back office forms will be unaffected by this date.{/ts}</span>
       {/if}
       </td>
     </tr>

Code changes requested to resolve r-run

I tested this change and hit a small snag. Price sets with an active_on date in the future were still hidden on the "Add Event Registration" screen. I think this is because the fix for #9764 (as mentioned above) added the additional WHERE clause in the wrong place. The adjustment described in the diff below fixed it for me and got the public event registration form, the "Add Event Registration" form and the "Change Selections" form all working with expectations in line with (b) as described above.

diff --git a/CRM/Price/BAO/PriceSet.php b/CRM/Price/BAO/PriceSet.php
index e913d3604f..661a4bd79f 100644
--- a/CRM/Price/BAO/PriceSet.php
+++ b/CRM/Price/BAO/PriceSet.php
@@ -471,12 +471,12 @@ WHERE     cpf.price_set_id = %1";
     $where = "
 WHERE price_set_id = %1
 AND is_active = 1
-AND ( active_on IS NULL OR active_on <= {$currentTime} )
 ";
     $dateSelect = '';
     if ($validOnly) {
       $dateSelect = "
 AND ( expire_on IS NULL OR expire_on >= {$currentTime} )
+AND ( active_on IS NULL OR active_on <= {$currentTime} )
 ";
     }

[twomice]

I'm for "b". My aim in CRM-19918 for which #9764 was the merged solution) was indeed to bring the "Change Selections" screen to a point where it would function consistently with the "Add Event Registration" screen. When it comes to the more fundamental question you've posed, "b" makes a lot more sense than "a".

[mattwire]

I'm for "b" as well. I had to work around this some time ago for a client by waiting for the option to expire, then changing it from public to admin and removing the expiry date.

However, it would be helpful if you could see on the backend that the option was expired - add "(expired)" in brackets to the label or something?

[mattwire]

It looks like the name of the original variable $getAllPriceFields was misleading (inverted) but the new variable name is good.
Can we get a more meaningful comment added in here somewhere explaining the intent of the code? Eg.
// Allow selection of expired price-set options on backend forms

[eileenmcnaughton]

This seems like the partner for #10804 - @agh1 suggested there that we come up with some UI magic to allow sold out options to be shown & hidden to reduce confusion

(These 2 PRs are an instance where I think it would be good to close the PRs, thrash out a proposal in gitlab & proceed once agreed)

[eileenmcnaughton]

If anyone will be at the sprint next week this & #10804 would be good to discuss while there

[eileenmcnaughton]

I've opened this to track this issue until an approach is agreed (at which point it can be looked at from a code review point of view) https://lab.civicrm.org/dev/core/issues/145