Skip to content

Fix DocAuthBaseStep throttle target#5354

Merged
zachmargolis merged 5 commits intomainfrom
margolis-fix-target-type
Aug 30, 2021
Merged

Fix DocAuthBaseStep throttle target#5354
zachmargolis merged 5 commits intomainfrom
margolis-fix-target-type

Conversation

@zachmargolis
Copy link
Copy Markdown
Contributor

  • Also add type checking to prevent this error in the future

Spotted by @aduth in #5302 (comment)

- Also add tpye checking to prevent this error in the future
def throttled_else_increment
Throttle.for(
target: user_id,
user: current_user,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

current_user doesn't always exist. See #user_id

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ah right, have to re-implement EffectiveUser

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

PTAL c875a2a

Copy link
Copy Markdown
Contributor

@aduth aduth Aug 30, 2021

Choose a reason for hiding this comment

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

Can we just use EffectiveUser ? Currently trying to sift through difference between flow_session and session.

c875a2a could work, but it'd be nice if we can avoid a DB call for User.find for current user like we do in EffectiveUser.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@flow_session = session

comes from
@flow = flow.new(self, current_session, @name)

which comes from
def current_session
user_session || session
end

So since user_session is sometimes nested, looks like no we can't just blindly use EffectiveUser :[

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

New change: 1763f5e

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So since user_session is sometimes nested, looks like no we can't just blindly use EffectiveUser :[

Ah, okay. That makes more sense. Is it reasonable that EffectiveUser could also try to pick from user_session?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If we really wanted to re-use the EffectiveUser mixin I bet we could modify it like this?

   def effective_user_id
     [
+      respond_to?(:flow_session) && flow_session[:doc_capture_user_id],
       session[:doc_capture_user_id],
       current_user&.id,
     ].find(&:present?)
   end

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we really wanted to re-use the EffectiveUser mixin I bet we could modify it like this?

Yeah, I think the idea with the mixin is to try a few potential places to grab the "current" user. Your suggestion is a bit more involved than I'd expected (vs. user_session[:doc_capture_user_id] I'd hoped it was). I'd be fine to just leave things as-is.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

one issue is that the steps/flow/world is very controller-like but are not actually controllers :[

@zachmargolis zachmargolis requested a review from aduth August 30, 2021 18:55
Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov>
@zachmargolis zachmargolis merged commit f0e850d into main Aug 30, 2021
@zachmargolis zachmargolis deleted the margolis-fix-target-type branch August 30, 2021 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants