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

#2697 captcha to feedback from submission #2792

Merged
merged 18 commits into from
Sep 1, 2022

Conversation

OskarKocjan
Copy link
Contributor

@OskarKocjan OskarKocjan commented Aug 8, 2022

Changes:

  • added ReCaptcha for sign-in sign-up
  • added /feedback endpoint to send emails from the curator portal instead of outside application
  • added modal for sending email
  • added YAML documentation for /feedback endpoint
  • added success/failure alert to feedback

Copy link
Contributor

@maciej-zarzeczny maciej-zarzeczny left a comment

Choose a reason for hiding this comment

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

Sending emails and recaptcha works well 👍 I left some comments regarding code. It would be good to add some feedback for the user whether the feedback was sent successfully or not. You can use SnackbarAlert component for this purpose. Also while testing I noticed that when there are some formik errors displayed in the feedback form after clicking "cancel" and opening the modal again the errors are still present. They should be reseted.
Screen Shot 2022-08-09 at 12 30 56

@codecov-commenter
Copy link

codecov-commenter commented Aug 10, 2022

Codecov Report

Merging #2792 (2c03455) into main (a9b141e) will decrease coverage by 4.37%.
The diff coverage is 42.93%.

@@            Coverage Diff             @@
##             main    #2792      +/-   ##
==========================================
- Coverage   63.01%   58.64%   -4.38%     
==========================================
  Files          18      116      +98     
  Lines        1214     4606    +3392     
  Branches      187     1243    +1056     
==========================================
+ Hits          765     2701    +1936     
- Misses        449     1905    +1456     
Impacted Files Coverage Δ
...ation/curator-service/api/src/util/validate-env.ts 100.00% <ø> (ø)
.../components/AcknowledgmentsPage/helperFunctions.ts 0.00% <0.00%> (ø)
...curator-service/ui/src/components/Footer/styled.ts 100.00% <ø> (ø)
verification/curator-service/ui/src/setupProxy.js 0.00% <ø> (ø)
verification/curator-service/api/src/index.ts 83.68% <11.11%> (-4.95%) ⬇️
...cation/curator-service/api/src/controllers/auth.ts 45.13% <13.63%> (-3.52%) ⬇️
...ervice/api/src/util/single-window-rate-limiters.ts 37.50% <37.50%> (ø)
...r-service/api/src/util/validate-recaptcha-token.ts 42.85% <42.85%> (ø)
...vice/ui/src/components/landing-page/SignUpForm.tsx 69.73% <50.00%> (ø)
...ce/ui/src/components/FeedbackEmailDialog/index.tsx 53.57% <53.57%> (ø)
... and 100 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Contributor

@maciej-zarzeczny maciej-zarzeczny left a comment

Choose a reason for hiding this comment

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

LGTM ✅

useState<boolean>(false);
const [alertInformationMessage, setAlertInformationMessage] =
useState<string>('');
const [alertInformationType, setAlertInformationType] = useState<
Copy link
Contributor

Choose a reason for hiding this comment

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

In cases like this you can also add all those state variables under one state object. This way you later have to only call setState once providing all the values instead of three times

Copy link
Contributor

@abhidg abhidg left a comment

Choose a reason for hiding this comment

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

Couple of possible secrets, otherwise looks good

@@ -218,7 +218,9 @@ describe('LandingPage', function () {
cy.get('#password').type('tT$5aaaaak');
cy.get('#passwordConfirmation').type('tT$5aaaaak');
cy.get('#isAgreementChecked').check();
for (let i = 0; i < 5; i++) {
for (let i = 0; i < 7; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why is the Sign-in button being clicked so many times?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is clicked so many times to trigger the login/register limiter that will be used after 4 unsuccessful attempts. I will change the number of clicks to the previous commit which is 5 times.

@@ -71,7 +71,9 @@ interface SignInFormProps {
setRegistrationScreenOn: (active: boolean) => void;
}

const RECAPTCHA_SITE_KEY = process.env.RECAPTCHA_SITE_KEY as string;
const RECAPTCHA_SITE_KEY = window.Cypress
? '6LeIxAcTAAAAAJcZVRqyHh71UMIEGNQ_MXjiZKhI'
Copy link
Contributor

Choose a reason for hiding this comment

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

is this supposed to be secret? not sure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your feedback! That's a great question. It is a ReCaptchas site-key for testing purposes which is available for all developers: https://developers.google.com/recaptcha/docs/faq#id-like-to-run-automated-tests-with-recaptcha.-what-should-i-do Since every developer have access to it and it is the same for everyone I didn't set it as a secret.

@@ -93,7 +92,9 @@ interface SignUpFormProps {
setRegistrationScreenOn: (active: boolean) => void;
}

const RECAPTCHA_SITE_KEY = process.env.RECAPTCHA_SITE_KEY as string;
const RECAPTCHA_SITE_KEY = window.Cypress
? '6LeIxAcTAAAAAJcZVRqyHh71UMIEGNQ_MXjiZKhI'
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

@abhidg abhidg merged commit 90898ec into main Sep 1, 2022
@abhidg abhidg deleted the 2697-captcha-to-feedback-from-submission branch September 1, 2022 14:33
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.

None yet

4 participants