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 through advanced search task #19127

Merged
merged 4 commits into from
Dec 9, 2020

Conversation

eileenmcnaughton
Copy link
Contributor

port #19125

@civibot
Copy link

civibot bot commented Dec 5, 2020

(Standard links)

@timindaburgh
Copy link

I made the changes in the three files that had additions and/or deletions. I then added the new file. After testing by placing two participants selected from a search into an upcoming active event, both events were correctly added to the records of the two participants. However, there was an error that came up upon saving the participants to the event:

getFieldValue failed

Otherwise, the patch did work and the database was updated accurately.

@eileenmcnaughton
Copy link
Contributor Author

Wow - this is a flakey bit of code - we all get different errors.

Still you are up & running again now - @totten I'm inclined to merge & release this as confirmed-to-be-better (although this is the version that worked for me & not you & different to the 5.33 pr atm)

@totten
Copy link
Member

totten commented Dec 8, 2020

However, there was an error that came up upon saving the participants to the event:

getFieldValue failed

Otherwise, the patch did work and the database was updated accurately.

Wow - this is a flakey bit of code - we all get different errors.

Yeah, this one has had it tough at QA. For me, there are a few distinct issues which all produced a similar symptom. I think this helps explain the mixed signals we've been getting.

Symptom: The registration-form is supposed to show a bunch of fields (e.g. pricing options, e.g. payment tracking, e.g. custom-fields) which depend on the specific event. These fields are displayed progressively (via AJAX) while you fill-out the form. If the AJAX call fails, then the fields don't show up.

At first glance, you may not even notice this problem. (I didn't notice when first testing the same patch on 5.33...) The form still looks plausible. The error isn't displayed in real time. You can even submit the form, and it will create a civicrm_participant record. But something is likely askew with the completeness/integrity because it didn't collect the standard data. And eventually (in a subsequent page-view) you do see an error message.

Recap: The symptom is that fields are missing from UI because an AJAX call to civicrm/contact/view/participant (CRM_Event_Page_Tab) fails with "HTTP 500". However, you may not see any explicit messages about this failure until you navigate to another screen.

Causes and Fixes: The symptom has had a few causes:

(1) There's the "getFieldValue required" error which @timindaburgh pointed out. This seems to be a reliable problem. I've seen it locally on multiple workstations/builds and on the autobuild test site, and I believe it's what Eileen noticed in r-running another patch. The problem arises because this form uses a helper (CRM_Contact_Page_View::checkUserPermission) which is not well-defined for this use-case. I pushed 2497350 to the 5.33 branch which appears to resolve this.

(2) qfKey is the CSRF protection token, and it's required by most forms/subforms. The AJAX request must have a qfKey that matches the subform (or else it'll generate HTTP 500). There are competing revisions to address this, e.g. turning off qfKey (ignoreKey=TRUE) or setting qfKey correctly. I believe that both approaches mitigiate qfKey failures, but it's better to keep CSRF protection enabled.

(3) The AJAX request for civicrm/contact/view/participant optionally accepts an action parameter, which determines the subform to display. If not passed explicitly, then it somehow picks a default/implicit action. In my previous testing, I had a problem where the default/implicit action was wrong. (This manifested just like issue (2) for qfKey.). Setting the action explicitly resolved it. No one else has reported this problem, and I can't reproduce it now. However, there's no downside to passing the action explicitly.

… 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.
…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.

@totten totten merged commit 141f95a into civicrm:5.32 Dec 9, 2020
@eileenmcnaughton eileenmcnaughton deleted the 532p branch December 9, 2020 09:30
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