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

Recaptcha initialization #191

Merged
merged 14 commits into from
Apr 8, 2020
Merged

Recaptcha initialization #191

merged 14 commits into from
Apr 8, 2020

Conversation

engcom-Foxtrot
Copy link
Contributor

@engcom-Foxtrot engcom-Foxtrot commented Apr 3, 2020

Description (*)

This PR accumulates commits from #183, #176 and #173

Fixed Issues (if relevant)

#21

Manual testing scenarios (*)

  1. See security-package: Changes for include Google API js file - Admin panel #183
  2. See security-package: Update for reCaptcha.js - changes for include Google API js script #176
  3. See security-package: Update for reCaptcha language code. Fixed incorrect language of reCaptcha #173

Questions or comments

Contribution checklist (*)

  • Author has signed the Adobe CLA
  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds are green)

@engcom-Delta
Copy link

✔️ QA passed
Result #176:
image

Result #183:
image

image

Result #173 : Text in reCAPTCHA v2 ("I am not a robot") blocks on French, reCAPTCHA v3 Invisible block for Newsletter on German.
image

@@ -50,16 +50,19 @@ define(
$(window).trigger('recaptchaapiready');
}.bind(this);

element = document.createElement('script');
scriptTag = document.getElementsByTagName('script')[0];
if (window.addedScriptTag === undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you rename variable to make it not too general?

Suggested change
if (window.addedScriptTag === undefined) {
if (window.addedRecaptchaScriptTag === undefined) {

Or maybe even better - just add variable inside this JS file and not leak it to window at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ihor-sviziev this variable should be global.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you explain why?

Copy link
Contributor

@naydav naydav Apr 6, 2020

Choose a reason for hiding this comment

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

Looks like we can replace global variable on new shared service.

The reCaptcha component will have dependency (via "import" by path) on this shared service.
Inside of the new service adding script tag will proceed only one time.

Thanks

@engcom-Foxtrot engcom-Foxtrot added the Component: Google reCAPTCHA Issues and Pull Requests related to reCAPTCHA should be marked with this label label Apr 6, 2020
@m2-community-project m2-community-project bot moved this from Ready for Review to Review in Progress in Pull Request Progress Apr 6, 2020
@engcom-Delta
Copy link

✔️ QA passed with latest commit
Result #176:
image

Result #183:
image

image

Result #173 : Text in reCAPTCHA v2 ("I am not a robot") blocks on French, reCAPTCHA v3 Invisible block for Newsletter on German.
image

Co-Authored-By: Ihor Sviziev <ihor-sviziev@users.noreply.github.com>
@naydav naydav merged commit 496055e into 1.0-develop Apr 8, 2020
@m2-community-project m2-community-project bot moved this from Review in Progress to Done in Pull Request Progress Apr 8, 2020
@lenaorobei lenaorobei deleted the recaptcha-initialization branch May 7, 2020 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Google reCAPTCHA Issues and Pull Requests related to reCAPTCHA should be marked with this label
Projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants