-
Notifications
You must be signed in to change notification settings - Fork 65
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
Annotate captcha assessments #1043
Annotate captcha assessments #1043
Conversation
… into annotateCaptcha
// shouldAnnotateCaptchaAssessment determines whether the previous assessment should be annotated | ||
// returns LEGITIMATE or FRAUDULENT if the assessment should be annotated | ||
// returns ANNOTATION_UNSPECIFIED otherwise | ||
func (r *Reconciler) shouldAnnotateCaptchaAssessment(userSignup *toolchainv1alpha1.UserSignup, newState string) recaptchapb.AnnotateAssessmentRequest_Annotation { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function name is a bit misleading, isn't it? It doesn't determine if it should be annotated or not but rather always returns a new/expected assessment annotation, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was using ANNOTATION_UNSPECIFIED
as the base case where we don't actually do the annotation. In the other function we check whether it's ANNOTATION_UNSPECIFIED
and if so we skip annotating the assessment. But I'm fine to change it if it's confusing. Do you have a suggestion to make it more clear?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I renamed the function to getCaptchaAssessmentAnnotation
, let me know if you think another name would be better
…using captcha config
… into annotateCaptcha
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1043 +/- ##
==========================================
+ Coverage 84.83% 84.85% +0.02%
==========================================
Files 55 55
Lines 4919 4972 +53
==========================================
+ Hits 4173 4219 +46
- Misses 572 580 +8
+ Partials 174 173 -1
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Thanks!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexeykazakov, rajivnathan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
e48a299
into
codeready-toolchain:master
https://issues.redhat.com/browse/SANDBOX-626
Adds logic to the usersignup controller to annotate a previous captcha assessment when a user is banned or unbanned.
When a user is banned the assessment is annotated as FRAUDULENT and if a user was banned mistakenly and approved again then it is annotated as LEGITIMATE.
Added configuration setup for recaptcha client to work: d0e0d02
I tested it on a dev cluster and it worked.
Added unit tests: 80ccd44