Remote repository: Keep in sync when repos are moved to another org/users

readthedocs/oauth/services/github.py

@@ -100,37 +100,26 @@ def create_repository(self, fields, privacy=None, organization=None):
         ]):
 
             repo, _ = RemoteRepository.objects.get_or_create(
-                remote_id=fields['id'],
-                vcs_provider=self.vcs_provider_slug
-            )
-            remote_repository_relation = repo.get_remote_repository_relation(
-                self.user, self.account
+                remote_id=str(fields["id"]),
+                vcs_provider=self.vcs_provider_slug,
             )
 
-            # It's possible that a user has access to a repository from within
-            # an organization without being member of that organization
-            # (external contributor). In this case, the repository will be
-            # listed under the ``/repos`` endpoint but not under ``/orgs``
-            # endpoint. Then, when calling this method (``create_repository``)
-            # we will have ``organization=None`` but we don't have to skip the
-            # creation of the ``RemoteRepositoryRelation``.
-            if repo.organization and organization and repo.organization != organization:
-                log.debug(
-                    'Not importing repository because mismatched orgs.',
-                    repository=fields['name'],
-                )
-                return None
-
-            if any([
-                # There is an organization associated with this repository:
-                # attach the organization to the repository
-                organization is not None,
-                # There is no organization and the repository belongs to a
-                # user: removes the organization linked to the repository
-                not organization and fields['owner']['type'] == 'User',
-            ]):
+            owner_type = fields["owner"]["type"]
+
+            # If there is an organization associated with this repository,
+            # attach the organization to the repository.
+            if (
+                organization
+                and owner_type == "Organization"
+                and organization.remote_id == str(fields["owner"]["id"])
+            ):
                 repo.organization = organization
 
+            # If there is no organization and the repository belongs to a user,
+            # remove the organization linked to the repository.
+            if not organization and owner_type == "User":
+                repo.organization = None
+
             repo.name = fields['name']
             repo.full_name = fields['full_name']
             repo.description = fields['description']
@@ -151,7 +140,12 @@ def create_repository(self, fields, privacy=None, organization=None):
 
             repo.save()
 
-            remote_repository_relation.admin = fields.get('permissions', {}).get('admin', False)
+            remote_repository_relation = repo.get_remote_repository_relation(
+                self.user, self.account
+            )
+            remote_repository_relation.admin = fields.get("permissions", {}).get(
+                "admin", False
+            )
             remote_repository_relation.save()
 
             return repo

readthedocs/rtd_tests/tests/test_oauth.py

@@ -13,11 +13,7 @@
 from readthedocs.integrations.models import GitHubWebhook, GitLabWebhook
 from readthedocs.oauth.constants import BITBUCKET, GITHUB, GITLAB
 from readthedocs.oauth.models import RemoteOrganization, RemoteRepository
-from readthedocs.oauth.services import (
-    BitbucketService,
-    GitHubService,
-    GitLabService,
-)
+from readthedocs.oauth.services import BitbucketService, GitHubService, GitLabService
 from readthedocs.projects import constants
 from readthedocs.projects.models import Project
 
@@ -29,8 +25,8 @@ class GitHubOAuthTests(TestCase):
     def setUp(self):
         self.client.login(username='eric', password='test')
         self.user = User.objects.get(pk=1)
-        self.project = Project.objects.get(slug='pip')
-        self.org = RemoteOrganization.objects.create(slug='rtfd')
+        self.project = Project.objects.get(slug="pip")
+        self.org = RemoteOrganization.objects.create(slug="rtfd", remote_id="1234")
         self.privacy = settings.DEFAULT_PRIVACY_LEVEL
         self.service = GitHubService(
             user=self.user,
@@ -55,21 +51,55 @@ def setUp(self):
                 "url": "https://api.github.com/repos/test/Hello-World/hooks/12345678",
             }
         ]
-
-    def test_make_project_pass(self):
-        repo_json = {
-            'name': 'testrepo',
-            'full_name': 'testuser/testrepo',
-            'id': '12345678',
-            'description': 'Test Repo',
-            'git_url': 'git://github.com/testuser/testrepo.git',
-            'private': False,
-            'ssh_url': 'ssh://git@github.com:testuser/testrepo.git',
-            'html_url': 'https://github.com/testuser/testrepo',
-            'clone_url': 'https://github.com/testuser/testrepo.git',
+        self.repo_response_data = {
+            "name": "testrepo",
+            "full_name": "testuser/testrepo",
+            "id": 12345678,
+            "description": "Test Repo",
+            "git_url": "git://github.com/testuser/testrepo.git",
+            "private": False,
+            "ssh_url": "ssh://git@github.com:testuser/testrepo.git",
+            "html_url": "https://github.com/testuser/testrepo",
+            "clone_url": "https://github.com/testuser/testrepo.git",
+            "owner": {
+                "type": "User",
+                "id": 1234,
+            },
         }
+
+    def test_create_remote_repository(self):
+        repo = self.service.create_repository(
+            self.repo_response_data,
+            privacy=self.privacy,
+        )
+        self.assertIsInstance(repo, RemoteRepository)
+        self.assertEqual(repo.name, "testrepo")
+        self.assertEqual(repo.full_name, "testuser/testrepo")
+        self.assertEqual(repo.remote_id, "12345678")
+        self.assertEqual(repo.vcs_provider, GITHUB)
+        self.assertEqual(repo.description, "Test Repo")
+        self.assertEqual(
+            repo.avatar_url,
+            settings.OAUTH_AVATAR_USER_DEFAULT_URL,
+        )
+        self.assertIn(self.user, repo.users.all())
+        self.assertEqual(repo.organization, None)
+        self.assertEqual(
+            repo.clone_url,
+            "https://github.com/testuser/testrepo.git",
+        )
+        self.assertEqual(
+            repo.ssh_url,
+            "ssh://git@github.com:testuser/testrepo.git",
+        )
+        self.assertEqual(repo.html_url, "https://github.com/testuser/testrepo")
+
+    def test_create_remote_repository_with_organization(self):
+        self.repo_response_data["owner"]["type"] = "Organization"
         repo = self.service.create_repository(
-            repo_json, organization=self.org, privacy=self.privacy,
+            self.repo_response_data,
+            organization=self.org,
+            privacy=self.privacy,
         )
         self.assertIsInstance(repo, RemoteRepository)
         self.assertEqual(repo.name, 'testrepo')
@@ -91,23 +121,103 @@ def test_make_project_pass(self):
         )
         self.assertEqual(repo.html_url, 'https://github.com/testuser/testrepo')
 
-    def test_make_project_fail(self):
-        repo_json = {
-            'name': '',
-            'full_name': '',
-            'id': '',
-            'description': '',
-            'git_url': '',
-            'private': True,
-            'ssh_url': '',
-            'html_url': '',
-            'clone_url': '',
-        }
+    def test_skip_creation_remote_repository_on_private_repos(self):
+        self.repo_response_data["private"] = True
         github_project = self.service.create_repository(
-            repo_json, organization=self.org, privacy=self.privacy,
+            self.repo_response_data,
+            organization=self.org,
+            privacy=self.privacy,
         )
         self.assertIsNone(github_project)
 
+    def test_project_was_moved_from_a_personal_account_to_an_organization(self):
+        github_project = self.service.create_repository(
+            self.repo_response_data,
+            privacy=self.privacy,
+        )
+        self.assertEqual(github_project.organization, None)
+
+        # Project belongs has been moved to an organization.
+        self.repo_response_data["owner"]["type"] = "Organization"
+
+        self.service.create_repository(
+            self.repo_response_data,
+            privacy=self.privacy,
+        )
+        github_project.refresh_from_db()
+        self.assertEqual(github_project.organization, None)
+
+        self.service.create_repository(
+            self.repo_response_data,
+            organization=self.org,
+            privacy=self.privacy,
+        )
+        github_project.refresh_from_db()
+        self.assertEqual(github_project.organization, self.org)
+
+    def test_project_was_moved_from_an_organization_to_a_personal_account(self):
+        # Project belongs to an organization.
+        self.repo_response_data["owner"]["type"] = "Organization"
+        github_project = self.service.create_repository(
+            self.repo_response_data,
+            organization=self.org,
+            privacy=self.privacy,
+        )
+        self.assertEqual(github_project.organization, self.org)
+
+        # Project belongs has been moved to a personal account.
+        self.repo_response_data["owner"]["type"] = "User"
+
+        # This operation doesn't have any effect.
+        self.service.create_repository(
+            self.repo_response_data,
+            organization=self.org,
+            privacy=self.privacy,
+        )
+        github_project.refresh_from_db()
+        self.assertEqual(github_project.organization, self.org)
+
+        self.service.create_repository(
+            self.repo_response_data,
+            privacy=self.privacy,
+        )
+        github_project.refresh_from_db()
+        self.assertEqual(github_project.organization, None)
+
+    def test_project_was_moved_to_another_organization(self):
+        another_remote_organization = RemoteOrganization.objects.create(
+            slug="another", remote_id="4321"
+        )
+
+        # Project belongs to an organization.
+        self.repo_response_data["owner"]["type"] = "Organization"
+        github_project = self.service.create_repository(
+            self.repo_response_data,
+            organization=self.org,
+            privacy=self.privacy,
+        )
+        self.assertEqual(github_project.organization, self.org)
+
+        # Project was moved to another organization.
+        self.repo_response_data["owner"]["id"] = 4321
+
+        self.service.create_repository(
+            self.repo_response_data,
+            organization=another_remote_organization,
+            privacy=self.privacy,
+        )
+        github_project.refresh_from_db()
+        self.assertEqual(github_project.organization, another_remote_organization)
+
+        # The remote ID doesn't match, the project isn't moved to the organization.
+        self.service.create_repository(
+            self.repo_response_data,
+            organization=self.org,
+            privacy=self.privacy,
+        )
+        github_project.refresh_from_db()
+        self.assertEqual(github_project.organization, another_remote_organization)
+
     def test_make_organization(self):
         org_json = {
             'id': 12345,
@@ -131,20 +241,11 @@ def test_import_with_no_token(self):
         self.assertEqual(services, [])
 
     def test_multiple_users_same_repo(self):
-        repo_json = {
-            'name': '',
-            'full_name': 'testrepo/multiple',
-            'id': '12345678',
-            'description': '',
-            'git_url': '',
-            'private': False,
-            'ssh_url': '',
-            'html_url': '',
-            'clone_url': '',
-        }
-
+        self.repo_response_data["owner"]["type"] = "Organization"
         github_project = self.service.create_repository(
-            repo_json, organization=self.org, privacy=self.privacy,
+            self.repo_response_data,
+            organization=self.org,
+            privacy=self.privacy,
         )
 
         user2 = User.objects.get(pk=2)
@@ -153,31 +254,43 @@ def test_multiple_users_same_repo(self):
             account=get(SocialAccount, user=self.user)
         )
         github_project_2 = service.create_repository(
-            repo_json, organization=self.org, privacy=self.privacy,
+            self.repo_response_data,
+            organization=self.org,
+            privacy=self.privacy,
         )
         self.assertIsInstance(github_project, RemoteRepository)
         self.assertIsInstance(github_project_2, RemoteRepository)
         self.assertEqual(github_project_2, github_project)
 
         github_project_3 = self.service.create_repository(
-            repo_json, organization=self.org, privacy=self.privacy,
+            self.repo_response_data,
+            organization=self.org,
+            privacy=self.privacy,
         )
         github_project_4 = service.create_repository(
-            repo_json, organization=self.org, privacy=self.privacy,
+            self.repo_response_data,
+            organization=self.org,
+            privacy=self.privacy,
         )
         self.assertIsInstance(github_project_3, RemoteRepository)
         self.assertIsInstance(github_project_4, RemoteRepository)
         self.assertEqual(github_project, github_project_3)
         self.assertEqual(github_project_2, github_project_4)
 
         github_project_5 = self.service.create_repository(
-            repo_json, organization=self.org, privacy=self.privacy,
+            self.repo_response_data,
+            organization=self.org,
+            privacy=self.privacy,
         )
         github_project_6 = service.create_repository(
-            repo_json, organization=self.org, privacy=self.privacy,
+            self.repo_response_data,
+            organization=self.org,
+            privacy=self.privacy,
         )
 
+        self.assertIsNotNone(github_project)
         self.assertEqual(github_project, github_project_5)
+        self.assertIsNotNone(github_project_2)
         self.assertEqual(github_project_2, github_project_6)
 
     @mock.patch('readthedocs.oauth.services.github.log')
@@ -232,23 +345,14 @@ def test_send_build_status_value_error(self, session, mock_logger):
         )
 
     @override_settings(DEFAULT_PRIVACY_LEVEL='private')
-    def test_make_private_project(self):
-        """
-        Test ability to import ``public`` repositories under ``private`` level.
-        """
-        repo_json = {
-            'name': 'testrepo',
-            'full_name': 'testuser/testrepo',
-            'id': '12345678',
-            'description': 'Test Repo',
-            'git_url': 'git://github.com/testuser/testrepo.git',
-            'private': False,
-            'ssh_url': 'ssh://git@github.com:testuser/testrepo.git',
-            'html_url': 'https://github.com/testuser/testrepo',
-            'clone_url': 'https://github.com/testuser/testrepo.git',
-        }
-        repo = self.service.create_repository(repo_json, organization=self.org)
-        self.assertIsNotNone(repo)
+    def test_create_public_repo_when_private_projects_are_enabled(self):
+        """Test ability to import ``public`` repositories under ``private`` level."""
+        self.repo_response_data["owner"]["type"] = "Organization"
+        repo = self.service.create_repository(
+            self.repo_response_data, organization=self.org
+        )
+        self.assertEqual(repo.organization, self.org)
+        self.assertEqual(repo.remote_id, str(self.repo_response_data["id"]))
 
     @mock.patch('readthedocs.oauth.services.github.log')
     @mock.patch('readthedocs.oauth.services.github.GitHubService.get_session')