dev/core#4907 Form-layer consolidation on getting the number of spaces for the event #28984
Conversation
|
🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷 Introduction for new contributors...
Quick links for reviewers...
|
|
The issue associated with the Pull Request can be viewed at https://lab.civicrm.org/dev/core/-/issues/4907 |
eileenmcnaughton
force-pushed
the
event_full_info
branch
from
2a73618
to
4025efd
Compare
eileenmcnaughton
force-pushed
the
event_full_info
branch
from
4025efd
to
755d788
Compare
| TRUE, | ||
| $this->getEventValue('has_waitlist') | ||
| ); | ||
| return is_numeric($availableSpaces) ? (int) $availableSpaces : 0; |
There was a problem hiding this comment.
Regarding https://lab.civicrm.org/dev/core/-/issues/4918, I think the problem is this function is trying to convert something that is inherently a mixed value into a single integer. It doesn't take into account that the available spaces could be infinite (which is not an integer, and is what NULL means here - i.e. not the same as 0).
It's true that it feels weird to have a function that sometimes returns a display string and sometimes a number, but the underlying thing is that it is truly a mixed value, of some kind.
One option is that if it's NULL, return INT_MAX. But that feels kludgy, and what if you're hosting a video party for the World Cup Final...
There was a problem hiding this comment.
If it is available spaces (i.e. those available after previous participants have registered). you could still host your World Cup party, couldn't you? You aren't worried about the maximum number of participants, only how much space is left? Or have I misunderstood things badly?
There was a problem hiding this comment.
Right but this function is ALWAYS returning 0 available spaces left when you leave the maximum number of participants field blank, because CRM_Event_BAO_Participant::eventFull() returns null in that case, and then this function converts null to 0 instead of converting it to "infinity".
The World Cup reference was partly a joke, that if instead of returning infinity, because that's not an integer, you return php's highest integer, but for such an event that might not be big enough.
When I tried that though I then started seeing some problems with the confirmation page and haven't looked closer yet.
There was a problem hiding this comment.
Every event has a limit to the number of participants, but in some cases you may not want to cap the number of people who register for an event.
An example is where you're using an event to track volunteer applications. You may only need 20 volunteers, but you want to select the 20 volunteers that have the best skillsets for the roles. So you would allow everyone to register their interest to volunteer for the event, and then manually identify the people who you will ask to volunteer.
Looking at the eventFull method, it returns NULL if there is no participant limit. (link) which seems like a fair result rather than 0.
There was a problem hiding this comment.
Just noting that the regression under discussion is now partially fixed and the remaining part is in https://lab.civicrm.org/dev/core/-/issues/4942
Overview
dev/core#4907 Form-layer consolidation on getting the number of spaces for the event
Before
CRM_Event_BAO_Participant::eventFull()is called 12 times with a mixture of parameters & a mixture of results. One of the anomalies is that sometimes it returns text rather than a number. This fixes one of those places (the event info screen) to NOT expect text to be returned (which seems like an antipattern)After
Unchanged UI
Technical Details
In the medium term we want the form layer to use apiv4 to get the available_spaces. The parity is not there at the moment - partly as fixed in #28983 but there is also some other stuff going on that does give me confidence they are like for like. However, we can update the forms to call
getEventValue('available_spaces')& it will use the old method for now but once we are confident we can just remove this extra temporary wrangling from this one placeI also added the
getPriceSetID()function for consistency with similar formsComments