FORMS-13811 | Adding rendition for hCaptcha #1174

navneet-ag · 2024-04-08T14:28:12Z

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

I have signed the Adobe Open Source CLA.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes and the overall coverage did not decrease.
All unit tests pass on CircleCi.
I ran all tests locally and they pass.

...ps/src/main/content/jcr_root/apps/core/fd/components/form/hcaptcha/v1/hcaptcha/hcaptcha.html

rismehta · 2024-04-12T05:46:29Z

bundles/af-core/src/main/java/com/adobe/cq/forms/core/components/models/form/HCaptcha.java

+ * Defines the form {@code Captcha} Sling Model used for the {@code /apps/core/fd/components/form/hcaptcha/v1/hcaptcha}
+ * component.
+ *
+ * @since com.adobe.cq.forms.core.components.models.form 2.0.0


Please bump the minor version, since you have created a new interface. also update the docs

bundles/af-core/pom.xml

rismehta

Please add test case for this code. Also, create a sample in core component on how one could create a custom captcha. You can check the existing code where we have test cases for custom components.

...e/src/main/java/com/adobe/cq/forms/core/components/internal/models/v1/form/HCaptchaImpl.java

...cr_root/apps/core/fd/components/form/hcaptcha/v1/hcaptcha/clientlibs/site/js/hcaptchaview.js

rismehta

check comments

nit23uec · 2024-04-18T13:37:38Z

bundles/af-core/src/main/java/com/adobe/cq/forms/core/components/models/form/HCaptcha.java

+ * @since com.adobe.cq.forms.core.components.models.form 5.4.0
+ */
+@ConsumerType
+public interface HCaptcha extends Captcha {}


we should not need this interface if its not adding any new method over the base interface.

This is just a marker interface to be consumed by HCaptchaImpl.

Why can't the impl just implement Captcha?

This is consumed by sightly, if we use HCaptchaImpl implemented Captcha then it would have not picked up the correct impl.

nit23uec · 2024-04-18T13:50:52Z

...cr_root/apps/core/fd/components/form/hcaptcha/v1/hcaptcha/clientlibs/site/js/hcaptchaview.js

+(function() {
+
+    "use strict";
+    class HCaptcha extends FormView.FormFieldBase {


since we are already inheriting from recaptcha RT, is there any reason to have dedicated clientlibs for hcaptcha? we could have even reused the bem block for all captcha providers (thinking about our guidance to customers in future to implement another captcha provider as a separate component).

I had a discussion long back with @sakshi-arora1, and it was a conscious decision to keep separate captcha components in AFv2. @sakshi-arora1 can comment more about the same.

The components should be different (in terms of inserting them). I'm just wondering whether we need to duplicate the code in each captcha component and what would be our recommendation for a custom captcha comp in future for customers?

So maybe we can have a captcha utils in place which can be reused by multiple captcha components ?

nit23uec · 2024-04-18T13:52:52Z

...ps/src/main/content/jcr_root/apps/core/fd/components/form/hcaptcha/v1/hcaptcha/hcaptcha.html

+  ~ See the License for the specific language governing permissions and
+  ~ limitations under the License.
+  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~*/-->
+<sly data-sly-use.renderer="${'hcaptcha.js'}"


again, since we are already inheriting recaptcha, any reason to redefine this sightly again? is it just for bem selectors?

Yep, just for the bem selectors.

Is there a styling use case for captcha?

For the error message, which appears if user tries to submit without filling the captcha

Ok. Personally I'd have preferred to keep bem selector same for both recaptcha and hcaptcha or any other captcha provider that may be integrated in future. And that would also have enabled anyone implementing a custom captcha component by just inherting an existing captcha component (for client libs and sightly). Wdyt?

In this approach, we should release a v2 version of Recaptcha core component with a generic bem model. As the current bem model is specific to recaptcha (https://github.com/adobe/aem-core-forms-components/blob/master/ui.af.apps/src/main/content/jcr_root/apps/core/fd/components/form/recaptcha/v1/recaptcha/clientlibs/site/css/recaptchaview.css#L9)

And for custom captcha, apart from client libs and sightly; cq:dialog might also need to be overridden for the custom captcha.

Ok. Personally I'd have preferred to keep bem selector same for both recaptcha and hcaptcha or any other captcha provider that may be integrated in future. And that would also have enabled anyone implementing a custom captcha component by just inherting an existing captcha component (for client libs and sightly). Wdyt?

From a technical perspective, maintaining unique BEM selectors for different captcha providers such as reCAPTCHA and hCaptcha helps ensure that the styling and behaviors of these components are distinctly manageable. This can be particularly useful in cases where the captchas might have differing visual or functional requirements that could conflict if merged under a single BEM namespace.

Furthermore, using distinct BEM selectors can avoid potential clashes in CSS and JavaScript (some customers don't use recommended best practices, and use BEM selectors in JS), which might occur if the same class names are applied across different libraries or functionalities. This separation ensures that updates or changes to one captcha implementation do not inadvertently affect the styling or functionality of another.

If we receive feedback indicating a need for a more unified approach, we should reconsider our strategy. For now, maintaining separate selectors provides us with greater control and minimizes risk of cross-component interference.

codecov · 2024-04-23T05:34:59Z

Codecov Report

Attention: Patch coverage is 82.60870% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 80.89%. Comparing base (c227001) to head (b1dbce5).

Files	Patch %	Lines
...mponents/internal/models/v1/form/HCaptchaImpl.java	82.60%	1 Missing and 3 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##                dev    #1174      +/-   ##
============================================
+ Coverage     80.87%   80.89%   +0.01%     
- Complexity      786      791       +5     
============================================
  Files            91       92       +1     
  Lines          2123     2146      +23     
  Branches        289      292       +3     
============================================
+ Hits           1717     1736      +19     
- Misses          252      253       +1     
- Partials        154      157       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

adobe-bot · 2024-04-23T05:53:51Z

Accessibility Violations Found

Id	Impact
label-title-only	serious
landmark-one-main	moderate
region	moderate
target-size	serious

adobe-bot · 2024-04-23T05:54:12Z

Accessibility Violations Found

Id	Impact
label-title-only	serious
landmark-one-main	moderate
region	moderate
target-size	serious

adobe-bot · 2024-04-23T05:54:18Z

Lighthouse scores (desktop)

	Performance	Accessibility	Best-Practices	SEO
Scores	100	96	100	75

adobe-bot · 2024-04-23T05:54:19Z

Lighthouse scores (mobile)

	Performance	Accessibility	Best-Practices	SEO
Scores	94	96	100	75

adobe-bot · 2024-04-23T05:54:29Z

Accessibility Violations Found

Id	Impact
label-title-only	serious
landmark-one-main	moderate
region	moderate
target-size	serious

adobe-bot · 2024-04-23T05:55:36Z

Accessibility Violations Found

Id	Impact
label-title-only	serious
landmark-one-main	moderate
region	moderate
target-size	serious

adobe-bot · 2024-04-23T05:56:11Z

Accessibility Violations Found

Id	Impact
label-title-only	serious
landmark-one-main	moderate
region	moderate
target-size	serious

adobe-bot · 2024-04-23T16:06:26Z

Lighthouse scores (mobile)

	Performance	Accessibility	Best-Practices	SEO
Scores	93	96	100	75

adobe-bot · 2024-04-23T16:06:26Z

Lighthouse scores (desktop)

	Performance	Accessibility	Best-Practices	SEO
Scores	100	96	100	75

adobe-bot · 2024-04-23T16:06:37Z

Accessibility Violations Found

Id	Impact
label-title-only	serious
landmark-one-main	moderate
region	moderate
target-size	serious

adobe-bot · 2024-04-23T16:08:41Z

Accessibility Violations Found

Id	Impact
label-title-only	serious
landmark-one-main	moderate
region	moderate
target-size	serious

adobe-bot · 2024-05-02T12:39:46Z

Lighthouse scores (mobile)

	Performance	Accessibility	Best-Practices	SEO
Scores	94	96	100	75

adobe-bot · 2024-05-02T12:39:46Z

Lighthouse scores (desktop)

	Performance	Accessibility	Best-Practices	SEO
Scores	100	96	100	75

adobe-bot · 2024-05-02T12:39:57Z

Accessibility Violations Found

Id	Impact
label-title-only	serious
landmark-one-main	moderate
region	moderate
target-size	serious

adobe-bot · 2024-05-02T12:41:27Z

Accessibility Violations Found

Id	Impact
label-title-only	serious
landmark-one-main	moderate
region	moderate
target-size	serious

adobe-bot · 2024-05-02T12:41:47Z

Accessibility Violations Found

Id	Impact
label-title-only	serious
landmark-one-main	moderate
region	moderate
target-size	serious

adobe-bot · 2024-05-02T13:10:14Z

Accessibility Violations Found

Id	Impact
label-title-only	serious
landmark-one-main	moderate
region	moderate
target-size	serious

adobe-bot · 2024-05-02T13:10:16Z

Lighthouse scores (desktop)

	Performance	Accessibility	Best-Practices	SEO
Scores	100	96	100	75

adobe-bot · 2024-05-02T13:10:16Z

Lighthouse scores (mobile)

	Performance	Accessibility	Best-Practices	SEO
Scores	93	96	100	75

adobe-bot · 2024-05-02T13:10:28Z

Accessibility Violations Found

Id	Impact
label-title-only	serious
landmark-one-main	moderate
region	moderate
target-size	serious

adobe-bot · 2024-05-02T13:10:33Z

Accessibility Violations Found

Id	Impact
label-title-only	serious
landmark-one-main	moderate
region	moderate
target-size	serious

* embedding forms-sdk api * adding hcaptcha and recaptcha models * :fix hcaptcha model SlingException * :fix deleting recaptcha model * FORMS-13811 | Adding rendition for hCaptcha * FORMS-13811 | Renamed public API * FORMS-13811 | Embedding hcaptcha js * FORMS-13811 | Adding unit test for sling model * FORMS-13811 | Adding cypress test * FORMS-13811 | Adressing review comments * Adding e2e test * FORMS-13811 | fix import statements * Update pom.xml * Adding FT for running Tests * FORMS-13811 | Removing java-8 build for master * Adding FT in test * FORMS-13811 | Tests should run only with latest addon * FORMS-13811 | Tests should run only with latest addon * increasing coverage * addressing review comments --------- Co-authored-by: Navneet Agarwal <navneeta@adobe.com>

github-actions bot force-pushed the captcha/hcaptcha branch 3 times, most recently from 863f7fb to 39f0ce7 Compare April 10, 2024 13:52

rismehta reviewed Apr 12, 2024

View reviewed changes

...ps/src/main/content/jcr_root/apps/core/fd/components/form/hcaptcha/v1/hcaptcha/hcaptcha.html Show resolved Hide resolved

rismehta reviewed Apr 12, 2024

View reviewed changes

bundles/af-core/pom.xml Outdated Show resolved Hide resolved

rismehta requested changes Apr 12, 2024

View reviewed changes

github-actions bot force-pushed the captcha/hcaptcha branch from ce3c8da to ed90550 Compare April 12, 2024 06:31