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

Feat / Registration recaptcha #630

Merged
merged 1 commit into from
Nov 1, 2024

Conversation

royschut
Copy link
Collaborator

@royschut royschut commented Nov 1, 2024

Feat / Registration ReCaptcha

This PR adds optional Recaptcha validation, and adds the verified token to the registration api calls (currently for Cleeng).
Recaptcha can be enabled from the Cleeng dashboard. The site token should then be added as a custom parameter to the app-config: captcha_site_key=<site_id> to have the validator (by Google) added to the DOM.

ReCaptcha can also be set as required for payment API calls. This is out-of-scope for this PR, but might be added later.

Screenshot 2024-10-31 at 16 28 18

Steps completed:

According to our definition of done, I have completed the following steps:

  • Acceptance criteria met
  • Unit tests added
  • Docs updated (including config and env variables)
  • Translations added
  • UX tested
  • Browsers / platforms tested
  • Rebased & ready to merge without conflicts
  • Reviewed own code

@royschut royschut marked this pull request as ready for review November 1, 2024 11:35
Copy link
Collaborator

@langemike langemike left a comment

Choose a reason for hiding this comment

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

Simple and effective approach

Copy link
Collaborator

@ChristiaanScheermeijer ChristiaanScheermeijer left a comment

Choose a reason for hiding this comment

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

Nice addition @royschut!

My first thought, though, is if we should do the logic (specific to ReCaptcha) in the container or somewhere else. It feels a bit counterintuitive to use the ReCaptcha ref. But I also realise that error handling and form validation will suffer if we do it elsewhere... 🤔 Just putting this here for discussion!

@royschut
Copy link
Collaborator Author

royschut commented Nov 1, 2024

My first thought, though, is if we should do the logic (specific to ReCaptcha) in the container or somewhere else. It feels a bit counterintuitive to use the ReCaptcha ref. But I also realise that error handling and form validation will suffer if we do it elsewhere... 🤔 Just putting this here for discussion!

Thanks! I've been considering alternatives, but the ReCaptcha validator should be added to the UI, and it should trigger validation onSubmit (so that form entering can be checked as a human action validator). So the alternative would be to send callbacks around, but I think that would only complicate things.

An alternative to the ref that I thought about, was to send form values to the Recaptcha component, so that it can validate by itself when certain values change. But I think onSubmit is the most reliable moment for success.

@royschut royschut mentioned this pull request Nov 1, 2024
8 tasks
@royschut royschut merged commit fac471c into jwplayer:develop Nov 1, 2024
9 checks passed
@royschut royschut deleted the feat/cleeng-recaptcha branch November 1, 2024 14:08
This was referenced Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants