Builds on #27049 to extract getPriceSetID() #27050

eileenmcnaughton · 2023-08-11T05:29:18Z

Builds on #27049 to extract getPriceSetID()

Before

discountID (newly) pass in to CRM_Event_Form_Registration::initEventFee( - but used only to get priceSetID

After

$priceSetID passed in

Note that the getPriceSetID function now handles setting the form property & form value, as used. So I removed that from the shared code. I didn't put it in the setting for ParticipantFeeSelection because it is currently interacting with an undefined property

Technical Details

Per #27049 the only other usage is a unit test @mlutfy wrote

civibot · 2023-08-11T05:29:20Z

Thank you for contributing to CiviCRM! ❤️ We will need to test and review the PR. 👷

Introduction for new contributors

If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
For more information about contributing, see CONTRIBUTING.md.

Quick links for reviewers

eileenmcnaughton · 2023-08-21T22:53:12Z

test this please

larssandergreen · 2023-08-21T23:24:45Z

CRM/Event/Form/ParticipantFeeSelection.php

+   * @noinspection PhpUnhandledExceptionInspection
+   * @noinspection PhpDocMissingThrowsInspection
+   */
+  public function getPriceSetID(): ?int {


Should probably use this at 127 as well for consistency.

Actually, $priceSetId is unused, we can just remove 127.

larssandergreen · 2023-08-21T23:43:13Z

CRM/Event/Form/Participant.php

@@ -1428,7 +1428,7 @@ public function buildEventFeeForm($form) {

      //retrieve custom information
      $form->_values = [];
-      CRM_Event_Form_Registration::initEventFee($form, $event['id'], FALSE, $form->getDiscountID());
+      CRM_Event_Form_Registration::initEventFee($form, $event['id'], FALSE, $this->getPriceSetID());


I don't think it makes any difference as things stand now, but shouldn't it be $form->getPriceSetID for consistency?

Hmm - I've been trying to go the other way for consistency - ditch the use of form & even the passing of it

Agreed. I guess it doesn't matter here as long as this doesn't get called from anywhere else, just looks a bit odd.

Not sure if this is affected or not https://github.com/eileenmcnaughton/civicrm-core/blob/master/tests/phpunit/CRM/Event/Form/ParticipantTest.php#L360

@seamuslee001 yeah that needs cleaning up - although as long as this PR passes tests it is out of scope for it

larssandergreen · 2023-08-22T01:13:25Z

Started giving this an r-run, which is quite tricky as some of this stuff doesn't work very well presently.

But to start, if you:

Add a discount to a quick config price set for an event
Register a participant
View the participant
Change selections

Then you can no longer edit the selections for the participant (that part doesn't show up at all).

Before

After

larssandergreen · 2023-08-22T01:28:57Z

Also noting, though I suppose it is technically out of scope here as it is the current behaviour on master: if I change selections for a registration with a discounted price set, I expect the options to show the discounted price set, but what I get is the current price set based on the current date. For example, if someone registered for a student ticket with an early bird discount and I need to change them to an adult ticket, I would expect they would still get the early bird price.

I thought initEventFee was supposed to get the price set that the participant currently had based on the discount id, but that doesn't seem to be happening.

eileenmcnaughton · 2023-08-22T01:40:31Z

@larssandergreen thanks for that - I think I agree with you that the price set they selected from should be the one that is loaded so I will check in on that

eileenmcnaughton · 2023-08-22T09:47:44Z

Oh - the issue is that ParticipantFeeForm no longer has the undefined property set _priceSetId - ideally we would pass it into the function rather than require it from the form - just trying to see where it is called from / when it would NOT be set....

larssandergreen · 2023-08-22T16:51:30Z

CRM/Event/Form/Registration/Register.php

@@ -423,7 +423,7 @@ public function buildQuickForm() {
      // build amount only when needed, skip incase of event full and waitlisting is enabled
      // and few other conditions check preProcess()
      if (!$this->_noFees) {
-        self::buildAmount($this);
+        self::buildAmount($this, FALSE, NULL, $this->_priceSetId);


$required was TRUE by default before, now FALSE - intentional?

larssandergreen · 2023-08-22T17:15:44Z

Confirming that missing price set options for Change Selections is fixed.

The discounted price set still isn't loading when editing a participant. I noticed that it looks like the discount id isn't loading when editing the registration at all. I think we should see a discount set here when the registration used a discounted price set.

This gets away from it needing to be set on the form as a property

eileenmcnaughton · 2023-08-22T20:59:16Z

whoa that discount code is a can of worms - it adds the property to the form only if the property context exists - which is only true for one of the forms involved Participant - but that ONLY uses if within the same function. I've moved that back to the form as a first step - it might be good to fix the ParticipantFeeSelection not offering the discount in a follow up as this should be a refactor only. I guess I need to check if only back-office users can use that form cos only back office users should get the choice.

Hmm that's a question - it makes sense that a back-office user can change the event selection within the same time-based discount set- but should a self-service user? I suspect not - if they want to do that they maybe have to contact the organization as the discount might depend on them paying on time & they could work the system by buying a cheap ticket early & getting the expensive one later

larssandergreen · 2023-08-22T23:39:52Z

OK, makes sense. Yes, let's do the discount thing separately.

Looks like the test fails are related.

eileenmcnaughton · 2023-08-23T00:51:07Z

Yeah - but super odd

Test Result (2 failures / +2)
api\v4\Entity\ConformanceTest.testEntitiesProvider
api\v4\Entity\ConformanceTest.testConformance with data set "Discount"

eileenmcnaughton · 2023-08-23T00:52:25Z

Ah - https://github.com/civicrm/civicrm-core/pull/27050/files#diff-dd32c0ff4ad7da7103eec712b4eadc292638dd9409343725d532ffc565fc7bb1

larssandergreen · 2023-08-23T02:27:06Z

Now just a concurrency test fail, so good.

larssandergreen · 2023-08-23T02:39:09Z

CRM/Event/Form/Registration/Register.php

@@ -540,12 +540,13 @@ public function buildQuickForm() {
   *   Form object.
   * @param bool $required
   *   True if you want to add formRule.
-   * @param int $discountId
+   * @param null $discountId


larssandergreen · 2023-08-23T02:52:44Z

Another small related bug in master. Not an issue here.

larssandergreen · 2023-08-23T03:16:29Z

OK, I've given this a fairly thorough run and I think it is ready to merge.

larssandergreen · 2023-08-23T03:16:58Z

Excepting that one docblock issue above

This is ths only one of the forms that has context as a property so the only one that does it

eileenmcnaughton · 2023-08-23T04:26:16Z

I fixed the docblock & the test magically passed

civibot bot added the master label Aug 11, 2023

eileenmcnaughton force-pushed the price branch 5 times, most recently from 278b758 to 1ff517e Compare August 11, 2023 07:59

eileenmcnaughton changed the title ~~[wip] bbbbbPrice~~ Builds on #27049 to extract getPriceSetID() Aug 11, 2023

eileenmcnaughton force-pushed the price branch 3 times, most recently from ae812bc to 158a4b6 Compare August 12, 2023 03:37

eileenmcnaughton mentioned this pull request Aug 14, 2023

Remove use of sometimes-php8-warning causing _quickConfig property…(use isQuickConfig() function) #27070

Merged

eileenmcnaughton force-pushed the price branch 9 times, most recently from e8fc6fb to 54b8d5c Compare August 18, 2023 03:45

larssandergreen reviewed Aug 21, 2023

View reviewed changes

Extract getPriceSetID

97d8f92

eileenmcnaughton force-pushed the price branch from 54b8d5c to 97d8f92 Compare August 21, 2023 23:47

larssandergreen reviewed Aug 22, 2023

View reviewed changes

eileenmcnaughton force-pushed the price branch from e1069a7 to 1943162 Compare August 22, 2023 19:40

Pass priceSetID into buildAmount

51c6be7

This gets away from it needing to be set on the form as a property

eileenmcnaughton force-pushed the price branch 5 times, most recently from e4de92b to 613887a Compare August 22, 2023 20:30

eileenmcnaughton force-pushed the price branch from 613887a to 6566de7 Compare August 23, 2023 00:53

larssandergreen reviewed Aug 23, 2023

View reviewed changes

Move setting of discount back to calling function

25687f5

This is ths only one of the forms that has context as a property so the only one that does it

eileenmcnaughton force-pushed the price branch from 6566de7 to 25687f5 Compare August 23, 2023 03:50

colemanw merged commit 6373878 into civicrm:master Aug 23, 2023

colemanw deleted the price branch August 23, 2023 11:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Builds on #27049 to extract getPriceSetID() #27050

Builds on #27049 to extract getPriceSetID() #27050

eileenmcnaughton commented Aug 11, 2023 •

edited

Loading

civibot bot commented Aug 11, 2023

eileenmcnaughton commented Aug 21, 2023

larssandergreen Aug 21, 2023

larssandergreen Aug 21, 2023

eileenmcnaughton Aug 21, 2023

larssandergreen Aug 21, 2023

eileenmcnaughton Aug 21, 2023

larssandergreen Aug 21, 2023

seamuslee001 Aug 21, 2023

eileenmcnaughton Aug 22, 2023

larssandergreen commented Aug 22, 2023 •

edited

Loading

larssandergreen commented Aug 22, 2023

eileenmcnaughton commented Aug 22, 2023

eileenmcnaughton commented Aug 22, 2023

larssandergreen Aug 22, 2023

eileenmcnaughton Aug 22, 2023

larssandergreen commented Aug 22, 2023 •

edited

Loading

eileenmcnaughton commented Aug 22, 2023

larssandergreen commented Aug 22, 2023

eileenmcnaughton commented Aug 23, 2023

eileenmcnaughton commented Aug 23, 2023

larssandergreen commented Aug 23, 2023

larssandergreen Aug 23, 2023

larssandergreen commented Aug 23, 2023

larssandergreen commented Aug 23, 2023

larssandergreen commented Aug 23, 2023

eileenmcnaughton commented Aug 23, 2023

Builds on #27049 to extract getPriceSetID() #27050

Builds on #27049 to extract getPriceSetID() #27050

Conversation

eileenmcnaughton commented Aug 11, 2023 • edited Loading

Before

After

Technical Details

civibot bot commented Aug 11, 2023

eileenmcnaughton commented Aug 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larssandergreen commented Aug 22, 2023 • edited Loading

larssandergreen commented Aug 22, 2023

eileenmcnaughton commented Aug 22, 2023

eileenmcnaughton commented Aug 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larssandergreen commented Aug 22, 2023 • edited Loading

eileenmcnaughton commented Aug 22, 2023

larssandergreen commented Aug 22, 2023

eileenmcnaughton commented Aug 23, 2023

eileenmcnaughton commented Aug 23, 2023

larssandergreen commented Aug 23, 2023

Choose a reason for hiding this comment

larssandergreen commented Aug 23, 2023

larssandergreen commented Aug 23, 2023

larssandergreen commented Aug 23, 2023

eileenmcnaughton commented Aug 23, 2023

eileenmcnaughton commented Aug 11, 2023 •

edited

Loading

larssandergreen commented Aug 22, 2023 •

edited

Loading

larssandergreen commented Aug 22, 2023 •

edited

Loading