LG-8664 | Log Attempt Event for fraud account ops#7951
Conversation
jmhooper
force-pushed
the
mattw/LG-8664_profile_reviewed
branch
from
0a65d6d to
07b89c3
Compare
jmhooper
force-pushed
the
mattw/LG-8664_profile_reviewed
branch
from
07b89c3 to
31f8336
Compare
changlog: Internal, Attempts API, Log events for fraud rejection
changelog: Internal, Attempts API, Log events for fraud rejection (LG-8664)
Needs text cleanup and feature flag still
app/models/fraud_event.rb
Outdated
| # It COULD make sense to put other stuff here and make this the primary way of tracking this, | ||
| # but the immediate need is to just have a way to link this data back to a user before there | ||
| # is a ServiceProviderIdentity. | ||
| class FraudEvent < ApplicationRecord |
There was a problem hiding this comment.
FWIW I'm kind of using this as a placeholder name. I worry "event" is overloaded in meaning.
The problem is that really the class name should be something like WeirdTableToStashSessionIdsInThisEdgeCase, and I haven't been able to find a clear name for it.
There was a problem hiding this comment.
maybe FraudReview? like the user is being reviewed for possible fraud, it's not confirmed fraud (which is what FraudEvent implies to me)
There was a problem hiding this comment.
This is definitely an improvement. That said, I think of "fraud review" as when happens when someone goes through these to approve/reject. But I like FraudReview a lot more.
There was a problem hiding this comment.
FraudReviewRequest? something like that
n1zyy
changed the title
app/models/profile.rb
Outdated
| fraud_event = user.fraud_event | ||
| irs_attempts_api_tracker&.fraud_review_adjudicated( | ||
| decision: 'pass', | ||
| fraud_event_id: fraud_event&.id, |
There was a problem hiding this comment.
in general I think it's a bad pattern to expose our internal DB primary key ids in any API. I would add a uuid column (and use our NonNullUuid concern)
There was a problem hiding this comment.
I like that, if we're going to keep this.
There's a question of whether we actually need this parameter. It sounds like the client wants session IDs. It seems like this extra detail we have for free may help make things easier, in which case I'm on board with making it a UUID. But I just want to call out that this is only being included because it felt like a good idea to me, not because the requirements actually call for an identifier.
There was a problem hiding this comment.
oh! in that case, let's drop from the event it if there is no business case for it (maybe add the UUID column just in case since we can't go back in time and add it for the past)
|
Just pushed up some changes that should have this in good shape. |
| @@ -1,4 +1,4 @@ | |||
| # Represents a record of a phone number that has beed opted out of SMS in AWS Pinpoint | |||
| # Represents a record of a phone number that has been opted out of SMS in AWS Pinpoint | |||
There was a problem hiding this comment.
Happened across this while looking at how NonNullUuid was used.
zachmargolis
left a comment
zachmargolis
left a comment
There was a problem hiding this comment.
LGTM, one last set of small comments
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
3 participants
🎫 Ticket
LG-8664
🛠 Summary of changes
When an account is either activated or shut down after review by the fraud team, we need to generate an AttemptEvent for it.
This requires keeping some session IDs around for the IRS to pass them offline, so this introduces a new FraudEvent (name subject to reconsideration in this PR) model for storing this information.