Skip to content
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

Fix missing contactID when registering for paid event from waitlist #23358

Merged
merged 2 commits into from
Jun 1, 2022

Conversation

mattwire
Copy link
Contributor

@mattwire mattwire commented May 4, 2022

Overview

See https://lab.civicrm.org/extensions/stripe/-/issues/347 but note this is not a Stripe issue and variants of this will happen with any payment processor because no contact ID is passed in when registering from the waitlist.

For a processor using payment propertyBag accessing contactID will cause CiviCRM to crash. For a payment processor using the passed in array there will at least be a PHP notice if contact_id is accessed and anything that needs the contact ID will not be setup correctly.

Before

Attempt to make a paid registration from the waitlist has no contact ID and can cause payment processors to fail.

After

Attempt to make a paid registration from the waitlist succeeds.

Technical Details

The registration code is rather complex but basically:

  • When registering via waitlist (civicrm/register/confirm?participantId=X) the parameters that get passed to the processPayment() function (https://github.com/civicrm/civicrm-core/blob/master/CRM/Event/Form/Registration/Confirm.php#L1262) does not include a contact ID in the $value array.
  • When registering through the "normal" register form a contact ID is included in the $value array as `$value['contact_id'].
  • We already calculated the primary contact ID and it's a simple step to pass it in if not already set.

Comments

There's not much testing on register from waitlist. I found CRM_Event_Form_Registration_RegisterTest::testDoubleWaitlistRegistration() which could probably be extended.

@civibot civibot bot added the master label May 4, 2022
@civibot
Copy link

civibot bot commented May 4, 2022

(Standard links)

deepak-srivastava added a commit to deepak-srivastava/civicrm-core that referenced this pull request May 20, 2022
mattwire pushed a commit to mattwire/civicrm-core that referenced this pull request May 22, 2022
@mattwire mattwire force-pushed the participantwaitlist branch from 629e898 to bff6a70 Compare May 22, 2022 20:22
@seamuslee001
Copy link
Contributor

@mattwire I had a client that faced this issue and the substantive part of your patch worked for me but I see for your test it is failing with

InvalidArgumentException: setAmount requires a numeric amount value

/home/jenkins/bknix-dfl/build/core-23358-2tktp/web/sites/all/modules/civicrm/Civi/Payment/PropertyBag.php:466
/home/jenkins/bknix-dfl/build/core-23358-2tktp/web/sites/all/modules/civicrm/Civi/Payment/PropertyBag.php:241
/home/jenkins/bknix-dfl/build/core-23358-2tktp/web/sites/all/modules/civicrm/Civi/Payment/PropertyBag.php:366
/home/jenkins/bknix-dfl/build/core-23358-2tktp/web/sites/all/modules/civicrm/Civi/Payment/PropertyBag.php:129
/home/jenkins/bknix-dfl/build/core-23358-2tktp/web/sites/all/modules/civicrm/CRM/Core/Payment/Dummy.php:192
/home/jenkins/bknix-dfl/build/core-23358-2tktp/web/sites/all/modules/civicrm/CRM/Event/Form/Registration/Confirm.php:1272

@mattwire mattwire force-pushed the participantwaitlist branch from bff6a70 to 9e14239 Compare May 26, 2022 15:32
@mattwire
Copy link
Contributor Author

@seamuslee001 Thankyou - tests should be passing now

@eileenmcnaughton eileenmcnaughton merged commit 2f09add into civicrm:master Jun 1, 2022
@eileenmcnaughton
Copy link
Contributor

the test did it

@agileware-justin
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants