Extract `getDiscountID()` #27049

eileenmcnaughton · 2023-08-11T04:53:45Z

Overview

Extract getDiscountID()

Before

CRM_Event_Form_Registration::initEventFee() does some weird magic checking if the property discountId is configured on the form & using that, if the property exists. This is a pretty clear code smell that we should pass in the value rather than have the function find it on the form.

CRM_Event_Form_Registration::initEventFee() is called from 3 places in universe :

CRM_Event_Form_ParticipantFeeSelection::buildQuickForm() (non-static, called only from the form)
CRM_Event_Form_Registration::preProcess() (non-static, called only from the form)
CRM_Event_Form_Participant::buildEventFeeForm()

This last one was made static/public to be used in unit tests. There are no core tests that do this anymore but @mlutfy wrote a test CRM_Taxcalculator_BackendEventWebinarTest that calls it from an extension. That test would be as likely to work after as before but it follows a pattern we ditched in core tests.

The function only handles one form CRM_Event_Form_Participant - and this is the only form that would ever have the $_discountId property

After

Each of the forms above

passes the discount ID into CRM_Event_Form_Registration::initEventFee()
implements a public function which is annotated with @api called getDiscountID()
the function types the ID to an int, or to NULL

Technical Details

This is part of a general cleanup to stop passing the form into functions & then checking the form for a possible property.

I'm also trying add consistent functions to retrieve ids from the various forms and to support those functions for extension users (where we can be confident the value would have a consistent meaning over time - even if we changed the internals of the function at some point).

Note I was actually trying to give getPriceSetID this treatment but discountID was in the way so I've done it first...

In general I'm trying to make some consisent public functions available on forms for retrieving relevant entity IDs rather than the current property / form variable patchwork

Comments

civibot · 2023-08-11T04:53:47Z

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

This is part of a general cleanup to stop passing the form into functions & then checking the form for a possible property. Instead each form that calls the function has a consistent (publicly supported) getDiscountID() function that it calls & discountID is passed in. Note I was actually trying to give getPriceSetID this treatment but discountID was in the way. In general I'm trying to make some consisent public functions available on forms for retrieving relevant entity IDs rather than the current property / form variable patchwork

larssandergreen · 2023-08-11T22:07:29Z

CRM/Event/Form/Participant.php

-      $this->_discountId = CRM_Core_DAO::getFieldValue('CRM_Event_DAO_Participant', $this->_id, 'discount_id');
-      if ($this->_discountId) {
+      if ($this->getDiscountID()) {
+        // This doesn't seem used....


Agreed that this doesn't seem to be used anywhere. ~~Seems like the previous logic was to get the discount ID from the participant, but as it is here we'd be getting the discount ID from the event itself. Is that always the same? Not sure.~~ (I was reading this wrong). Maybe cleaner just to get rid of it, since it does nothing.

I wonder if we can just get rid of the the whole getting the discount ID from the participant thing entirely. It doesn't seem to do anything at all. Then we can simplify getDiscountID().

@larssandergreen I was referring to the set not being used - I think loading it from the participant makes sense in edit mode

larssandergreen · 2023-08-11T22:41:55Z

CRM/Event/Form/ParticipantFeeSelection.php

@@ -390,4 +407,23 @@ private function emailReceipt(array $params): void {
    CRM_Core_BAO_MessageTemplate::sendTemplate($sendTemplateParams);
  }

+  /**


Seems like $this->_eventId would always be defined, so do we need to have a method to get it here?

Or if there is a reason to have it, I think we can just have it return $this->_eventId

SO where I am going with these is the idea that you should be able to call getEventID() & similar getfunctions at any point & they don't rely on some order of events where things are set first, but you would always get ... the event ID.

Also - I've marked as supported for external use - with the goal of providing some consistency to extensions wanting to access basic information about what is being processed (without directly accessing the properties which could be inconsistent)

seamuslee001 · 2023-08-14T07:22:57Z

CRM/Event/Form/Participant.php

@@ -1436,8 +1436,8 @@ public function buildEventFeeForm($form) {

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


@eileenmcnaughton this function doesn't seem to have a 4th function param

@seamuslee001 https://github.com/civicrm/civicrm-core/pull/27049/files#diff-dcb99c146eb90c35dfb3a56e963fe84c57faca51fbaf9f611ca0a7922c8f509cR579

eileenmcnaughton · 2023-08-14T21:02:46Z

I added these to https://docs.civicrm.org/dev/en/latest/hooks/#pitfalls-of-hooks

Builds on #27049 to extract getPriceSetID()

civibot bot added the master label Aug 11, 2023

eileenmcnaughton force-pushed the discount branch 6 times, most recently from 93a1743 to 13fc461 Compare August 11, 2023 05:11

eileenmcnaughton changed the title ~~Extract getDiscountPriceSetID()~~ Extract getDiscountID() Aug 11, 2023

eileenmcnaughton force-pushed the discount branch 2 times, most recently from 3f0689a to afb392b Compare August 11, 2023 05:13

eileenmcnaughton force-pushed the discount branch from afb392b to cd6e40b Compare August 11, 2023 05:21

eileenmcnaughton mentioned this pull request Aug 11, 2023

Builds on #27049 to extract getPriceSetID() #27050

Merged

larssandergreen reviewed Aug 11, 2023

View reviewed changes

eileenmcnaughton mentioned this pull request Aug 14, 2023

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

Merged

seamuslee001 reviewed Aug 14, 2023

View reviewed changes

seamuslee001 merged commit e5cdc41 into civicrm:master Aug 17, 2023

seamuslee001 deleted the discount branch August 17, 2023 21:22

colemanw added a commit that referenced this pull request Aug 23, 2023

Merge pull request #27050 from eileenmcnaughton/price

6373878

Builds on #27049 to extract getPriceSetID()

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract `getDiscountID()` #27049

Extract `getDiscountID()` #27049

eileenmcnaughton commented Aug 11, 2023 •

edited

Loading

civibot bot commented Aug 11, 2023

larssandergreen Aug 11, 2023 •

edited

Loading

larssandergreen Aug 11, 2023

eileenmcnaughton Aug 12, 2023

larssandergreen Aug 11, 2023

larssandergreen Aug 11, 2023

eileenmcnaughton Aug 12, 2023

seamuslee001 Aug 14, 2023

eileenmcnaughton Aug 14, 2023

eileenmcnaughton commented Aug 14, 2023

Extract getDiscountID() #27049

Extract getDiscountID() #27049

Conversation

eileenmcnaughton commented Aug 11, 2023 • edited Loading

Overview

Before

After

Technical Details

Comments

civibot bot commented Aug 11, 2023

larssandergreen Aug 11, 2023 • edited Loading

Choose a reason for hiding this comment

larssandergreen Aug 11, 2023

Choose a reason for hiding this comment

eileenmcnaughton Aug 12, 2023

Choose a reason for hiding this comment

larssandergreen Aug 11, 2023

Choose a reason for hiding this comment

larssandergreen Aug 11, 2023

Choose a reason for hiding this comment

eileenmcnaughton Aug 12, 2023

Choose a reason for hiding this comment

seamuslee001 Aug 14, 2023

Choose a reason for hiding this comment

eileenmcnaughton Aug 14, 2023

Choose a reason for hiding this comment

eileenmcnaughton commented Aug 14, 2023

Extract `getDiscountID()` #27049

Extract `getDiscountID()` #27049

eileenmcnaughton commented Aug 11, 2023 •

edited

Loading

larssandergreen Aug 11, 2023 •

edited

Loading