From 0946c3e2acc262cc5a18765a24136a0544900c46 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 20 Feb 2025 14:11:28 -0500 Subject: [PATCH 1/3] Remote repositories: filter by vcs provider when deleting --- readthedocs/oauth/services/base.py | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/readthedocs/oauth/services/base.py b/readthedocs/oauth/services/base.py index 3dc9e39f545..47ead70d1b9 100644 --- a/readthedocs/oauth/services/base.py +++ b/readthedocs/oauth/services/base.py @@ -235,11 +235,12 @@ def sync(self): r.remote_id for r in all_remote_repositories if r is not None ] ( - self.user.remote_repository_relations.exclude( - remote_repository__remote_id__in=repository_remote_ids, + self.user.remote_repository_relations.filter( + account=self.account, remote_repository__vcs_provider=self.vcs_provider_slug, + ).exclude( + remote_repository__remote_id__in=repository_remote_ids, ) - .filter(account=self.account) .delete() ) @@ -248,12 +249,12 @@ def sync(self): o.remote_id for o in remote_organizations if o is not None ] ( - self.user.remote_organization_relations.exclude( - remote_organization__remote_id__in=organization_remote_ids, + self.user.remote_organization_relations.filter( + account=self.account, remote_organization__vcs_provider=self.vcs_provider_slug, - ) - .filter(account=self.account) - .delete() + ).exclude( + remote_organization__remote_id__in=organization_remote_ids, + ).delete() ) def get_next_url_to_paginate(self, response): From 7652fab7ce35096d7ee988af958ebe8e93148bee Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 20 Feb 2025 14:19:54 -0500 Subject: [PATCH 2/3] Linter --- readthedocs/oauth/services/base.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/readthedocs/oauth/services/base.py b/readthedocs/oauth/services/base.py index 47ead70d1b9..5883f0a8bae 100644 --- a/readthedocs/oauth/services/base.py +++ b/readthedocs/oauth/services/base.py @@ -238,7 +238,8 @@ def sync(self): self.user.remote_repository_relations.filter( account=self.account, remote_repository__vcs_provider=self.vcs_provider_slug, - ).exclude( + ) + .exclude( remote_repository__remote_id__in=repository_remote_ids, ) .delete() @@ -252,9 +253,11 @@ def sync(self): self.user.remote_organization_relations.filter( account=self.account, remote_organization__vcs_provider=self.vcs_provider_slug, - ).exclude( + ) + .exclude( remote_organization__remote_id__in=organization_remote_ids, - ).delete() + ) + .delete() ) def get_next_url_to_paginate(self, response): From 1d437dc7bd6a7bdbe01c6c615fa5010fc90073bd Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 20 Mar 2025 17:40:29 -0500 Subject: [PATCH 3/3] Test --- .../rtd_tests/tests/test_oauth_sync.py | 41 +++++++++++++++---- 1 file changed, 34 insertions(+), 7 deletions(-) diff --git a/readthedocs/rtd_tests/tests/test_oauth_sync.py b/readthedocs/rtd_tests/tests/test_oauth_sync.py index 5773f6e6357..b8290d7f3d8 100644 --- a/readthedocs/rtd_tests/tests/test_oauth_sync.py +++ b/readthedocs/rtd_tests/tests/test_oauth_sync.py @@ -1,7 +1,7 @@ import django_dynamic_fixture as fixture import requests_mock from allauth.socialaccount.models import SocialAccount, SocialToken -from allauth.socialaccount.providers.github.views import GitHubOAuth2Adapter +from allauth.socialaccount.providers.github.provider import GitHubProvider from django.contrib.auth.models import User from django.test import TestCase @@ -12,6 +12,8 @@ RemoteRepository, RemoteRepositoryRelation, ) +from django_dynamic_fixture import get +from allauth.socialaccount.providers.gitlab.provider import GitLabProvider from readthedocs.oauth.services import GitHubService from readthedocs.projects.models import Project @@ -64,7 +66,7 @@ def setUp(self): self.socialaccount = fixture.get( SocialAccount, user=self.user, - provider=GitHubOAuth2Adapter.provider_id, + provider=GitHubProvider.id, ) self.token = fixture.get( SocialToken, @@ -122,6 +124,25 @@ def test_sync_delete_stale(self, mock_request): account=self.socialaccount, ) + # Project from another provider (GitLab) that has the same ID should not be removed. + gitlab_socialaccount = fixture.get( + SocialAccount, + user=self.user, + provider=GitLabProvider.id, + ) + gitlab_repo = fixture.get( + RemoteRepository, + full_name="user/repo", + remote_id=repo_3.remote_id, + vcs_provider=GitLabProvider.id, + ) + fixture.get( + RemoteRepositoryRelation, + remote_repository=gitlab_repo, + user=self.user, + account=gitlab_socialaccount, + ) + org = fixture.get( RemoteOrganization, name="organization", @@ -133,18 +154,21 @@ def test_sync_delete_stale(self, mock_request): account=self.socialaccount, ) - self.assertEqual(RemoteRepository.objects.count(), 3) - self.assertEqual(RemoteRepositoryRelation.objects.count(), 3) + self.assertEqual(RemoteRepository.objects.count(), 4) + self.assertEqual(RemoteRepositoryRelation.objects.count(), 4) self.assertEqual(RemoteOrganization.objects.count(), 1) self.assertEqual(RemoteOrganizationRelation.objects.count(), 1) + assert self.socialaccount.remote_repository_relations.count() == 3 + assert self.socialaccount.remote_organization_relations.count() == 1 + self.service.sync() # After calling .sync, old-repository remote relation should be deleted, # project-linked-repository remote relation should be conserved, # and only the one's returned by the API should be present (organization/repository) - self.assertEqual(RemoteRepository.objects.count(), 3) - self.assertEqual(RemoteRepositoryRelation.objects.count(), 1) + self.assertEqual(RemoteRepository.objects.count(), 4) + self.assertEqual(RemoteRepositoryRelation.objects.count(), 2) self.assertTrue( RemoteRepository.objects.filter( full_name="organization/repository" @@ -158,6 +182,9 @@ def test_sync_delete_stale(self, mock_request): self.assertEqual(RemoteOrganization.objects.count(), 1) self.assertEqual(RemoteOrganizationRelation.objects.count(), 0) + assert self.socialaccount.remote_repository_relations.count() == 1 + assert self.socialaccount.remote_organization_relations.count() == 0 + @requests_mock.Mocker(kw="mock_request") def test_sync_repositories(self, mock_request): mock_request.get( @@ -284,7 +311,7 @@ def test_sync_repositories_only_creates_one_remote_repo_per_vcs_repo( user_2_socialaccount = fixture.get( SocialAccount, user=user_2, - provider=GitHubOAuth2Adapter.provider_id, + provider=GitHubProvider.id, ) fixture.get( SocialToken,