LG-14653 | Log state_id_type on doc auth result event

[n1zyy]

🎫 Ticket

Link to the relevant ticket:
LG-14653

🛠 Summary of changes

After we began enforcing expiration_date on documents during proofing, I worked on a story to build reporting on the impact of those changes. A missing piece of data is the ID type: are non-DL type IDs being disproportionately impacted, for example? We know some states handle these differently.

This begins logging that data on the 'IdV: doc auth verify proofing results' event. It was previously logged on another event, but it's not practical to join them in Cloudwatch.

[n1zyy]

I couldn't find prior art for handling this situation...

I basically wanted:

  expect(@analytics).to have_logged_event(
    'IdV: doc auth verify proofing results',
    hash_including(
      {
        proofing_results => context => stages => state_id => { state_id_type: 'drivers_license' }
      },
    ),
  )

except that's not valid code. I started nesting hash_including but it got really unwieldy fast.

hash_including doesn't look deep in the hash, e.g., just doing hash_including(state_id_type: 'drivers_license') expects a top-level key named state_id_type; it doesn't dig down recursively until it finds a match.

The test above feels inelegant, except compared to every other option I tried.

[zachmargolis]

we have one double-nested hash-including here:

identity-idp/spec/features/idv/analytics_spec.rb

Line 311 in 0e9bba2

success: true, errors: hash_including(message: nil), destination: :link_sent, flow_path: 'hybrid', step: 'hybrid_handoff', analytics_id: 'Doc Auth', telephony_response: hash_including(errors: {}, message_id: 'fake-message-id', request_id: 'fake-message-request-id', success: true), selfie_check_required: boolean

[zachmargolis]

but I think that you're right, doing a point-check is clearer here

[lmgeorge]

It's almost like we need have_logged_event to support a pattern like:

expect(@analytics).to have_logged_event(
    event_name, 
    hash_deeply_including(
        [:proofing_results, :context, :stages, :state_id],
        {state_id_type: 'drivers_license'},
    ),
)

[n1zyy]

@lmgeorge Something like this came up in the #rspec-league channel when I asked about this, even with some example code. I'm reluctant to add it, though. I feel like it would normalize having deeply-nested hashes for important things, but I feel like it's an antipattern.

I think I agree more with the comment in that thread that something being clunky to test is often a smell that the code (or here, data structure) is too convoluted. That's not an easy fix in this PR, though.

[lmgeorge]

Agreed. This is one of those do you fix "all the things" or just try to make your current need work. Either way, adding a custom matcher or dramatically restructuring the analytics event is beyond the scope of this PR.

[lmgeorge]

LGTM!