Expand throttle to include string targets#5302
Conversation
- Also combine separate services into methods on the AR model, via shared .for method
zachmargolis
requested review from
aduth,
jmhooper,
mitchellhenke and
stevegsa
app/models/throttle.rb
Outdated
| # @param [User,Integer,String] target User, its ID, or a string identifier | ||
| # @param [Symbol] throttle_type | ||
| # @return [Throttle] | ||
| def self.for(target:, throttle_type:) | ||
| throttle = case target | ||
| when User | ||
| find_or_create_by(user: target, throttle_type: throttle_type) | ||
| when Integer | ||
| find_or_create_by(user_id: target, throttle_type: throttle_type) | ||
| when String | ||
| find_or_create_by(target: target, throttle_type: throttle_type) | ||
| else | ||
| raise "Unknown throttle target class=#{target.class}" | ||
| end |
There was a problem hiding this comment.
To keep the old style code, I would have had to update our old Throttler::FindOrCreate and all the signatures of those separate service classes....or just move things onto the model where they can be instance methods and keep a lot of context together
That choice pushed me to just do the big refactor
| !expired? && maxed? | ||
| end | ||
|
|
||
| def throttled_else_increment? |
There was a problem hiding this comment.
Open to new name suggestions here! It's not a normal predicate method because it has a side effect. Maybe just drop the ? at the end? 😬
aduth
left a comment
aduth
left a comment
There was a problem hiding this comment.
This feels like a nice improvement over the services 👍
I didn't know #5301 would also touch the Throttle model, but I think these changes won't have any problems combining
Nope, I think I'll have some updates to make, but not fundamentally conflicting.
|
|
||
| def throttled? | ||
| Throttler::IsThrottled.call(document_capture_session.user_id, :idv_acuant) | ||
| return unless document_capture_session.user |
There was a problem hiding this comment.
Should it return a boolean? Is it necessary at all, given that I can't see that the previous logic would have guarded for this?
There was a problem hiding this comment.
I think we can get rid of the guard... 45c436f
app/models/throttle.rb
Outdated
| return self if maxed? | ||
| update(attempts: attempts + 1, attempted_at: Time.zone.now) | ||
| self |
There was a problem hiding this comment.
Kinda curious about these returning the instance itself. It seems to have had existed previously, though unclear if we want to change that as part of the refactoring. We already are in the case of throttled_else_increment? which previously returned the throttle instance if it was throttled (now returns true).
There was a problem hiding this comment.
Yeah I kept this to match the old behavior, but I have no idea what an expected return value for increment would be... in other languages I'd make this void or return the count... I guess I'll have it return the count
Co-authored-by: Andrew Duthie <aduth@users.noreply.github.com>
- Also add memoization Co-authored-by: Andrew Duthie <aduth@users.noreply.github.com>
| def throttled_else_increment | ||
| Throttler::IsThrottledElseIncrement.call(user_id, :idv_acuant) | ||
| Throttle.for( | ||
| target: user_id, |
There was a problem hiding this comment.
Should this have been... ?
| target: user_id, | |
| user: user_id, |
There was a problem hiding this comment.
🤦 I think it should have been:
| target: user_id, | |
| user: current_user, |
but yes
3 participants
shared .for method
This was motivated by LG-4942 where we'll want to be able to throttle IDV attempts by SSN, which can be independent from users
I didn't know #5301 would also touch the Throttle model, but I think these changes won't have any problems combining