Procatcha support #786
Procatcha support #786
Conversation
|
Hey @light-source, Sorry for the delay here - just wanted to let you know that I've cleared up some time today for looking at this in more detail. |
There was a problem hiding this comment.
Ok, wow, this is really good right away!
Originally the integrations code was really about adding a sign-up checkbox to forms by other plugins, but I agree that it makes sense to have the Procaptcha specific settings live there.
I may have to work on expanding the forms editor to show a description or help text that links to that specific settings page, just so that users know what the new field is about and how to get it to work.
I left a few minor review comments but once those are taken care of, I think this is good to go!
| // something went wrong, maybe connection issue, but we still shouldn't allow the request. | ||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
Should we additionally check the response code here, ie wp_remote_retrieve_response_code() >= 400?
|
@light-source I just noticed the general settings section - didn't realize this was on the main settings page. That is a bit too prominent for my liking, so I have removed it in favor of a description on the new field. Sorry about that...
I've also tackled a few of my comments, so now only the (probably not so critical) response code check remains. Let me know your thoughts on that one please. |
|
Hi @dannyvankooten, Thank you for the review and for applying the improvements! I'm glad to hear you like the approach. 1. The general settings I hear you and think that the description is a nice workaround to keep it user-friendly. Since your description supports HTML, to improve UX, I would suggest adding a service link, and turning the bolden text into the internal link: If you don't mind I could make a new PR. 2. Response code check As I see you already added it as well. It makes sense to check the response code rather than a 'cold return' since you have a logger in place. Thank you for adding it. Side note: Plugin code quality Wanted to express that I was enjoying dealing with your plugin, it's a pleasure to see the right things, like OOP & type declarations in place along with good architecture. Nowadays, many plugins, despite being renowned and used by thousands, have painful code bases. |
|
Perfect @light-source, thanks a ton. I've modified the description so that it now links to the integration settings page instead, good suggestion! I think this is good to go out; so we'll be including it in the next plugin release, due on Monday. 🎉 By the way, although I don't think this is an issue with the code we're discussing here: wheneve I select |
|
@dannyvankooten exciting news! About the captcha type: the local type setting should be consistent with the account type setting, to be the same or stricter. E.g. if you've frictionless in the account settings (default), then you can use 'frictionless' (default) or 'image' (stricter) locally. If you want to use 'pure pow', you need to define it on the account level as well. |
Hello Danny!
I'm Maxim Akimov, a WordPress plugin developer. I collaborate with the Prosopo team and I am responsible for the integration with your plugin. (here is the related WP support thread)
After getting familiar with the plugin architecture, I have done the following:
Let me briefly explain the reasons behind the key decisions:
1. New integration
I opted for the new integration as it allows us to keep the vast of the integration code independent, without changing the core code.
Making all the Procaptcha settings part of the general settings would stretch them and could distract users while adding as part of form settings would require filling the same data multiple times.
The added 'Procaptcha status' to the general settings simplifies the navigation while keeping the settings short.
2. Procaptcha field type
I opted for the button, as unlike auto-injection it allows users not just to enable or disable the captcha for specific forms, but also precisely control its position in the form.
Meanwhile, the field button uses the existing setup and just adds a hidden input. This input is replaced with the real captcha element during rendering.
It allowed me to avoid big changes in the JS code while having a captcha element dynamic. It means if in the future we've something to change or improve in the element, we won't have the compatibility issue, as it would be in the case with the static HTML.
For the new and modified code, I setup IDE with CodeSniffer to follow your phpcs.xml rules.
Please review the PR, and let me know if you've any notes or concerns. I'm also open to changing the chosen integration approach, in case you don't like it.
Best regards,
Maxim