-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Remote Repository and Remote Organization Normalization #7949
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
Changes from all commits
307a1cb
7bd1a5f
9476a36
1bd03c7
c3592eb
c6699fb
bf5c6b1
d90ecad
2c74027
9f30059
42a516d
48114e6
421bcae
afc7af0
28c6a18
37d89af
8d126db
dff138e
5d7c56f
c194ac7
76eff0d
77c1777
076fe93
3376795
9f7924d
b375696
c463880
f72a220
cff0bfd
2df6a4e
e714315
bf8a167
69df3d8
e1f4ac7
570e064
ac0c863
8354b66
41bebc9
7230e01
1d48b93
8df0032
bddaca2
3575d5c
8b89483
15ddb3d
42e1d52
ef99b33
6c0b22d
56682ae
eb4c625
5c1fd68
3e97c21
fde005b
018dc6b
bae9166
9b8e829
39cd6fa
b8d3c09
2c830e5
7950ca5
e748df6
c0a9a16
0c04f9f
feeb903
53d8f75
3ec81be
d6c5948
7b64f37
58973c9
ab7e093
e36719e
88d896d
629f355
055138a
578008f
233ac8e
9d41148
53d705b
7726375
89960c3
be8282a
8dcdd42
3489bca
e9619cd
f275bbb
097ba39
705cded
1e5dd5e
6b2f38a
6f7eefd
dc40044
265cde0
ec7729c
a718e78
cea826b
29c437e
920db54
d7a0445
76604af
349ba5a
36dd7fe
12e6f64
5317c80
99cbb6f
a449fd8
08a4641
17621bc
7fe6116
1eea07f
f5e01a6
53a1b95
20549f5
73c0447
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -86,7 +86,7 @@ def decide_if_cors(sender, request, **kwargs): # pylint: disable=unused-argumen | |
|
||
|
||
@receiver(pre_delete, sender=settings.AUTH_USER_MODEL) | ||
def delete_projects_and_organizations(sender, instance, *args, **kwargs): | ||
def delete_projects(sender, instance, *args, **kwargs): | ||
# Here we count the owner list from the projects that the user own | ||
# Then exclude the projects where there are more than one owner | ||
# Add annotate before filter | ||
|
@@ -98,16 +98,7 @@ def delete_projects_and_organizations(sender, instance, *args, **kwargs): | |
).exclude(num_users__gt=1) | ||
) | ||
|
||
# Here we count the users list from the organization that the user belong | ||
# Then exclude the organizations where there are more than one user | ||
oauth_organizations = ( | ||
RemoteOrganization.objects.annotate(num_users=Count('users') | ||
).filter(users=instance.id | ||
).exclude(num_users__gt=1) | ||
) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why did we remove this code? Do we run it somewhere else? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are not deleting There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we want, we could do a cleanup of organizations/repositories at some point by deleting those that has 0 users connected. I don't think we need this now, tho. |
||
|
||
projects.delete() | ||
oauth_organizations.delete() | ||
|
||
|
||
signals.check_request_enabled.connect(decide_if_cors) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,52 +1,50 @@ | ||
# -*- coding: utf-8 -*- | ||
import django_dynamic_fixture | ||
import pytest | ||
|
||
from django.contrib.auth.models import User | ||
|
||
from readthedocs.oauth.models import RemoteOrganization | ||
from readthedocs.projects.models import Project | ||
|
||
|
||
@pytest.mark.django_db | ||
class TestProjectOrganizationSignal: | ||
|
||
@pytest.mark.parametrize('model_class', [Project, RemoteOrganization]) | ||
def test_project_organization_get_deleted_upon_user_delete(self, model_class): | ||
"""If the user has Project or RemoteOrganization where he is the only | ||
user, upon deleting his account, the Project or RemoteOrganization | ||
def test_project_get_deleted_upon_user_delete(self): | ||
"""If the user has Project where he is the only | ||
user, upon deleting his account, the Project | ||
should also get deleted.""" | ||
|
||
obj = django_dynamic_fixture.get(model_class) | ||
project = django_dynamic_fixture.get(Project) | ||
user1 = django_dynamic_fixture.get(User) | ||
obj.users.add(user1) | ||
project.users.add(user1) | ||
|
||
obj.refresh_from_db() | ||
assert obj.users.all().count() == 1 | ||
project.refresh_from_db() | ||
assert project.users.all().count() == 1 | ||
|
||
# Delete the user | ||
user1.delete() | ||
# The object should not exist | ||
obj = model_class.objects.all().filter(id=obj.id) | ||
assert not obj.exists() | ||
project = Project.objects.all().filter(id=project.id) | ||
assert not project.exists() | ||
|
||
@pytest.mark.parametrize('model_class', [Project, RemoteOrganization]) | ||
def test_multiple_users_project_organization_not_delete(self, model_class): | ||
"""Check Project or RemoteOrganization which have multiple users do not | ||
def test_multiple_users_project_not_delete(self): | ||
"""Check Project which have multiple users do not | ||
get deleted when any of the user delete his account.""" | ||
|
||
obj = django_dynamic_fixture.get(model_class) | ||
project = django_dynamic_fixture.get(Project) | ||
user1 = django_dynamic_fixture.get(User) | ||
user2 = django_dynamic_fixture.get(User) | ||
obj.users.add(user1, user2) | ||
project.users.add(user1, user2) | ||
|
||
obj.refresh_from_db() | ||
assert obj.users.all().count() > 1 | ||
project.refresh_from_db() | ||
assert project.users.all().count() > 1 | ||
# Delete 1 user of the project | ||
user1.delete() | ||
|
||
# The project should still exist and it should have 1 user | ||
obj.refresh_from_db() | ||
assert obj.id | ||
obj_users = obj.users.all() | ||
project.refresh_from_db() | ||
assert project.id | ||
obj_users = project.users.all() | ||
assert len(obj_users) == 1 | ||
assert user2 in obj_users |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
GITHUB = 'github' | ||
GITLAB = 'gitlab' | ||
BITBUCKET = 'bitbucket' | ||
|
||
VCS_PROVIDER_CHOICES = ( | ||
(GITHUB, 'GitHub'), | ||
(GITLAB, 'GitLab'), | ||
(BITBUCKET, 'Bitbucket'), | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,60 @@ | ||
import json | ||
|
||
from django.core.management.base import BaseCommand | ||
|
||
from readthedocs.oauth.models import RemoteRepository | ||
|
||
|
||
class Command(BaseCommand): | ||
help = "Load Project and RemoteRepository Relationship from JSON file" | ||
|
||
def add_arguments(self, parser): | ||
# File path of the json file containing relationship data | ||
parser.add_argument( | ||
'--file', | ||
required=True, | ||
nargs=1, | ||
type=str, | ||
help='File path of the json file containing relationship data.', | ||
) | ||
|
||
def handle(self, *args, **options): | ||
file = options.get('file')[0] | ||
|
||
try: | ||
# Load data from the json file | ||
with open(file, 'r') as f: | ||
data = json.load(f) | ||
except Exception as e: | ||
self.stdout.write( | ||
self.style.ERROR( | ||
f'Exception occurred while trying to load the file "{file}". ' | ||
f'Exception: {e}.' | ||
) | ||
) | ||
return | ||
|
||
for item in data: | ||
try: | ||
update_count = RemoteRepository.objects.filter( | ||
remote_id=item['remote_id'] | ||
).update(project_id=item['project_id']) | ||
|
||
if update_count < 1: | ||
self.stdout.write( | ||
self.style.ERROR( | ||
f"Could not update {item['slug']}'s " | ||
f"relationship with {item['html_url']}, " | ||
f"remote_id {item['remote_id']}, " | ||
f"username: {item['username']}." | ||
) | ||
) | ||
|
||
except Exception as e: | ||
self.stdout.write( | ||
self.style.ERROR( | ||
f"Exception occurred while trying to update {item['slug']}'s " | ||
f"relationship with {item['html_url']}, " | ||
f"username: {item['username']}, Exception: {e}." | ||
) | ||
) |
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.
What is
own
here? Could probably use a better nameThere 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.
I'm not sure why it's called
own
here, I think it refers to the VCS provider names ('github', 'gitlab' etc). We can change the variable name but the quey param name is for the API V2There 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.
We could probably make a note on this for the APIv3 where these endpoints are not created yet: #7510