Don't trigger a sync twice on creation/deletion for GitHub

readthedocs/api/v2/views/integrations.py

@@ -196,12 +196,19 @@ def get_response_push(self, project, branches):
             'versions': list(to_build),
         }
 
-    def sync_versions(self, project):
-        version = sync_versions(project)
+    def sync_versions(self, project, sync=True):
+        """
+        Trigger a sync and returns a response indicating if the build was triggered or not.
+
+        If `sync` is False, the sync isn't triggered and a response indicating so is returned.
+        """
+        version = None
+        if sync:
+            version = sync_versions(project)
         return {
             'build_triggered': False,
             'project': project.slug,
-            'versions': [version],
+            'versions': [version] if version else [],
             'versions_synced': version is not None,
         }
 
@@ -379,6 +386,18 @@ def handle_webhook(self):
                 raise ParseError('Parameter "ref" is required')
         # Sync versions on other PUSH events that create or delete
         elif event in (GITHUB_CREATE, GITHUB_DELETE, GITHUB_PUSH):
+            if event == GITHUB_PUSH:
+                # GitHub will send push and create/delete events on a creation/deletion.
+                # If we receive a push event we need to check if the webhook doesn't
+                # already have the create/delete events. So we don't trigger the sync twice.
+                # We listen to push events for creation/deletion for old webhooks only.
+                integration = self.get_integration()
+                events = integration.provider_data.get('events', [])
+                if (
+                    (created and GITHUB_CREATE in events) or
+                    (deleted and GITHUB_DELETE in events)
+                ):
+                    return self.sync_versions(self.project, sync=False)
             return self.sync_versions(self.project)
 
         elif (

readthedocs/integrations/migrations/0006_set-default-value-provider-data.py

@@ -0,0 +1,21 @@
+# -*- coding: utf-8 -*-
+# Generated by Django 1.11.27 on 2020-02-17 18:14
+from __future__ import unicode_literals
+
+from django.db import migrations
+import jsonfield.fields
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('integrations', '0005_change_default_integration_secret'),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name='integration',
+            name='provider_data',
+            field=jsonfield.fields.JSONField(default=dict, verbose_name='Provider data'),
+        ),
+    ]

readthedocs/integrations/models.py

@@ -252,7 +252,7 @@ class Integration(models.Model):
         max_length=32,
         choices=INTEGRATIONS,
     )
-    provider_data = JSONField(_('Provider data'))
+    provider_data = JSONField(_('Provider data'), default=dict)
     exchanges = GenericRelation(
         'HttpExchange',
         related_query_name='integrations',

readthedocs/rtd_tests/tests/test_api.py

@@ -1345,6 +1345,88 @@ def test_github_skip_signature_validation(self, trigger_build):
         )
         self.assertEqual(resp.status_code, 200)
 
+    def test_github_sync_on_push_event(self, trigger_build):
+        """Sync if the webhook doesn't have the create/delete events, but we recieve a push event with created/deleted."""
+        integration = Integration.objects.create(
+            project=self.project,
+            integration_type=Integration.GITHUB_WEBHOOK,
+            provider_data={
+                'events': [],
+            },
+            secret=None,
+        )
+
+        client = APIClient()
+
+        headers = {
+            GITHUB_EVENT_HEADER: GITHUB_PUSH,
+        }
+        payload = {
+            'ref': 'master',
+            'created': True,
+            'deleted': False,
+        }
+        resp = client.post(
+            reverse(
+                'api_webhook_github',
+                kwargs={'project_slug': self.project.slug}
+            ),
+            payload,
+            format='json',
+            **headers
+        )
+        self.assertTrue(resp.json()['versions_synced'])
+
+    def test_github_dont_trigger_double_sync(self, trigger_build):
+        """Don't trigger a sync twice if the webhook has the create/delete events."""
+        integration = Integration.objects.create(
+            project=self.project,
+            integration_type=Integration.GITHUB_WEBHOOK,
+            provider_data={
+                'events': [
+                    GITHUB_CREATE,
+                    GITHUB_DELETE,
+                ],
+            },
+            secret=None,
+        )
+
+        client = APIClient()
+
+        headers = {
+            GITHUB_EVENT_HEADER: GITHUB_PUSH,
+        }
+        payload = {
+            'ref': 'master',
+            'created': True,
+            'deleted': False,
+        }
+        resp = client.post(
+            reverse(
+                'api_webhook_github',
+                kwargs={'project_slug': self.project.slug}
+            ),
+            payload,
+            format='json',
+            **headers
+        )
+        self.assertFalse(resp.json()['versions_synced'])
+
+        headers = {
+            GITHUB_EVENT_HEADER: GITHUB_CREATE,
+        }
+        payload = {'ref': 'master'}
+        resp = client.post(
+            reverse(
+                'api_webhook_github',
+                kwargs={'project_slug': self.project.slug}
+            ),
+            payload,
+            format='json',
+            **headers
+        )
+        self.assertTrue(resp.json()['versions_synced'])
+
     def test_gitlab_webhook_for_branches(self, trigger_build):
         """GitLab webhook API."""
         client = APIClient()