Remove user dependency from installation tracking API

- Migration to drop user field from UserInstallation model, users relation in Experiment model. - Remove `distinct('user')` filter from `installation_count` property in Experiment model. - Drop auth requirement from installation tracking API resources. - Remove unused `experiment-installation-list` API resource. - Remove list of installed experiments from `/api/me` resource. - Stop doing a full sync of experiments with the server, list of enabled experiments from the add-on is authoritative - Separate call to fetch installed experiments and request to fetch `/api/me` resource in frontend Me model. - Remove deletion of user installations from user retire API resource. - Remove UserInstallations from admin - Bump add-on to v0.8.0 - Updated tests. Fixes mozilla#1040
Happy-Ferret · Jul 11, 2016 · ab578fd · ab578fd
1 parent 01b9e6a
commit ab578fd
Show file tree

Hide file tree

Showing 12 changed files with 77 additions and 167 deletions.
diff --git a/addon/index.js b/addon/index.js
@@ -164,12 +164,10 @@ function setupApp() {
 
     app.on('uninstall-experiment', uninstallExperiment);
 
-    app.on('sync-installed', serverInstalled => {
-      syncAllAddonInstallations(serverInstalled).then(() => {
-        app.send('sync-installed-result', {
-          clientUUID: store.clientUUID,
-          installed: store.installedAddons
-        });
+    app.on('sync-installed', () => {
+      app.send('sync-installed-result', {
+        clientUUID: store.clientUUID,
+        installed: store.installedAddons
       });
     });
 
@@ -347,29 +345,6 @@ function updateExperiments() {
   });
 }
 
-function syncAllAddonInstallations(serverInstalled) {
-  const availableIDs = Object.keys(store.availableExperiments);
-  const clientIDs = Object.keys(store.installedAddons);
-  const serverIDs = [];
-
-  for (let key in serverInstalled) { // eslint-disable-line prefer-const
-    if (serverInstalled[key].client_id === store.clientUUID) {
-      serverIDs.push(key);
-    }
-  }
-
-  return Promise.all(availableIDs.filter(id => {
-    const cidx = clientIDs.indexOf(id);
-    const sidx = serverIDs.indexOf(id);
-    // Both missing? Okay.
-    if (cidx === -1 && sidx === -1) { return false; }
-    // Both found? Okay.
-    if (cidx !== -1 && sidx !== -1) { return false; }
-    // One found, one not? Better sync.
-    return true;
-  }).map(syncAddonInstallation));
-}
-
 function uninstallExperiment(experiment) {
   if (isTestpilotAddonID(experiment.addon_id)) {
     AddonManager.getAddonByID(experiment.addon_id, a => {

diff --git a/addon/package.json b/addon/package.json
@@ -1,7 +1,7 @@
 {
   "title": "Test Pilot",
   "name": "testpilot-addon",
-  "version": "0.7.2",
+  "version": "0.8.0",
   "private": true,
   "description": "Test Pilot is a privacy-sensitive user research program focused on getting new features into Firefox faster.",
   "repository": "mozilla/testpilot",

diff --git a/testpilot/experiments/admin.py b/testpilot/experiments/admin.py
@@ -2,8 +2,7 @@
 
 from hvad.admin import TranslatableAdmin, TranslatableTabularInline
 
-from .models import (Experiment, ExperimentDetail, ExperimentTourStep,
-                     UserInstallation)
+from .models import (Experiment, ExperimentDetail, ExperimentTourStep)
 from ..utils import (show_image, parent_link, related_changelist_link,
                      translated)
 
@@ -25,7 +24,6 @@ class ExperimentAdmin(TranslatableAdmin):
                     translated('title'), translated('short_title'),
                     'version', 'addon_id',
                     related_changelist_link('details'),
-                    related_changelist_link('users'),
                     'created', 'modified',)
 
     raw_id_fields = ('contributors',)
@@ -51,17 +49,7 @@ class ExperimentTourStepAdmin(TranslatableAdmin):
                     'created', 'modified',)
 
 
-class UserInstallationAdmin(admin.ModelAdmin):
-
-    list_display = ('id', parent_link('experiment'), parent_link('user'),
-                    'client_id',
-                    'created', 'modified',)
-
-    list_filter = ('experiment',)
-
-
 for x in ((Experiment, ExperimentAdmin),
           (ExperimentDetail, ExperimentDetailAdmin),
-          (ExperimentTourStep, ExperimentTourStepAdmin),
-          (UserInstallation, UserInstallationAdmin),):
+          (ExperimentTourStep, ExperimentTourStepAdmin),):
     admin.site.register(*x)
diff --git a/testpilot/experiments/migrations/0019_auto_20160707_2050.py b/testpilot/experiments/migrations/0019_auto_20160707_2050.py
@@ -0,0 +1,26 @@
+# -*- coding: utf-8 -*-
+from __future__ import unicode_literals
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('experiments', '0018_auto_20160709_0247'),
+    ]
+
+    operations = [
+        migrations.RemoveField(
+            model_name='experiment',
+            name='users',
+        ),
+        migrations.AlterUniqueTogether(
+            name='userinstallation',
+            unique_together=set([('experiment', 'client_id')]),
+        ),
+        migrations.RemoveField(
+            model_name='userinstallation',
+            name='user',
+        ),
+    ]
diff --git a/testpilot/experiments/models.py b/testpilot/experiments/models.py
@@ -52,16 +52,14 @@ class Meta:
     gradient_start = ColorField(default='#e07634')
     gradient_stop = ColorField(default='#4cffa8')
 
-    users = models.ManyToManyField(User, through='UserInstallation')
     contributors = models.ManyToManyField(User, related_name='contributor')
 
     created = models.DateTimeField(auto_now_add=True)
     modified = models.DateTimeField(auto_now=True)
 
     @cached_property
     def installation_count(self):
-        return UserInstallation.objects.distinct('user').filter(
-            experiment=self).count()
+        return UserInstallation.objects.filter(experiment=self).count()
 
     def __str__(self):
         return self.title
@@ -109,11 +107,10 @@ class Meta:
 class UserInstallation(models.Model):
 
     experiment = models.ForeignKey(Experiment)
-    user = models.ForeignKey(User)
     client_id = models.CharField(blank=True, max_length=128)
 
     created = models.DateTimeField(auto_now_add=True)
     modified = models.DateTimeField(auto_now=True)
 
     class Meta:
-        unique_together = ('experiment', 'user', 'client_id',)
+        unique_together = ('experiment', 'client_id',)
diff --git a/testpilot/experiments/serializers.py b/testpilot/experiments/serializers.py
@@ -66,8 +66,7 @@ def get_contributors(self, obj):
 
     def get_installations_url(self, obj):
         request = self.context['request']
-        path = reverse('experiment-installation-list',
-                       args=(obj.pk,))
+        path = '%s/installations/' % reverse('experiment-detail', args=(obj.pk,))
         return request.build_absolute_uri(path)
 
     def get_survey_url(self, obj):

diff --git a/testpilot/experiments/tests.py b/testpilot/experiments/tests.py
@@ -105,8 +105,7 @@ def test_index(self):
                         "survey_url": "https://qsurvey.mozilla.com/s3/%s" %
                         experiment.slug,
                         "installation_count": UserInstallation.objects
-                        .distinct('user').filter(experiment=experiment)
-                        .count(),
+                        .filter(experiment=experiment).count(),
                         "installations_url":
                             "http://testserver/api/experiments/%s/installations/" %
                             experiment.pk,
@@ -177,19 +176,9 @@ def test_contributors(self):
 
     def test_installations(self):
         """Experiments should support an /installations/ psuedo-collection"""
-        user = self.user
         experiment = self.experiments['test-1']
         client_id = '8675309'
 
-        # Ensure list GET without authentication is a 403
-        url = reverse('experiment-installation-list',
-                      args=(experiment.pk,))
-        resp = self.client.get(url)
-        self.assertEqual(403, resp.status_code)
-
-        self.client.login(username=self.username,
-                          password=self.password)
-
         # Ensure the installation to be created is initially 404
         url = reverse('experiment-installation-detail',
                       args=(experiment.pk, client_id))
@@ -198,29 +187,26 @@ def test_installations(self):
 
         # Create the first installation of interest
         UserInstallation.objects.create(
-            experiment=experiment, user=user, client_id=client_id)
+            experiment=experiment, client_id=client_id)
 
         # Also create a few installations that shouldn't appear in results
         UserInstallation.objects.create(
-            experiment=self.experiments['test-2'], user=user,
+            experiment=self.experiments['test-2'],
             client_id=client_id)
 
         UserInstallation.objects.create(
-            experiment=experiment, user=self.users['experimenttest-1'],
+            experiment=experiment,
             client_id='someotherclient')
 
-        # Ensure that the desired installation appears in the API list result
-        data = self.jsonGet('experiment-installation-list',
-                            experiment_pk=experiment.pk)
-        self.assertEqual(1, len(data))
-        self.assertEqual(client_id, data[0]['client_id'])
-
         # Ensure that the desired installation is found at its URL
         url = reverse('experiment-installation-detail',
                       args=(experiment.pk, client_id))
         resp = self.client.get(url)
         self.assertEqual(200, resp.status_code)
 
+        self.assertEqual(2, (UserInstallation.objects
+                             .filter(experiment=experiment).count()))
+
         # Create another client installation via PUT
         self.handler.records = []
         client_id_2 = '123456789'
@@ -229,21 +215,19 @@ def test_installations(self):
         resp = self.client.put(url, {})
         self.assertEqual(200, resp.status_code)
 
+        # Ensure that the API list result reflects the addition
+        self.assertEqual(3, (UserInstallation.objects
+                             .filter(experiment=experiment).count()))
+
         # Ensure that a testpilot.test-install log event was emitted
         record = self.handler.records[0]
         formatter = JsonLogFormatter(logger_name='testpilot.test-install')
         details = json.loads(formatter.format(record))
         fields = details['Fields']
 
         self.assertEqual('testpilot.test-install', record.name)
-        self.assertEqual(fields['uid'], user.id)
         self.assertEqual(fields['context'], experiment.title)
 
-        # Ensure that the API list result reflects the addition
-        data = self.jsonGet('experiment-installation-list',
-                            experiment_pk=experiment.pk)
-        self.assertEqual(2, len(data))
-
         # Delete the new client installation with DELETE
         client_id_2 = '123456789'
         url = reverse('experiment-installation-detail',
@@ -252,6 +236,5 @@ def test_installations(self):
         self.assertEqual(410, resp.status_code)
 
         # Ensure that the API list result reflects the deletion
-        data = self.jsonGet('experiment-installation-list',
-                            experiment_pk=experiment.pk)
-        self.assertEqual(1, len(data))
+        self.assertEqual(2, (UserInstallation.objects
+                             .filter(experiment=experiment).count()))
diff --git a/testpilot/experiments/urls.py b/testpilot/experiments/urls.py
@@ -6,6 +6,4 @@
     '',
     url(r'^(?P<experiment_pk>[\w-]+)/installations/(?P<client_id>[\w-]+)',
         views.installation_detail, name='experiment-installation-detail'),
-    url(r'^(?P<experiment_pk>[\w-]+)/installations/',
-        views.installation_list, name='experiment-installation-list'),
 )
diff --git a/testpilot/experiments/views.py b/testpilot/experiments/views.py
@@ -1,5 +1,5 @@
 from rest_framework import viewsets, status
-from rest_framework.permissions import IsAuthenticated
+from rest_framework.permissions import AllowAny
 from rest_framework.response import Response
 from rest_framework.decorators import (detail_route, permission_classes,
                                        api_view)
@@ -44,24 +44,14 @@ def details(self, request, pk=None):
         return Response(serializer.data)
 
 
-@api_view(['GET'])
-@permission_classes([IsAuthenticated])
-def installation_list(request, experiment_pk):
-    experiment = get_object_or_404(Experiment, pk=experiment_pk)
-    queryset = UserInstallation.objects.filter(
-        user=request.user, experiment=experiment)
-    return Response(UserInstallationSerializer(
-        queryset, many=True, context={'request': request}).data)
-
-
 @api_view(['GET', 'PUT', 'DELETE'])
-@permission_classes([IsAuthenticated])
+@permission_classes([AllowAny])
 def installation_detail(request, experiment_pk, client_id):
     experiment = get_object_or_404(Experiment, pk=experiment_pk)
 
     if 'PUT' == request.method:
         installation, created = UserInstallation.objects.get_or_create(
-            user=request.user, experiment=experiment, client_id=client_id)
+            experiment=experiment, client_id=client_id)
         installation.save()
         logging.getLogger('testpilot.test-install').info('', extra={
             'uid': request.user.id,
@@ -70,7 +60,7 @@ def installation_detail(request, experiment_pk, client_id):
     else:
         installation = get_object_or_404(
             UserInstallation,
-            user=request.user, experiment=experiment, client_id=client_id)
+            experiment=experiment, client_id=client_id)
 
     if 'DELETE' == request.method:
         installation.delete()

diff --git a/testpilot/frontend/static-src/app/models/me.js b/testpilot/frontend/static-src/app/models/me.js
@@ -32,40 +32,35 @@ export default Model.extend({
   },
 
   fetch() {
+    return Promise.all([
+      this.fetchInstalledExperiments(),
+      this.fetchMeResource()
+    ]);
+  },
+
+  fetchInstalledExperiments() {
+    this.hasAddon = Boolean(window.navigator.testpilotAddon);
+    if (!this.hasAddon) { return false; }
+
+    // HACK: Previous versions of the add-on expected installation data from
+    // the server, but that's no longer available. So, let's fake it, with an
+    // empty data structure as appropriate to the addon version (or lack of one
+    // exposed)
+    const installedData = window.navigator.testpilotAddonVersion ? {} : [];
+
+    return app.waitForMessage('sync-installed', installedData).then(result => {
+      this.clientUUID = result.clientUUID;
+      this.installed = result.installed;
+    });
+  },
+
+  fetchMeResource() {
     return fetch(this.url, {
       headers: { 'Accept': 'application/json' },
       credentials: 'same-origin'
     }).then(response => response.json()).then(userData => {
       this.user = userData;
       if (!this.user.profile) { return false; }
-
-      this.hasAddon = Boolean(window.navigator.testpilotAddon);
-      if (!this.hasAddon) { return false; }
-
-      let installedData;
-      if (!window.navigator.testpilotAddonVersion) {
-        // Previous add-ons didn't expose version info, so assume
-        // old API data
-        installedData = [];
-        for (let k in userData.installed) { // eslint-disable-line prefer-const
-          if (userData.installed.hasOwnProperty(k)) {
-            installedData.push(userData.installed[k]);
-          }
-        }
-      } else {
-        // TODO: Need a semver matching check here someday, but for
-        // now we can assume
-        // just the presence of a version means we're in the future.
-        installedData = userData.installed;
-      }
-
-      return app.waitForMessage('sync-installed', installedData)
-        .then(result => {
-          this.clientUUID = result.clientUUID;
-          this.installed = result.installed;
-        }).catch((err) => {
-          console.error('sync-installed failed', err); // eslint-disable-line no-console
-        });
     });
   },