dev/core#1755 Fix reCaptcha on Mailing Subscribe

[mlutfy]

Overview

https://lab.civicrm.org/dev/core/-/issues/1755

• Enable reCaptcha on site
• Go to the mailing subscribe form (ex: https://civicrm.org/civicrm/mailing/subscribe?reset=1)
• Submit the form without answering the reCaptcha challenge.

The form submission will be accepted, without the challenge.

Before

After

Technical Details

• I cleaned up the code because the verification to see if the settings exist is already in the enableCaptchaOnForm function.
• The "gross hack for now" goes back to 2009 and is not clear. Maybe there was a Drupal block back then? reference

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[eileenmcnaughton]

test this please

[mlutfy]

jenkins, test this please

[mlutfy]

jenkins, test this please

(I'm not sure I understand the test output, if relevant or not)

test output includes warnings such as:

ok 4958 - api_v3_WebsiteTest::testGetFields with data set #1 (4)
1..4958
PHP Deprecated:  Civi\Payment\PropertyBag related deprecation warnings:
Unknown property 'version'. We have merged this in for now as a custom property. Please rewrite your code to use PropertyBag->setCustomProperty if it is a genuinely custom property, or a standardised setter like PropertyBag->setContactID for standard properties
Unknown property 'debug'. We have merged this in for now as a custom property. Please rewrite your code to use PropertyBag->setCustomProperty if it is a genuinely custom property, or a standardised setter like PropertyBag->setContactID for standard properties
Unknown property 'price_2'. We have merged this in for now as a custom property. Please rewrite your code to use PropertyBag->setCustomProperty if it is a genuinely custom property, or a standardised setter like PropertyBag->setContactID for standard properties
Unknown property 'id'. We have merged this in for now as a custom property. Please rewrite your code to use PropertyBag->setCustomProperty if it is a genuinely custom property, or a sta in /home/jenkins/bknix-dfl/build/core-17305-3c8xz/web/sites/all/modules/civicrm/CRM/Core/Error/Log.php on line 58
PHP Stack trace:
PHP   1. {main}() /home/jenkins/bknix-dfl/civicrm-buildkit/extern/phpunit6/phpunit6.phar:0
PHP   2. PHPUnit\TextUI\Command::main() /home/jenkins/bknix-dfl/civicrm-buildkit/extern/phpunit6/phpunit6.phar:570
PHP   3. PHPUnit\TextUI\Command->run() phar:///home/jenkins/bknix-dfl/civicrm-buildkit/extern/phpunit6/phpunit6.phar/phpunit/TextUI/Command.php:148
PHP   4. PHPUnit\TextUI\TestRunner->doRun() phar:///home/jenkins/bknix-dfl/civicrm-buildkit/extern/phpunit6/phpunit6.phar/phpunit/TextUI/Command.php:195
PHP   5. Civi\Payment\PropertyBag::writeLegacyWarnings() /home/jenkins/bknix-dfl/build/core-17305-3c8xz/web/sites/all/modules/civicrm/Civi/Payment/PropertyBag.php:0
PHP   6. CRM_Core_Error_Log->warning() /home/jenkins/bknix-dfl/build/core-17305-3c8xz/web/sites/all/modules/civicrm/Civi/Payment/PropertyBag.php:198
PHP   7. CRM_Core_Error_Log->log() /home/jenkins/bknix-dfl/build/core-17305-3c8xz/web/sites/all/modules/civicrm/vendor/psr/log/Psr/Log/AbstractLogger.php:85
PHP   8. trigger_error() /home/jenkins/bknix-dfl/build/core-17305-3c8xz/web/sites/all/modules/civicrm/CRM/Core/Error/Log.php:58

[eileenmcnaughton]

@mlutfy it's not blocking on this PR - I think agreeing an approach around this stuff is blocking progress on the property bag (& I think at the moment we're gonna hit pause on it) but for this PR it can be ignored.

[eileenmcnaughton]

@jusfreeman I feel sure you volunteered to review this one...

[agileware-justin]

@eileenmcnaughton happy to be volunteered to review PRs (and providing I have sufficient staff resources available) can do. However, we don't use this feature at all - have found it to be too problematic.

[eileenmcnaughton]

@jusfreeman yes but you mentioned the word so in the meetup so you seemed an obvious victim

[mlutfy]

It takes 5-10 minutes to test on a site with reCaptcha enabled, and would be much appreciated, since civicrm.org uses this :)

(or we should flag it for deprecation, but afform is not yet able to support this use-case, unfortunately)

[agileware-justin]

I have asked Pengyi to review this today which includes Review Standards and the short template.

C'mon, 5 - 10 minutes? That's not realistic at all. I've spent that long just reviewing this issue again.

Agileware Ref: CIVICRM-1529

[eileenmcnaughton]

Thanks @jusfreeman & yes I agree review does take time

[agileware-pengyi]

• General standards
  • (r-explain) Pass
  • (r-user) Pass
  • (r-doc) Pass
  • (r-run) Issue: Recaptcha is working on the mailing subscribe form. But the success message is not shown after submitting the form. You can see the message in the first gif of description. This might be a different issue?
• Developer standards
  • (r-tech) Pass: My review is based on WordPress and cannot see any reason to have the "gross hack". So good to remove it.
  • (r-code) Pass
  • (r-maint) Pass
  • (r-test) Pass

[agileware-justin]

@eileenmcnaughton @mlutfy review done

Time worked, 1 hour 15 minutes

[eileenmcnaughton]

Thanks @jusfreeman & thanks for adding some visibility on review time.

@mlutfy can you check the issue @agileware-pengyi pointed out

[mlutfy]

Sorry for my previous comment about review time, you're right.

"Recaptcha is working on the mailing subscribe form. But the success message is not shown after submitting the form. You can see the message in the first gif of description."

reCaptcha is displayed, but it's not validated. In the first GIF, I did not click on the reCaptcha to pass the challenge.

(I'm not sure if I understand correctly)

Thank you for the review!

[eileenmcnaughton]

@mlutfy I understood the comment as suggesting the 'your subscription has been sent' text disappears

[mlutfy]

hmm, I'm still not sure I see it.

Here's an example from civicrm.org: https://civicrm.org/civicrm/mailing/subscribe

(I pre-filled the form, so that you don't have to stare while I type and pass a captcha)

[eileenmcnaughton]

@agileware-pengyi can you please clarify?

[agileware-pengyi]

@mlutfy @eileenmcnaughton I didn't see the message in my test, but your test does have it. My test environment is the wp-demo from the buildkit.

[eileenmcnaughton]

@agileware-pengyi do you know if the message was there before? It's starting to sound like it might not be there with or without the patch in WP

[agileware-pengyi]

@eileenmcnaughton yeah, just checked it and no message without the patch. So I think this is passed.

[eileenmcnaughton]

@agileware-pengyi thanks for double checking - I'm guessing this got you over the 1.5 hours mark.

Merging