Document analytics methods in end-to-end sign-in flow#10736
Conversation
changelog: Internal, Analytics, Document analytics for critical-path sign-in flow
zachmargolis
left a comment
zachmargolis
left a comment
There was a problem hiding this comment.
LGTM! thanks for documenting these
| # @param [Integer] ialmax Whether the user registration was for an IALMax request | ||
| # @param [String] service_provider_name The friendly name of the service provider | ||
| # @param ['account-page','agency-page'] page_occurence Where the user concluded registration | ||
| # @param ['new_sp','new_attributes','reverified_after_consent'] needs_completion_screen_reason The |
There was a problem hiding this comment.
it doesn't matter but I notice we are using double quotes for other string values, single quotes here
There was a problem hiding this comment.
Ah I think I saw another method where we used single quotes which is why I used them here, so we're clearly a big mess of inconsistency 🙃
There was a problem hiding this comment.
If you're referring to lines like:
# @param ["authentication","reauthentication","confirmation"] context User session context
Those were copied from other methods, hence the internal inconsistency.
I may follow-up to do a bulk normalization, though I feel like it's inevitable we'll regress to having some inconsistency, unless we care to enforce it somehow.
There was a problem hiding this comment.
I took a look at rubocop-yard but unfortunately, it doesn't appear to handle string literals at all. Either way, a task for another time
| # @param [Array] sp_request_requested_attributes Attributes requested by the service provider | ||
| # @param [Array] sp_session_requested_attributes Attributes requested by the service provider |
There was a problem hiding this comment.
can we describe what the difference is between these two? I'm guessing one is per-request and the other is something else?
There was a problem hiding this comment.
I looked into this a bit, and... I think they're effectively the same? I'll double-check though. And if they are the same, maybe I'll follow-up or create a ticket to remove one or the other.
There was a problem hiding this comment.
I'm not seeing anything in the logs for the _request_ one in the last 2 weeks... very mysterious
| filter ispresent(properties.event_properties.sp_request_requested_attributes) or ispresent(properties.event_properties.sp_request_requested_attributes.0)
There was a problem hiding this comment.
I think the issue is that value reads from service_provider_request, but service_provider_request relies on the request_id URL query parameter, which we wouldn't expect to be present in the completions controller.
I can't imagine that service_provider_request has much utility to be honest, and we should probably be calling sp_session in most places instead. I can see about following-up on that.
| # @param [Array] sp_request_requested_attributes Attributes requested by the service provider | ||
| # @param [Array] sp_session_requested_attributes Attributes requested by the service provider |
There was a problem hiding this comment.
ditto about differentiation
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
2 participants
🛠 Summary of changes
Adds missing documentation for analytics methods called in an end-to-end partner authentication for a fully-registered user authenticating with phone.
This builds upon #10571, identifying a common flow as low-hanging fruit for fixing the largest possible number of specs which only need
allowed_extra_analyticsdue to exercising a critical user flow.📜 Testing Plan
Verify build passes.