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

Afform - Add support for ReCaptcha v2 #24860

Merged
merged 1 commit into from
Nov 1, 2022

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Oct 30, 2022

Overview

Supports the Recapta2 element in FormBuilder.

image

Technical Details

Depends on #24862 for Afform to support extensions adding new element types.

Comments

This refactors #24689, moving the code to an extension and making it event-driven.

@civibot
Copy link

civibot bot commented Oct 30, 2022

(Standard links)

@seamuslee001
Copy link
Contributor

This seems like the right direction to me I'm just wondering @colemanw if its worth splitting out the work on the general afform type from the recaptcha work just to try and make things a little mergeable?

@colemanw
Copy link
Member Author

Ok I've broken the Afform part into #24862

@kurund
Copy link
Contributor

kurund commented Oct 31, 2022

@colemanw

Thanks, #24862 looks good.

While checking this PR, I am getting validation error even if the Recaptcha is checked and valid.

Also would be good to rebase other PR is merged.

@colemanw colemanw force-pushed the formbuilder-recaptcha2 branch from 7a22510 to 434d65c Compare October 31, 2022 16:06
@colemanw colemanw added the merge ready PR will be merged after a few days if there are no objections label Oct 31, 2022
@colemanw
Copy link
Member Author

Thanks @kurund. I've merged the other PR, rebased this one, and fixed the bug you noticed. I think this is done.

As a followup, I noticed that the validation errors (including this one) don't bubble all the way up to actually get shown to the user. I think we'll need to extend the CRM_Core_Exception class to distinguish between safe messages and debug messages (currently none are visible to users without "view debug output" permission)

@kurund
Copy link
Contributor

kurund commented Nov 1, 2022

thanks @kurund. I've merged the other PR, rebased this one, and fixed the bug you noticed. I think this is done.

Yes, it's working as expected now.

As a followup, I noticed that the validation errors (including this one) don't bubble all the way up to actually get shown to the user. I think we'll need to extend the CRM_Core_Exception class to distinguish between safe messages and debug messages (currently none are visible to users without "view debug output" permission)

I feel CRM_Core_Exception is not passing the errorData correctly. So we might directly need to return the error message.

diff --git a/ext/afform/core/Civi/Api4/Action/Afform/Submit.php b/ext/afform/core/Civi/Api4/Action/Afform/Submit.php
index f9c301cb1b..e2c64b1c37 100644
--- a/ext/afform/core/Civi/Api4/Action/Afform/Submit.php
+++ b/ext/afform/core/Civi/Api4/Action/Afform/Submit.php
@@ -67,8 +67,10 @@ class Submit extends AbstractProcessor {
     $event = new AfformValidateEvent($this->_afform, $this->_formDataModel, $this, $entityValues);
     \Civi::dispatcher()->dispatch('civi.afform.validate', $event);
     $errors = $event->getErrors();
+    $detailedErrors = implode('<br/>', $errors);
+
     if ($errors) {
-      throw new \CRM_Core_Exception(ts('Validation Error', ['plural' => '%1 Validation Errors', 'count' => count($errors)]), 0, ['validation' => $errors]);
+      throw new \CRM_Core_Exception($detailedErrors);
     }

I also think Validation Error is kind of redundant as we also show 'Form Error' before that.

Copy link
Contributor

@kurund kurund left a comment

Choose a reason for hiding this comment

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

@colemanw

Good merge after test fixes. We can handle the sever error message display separately if you don't agree with the above suggestion.

Fixes dev/core#3173
Co-authored-by: Coleman Watts <[email protected]>
@colemanw
Copy link
Member Author

colemanw commented Nov 1, 2022

@kurund the reason I'm putting errors as an array is because concatenating html with plain text into a string leads to escaping issues. Better to output the array of errors and let the javascript template render the markup.

@colemanw colemanw merged commit 1956207 into civicrm:master Nov 1, 2022
@colemanw colemanw deleted the formbuilder-recaptcha2 branch November 1, 2022 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants