Skip to content

LG-12075: Rename attempt properties in analytics for clarity #10011

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.

Sign up for GitHub

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

Merged
night-jellyfish merged 12 commits into from
Feb 3, 2024
Merged

LG-12075: Rename attempt properties in analytics for clarity #10011

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
96a1f16
Rename image_added + opened event attrs
Jan 20, 2024
43e1f45
Rename image_submitted + validated attrs
Jan 22, 2024
b0aef35
Rename IdV: warning shown event attr
Jan 24, 2024
8f2364c
Rename idv_doc_auth_warning_visited attrs
Jan 25, 2024
80f0281
Rename idv_doc_auth_exception_visited attrs
Jan 25, 2024
7d61c91
Rename idv_session_error_visited attrs
Jan 26, 2024
3da5504
Rename idv_verify_by_mail_enter_code_submitted attrs
Jan 30, 2024
63a5d02
Rename idv_phone_error_visited attrs
Jan 30, 2024
0129914
changelog: Internal, Doc Auth, Rename attempt properties in analytics…
Jan 31, 2024
6bb1e44
Apply suggestions from code review
Jan 31, 2024
153866a
Update recently merged spec files for these changes after rebase
Jan 31, 2024
9ee83f3
Lintfixes after making comments longer
Jan 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions app/controllers/concerns/idv/verify_info_concern.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,14 +112,14 @@ def idv_failure_log_rate_limited(rate_limit_type)
def idv_failure_log_error
analytics.idv_doc_auth_exception_visited(
step_name: STEP_NAME,
remaining_attempts: resolution_rate_limiter.remaining_count,
remaining_submit_attempts: resolution_rate_limiter.remaining_count,
)
end

def idv_failure_log_warning
analytics.idv_doc_auth_warning_visited(
step_name: STEP_NAME,
remaining_attempts: resolution_rate_limiter.remaining_count,
remaining_submit_attempts: resolution_rate_limiter.remaining_count,
)
end

Expand Down
8 changes: 4 additions & 4 deletions app/controllers/idv/phone_errors_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ class PhoneErrorsController < ApplicationController
before_action :ignore_form_step_wait_requests

def warning
@remaining_attempts = rate_limiter.remaining_count
@remaining_submit_attempts = rate_limiter.remaining_count
Copy link
Copy Markdown
Contributor

@dawei-nava dawei-nava Jan 31, 2024

Choose a reason for hiding this comment

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

I suspect this attribut/property is used in ui erb file? If we renamed it, we will need rename there if used.

Copy link
Copy Markdown
Contributor Author

@night-jellyfish night-jellyfish Feb 1, 2024

Choose a reason for hiding this comment

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

Yes, that's correct! I renamed it here.

The var we talked about above in the timeout method, I can't find in use anywhere outside the spec. It should be in use by app/views/idv/phone_errors/timeout.html.erb, but it isn't, even in main.

That's why I think maybe that one could be deleted entirely? But it does seem outside the scope of this PR.

Copy link
Copy Markdown
Contributor

@dawei-nava dawei-nava Feb 1, 2024

Choose a reason for hiding this comment

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

@night-jellyfish , how about here?

identity-idp/app/views/idv/phone_errors/warning.html.erb

Lines 30 to 32 in eb7aa03

<p>
<%= t('idv.failure.phone.warning.attempts_html', count: @remaining_attempts) %>
</p>

Copy link
Copy Markdown
Contributor Author

@night-jellyfish night-jellyfish Feb 1, 2024

Choose a reason for hiding this comment

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

Yep! That's the one I changed in the first link in my previous comment.


if idv_session.previous_phone_step_params
@phone = idv_session.previous_phone_step_params[:phone]
Expand All @@ -21,12 +21,12 @@ def warning
end

def timeout
@remaining_step_attempts = rate_limiter.remaining_count
@remaining_submit_attempts = rate_limiter.remaining_count
Copy link
Copy Markdown
Contributor Author

@night-jellyfish night-jellyfish Jan 31, 2024

Choose a reason for hiding this comment

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

The controller did have a method that was remaining_step, but as it was defined the exact same way as others in this file named remaining_attempt, I changed it to be consistent.

Copy link
Copy Markdown
Contributor

@dawei-nava dawei-nava Jan 31, 2024

Choose a reason for hiding this comment

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

@night-jellyfish , can you point to where is the method?

Copy link
Copy Markdown
Contributor Author

@night-jellyfish night-jellyfish Jan 31, 2024

Choose a reason for hiding this comment

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

That's a good point. It doesn't seem like it's ever called, outside of the spec files. Perhaps it can just be removed then?

Copy link
Copy Markdown
Contributor

@dawei-nava dawei-nava Feb 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@night-jellyfish , i think like job_fail, the reading of rate_limiter.remaining_count may have side effects that needed, like updating the expiry time of underlying Redis entry, otherwise the RateLimiter will not be refreshed though these urls are visisted.

That raises another question why we create a new RateLimiter instance every time here? It works since we only use it once in one request/response conversation, and also it's backed by Redis with the same key.

Anyway, interesting details.

Copy link
Copy Markdown
Contributor Author

@night-jellyfish night-jellyfish Feb 2, 2024

Choose a reason for hiding this comment

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

Oh interesting! I don't fully understand the ins and outs of Redis, but that sounds plausible. Do you think it's worth a refactoring ticket to only create a new instance once?

Copy link
Copy Markdown
Contributor

@dawei-nava dawei-nava Feb 2, 2024

Choose a reason for hiding this comment

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

@night-jellyfish , i think it's fine to leave it as is, for Redis, just treat it as a database.

Copy link
Copy Markdown
Contributor Author

@night-jellyfish night-jellyfish Feb 2, 2024

Choose a reason for hiding this comment

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

I see. Just for my own understanding then - are you saying it'd be better to re-use the same RateLimiter instance because we could just reuse a single row in the Redis DB vs creating a new row each time?

track_event(type: :timeout)
end

def jobfail
@remaining_attempts = rate_limiter.remaining_count
@remaining_submit_attempts = rate_limiter.remaining_count
Copy link
Copy Markdown
Contributor Author

@night-jellyfish night-jellyfish Jan 31, 2024

Choose a reason for hiding this comment

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

I tried creating a private method in the controller so that the method
could be used as the definition, instead of the same code 3x in this file. But I
found doing so broke tests in an off-by-one sort of way - I think
somehow the private method held onto the value and was not updated the
same way as when it's defined each time. So I left the definition the same.

Copy link
Copy Markdown
Contributor

@dawei-nava dawei-nava Feb 2, 2024

Choose a reason for hiding this comment

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

@night-jellyfish , which test failed? I changed to private method and the spec and feature test for phone_errors_controller seems fine.

Copy link
Copy Markdown
Contributor Author

@night-jellyfish night-jellyfish Feb 2, 2024

Choose a reason for hiding this comment

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

Strange! I just tried again. When I try I get 2 failures:

 expected: ("IdV: phone error visited", {:remaining_submit_attempts=>4, :sample_bucket1=>:sample_value1, :sample_bucket2=>:sample_value2, :type=>:jobfail})
              got: ("IdV: phone error visited", {:remaining_submit_attempts=>5, :sample_bucket1=>:sample_value1, :sample_bucket2=>:sample_value2, :type=>:jobfail})

from

rspec ./spec/controllers/idv/phone_errors_controller_spec.rb:158 # Idv::PhoneErrorsController#warning with rate limit attempts logs an event
rspec ./spec/controllers/idv/phone_errors_controller_spec.rb:211 # Idv::PhoneErrorsController#jobfail with rate limit attempts logs an event

When I stop using the private method, they pass again.

track_event(type: :jobfail)
end

Expand Down Expand Up @@ -63,7 +63,7 @@ def track_event(type:)
if type == :failure
attributes[:limiter_expires_at] = @expires_at
else
attributes[:remaining_attempts] = @remaining_attempts
attributes[:remaining_submit_attempts] = @remaining_submit_attempts
end

analytics.idv_phone_error_visited(**attributes)
Expand Down
4 changes: 2 additions & 2 deletions app/controllers/idv/session_errors_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ def warning
)

@step_indicator_steps = step_indicator_steps
@remaining_attempts = rate_limiter.remaining_count
@remaining_submit_attempts = rate_limiter.remaining_count
log_event(based_on_limiter: rate_limiter)
end

Expand Down Expand Up @@ -93,7 +93,7 @@ def log_event(based_on_limiter: nil)
type: params[:action],
}

options[:attempts_remaining] = based_on_limiter.remaining_count if based_on_limiter
options[:submit_attempts_remaining] = based_on_limiter.remaining_count if based_on_limiter

analytics.idv_session_error_visited(**options)
end
Expand Down
4 changes: 2 additions & 2 deletions app/forms/gpo_verify_form.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ def submit
enqueued_at: gpo_confirmation_code&.code_sent_at,
which_letter: which_letter,
letter_count: letter_count,
attempts: attempts,
submit_attempts: submit_attempts,
pii_like_keypaths: [[:errors, :otp], [:error_details, :otp]],
pending_in_person_enrollment: !!pending_profile&.in_person_enrollment&.pending?,
fraud_check_failed: fraud_check_failed,
Expand Down Expand Up @@ -76,7 +76,7 @@ def letter_count
pending_profile&.gpo_confirmation_codes&.count
end

def attempts
def submit_attempts
RateLimiter.new(user: user, rate_limit_type: :verify_gpo_key).attempts
Comment thread
night-jellyfish marked this conversation as resolved.
Outdated
end

Expand Down
10 changes: 5 additions & 5 deletions app/forms/idv/api_image_upload_form.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,10 +144,10 @@ def doc_side_classification(client_response)

def extra_attributes
return @extra_attributes if defined?(@extra_attributes) &&
@extra_attributes&.dig('attempts') == attempts
@extra_attributes&.dig('submit_attempts') == submit_attempts
@extra_attributes = {
attempts: attempts,
remaining_attempts: remaining_attempts,
submit_attempts: submit_attempts,
remaining_submit_attempts: remaining_submit_attempts,
user_id: user_uuid,
pii_like_keypaths: DocPiiForm.pii_like_keypaths,
flow_path: params[:flow_path],
Expand Down Expand Up @@ -186,11 +186,11 @@ def selfie_image_fingerprint
end
end

def remaining_attempts
def remaining_submit_attempts
rate_limiter.remaining_count if document_capture_session
end

def attempts
def submit_attempts
rate_limiter.attempts if document_capture_session
end

Expand Down
12 changes: 6 additions & 6 deletions app/javascript/packages/document-capture/components/acuant-capture.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,9 @@ interface ImageAnalyticsPayload {
*/
source: ImageSource;
/**
* Total number of attempts at this point
* Total number of attempts to capture / upload an image at this point
*/
attempt?: number;
captureAttempts?: number;
/**
* Size of the image in bytes
*/
Expand Down Expand Up @@ -334,7 +334,7 @@ function AcuantCapture(
useMemo(() => setOwnErrorMessage(null), [value]);
const { isMobile } = useContext(DeviceContext);
const { t, formatHTML } = useI18n();
const [attempt, incrementAttempt] = useCounter(1);
const [captureAttempts, incrementCaptureAttempts] = useCounter(1);
const [acuantFailureCookie, setAcuantFailureCookie, refreshAcuantFailureCookie] =
useCookie('AcuantCameraHasFailed');
const [imageCaptureText, setImageCaptureText] = useState('');
Expand Down Expand Up @@ -384,10 +384,10 @@ function AcuantCapture(
>(payload: P): P {
const enhancedPayload = {
...payload,
attempt,
captureAttempts,
acuantCaptureMode: payload.source === 'upload' ? null : acuantCaptureMode,
};
incrementAttempt();
incrementCaptureAttempts();
return enhancedPayload;
}

Expand Down Expand Up @@ -516,7 +516,7 @@ function AcuantCapture(
}

function onSelfieCaptureSuccess({ image }: { image: string }) {
trackEvent('idv_sdk_selfie_image_added', { attempt });
trackEvent('idv_sdk_selfie_image_added', { captureAttempts });

onChangeAndResetError(image);
onResetFailedCaptureAttempts();
Expand Down
6 changes: 3 additions & 3 deletions app/javascript/packages/document-capture/components/document-capture-review-issues.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,15 @@ import type { ReviewIssuesStepValue } from './review-issues-step';
interface DocumentCaptureReviewIssuesProps extends FormStepComponentProps<ReviewIssuesStepValue> {
isFailedDocType: boolean;
isFailedSelfieLivenessOrQuality: boolean;
remainingAttempts: number;
remainingSubmitAttempts: number;
captureHints: boolean;
hasDismissed: boolean;
}

function DocumentCaptureReviewIssues({
isFailedDocType,
isFailedSelfieLivenessOrQuality,
remainingAttempts = Infinity,
remainingSubmitAttempts = Infinity,
captureHints,
registerField = () => undefined,
unknownFieldErrors = [],
Expand All @@ -52,7 +52,7 @@ function DocumentCaptureReviewIssues({
<DocumentCaptureSubheaderOne selfieCaptureEnabled={selfieCaptureEnabled} />
<UnknownError
unknownFieldErrors={unknownFieldErrors}
remainingAttempts={remainingAttempts}
remainingSubmitAttempts={remainingSubmitAttempts}
isFailedDocType={isFailedDocType}
isFailedSelfieLivenessOrQuality={isFailedSelfieLivenessOrQuality}
altIsFailedSelfieDontIncludeAttempts
Expand Down
12 changes: 6 additions & 6 deletions app/javascript/packages/document-capture/components/document-capture-warning.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ interface DocumentCaptureWarningProps {
isFailedDocType: boolean;
isFailedResult: boolean;
isFailedSelfieLivenessOrQuality: boolean;
remainingAttempts: number;
remainingSubmitAttempts: number;
actionOnClick?: () => void;
unknownFieldErrors: FormStepError<{ front: string; back: string }>[];
hasDismissed: boolean;
Expand All @@ -40,7 +40,7 @@ function DocumentCaptureWarning({
isFailedDocType,
isFailedResult,
isFailedSelfieLivenessOrQuality,
remainingAttempts,
remainingSubmitAttempts,
actionOnClick,
unknownFieldErrors = [],
hasDismissed,
Expand All @@ -66,7 +66,7 @@ function DocumentCaptureWarning({

trackEvent('IdV: warning shown', {
location: 'doc_auth_review_issues',
remaining_attempts: remainingAttempts,
remaining_submit_attempts: remainingSubmitAttempts,
heading,
subheading: subheadingText,
error_message_displayed: errorMessageDisplayed,
Expand All @@ -93,7 +93,7 @@ function DocumentCaptureWarning({
<div ref={errorMessageDisplayedRef}>
<UnknownError
unknownFieldErrors={unknownFieldErrors}
remainingAttempts={remainingAttempts}
remainingSubmitAttempts={remainingSubmitAttempts}
isFailedDocType={isFailedDocType}
isFailedSelfieLivenessOrQuality={isFailedSelfieLivenessOrQuality}
hasDismissed={hasDismissed}
Expand All @@ -102,10 +102,10 @@ function DocumentCaptureWarning({

{!isFailedDocType &&
!isFailedSelfieLivenessOrQuality &&
remainingAttempts <= DISPLAY_ATTEMPTS && (
remainingSubmitAttempts <= DISPLAY_ATTEMPTS && (
<p>
<HtmlTextWithStrongNoWrap
text={t('idv.failure.attempts_html', { count: remainingAttempts })}
text={t('idv.failure.attempts_html', { count: remainingSubmitAttempts })}
/>
</p>
)}
Expand Down
2 changes: 1 addition & 1 deletion app/javascript/packages/document-capture/components/document-capture.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ function DocumentCapture({ onStepChange = () => {} }: DocumentCaptureProps) {
form:
submissionError instanceof UploadFormEntriesError
? withProps({
remainingAttempts: submissionError.remainingAttempts,
remainingSubmitAttempts: submissionError.remainingSubmitAttempts,
isFailedResult: submissionError.isFailedResult,
isFailedDocType: submissionError.isFailedDocType,
isFailedSelfieLivenessOrQuality:
Expand Down
10 changes: 5 additions & 5 deletions app/javascript/packages/document-capture/components/review-issues-step.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ export interface ReviewIssuesStepValue {
}

interface ReviewIssuesStepProps extends FormStepComponentProps<ReviewIssuesStepValue> {
remainingAttempts?: number;
remainingSubmitAttempts?: number;
isFailedResult?: boolean;
isFailedDocType?: boolean;
isFailedSelfieLivenessOrQuality?: boolean;
Expand All @@ -53,7 +53,7 @@ function ReviewIssuesStep({
onError = () => {},
registerField = () => undefined,
toPreviousStep = () => undefined,
remainingAttempts = Infinity,
remainingSubmitAttempts = Infinity,
isFailedResult = false,
isFailedDocType = false,
isFailedSelfieLivenessOrQuality = false,
Expand All @@ -62,7 +62,7 @@ function ReviewIssuesStep({
failedImageFingerprints = { front: [], back: [] },
}: ReviewIssuesStepProps) {
const { trackEvent } = useContext(AnalyticsContext);
const [hasDismissed, setHasDismissed] = useState(remainingAttempts === Infinity);
const [hasDismissed, setHasDismissed] = useState(remainingSubmitAttempts === Infinity);
const { onPageTransition, changeStepCanComplete } = useContext(FormStepsContext);
const [skipWarning, setSkipWarning] = useState(false);
useDidUpdateEffect(onPageTransition, [hasDismissed]);
Expand Down Expand Up @@ -121,7 +121,7 @@ function ReviewIssuesStep({
isFailedDocType={isFailedDocType}
isFailedResult={isFailedResult}
isFailedSelfieLivenessOrQuality={isFailedSelfieLivenessOrQuality}
remainingAttempts={remainingAttempts}
remainingSubmitAttempts={remainingSubmitAttempts}
unknownFieldErrors={unknownFieldErrors}
actionOnClick={onWarningPageDismissed}
hasDismissed={false}
Expand All @@ -133,7 +133,7 @@ function ReviewIssuesStep({
<DocumentCaptureReviewIssues
isFailedDocType={isFailedDocType}
isFailedSelfieLivenessOrQuality={isFailedSelfieLivenessOrQuality}
remainingAttempts={remainingAttempts}
remainingSubmitAttempts={remainingSubmitAttempts}
captureHints={captureHints}
value={value}
unknownFieldErrors={unknownFieldErrors}
Expand Down
10 changes: 5 additions & 5 deletions app/javascript/packages/document-capture/components/unknown-error.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ interface UnknownErrorProps extends ComponentProps<'p'> {
unknownFieldErrors: FormStepError<{ front: string; back: string }>[];
isFailedDocType: boolean;
isFailedSelfieLivenessOrQuality: boolean;
remainingAttempts: number;
remainingSubmitAttempts: number;
altFailedDocTypeMsg?: string | null;
altIsFailedSelfieDontIncludeAttempts?: boolean;
hasDismissed: boolean;
Expand Down Expand Up @@ -44,7 +44,7 @@ function UnknownError({
unknownFieldErrors = [],
isFailedDocType = false,
isFailedSelfieLivenessOrQuality = false,
remainingAttempts,
remainingSubmitAttempts,
altFailedDocTypeMsg = null,
altIsFailedSelfieDontIncludeAttempts = false,
hasDismissed,
Expand Down Expand Up @@ -72,10 +72,10 @@ function UnknownError({
}
if (isFailedDocType && err) {
return (
<p key={`${err.message}-${remainingAttempts}`}>
<p key={`${err.message}-${remainingSubmitAttempts}`}>
{err.message}{' '}
<HtmlTextWithStrongNoWrap
text={t('idv.warning.attempts_html', { count: remainingAttempts })}
text={t('idv.warning.attempts_html', { count: remainingSubmitAttempts })}
/>
</p>
);
Expand All @@ -87,7 +87,7 @@ function UnknownError({
<p>
{!altIsFailedSelfieDontIncludeAttempts && (
<HtmlTextWithStrongNoWrap
text={t('idv.warning.attempts_html', { count: remainingAttempts })}
text={t('idv.warning.attempts_html', { count: remainingSubmitAttempts })}
/>
)}
</p>
Expand Down
2 changes: 1 addition & 1 deletion app/javascript/packages/document-capture/context/upload.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ export interface UploadErrorResponse {
/**
* Number of remaining doc capture attempts for user.
*/
remaining_attempts?: number;
remaining_submit_attempts?: number;

/**
* Boolean to decide if capture hints should be shown with error.
Expand Down
6 changes: 3 additions & 3 deletions app/javascript/packages/document-capture/services/upload.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ export class UploadFormEntryError extends FormError {
export class UploadFormEntriesError extends FormError {
formEntryErrors: UploadFormEntryError[] = [];

remainingAttempts = Infinity;
remainingSubmitAttempts = Infinity;

isFailedResult = false;

Expand Down Expand Up @@ -112,8 +112,8 @@ const upload: UploadImplementation = async function (payload, { method = 'POST',
error.formEntryErrors = result.errors.map(toFormEntryError);
}

if (result.remaining_attempts) {
error.remainingAttempts = result.remaining_attempts;
if (result.remaining_submit_attempts) {
error.remainingSubmitAttempts = result.remaining_submit_attempts;
}

if (result.ocr_pii) {
Expand Down
8 changes: 4 additions & 4 deletions app/presenters/image_upload_response_presenter.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ def errors
end
end

def remaining_attempts
@form_response.to_h[:remaining_attempts]
def remaining_submit_attempts
@form_response.to_h[:remaining_submit_attempts]
end

def status
Expand All @@ -40,9 +40,9 @@ def as_json(*)
else
json = { success: false,
errors: errors,
remaining_attempts: remaining_attempts,
remaining_submit_attempts: remaining_submit_attempts,
doc_type_supported: doc_type_supported? }
if remaining_attempts&.zero?
if remaining_submit_attempts&.zero?
if @form_response.extra[:flow_path] == 'standard'
json[:redirect] = idv_session_errors_rate_limited_url
else # hybrid flow on mobile
Expand Down
Loading