-
-
Notifications
You must be signed in to change notification settings - Fork 813
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move check for existing participants (for a specific contact/event) out of event registration forms #30698
base: master
Are you sure you want to change the base?
Conversation
🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷 Introduction for new contributors...
Quick links for reviewers...
|
c59ffd9
to
7ae9159
Compare
@jensschuppe I agree with your instinct to figure out the signature up front. I'm trying to think through the logic you have exposed Fundamentally the code seems to be checking for duplicates on the contact ID & event ID with filters on status & role & (of doubtful benefit) on 'is_test'
I think (and @colemanw is the person to confirm) we should maybe consider adding a v4 API
The question becomes - what does the signature look like & @colemanw will likely have thoughts - but
The main difference between this & calling A couple of other notes
|
@MegaphoneJon I feel like you might know some of the is_counted stuff |
But...what if I pretended I didn't? There are several different functions that try to count this. Some are only for waitlists, some exclude the waitlist, and others are just wrong (e.g. ignoring IMO, the most comprehensive and correct implementation (and I've banged on it a bit) is |
IIRC - API4 does not use this function. |
@MegaphoneJon I agree - pretending not to know is often the best approach! |
I just skimmed over this code to see if it was redundant with the participant |
Just changed the title to clarify that this PR is about checking for existing participants for a specific contact and event (i.e. not registering the same person twice). Checking for available seats and price options are next steps 😄 |
I had a go at pseudocoding where this might eventually wind up as part of a validate action over at |
Just looking at the comments - I came to most of the same conclusions when I had a go at putting this in a more 'final' place - #30768 |
3ecb7a3
to
7d9bb4d
Compare
@colemanw came up with the idea of providing this business logic functionality outside of the API (as that might get updated eventually to e.g. APIv5) on a more abstract level by having a generic BAO/Record validate Symfony event. We agreed to have the logic it in the BAO and the API invoke that low-level event which in turn calls the actual logic in the BAO. Also, this is just part of the complete validation for participants (or better: event registrations, which might be about more to validate than just a single contact's participant objects). I renamed the new methods accordingly to reflect that it's the logic looking for counted participants for a contact/event combination. So this PR is for moving the logic out of the forms into the BAO as a first step. Having an API, a validate Symfony event, etc. comes next. |
7d9bb4d
to
aecb01a
Compare
The failing test suggests that the |
aecb01a
to
3116ec4
Compare
As this came back green, I'd suggest merging it and to have follow-up PRs for:
|
@jensschuppe I think the main value of this PR is the unit tests that would be added with it - then we can move the code around safely - ie I think the code would live better on the api layer per #30768 - because the api lends itself to a saner signature - ie
Of course there is the idea that logic should sit on the |
489ff75
to
454af83
Compare
In reaction to @eileenmcnaughton's comment and following what we've been discussing in the dev-financial mattermost channel and in meetings with @JoeMurray, @monishdeb, and @colemanw, I tried to capture more business logic of the event registration validation in BAO methods and call them from the form layer, while also trying to understand (and illustrate in the code) the different aspects of validation, such as differences between admin and public forms. I (and others mentioned above) disagree about business logic living in the API. The API should be a thin wrapper around BAO code and the interface/signature should still be discussed, as a simple I see that intermediate merges maybe don't make sense, so I'm continuing my work on this PR with the principles we tried to lay out in our previous meeting:
|
454af83
to
cb78e25
Compare
Hmm OK - I think my main point is that we should be front ending the focus on unit tests with this work |
Overview
Validating event registrations implies checking for existing participants, which has been done in the form classes where needed by querying
CRM_Event_BAO_Participant
objects. This PR tries to refactor that part out into the Participant BAO class and using the API for doing so.Before
Different event registration and participant forms did this validation on their own (with differing criteria!), there was no central point of checking for existing participants that could've been reliably called from elsewhere.
After
Checking for existing participants is now a static method (actually two) on
CRM_Event_BAO_Participant
and can be called from elsewhere - eventually an API action validating an order.Technical Details
The new methods have a fairly extensive signature, which I'm not sure is necessary and which I'd like to be discussed, hence this PR being a draft for now. The calls I added (replacing individual queries) had differing criteria for checking for existing participants, which I tried to retain semantically.
Comments
Shouldn't we rely on
is_counted
only and not bother withOn waitlist
orCancelled
status separately? This would make the signatures of the new methods cleaner. And if you configureOn waitlist
orCancelled
to be counted, the current assumptions don't really fit anymore …Also, why does
\CRM_Event_Form_Registration_Register::checkRegistration()
take roles into account and other forms don't?Also, I'm quite sure the checks in
\CRM_Event_Form_Participant::formRule()
and\CRM_Event_Form_Task_Register::postProcess()
should not include test participants, as their current implementations do, and they don't care foris_counted
or other status-related attributes at all.Lastly, did I miss any more checks that can be replaced by the new method?
@colemanw, @JoeMurray, @eileenmcnaughton happy to hear your thoughts!