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

CiviEvent - Error registering participants via search task #19125

Merged
merged 4 commits into from
Dec 9, 2020

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Dec 5, 2020

Overview

This aims to fix smarty fatal error on using a search-task to register contacts for an event. Steps to reproduce:

  • Perform an advanced search
  • Select one or more contacts
  • Choose the register contacts to an event search task

Before

Form does not display. There's a Smarty error.

After

Form displays. No error.

Comment

This is an expansion on the prior draft #19118. It is working for me but I think warrants a better fix in master (separate the forms)

@civibot civibot bot added the 5.33 label Dec 5, 2020
@civibot
Copy link

civibot bot commented Dec 5, 2020

(Standard links)

@@ -0,0 +1 @@
<?php
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eileenmcnaughton why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

arg - the extra files would be part of the bigger fix - but the 2 small changes you might like to pull into your PR & I'll close this

@totten totten changed the title Ev533 CiviEvent - Error registering through advanced search task Dec 5, 2020
@eileenmcnaughton eileenmcnaughton force-pushed the ev533 branch 2 times, most recently from 71bb192 to 63b91d5 Compare December 5, 2020 05:07
@totten totten changed the title CiviEvent - Error registering through advanced search task CiviEvent - Error registering participants via search task Dec 5, 2020
@totten
Copy link
Member

totten commented Dec 5, 2020

OK, when I r-run this, it's definitely better. :) It creates the registration, and there's no hard crash.

But...

I fill out the form like so:

Screen Shot 2020-12-04 at 10 07 16 PM

The first time, it submitted but displayed this error:

Screen Shot 2020-12-04 at 10 04 55 PM

Which was weird. I figure maybe a fluke or some other hackery on my local, so I tried again. Again, it creates the registration (yay) but produces an error:

Screen Shot 2020-12-04 at 10 07 26 PM

The second error seems to be more stable - i.e. it showed up again on the 3rd and 4th tries.

… URLs

When use the search-task to register event participants, the qfKey is
mismatched, and the action is VIEW. It should be ADD, and the qfKey
should be for CRM_Event_Form_Participant.
@totten
Copy link
Member

totten commented Dec 5, 2020

There's more discussion/debugging in Mattermost log.

@eileenmcnaughton I've pushed up a change which (1) ensures the action is ADD (my system was donig VIEW), (2) calculates the qfKey for the ADD subform (CRM_Event_Form_Participant), (3) reinstates qfKey enforcement, and (4) removes the extra qfKey.

However, I don't think we can get further review+backport tonight. (We'll punt backport to 5.32.2.)

@@ -90,7 +90,7 @@ public function edit() {
$controller = new CRM_Core_Controller_Simple(
'CRM_Event_Form_Participant',
'Create Participation',
$this->_action, FALSE, FALSE, TRUE
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The removal of this TRUE has caused the 500 console error that I added it to avoid to come back. The other symptom is the fees not loading.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's weird. I was seeing the opposite - even with ignoreKey=TRUE here (PR-branch without df80fba), the AJAX call replied 500 and the fees subform didn't render. With df80fba (with a valid key -- and with ignoreKey=FALSE), the AJAX call and the subform worked.

I wonder if there might be some subtle difference in the pageflows or use-cases that we're trying?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@totten maybe - I have a good idea of a better fix - but we need to get 'something' out - to be honest I can't understand how the key would be rejected if ignoreKey is false

I am doing a basic search & then selecting some contacts & choosing register participants

I did test that on the pr test site & it failed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, my procedure is very similar, ie

  • Open advanced search
  • Search for all contacts (no criteria)
  • Check the first "Individual" in the demo data ("Adams, Heidi")
  • In the task list, choose to register
  • For the event, choose any of the three demo events (eg "Rain-forest Cup")

@eileenmcnaughton I can reproduce the error that appears on the test site. (Incidentally, I'm doing this on a different workstaiton/build.) Notably, in both test site and local, the 500 error is not related to qfKey. It's about "getFieldValue failed" (which curiously arose before but had seemed transitory). Here's a backtrace.

Screen Shot 2020-12-07 at 9 22 41 PM

The thing is... to my reading, it's basically saying that the event-registration form's preProcess() is applying a mismatched security check. (It's trying to use a contactId and CRM_Contact_Page_View::checkUserPermission() -- but in the context of a search-task, it doesn't make sense to use a singular contactId for assessing permissions on a bulk action.)

I've pushed up another patch which fixes this symptom for me, but it definitely needs some more eyes to see if it's faithful to contract/permissioning of this route/controller.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@totten given that

  1. the person who has reported this has applied the patch in the 5.32 branch & says it is working and
  2. we are pretty clear none of this is the final fix

I'm inclined to merge what that person merged & work on the right fix in master

Copy link
Member

@totten totten Dec 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The report on 5.32 PR actually pointed to the same problem raised here (just a diff part of the manifestation -- the HTTP 500 error you noted above is the same "getFieldValue required" error he observed) - I think they just didn't realize that the screen was misbehaving (because it's a misbehavior of omission; because the immediate error-message is hidden in AJAX console; and the UI-level error message is delayed). So the revision they tested wasn't working.

Added some more comments over there to try to distinguish 3 issues floating around here.

To my testing, the current revision of this branch addresses all 3.

…ative)

When preparing the event-registration form, `CRM_Event_Page_Tab::preProcess()` looks up the designated contact ID and
calls `checkUserPermission()`.  The error message is (effectively) reporting that the permission-check fails because
there is no contact ID.

And this is a legit thing to complain about -- if you're embedding the event-registration form as part of a
search-task, then there is no *singular* contact ID.  (There is a potentially very long list of contact IDs - which is
probably stored/reported some other way.) The permission-check for this context seems like it ought to be different.

I labeled the commit (tentative) because I'm not entirely certain that I understand the contract of this
route/controller.  It has multiple overlapping use-cases, and I'm not certain if I've tracked them all.  However, this
patch does seem to fix the "getFieldValue failed" problem when running as a search-task.
@totten
Copy link
Member

totten commented Dec 9, 2020

Did a final r-run on autobuild test site. Tested both the search-task (registering 1x and 3x contactx) and the standalone menu item ("Events => Register event participants" with 1x contact). In all cases, the full form displays. No AJAX HTTP 500. No lingering error-notices.

Merging this as near-term fix per MM discussion. Eileen's aiming to do a cleaner/separate route for this in master.

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.

3 participants