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

dev/core/#3849 Fix profile group add for events #24633

Conversation

larssandergreen
Copy link
Contributor

Overview

This code didn't properly add contacts to groups when using profiles that were set to add to groups in event registrations with multiple participants or multiple profiles.

Before

Secondary registrants were added to groups for the primary registrant profiles.
Additionally, when more than two profiles were in use, contacts were not added to all groups, depending on configuration of the event (didn't spend too much time digging into the details on what exactly was happening).

After

All registrants are added to the appropriate groups for the profiles set in the event registration.

@civibot
Copy link

civibot bot commented Sep 27, 2022

(Standard links)

@civibot civibot bot added the master label Sep 27, 2022
@eileenmcnaughton
Copy link
Contributor

@twomice here is the one

@eileenmcnaughton
Copy link
Contributor

@larssandergreen I'm not going to insist on a unit test on this one cos it's probably a tricky one to do a test for but if you want help writing one next time jump on chat.civicrm.org....

@twomice
Copy link
Contributor

twomice commented Oct 11, 2022

I'm testing r-run on this now.

@twomice
Copy link
Contributor

twomice commented Oct 11, 2022

r-run: Pass

Steps to reproduce:

  1. Create 4 new groups with appropriate names for this purpose:
    • pull-24633: primary 1
    • pull-24633: primary 2
    • pull-24633: additional 1
    • pull-24633: additional 2
  2. Create 4 new profiles, each one configured for "Add contacts to a group?" to the like-named Group:
    • pull-24633: primary 1
    • pull-24633: primary 2
    • pull-24633: additional 1
    • pull-24633: additional 2
  3. Create an event "Test Event" with online registration:
    • Register multiple participants? Yes
    • Include Profile (top of page): "pull-24633: primary 1"
    • Include Profile (bottom of page): "pull-24633: primary 2"
    • Profile for Additional Participants (top of page): "pull-24633: additional 1"
    • Profile for Additional Participants (bottom of page): "pull-24633: additional 2"
  4. Navigate to the online registration form for event "Test 1" and submimt a single registration for 2 participants (noting that these are new gropus and therefore should not have any contacts assigned to them).
  5. Observe groups for thes two contacts:
    • Expected: the Primary contact should have been added to groups "pull-24633: primary 1" and "pull-24633: primary 2"; and the additional participant should have been added to groups "pull-24633: additional 1" and "pull-24633: additional 2"

Before (on master):

  1. Perform "Steps to Reproduce" and observe groups:
    • Observe (incorrect): the Primary contact has been added only to group "pull-24633: additional 1"; and the additional participant has been added only to group "pull-24633: primary 1" and "pull-24633: additional 2"

After (on patched master with this PR):

  1. Perform "Steps to Reproduce" and observe groups:
    • Observed (correct): the Primary contact should have been added to groups "pull-24633: primary 1" and "pull-24633: primary 2"; and the additional participant should have been added to groups "pull-24633: additional 1" and "pull-24633: additional 2"

@eileenmcnaughton
Copy link
Contributor

thanks @twomice @larssandergreen

@eileenmcnaughton eileenmcnaughton merged commit b7ee8e8 into civicrm:master Oct 11, 2022
@larssandergreen
Copy link
Contributor Author

Thanks for reviewing @twomice and thanks @eileenmcnaughton.

@eileenmcnaughton
Copy link
Contributor

a small inroads into your PRs :-)

@larssandergreen larssandergreen deleted the fix-profile-group-add-for-multi-participant-event-registrations branch November 5, 2022 00:12
@colemanw
Copy link
Member

FYI this caused a regression because the new code adds a permission check that fails for anon users. See #25025

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

Successfully merging this pull request may close these issues.

4 participants