Repository: readthedocs/readthedocs.org
Issue: Versions aren't deleted if there is one with the same commit

If we create two versions pointing to the same commit (say, master and a tag on master). If we delete the tag, when syncing the repositories that tag isn't deleted from the versions list (because master has the same commit).

This is also seeing when creating two tags that point to the same commit.

Changed files:
readthedocs/restapi/utils.py, readthedocs/rtd_tests/tests/test_sync_versions.py

diff --git a/readthedocs/restapi/utils.py b/readthedocs/restapi/utils.py
index 0c91de816..8637cd177 100644
--- a/readthedocs/restapi/utils.py
+++ b/readthedocs/restapi/utils.py
@@ -2,16 +2,26 @@
 """Utility functions that are used by both views and celery tasks."""
 
 from __future__ import (
-    absolute_import, division, print_function, unicode_literals)
+    absolute_import,
+    division,
+    print_function,
+    unicode_literals,
+)
 
 import hashlib
 import logging
 
 from rest_framework.pagination import PageNumberPagination
 
-from readthedocs.builds.constants import (LATEST, LATEST_VERBOSE_NAME,
-                                          NON_REPOSITORY_VERSIONS, STABLE,
-                                          STABLE_VERBOSE_NAME)
+from readthedocs.builds.constants import (
+    BRANCH,
+    LATEST,
+    LATEST_VERBOSE_NAME,
+    NON_REPOSITORY_VERSIONS,
+    STABLE,
+    STABLE_VERBOSE_NAME,
+    TAG,
+)
 from readthedocs.builds.models import Version
 from readthedocs.search.indexes import PageIndex, ProjectIndex, SectionIndex
 
@@ -133,15 +143,25 @@ def set_or_create_version(project, slug, version_id, verbose_name, type_):
 
 def delete_versions(project, version_data):
     """Delete all versions not in the current repo."""
-    current_versions = []
-    if 'tags' in version_data:
-        for version in version_data['tags']:
-            current_versions.append(version['identifier'])
-    if 'branches' in version_data:
-        for version in version_data['branches']:
-            current_versions.append(version['identifier'])
+    # We use verbose_name for tags
+    # because several tags can point to the same identifier.
+    versions_tags = [
+        version['verbose_name']
+        for version in version_data.get('tags', [])
+    ]
+    versions_branches = [
+        version['identifier']
+        for version in version_data.get('branches', [])
+    ]
     to_delete_qs = project.versions.all()
-    to_delete_qs = to_delete_qs.exclude(identifier__in=current_versions)
+    to_delete_qs = to_delete_qs.exclude(
+        type=TAG,
+        verbose_name__in=versions_tags,
+    )
+    to_delete_qs = to_delete_qs.exclude(
+        type=BRANCH,
+        identifier__in=versions_branches,
+    )
     to_delete_qs = to_delete_qs.exclude(uploaded=True)
     to_delete_qs = to_delete_qs.exclude(active=True)
     to_delete_qs = to_delete_qs.exclude(slug__in=NON_REPOSITORY_VERSIONS)
diff --git a/readthedocs/rtd_tests/tests/test_sync_versions.py b/readthedocs/rtd_tests/tests/test_sync_versions.py
index 7612ec494..9b7516015 100644
--- a/readthedocs/rtd_tests/tests/test_sync_versions.py
+++ b/readthedocs/rtd_tests/tests/test_sync_versions.py
@@ -105,12 +105,13 @@ class TestSyncVersions(TestCase):
             self.pip.get_stable_version().identifier,
         )
 
-    def test_new_tag_update_inactive(self):
+    def test_new_tag_dont_update_inactive(self):
 
         Version.objects.create(
             project=self.pip,
             identifier='0.8.3',
             verbose_name='0.8.3',
+            type=TAG,
             active=False,
         )
 
@@ -142,13 +143,13 @@ class TestSyncVersions(TestCase):
             data=json.dumps(version_post_data),
             content_type='application/json',
         )
-        # Version 0.9 becomes the stable version and active
-        version_9 = Version.objects.get(slug='0.9')
+        # Version 0.9 becomes the stable version, but it's inactive
+        version_9 = self.pip.versions.get(slug='0.9')
         self.assertEqual(
             version_9.identifier,
             self.pip.get_stable_version().identifier,
         )
-        self.assertTrue(version_9.active)
+        self.assertFalse(version_9.active)
 
         # Version 0.8.3 is still inactive
         version_8 = Version.objects.get(slug='0.8.3')
@@ -651,6 +652,97 @@ class TestSyncVersions(TestCase):
         )
         self.assertTrue(version_latest.machine)
 
+    def test_deletes_version_with_same_identifier(self):
+        version_post_data = {
+            'branches': [
+                {
+                    'identifier': 'origin/master',
+                    'verbose_name': 'master',
+                },
+            ],
+            'tags': [
+                {
+                    'identifier': '1234',
+                    'verbose_name': 'one',
+                },
+            ],
+        }
+
+        resp = self.client.post(
+            reverse('project-sync-versions', args=[self.pip.pk]),
+            data=json.dumps(version_post_data),
+            content_type='application/json',
+        )
+        self.assertEqual(resp.status_code, 200)
+
+        # We only have one version with an identifier `1234`
+        self.assertEqual(
+            self.pip.versions.filter(identifier='1234').count(),
+            1
+        )
+
+        # We add a new tag with the same identifier
+        version_post_data = {
+            'branches': [
+                {
+                    'identifier': 'origin/master',
+                    'verbose_name': 'master',
+                },
+            ],
+            'tags': [
+                {
+                    'identifier': '1234',
+                    'verbose_name': 'two',
+                },
+                {
+                    'identifier': '1234',
+                    'verbose_name': 'one',
+                },
+            ],
+        }
+
+        resp = self.client.post(
+            reverse('project-sync-versions', args=[self.pip.pk]),
+            data=json.dumps(version_post_data),
+            content_type='application/json',
+        )
+        self.assertEqual(resp.status_code, 200)
+
+        # We have two versions with an identifier `1234`
+        self.assertEqual(
+            self.pip.versions.filter(identifier='1234').count(),
+            2
+        )
+
+        # We delete one version with identifier `1234`
+        version_post_data = {
+            'branches': [
+                {
+                    'identifier': 'origin/master',
+                    'verbose_name': 'master',
+                },
+            ],
+            'tags': [
+                {
+                    'identifier': '1234',
+                    'verbose_name': 'one',
+                },
+            ],
+        }
+
+        resp = self.client.post(
+            reverse('project-sync-versions', args=[self.pip.pk]),
+            data=json.dumps(version_post_data),
+            content_type='application/json',
+        )
+        self.assertEqual(resp.status_code, 200)
+
+        # We have only one version with an identifier `1234`
+        self.assertEqual(
+            self.pip.versions.filter(identifier='1234').count(),
+            1
+        )
+
 
 class TestStableVersion(TestCase):
     fixtures = ['eric', 'test_data']