Skip to content

LG-9136: Add sign in attempt count to analytics#8036

Merged
jc-gsa merged 2 commits intomainfrom
LG-9136-include-sign-in-attempt-count
Mar 28, 2023
Merged

LG-9136: Add sign in attempt count to analytics#8036
jc-gsa merged 2 commits intomainfrom
LG-9136-include-sign-in-attempt-count

Conversation

@jc-gsa
Copy link
Contributor

@jc-gsa jc-gsa commented Mar 21, 2023

🎫 Ticket

LG-9136

@jc-gsa jc-gsa force-pushed the LG-9136-include-sign-in-attempt-count branch from 12fba78 to 395e0e5 Compare March 21, 2023 04:04
@jc-gsa jc-gsa force-pushed the LG-9136-include-sign-in-attempt-count branch from 395e0e5 to ffda26a Compare March 21, 2023 04:19
@mdiarra3 mdiarra3 requested a review from a team March 22, 2023 14:05
Comment on lines +232 to +235
success: false,
user_id: user.uuid,
user_locked_out: false,
bad_password_count: 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we have a spec demonstrating where the count is greater than 0?

On a similar note, should the count consider the current submission? For example, would it be wrong in this spec that the attempt was unsuccessful (success: false 3 lines above) but the bad_password_count is 0? In other words, should the incrementing of the bad password count happen before we log the result, or at least account for it in the logging?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the approach.

@jc-gsa jc-gsa requested a review from a team March 22, 2023 22:46
Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@jc-gsa jc-gsa merged commit 3cef589 into main Mar 28, 2023
@jc-gsa jc-gsa deleted the LG-9136-include-sign-in-attempt-count branch March 28, 2023 12:01
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.

2 participants