Merge pull request readthedocs#6275 from saadmk11/subproject-bug

API V3 Subproject Creation Bug fix

Author: humitos

readthedocs/api/v3/serializers.py

@@ -569,7 +569,19 @@ def validate_child(self, value):
         user = self.context['request'].user
         if user not in value.users.all():
             raise serializers.ValidationError(
-                'You do not have permissions on the child project',
+                _('You do not have permissions on the child project'),
+            )
+
+        # Check the child project is not a subproject already
+        if value.superprojects.exists():
+            raise serializers.ValidationError(
+                _('Child is already a subproject of another project'),
+            )
+
+        # Check the child project is already a superproject
+        if value.subprojects.exists():
+            raise serializers.ValidationError(
+                _('Child is already a superproject'),
             )
         return value
 
@@ -578,7 +590,7 @@ def validate_alias(self, value):
         subproject = self.parent_project.subprojects.filter(alias=value)
         if subproject.exists():
             raise serializers.ValidationError(
-                'A subproject with this alias already exists',
+                _('A subproject with this alias already exists'),
             )
         return value
 
@@ -587,13 +599,13 @@ def validate(self, data):
         # Check the parent and child are not the same project
         if data['child'].slug == self.parent_project.slug:
             raise serializers.ValidationError(
-                'Project can not be subproject of itself',
+                _('Project can not be subproject of itself'),
             )
 
         # Check the parent project is not a subproject already
         if self.parent_project.superprojects.exists():
             raise serializers.ValidationError(
-                'Subproject nesting is not supported',
+                _('Subproject nesting is not supported'),
             )
         return data
 

readthedocs/api/v3/tests/test_subprojects.py

@@ -89,17 +89,18 @@ def test_projects_subprojects_list_post_with_others_as_child(self):
         self.assertEqual(self.project.subprojects.count(), 1)
 
     def test_projects_subprojects_list_post_with_subproject_of_itself(self):
-        self.assertEqual(self.project.subprojects.count(), 1)
+        newproject = self._create_new_project()
+        self.assertEqual(newproject.subprojects.count(), 0)
         self.client.credentials(HTTP_AUTHORIZATION=f'Token {self.token.key}')
         data = {
-            'child': self.project.slug,
+            'child': newproject.slug,
             'alias': 'subproject-alias',
         }
         response = self.client.post(
             reverse(
                 'projects-subprojects-list',
                 kwargs={
-                    'parent_lookup_parent__slug': self.project.slug,
+                    'parent_lookup_parent__slug': newproject.slug,
                 },
             ),
             data,
@@ -109,7 +110,57 @@ def test_projects_subprojects_list_post_with_subproject_of_itself(self):
             'Project can not be subproject of itself',
             response.json()['non_field_errors'],
         )
-        self.assertEqual(self.project.subprojects.count(), 1)
+        self.assertEqual(newproject.subprojects.count(), 0)
+
+    def test_projects_subprojects_list_post_with_child_already_superproject(self):
+        newproject = self._create_new_project()
+        self.assertEqual(newproject.subprojects.count(), 0)
+        self.assertTrue(self.project.subprojects.exists())
+        self.client.credentials(HTTP_AUTHORIZATION=f'Token {self.token.key}')
+        data = {
+            'child': self.project.slug,
+            'alias': 'subproject-alias',
+        }
+        response = self.client.post(
+            reverse(
+                'projects-subprojects-list',
+                kwargs={
+                    'parent_lookup_parent__slug': newproject.slug,
+                },
+            ),
+            data,
+        )
+        self.assertEqual(response.status_code, 400)
+        self.assertIn(
+            'Child is already a superproject',
+            response.json()['child'],
+        )
+        self.assertEqual(newproject.subprojects.count(), 0)
+
+    def test_projects_subprojects_list_post_with_child_already_subproject(self):
+        newproject = self._create_new_project()
+        self.assertEqual(newproject.subprojects.count(), 0)
+        self.assertTrue(self.subproject.superprojects.exists())
+        self.client.credentials(HTTP_AUTHORIZATION=f'Token {self.token.key}')
+        data = {
+            'child': self.subproject.slug,
+            'alias': 'subproject-alias',
+        }
+        response = self.client.post(
+            reverse(
+                'projects-subprojects-list',
+                kwargs={
+                    'parent_lookup_parent__slug': newproject.slug,
+                },
+            ),
+            data,
+        )
+        self.assertEqual(response.status_code, 400)
+        self.assertIn(
+            'Child is already a subproject of another project',
+            response.json()['child'],
+        )
+        self.assertEqual(newproject.subprojects.count(), 0)
 
     def test_projects_subprojects_list_post_nested_subproject(self):
         newproject = self._create_new_project()

readthedocs/rtd_tests/tests/test_subprojects.py

@@ -150,6 +150,57 @@ def test_excludes_existing_subprojects(self):
             [''],
         )
 
+    def test_subproject_cant_be_subproject(self):
+        user = fixture.get(User)
+        project = fixture.get(Project, users=[user])
+        another_project = fixture.get(Project, users=[user])
+        subproject = fixture.get(Project, users=[user])
+        relation = fixture.get(
+            ProjectRelationship, parent=project, child=subproject,
+        )
+
+        form = ProjectRelationshipForm(
+            {'child': subproject.pk},
+            project=project,
+            user=user,
+        )
+        self.assertFalse(form.is_valid())
+        self.assertRegex(
+            form.errors['child'][0],
+            'Select a valid choice',
+        )
+
+        form = ProjectRelationshipForm(
+            {'child': subproject.pk},
+            project=another_project,
+            user=user,
+        )
+        self.assertFalse(form.is_valid())
+        self.assertRegex(
+            form.errors['child'][0],
+            'Select a valid choice',
+        )
+
+    def test_superproject_cant_be_subproject(self):
+        user = fixture.get(User)
+        project = fixture.get(Project, users=[user])
+        another_project = fixture.get(Project, users=[user])
+        subproject = fixture.get(Project, users=[user])
+        relation = fixture.get(
+            ProjectRelationship, parent=project, child=subproject,
+        )
+
+        form = ProjectRelationshipForm(
+            {'child': project.pk},
+            project=another_project,
+            user=user,
+        )
+        self.assertFalse(form.is_valid())
+        self.assertRegex(
+            form.errors['child'][0],
+            'Select a valid choice',
+        )
+
     def test_exclude_self_project_as_subproject(self):
         user = fixture.get(User)
         project = fixture.get(Project, users=[user])