chore(control_silo): Move User model to new home in users package #75597
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Moves the User model to new home in sentry/users/models/ and tests to tests/sentry/users/models ref(issue # here)
github-actions
bot
added
the
Scope: Backend
Christinarlong
commented
Aug 5, 2024
There was a problem hiding this comment.
Added a lot of return & parameter typing for this file, pls double check if the types are wrong. I'll point out some specific ones I'm unsure on
src/sentry/users/models/user.py
Outdated
| # sessions and refresh their nonce. | ||
| @receiver(user_logged_out, sender=User) | ||
| def refresh_user_nonce( | ||
| sender: User | RpcUser, request: HttpRequest, user: User | None, **kwargs: Any |
There was a problem hiding this comment.
unsure if request can be None too ?
There was a problem hiding this comment.
All of the current callers provide the request, but signals make no promises on providing all of their parameters. Annotating this as HttpRequest | None seems wise.
|
Test Failures Detected: Due to failing tests, we cannot provide coverage reports at this time. ❌ Failed Test Results:Completed 21745 tests with View the full list of failed testspytest
|
batch.count shouldn't be able to go negative. This can happen when a batch is complete, and has no additional data so low=`max id` and up=0. This edge case came up in testing as our batch sizes are 1, and other tests created enough rows to cause the sequence state to get high enough that the incorrect batch.count resulted in backfills aborting early. I've also added a `__str__` to make future debugging easier.
Christinarlong
requested review from
AbhiPrasad and
s1gr1d
and removed request for
a team
markstory
approved these changes
Aug 13, 2024
src/sentry/users/models/user.py
Outdated
| # sessions and refresh their nonce. | ||
| @receiver(user_logged_out, sender=User) | ||
| def refresh_user_nonce( | ||
| sender: User | RpcUser, request: HttpRequest, user: User | None, **kwargs: Any |
There was a problem hiding this comment.
All of the current callers provide the request, but signals make no promises on providing all of their parameters. Annotating this as HttpRequest | None seems wise.
src/sentry/users/models/user.py
Outdated
Comment on lines
597
to
598
| def refresh_api_user_nonce( | ||
| sender: RpcUser, request: HttpRequest, user: User | None, **kwargs: Any |
There was a problem hiding this comment.
Same here, I would assume that all parameters could be None.
Christinarlong
deleted the
christinarlong/consolidate-users-models-user
branch
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
ameliahsu
pushed a commit
that referenced
this pull request
Aug 13, 2024
…5597) Part of consolidating (user) control silo resources to be located with other user related models. Leaves behind a shim for any getsentry references. Also includes a fix for backfill_outboxes to ensure batch.count doesn't become negative which caused tests in test_backfill_outboxes to fail. Ref(#73856) --------- Co-authored-by: Mark Story <[email protected]>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Part of consolidating (user) control silo resources to be located with other user related models.
Ref(#73856)