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

LG-14673: Add SP info to fraud review events #11390

Merged
merged 7 commits into from
Nov 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 6 additions & 1 deletion app/services/analytics.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,11 @@ class Analytics

attr_reader :user, :request, :sp, :session, :ahoy

# @param [User] user
# @param [ActionDispatch::Request,nil] request
# @param [String,nil] sp Service provider issuer string.
# @param [Hash] session
# @param [Ahoy::Tracker,nil] ahoy
def initialize(user:, request:, sp:, session:, ahoy: nil)
@user = user
@request = request
Expand All @@ -21,6 +26,7 @@ def track_event(event, attributes = {})
event_properties: attributes.except(:user_id).compact,
new_event: first_event_this_session?,
path: request&.path,
service_provider: sp,
Copy link
Contributor

Choose a reason for hiding this comment

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

I had to triple check, but I forgot what the type of sp was as an attribute, since we're here, WDYT of adding type documentation to the initialzer for this class like

# @param sp [String] issuer string for the service provider for this analytics event

Copy link
Contributor

Choose a reason for hiding this comment

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

On that note, would it be worthwhile to rename the sp param to sp_issuer or just issuer?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I agree with that... probably not going to do in this PR but standardizing usage of sp vs issuer vs sp_issuer across the codebase would be great. I think I'd be in favor of:

sp = A ServiceProvider instance
sp_issuer = sp.issuer, an issuer string

session_duration: session_duration,
user_id: attributes[:user_id] || user.uuid,
locale: I18n.locale,
Expand Down Expand Up @@ -58,7 +64,6 @@ def request_attributes
user_ip: request.remote_ip,
hostname: request.host,
pid: Process.pid,
service_provider: sp,
trace_id: request.headers['X-Amzn-Trace-Id'],
}

Expand Down
31 changes: 20 additions & 11 deletions lib/action_account.rb
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,7 @@ def run(args:, config:)
messages = []

users.each do |user|
profile = nil
profile_fraud_review_pending_at = nil
success = false

Expand All @@ -187,7 +188,6 @@ def run(args:, config:)
elsif FraudReviewChecker.new(user).fraud_review_eligible?
profile = user.fraud_review_pending_profile
profile_fraud_review_pending_at = profile.fraud_review_pending_at
profile_age_in_seconds = profile.profile_age_in_seconds
profile.reject_for_fraud(notify_user: true)
success = true

Expand All @@ -211,18 +211,23 @@ def run(args:, config:)
end

Analytics.new(
user: user, request: nil, session: {}, sp: nil,
user: user,
request: nil,
session: {},
sp: profile&.initiating_service_provider_issuer,
).fraud_review_rejected(
success:,
errors: analytics_error_hash,
exception: nil,
profile_fraud_review_pending_at: profile_fraud_review_pending_at,
profile_age_in_seconds: profile_age_in_seconds,
profile_fraud_review_pending_at:,
profile_age_in_seconds: profile&.profile_age_in_seconds,
Copy link
Contributor

Choose a reason for hiding this comment

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

(suggestion, non-blocking): I think it is worthwhile keeping the profile attr wrapping methods to streamline reading as profile attributes are inconsistently accessed with safe navigation (see line 281 where we don't use safe navigation).

)
end

if config.include_missing?
(uuids - users.map(&:uuid)).each do |missing_uuid|
missing_uuids = (uuids - users.map(&:uuid))

if config.include_missing? && !missing_uuids.empty?
missing_uuids.each do |missing_uuid|
table, messages = log_message(
uuid: missing_uuid,
log: log_text[:missing_uuid],
Expand Down Expand Up @@ -264,6 +269,7 @@ def run(args:, config:)

messages = []
users.each do |user|
profile = nil
profile_fraud_review_pending_at = nil
success = false

Expand All @@ -273,7 +279,6 @@ def run(args:, config:)
elsif FraudReviewChecker.new(user).fraud_review_eligible?
profile = user.fraud_review_pending_profile
profile_fraud_review_pending_at = profile.fraud_review_pending_at
profile_age_in_seconds = profile.profile_age_in_seconds
profile.activate_after_passing_review
success = true

Expand Down Expand Up @@ -305,18 +310,22 @@ def run(args:, config:)
end

Analytics.new(
user: user, request: nil, session: {}, sp: nil,
user: user,
request: nil,
session: {},
sp: profile&.initiating_service_provider_issuer,
).fraud_review_passed(
success:,
errors: analytics_error_hash,
exception: nil,
profile_fraud_review_pending_at: profile_fraud_review_pending_at,
profile_age_in_seconds: profile_age_in_seconds,
profile_age_in_seconds: profile&.profile_age_in_seconds,
)
end

if config.include_missing?
(uuids - users.map(&:uuid)).each do |missing_uuid|
missing_uuids = (uuids - users.map(&:uuid))
if config.include_missing? && !missing_uuids.empty?
missing_uuids.each do |missing_uuid|
table, messages = log_message(
uuid: missing_uuid,
log: log_text[:missing_uuid],
Expand Down
40 changes: 40 additions & 0 deletions spec/lib/action_account_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,26 @@
errors: { message: 'Error: Could not find user with that UUID' },
)
end

context 'when profile has initiating_service_provider_issuer' do
let(:user) do
create(
:profile,
:fraud_review_pending,
initiating_service_provider_issuer: 'test-issuer',
).user
end

it 'attributes analytics events to the SP' do
expect(Analytics).to receive(:new).
with(hash_including(sp: 'test-issuer')).
and_return(analytics)

subtask.run(args:, config:)

expect(analytics).to have_logged_event('Fraud: Profile review rejected')
end
end
end
end

Expand Down Expand Up @@ -241,6 +261,26 @@
errors: { message: 'Error: Could not find user with that UUID' },
)
end

context 'when profile has initiating_service_provider_issuer' do
let(:user) do
create(
:profile,
:fraud_review_pending,
initiating_service_provider_issuer: 'test-issuer',
).user
end

it 'attributes analytics events to the SP' do
expect(Analytics).to receive(:new).
with(hash_including(sp: 'test-issuer')).
and_return(analytics)

subtask.run(args:, config:)

expect(analytics).to have_logged_event('Fraud: Profile review passed')
end
end
end
end

Expand Down
15 changes: 14 additions & 1 deletion spec/services/analytics_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
user_id: current_user.uuid,
new_event: true,
path: path,
service_provider: 'http://localhost:3000',
session_duration: nil,
locale: I18n.locale,
git_sha: IdentityConfig::GIT_SHA,
Expand All @@ -27,7 +28,6 @@
browser_bot: false,
hostname: FakeRequest.new.host,
pid: Process.pid,
service_provider: 'http://localhost:3000',
trace_id: nil,
}
end
Expand Down Expand Up @@ -194,6 +194,19 @@
end
end
end

context 'when no request specified' do
let(:request) { nil }
context 'but an SP was specified via initializer' do
it 'logs the SP' do
expect(ahoy).to receive(:track).with(
'Trackable Event',
hash_including(service_provider: 'http://localhost:3000'),
)
analytics.track_event('Trackable Event')
end
end
end
end

it 'tracks session duration' do
Expand Down