Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(staff): Create initial staff class for _admin mode #61653
feat(staff): Create initial staff class for _admin mode #61653
Changes from all commits
51888f4
bbf4965
09d9343
b811e20
819cc23
f44d24c
a940525
45de444
1cfabf6
aaf55dc
008b4e6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
considering that
Staff
initializer can throw errors, would it be more prudent to have this around a try/except so that we don't have unintended behaviors or errors? Something along the lines of:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After reading the code all the way, I now know
_populate
is at the end supposed to help establish this value before being referenced, but if that's the case, it would be better in the__init__
to establish this value first as a default for safer code. Reading this code from top to bottom, it's not clear that the value gets dynamically added to the instance; I thought "Oh no, we're referencing a value that doesn't exist!" hahaWe have to go 2 levels deep (from populate -> set_logged_in/out) to know this is a class variable we can use in this class, and if something happens in that chain, we could be blowing up here/throwing an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a lot of checks and calculations happening in this function. Can we break those apart? I think the code would be much easier to read, and the flow/logic much more feasible to follow in the mind!
We also seem to be populating some keys and not others if a condition fails. For example, if
idl
is less than current, we will return early and not populate the rest of the keys (exp
). Is that what we expect? Should the rest of the keys still be defaulted so they exist? Could we pull the populating/checking of those keys out to another function so that we could handle that decision more easily?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this conditional supposed to be mutually exclusive? Currently we check for
data
being empty only ifcookie_token
is present. Should we be checking for data presence in general?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: do we have constants for these keys/references?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we name this function to something else? Populate is very generic, I'm not sure what it's supposed to be populating, and if it's okay to rerun this function or add to another private function.
If this is only meant to be run once, could we add some guard rails/checks for that? Or restructure this function so that it returns the values we want to populate and the
__init__
can be responsible for adding them to the class?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to do this logic in this function at this level? Right now,
is_activate
property needs_populate
to finish first so that_is_active
gets added to the class, now_populate
needsis_active
to function correctly. It's creating this cyclic dependency effect, and it would be best to pull that out.