LG-12075: Rename attempt properties in analytics for clarity by night-jellyfish · Pull Request #10011 · 18F/identity-idp

night-jellyfish · 2024-01-31T03:15:47Z

🎫 Ticket

LG-12075

🛠 Summary of changes

We had a few attributes in analytics events named very similarly (attempt, attempts, remaining_attempts), despite meaning different things. Sometimes they meant the submission attempt, and sometimes they meant the document capture attempt.

This PR renames the attributes to clarify what kind of attempt they're pointing to.

📜 Testing Plan

Alternatively:

Run make watch_events
Do the action needed to cause a few of the events listed above
Make sure the output contains the expected version of the attributes

Notes

This PR is easiest to review in stages by commit (each commit is self contained).

night-jellyfish · 2024-01-31T03:22:43Z

app/services/analytics_events.rb

A little unsure if this should be "code entry attempts" or something
instead...but the definition of this attribute seems to be from the
same areas I have renamed submit_attempts, so that's why I chose that.

night-jellyfish · 2024-01-31T03:23:49Z

app/controllers/idv/phone_errors_controller.rb

The controller did have a method that was remaining_step, but as it was defined the exact same way as others in this file named remaining_attempt, I changed it to be consistent.

@night-jellyfish , can you point to where is the method?

That's a good point. It doesn't seem like it's ever called, outside of the spec files. Perhaps it can just be removed then?

@night-jellyfish , i think like job_fail, the reading of rate_limiter.remaining_count may have side effects that needed, like updating the expiry time of underlying Redis entry, otherwise the RateLimiter will not be refreshed though these urls are visisted.

That raises another question why we create a new RateLimiter instance every time here? It works since we only use it once in one request/response conversation, and also it's backed by Redis with the same key.

Anyway, interesting details.

Oh interesting! I don't fully understand the ins and outs of Redis, but that sounds plausible. Do you think it's worth a refactoring ticket to only create a new instance once?

@night-jellyfish , i think it's fine to leave it as is, for Redis, just treat it as a database.

I see. Just for my own understanding then - are you saying it'd be better to re-use the same RateLimiter instance because we could just reuse a single row in the Redis DB vs creating a new row each time?

night-jellyfish · 2024-01-31T03:24:35Z

app/controllers/idv/phone_errors_controller.rb

I tried creating a private method in the controller so that the method
could be used as the definition, instead of the same code 3x in this file. But I
found doing so broke tests in an off-by-one sort of way - I think
somehow the private method held onto the value and was not updated the
same way as when it's defined each time. So I left the definition the same.

@night-jellyfish , which test failed? I changed to private method and the spec and feature test for phone_errors_controller seems fine.

Strange! I just tried again. When I try I get 2 failures:

expected: ("IdV: phone error visited", {:remaining_submit_attempts=>4, :sample_bucket1=>:sample_value1, :sample_bucket2=>:sample_value2, :type=>:jobfail}) got: ("IdV: phone error visited", {:remaining_submit_attempts=>5, :sample_bucket1=>:sample_value1, :sample_bucket2=>:sample_value2, :type=>:jobfail})

from

rspec ./spec/controllers/idv/phone_errors_controller_spec.rb:158 # Idv::PhoneErrorsController#warning with rate limit attempts logs an event rspec ./spec/controllers/idv/phone_errors_controller_spec.rb:211 # Idv::PhoneErrorsController#jobfail with rate limit attempts logs an event

When I stop using the private method, they pass again.

app/forms/gpo_verify_form.rb

app/services/analytics_events.rb

dawei-nava · 2024-01-31T23:38:38Z

app/controllers/idv/phone_errors_controller.rb

I suspect this attribut/property is used in ui erb file? If we renamed it, we will need rename there if used.

Yes, that's correct! I renamed it here.

The var we talked about above in the timeout method, I can't find in use anywhere outside the spec. It should be in use by app/views/idv/phone_errors/timeout.html.erb, but it isn't, even in main.

That's why I think maybe that one could be deleted entirely? But it does seem outside the scope of this PR.

@night-jellyfish , how about here?

identity-idp/app/views/idv/phone_errors/warning.html.erb

Lines 30 to 32 in eb7aa03

<p>

<%= t('idv.failure.phone.warning.attempts_html', count: @remaining_attempts) %>

</p>

Yep! That's the one I changed in the first link in my previous comment.

dawei-nava

LGTM.

These values permeated through a lot of the code, so this ended up having a lot of ripple effects.

A little unsure if this should be "code entry attempts" or something instead...but the definition of this attribute seems to be from the same areas I have renamed submit attempts, so that's why I chose that.

The controller did have a method that was `remaining_step`, but as it was defined the exact same way as others named `remaining_attempt`, I changed it to be consistent. I tried creating a private method in the controller so that the method could be used as the definition, instead of the same code 3x. But I found doing so broke tests in an off-by-one sort of way - I think somehow the private method held onto the value and was not updated the same way as when it's defined each time.

… for clarity

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>