Remote repositories: filter by vcs provider when deleting (#12015)

This wasn't a problem since we are filtering by account, and the account
can only have one type of vcs repositories, but just for consistency.

Author: stsewd

readthedocs/oauth/services/base.py

@@ -254,22 +254,26 @@ def sync(self):
         all_remote_repositories = remote_repositories + remote_repositories_organizations
         repository_remote_ids = [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,
             )
-            .filter(account=self.account)
+            .exclude(
+                remote_repository__remote_id__in=repository_remote_ids,
+            )
             .delete()
         )
 
         # Delete RemoteOrganization where the user doesn't have access anymore
         organization_remote_ids = [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)
+            .exclude(
+                remote_organization__remote_id__in=organization_remote_ids,
+            )
             .delete()
         )
 

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,