Skip to content
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

ref(control_silo): Consolidate User models (and authenticator) and tests #74222

Closed

Conversation

Christinarlong
Copy link
Contributor

Moves all User and authenticator specific models and tests to their own directories.

ref(#73856)

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jul 12, 2024
bin/mock-user Outdated
@@ -8,7 +8,7 @@ import argparse


def main(username, newsletter_consent_prompt=None):
from sentry.models.user import User
from sentry.users.models.users.user import User
Copy link
Member

Choose a reason for hiding this comment

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

Could this be sentry.users.models.user? Or is the additional layer of modules helpful?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure it definitely could go there. Just kinda worried that with all the other models, the models directory could grow p. large though we could do something like:

sentry.users.models
|
|--> auth
         |
         |authenticator.py
         
|--> emails
        
| user.py
| useremail.py

does this feel cleaner/makes more sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nvm, read the rest of the review. 10 is p. few so I'm down to just flatten and put everything in models

src/sentry/models/__init__.py Outdated Show resolved Hide resolved
@@ -1,204 +1,3 @@
from __future__ import annotations
from ..users.models.auth.authenticator import Authenticator # NOQA
Copy link
Member

Choose a reason for hiding this comment

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

Could we do from sentry.users.models.authenticator import Authenticator. The users app will have fewer than 10 models in it and I don't think the additional layer of directories adds much.

src/sentry/models/authenticator.py Outdated Show resolved Hide resolved
src/sentry/models/avatars/__init__.py Outdated Show resolved Hide resolved
src/sentry/models/userrole.py Outdated Show resolved Hide resolved
src/sentry/users/models/__init__.py Outdated Show resolved Hide resolved
Copy link

codecov bot commented Jul 15, 2024

Codecov Report

Attention: Patch coverage is 93.52381% with 68 lines in your changes missing coverage. Please review.

Project coverage is 78.12%. Comparing base (0b202f0) to head (bc38c44).
Report is 981 commits behind head on master.

Files Patch % Lines
src/sentry/users/models/user.py 90.42% 20 Missing and 7 partials ⚠️
src/sentry/users/models/user_option.py 85.45% 11 Missing and 5 partials ⚠️
src/sentry/users/models/userip.py 82.89% 7 Missing and 6 partials ⚠️
src/sentry/users/models/authenticator.py 94.73% 2 Missing and 4 partials ⚠️
src/sentry/users/models/userpermission.py 94.44% 1 Missing and 1 partial ⚠️
src/sentry/models/grouphistory.py 50.00% 1 Missing ⚠️
src/sentry/plugins/bases/notify.py 0.00% 1 Missing ⚠️
src/sentry/users/models/useremail.py 98.88% 1 Missing ⚠️
src/sentry/users/models/userrole.py 98.03% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master   #74222       +/-   ##
===========================================
+ Coverage   64.63%   78.12%   +13.49%     
===========================================
  Files        6662     6673       +11     
  Lines      297751   298048      +297     
  Branches    51252    51292       +40     
===========================================
+ Hits       192457   232862    +40405     
+ Misses      98791    58908    -39883     
+ Partials     6503     6278      -225     
Files Coverage Δ
src/sentry/api/authentication.py 84.98% <100.00%> (+12.96%) ⬆️
src/sentry/api/bases/user.py 85.39% <100.00%> (+19.10%) ⬆️
src/sentry/api/endpoints/auth_index.py 80.30% <100.00%> (+41.66%) ⬆️
src/sentry/api/endpoints/authenticator_index.py 84.00% <100.00%> (+28.00%) ⬆️
src/sentry/api/endpoints/group_ai_autofix.py 71.42% <100.00%> (+29.67%) ⬆️
src/sentry/api/endpoints/group_details.py 85.00% <100.00%> (+8.75%) ⬆️
src/sentry/api/endpoints/group_event_details.py 84.04% <100.00%> (+13.82%) ⬆️
.../sentry/api/endpoints/group_integration_details.py 75.00% <100.00%> (+54.16%) ⬆️
src/sentry/api/endpoints/group_integrations.py 94.00% <100.00%> (+24.00%) ⬆️
...y/api/endpoints/group_similar_issues_embeddings.py 100.00% <100.00%> (+46.55%) ⬆️
... and 220 more

... and 1506 files with indirect coverage changes

@getsantry
Copy link
Contributor

getsantry bot commented Aug 5, 2024

This pull request has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you add the label WIP, I will leave it alone unless WIP is removed ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@getsantry getsantry bot added the Stale label Aug 5, 2024
@Christinarlong
Copy link
Contributor Author

Breaking down into smaller PRs

@github-actions github-actions bot locked and limited conversation to collaborators Aug 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants