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

Simplify reCAPTCHA on ContributionBase form #20166

Merged
merged 2 commits into from
Apr 27, 2021

Conversation

mattwire
Copy link
Contributor

Overview

This is a refactor in preparation for moving the reCAPTCHA code to the new reCAPTCHA extension for the campaign signature form.
See https://lab.civicrm.org/dev/core/-/issues/2571

Before

More complex code.

After

Simpler code, will be easier to review next step which actually moves the code that adds the reCAPTCHA.

Technical Details

  • Use CRM_Utils_ReCAPTCHA::enableCaptchaOnForm directly in ContributionBase - this was left over from a previous older refactor.
  • Remove warning about reCaptcha missing key from contribution page - this is the only place that implements this "check" for reCAPTCHA enabled but not configured. It does not seem necessary as it is not implemented elsewhere - we could implement a system status check in the extension once all the code is moved across to check the reCAPTCHA configuration if required - but it seems unnecessary as the reCAPTCHA element will not be displayed if not configured and that should be trigger enough to check the settings..

Comments

@civibot
Copy link

civibot bot commented Apr 27, 2021

(Standard links)

@civibot civibot bot added the master label Apr 27, 2021
@eileenmcnaughton
Copy link
Contributor

OK - I agree that a system check makes more sense & that displaying this might not always be appropriate. The functionality seems trivial enough/ arguable enough that removing it to simplify follow on work seems OK to me

@eileenmcnaughton eileenmcnaughton merged commit 6e53da3 into civicrm:master Apr 27, 2021
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.

2 participants