diff --git a/lms/djangoapps/experiments/factories.py b/lms/djangoapps/experiments/factories.py index a0f028a670..d0aa1d89e1 100644 --- a/lms/djangoapps/experiments/factories.py +++ b/lms/djangoapps/experiments/factories.py @@ -1,6 +1,6 @@ import factory -from experiments.models import ExperimentData +from experiments.models import ExperimentData, ExperimentKeyValue from student.tests.factories import UserFactory @@ -12,3 +12,12 @@ class ExperimentDataFactory(factory.DjangoModelFactory): experiment_id = factory.fuzzy.FuzzyInteger(0) key = factory.Sequence(lambda n: n) value = factory.Faker('word') + + +class ExperimentKeyValueFactory(factory.DjangoModelFactory): + class Meta(object): + model = ExperimentKeyValue + + experiment_id = factory.fuzzy.FuzzyInteger(0) + key = factory.Sequence(lambda n: n) + value = factory.Faker('word') diff --git a/lms/djangoapps/experiments/filters.py b/lms/djangoapps/experiments/filters.py index cfbf9be11e..db2b3f1e27 100644 --- a/lms/djangoapps/experiments/filters.py +++ b/lms/djangoapps/experiments/filters.py @@ -1,9 +1,15 @@ import django_filters -from experiments.models import ExperimentData +from experiments.models import ExperimentData, ExperimentKeyValue class ExperimentDataFilter(django_filters.FilterSet): class Meta(object): model = ExperimentData fields = ['experiment_id', 'key', ] + + +class ExperimentKeyValueFilter(django_filters.FilterSet): + class Meta(object): + model = ExperimentKeyValue + fields = ['experiment_id', 'key', ] diff --git a/lms/djangoapps/experiments/migrations/0002_auto_20170627_1402.py b/lms/djangoapps/experiments/migrations/0002_auto_20170627_1402.py new file mode 100644 index 0000000000..609513a7c5 --- /dev/null +++ b/lms/djangoapps/experiments/migrations/0002_auto_20170627_1402.py @@ -0,0 +1,34 @@ +# -*- coding: utf-8 -*- +from __future__ import unicode_literals + +from django.db import migrations, models +import django_extensions.db.fields + + +class Migration(migrations.Migration): + + dependencies = [ + ('experiments', '0001_initial'), + ] + + operations = [ + migrations.CreateModel( + name='ExperimentKeyValue', + fields=[ + ('id', models.AutoField(verbose_name='ID', serialize=False, auto_created=True, primary_key=True)), + ('created', django_extensions.db.fields.CreationDateTimeField(auto_now_add=True, verbose_name='created')), + ('modified', django_extensions.db.fields.ModificationDateTimeField(auto_now=True, verbose_name='modified')), + ('experiment_id', models.PositiveSmallIntegerField(verbose_name=b'Experiment ID', db_index=True)), + ('key', models.CharField(max_length=255)), + ('value', models.TextField()), + ], + options={ + 'verbose_name': 'Experiment Data', + 'verbose_name_plural': 'Experiment Data', + }, + ), + migrations.AlterUniqueTogether( + name='experimentkeyvalue', + unique_together=set([('experiment_id', 'key')]), + ), + ] diff --git a/lms/djangoapps/experiments/models.py b/lms/djangoapps/experiments/models.py index 008620ef36..0b828ff1de 100644 --- a/lms/djangoapps/experiments/models.py +++ b/lms/djangoapps/experiments/models.py @@ -20,3 +20,18 @@ class ExperimentData(TimeStampedModel): unique_together = ( ('user', 'experiment_id', 'key'), ) + + +class ExperimentKeyValue(TimeStampedModel): + experiment_id = models.PositiveSmallIntegerField( + null=False, blank=False, db_index=True, verbose_name='Experiment ID' + ) + key = models.CharField(null=False, blank=False, max_length=255) + value = models.TextField() + + class Meta(object): + verbose_name = 'Experiment Data' + verbose_name_plural = 'Experiment Data' + unique_together = ( + ('experiment_id', 'key'), + ) diff --git a/lms/djangoapps/experiments/permissions.py b/lms/djangoapps/experiments/permissions.py index 2e1df6861f..567bc91350 100644 --- a/lms/djangoapps/experiments/permissions.py +++ b/lms/djangoapps/experiments/permissions.py @@ -1,3 +1,5 @@ +from rest_framework.permissions import SAFE_METHODS, BasePermission + from openedx.core.lib.api import permissions @@ -17,3 +19,8 @@ class IsStaffOrOwner(permissions.IsStaffOrOwner): # The view will handle filtering for the current user return True + + +class IsStaffOrReadOnly(BasePermission): + def has_permission(self, request, view): + return request.user.is_staff or request.method in SAFE_METHODS diff --git a/lms/djangoapps/experiments/serializers.py b/lms/djangoapps/experiments/serializers.py index 895d72d8ce..a4f6e40659 100644 --- a/lms/djangoapps/experiments/serializers.py +++ b/lms/djangoapps/experiments/serializers.py @@ -1,7 +1,7 @@ from django.contrib.auth import get_user_model from rest_framework import serializers -from .models import ExperimentData +from .models import ExperimentData, ExperimentKeyValue User = get_user_model() # pylint:disable=invalid-name @@ -20,3 +20,9 @@ class ExperimentDataSerializer(serializers.ModelSerializer): class Meta(ExperimentDataCreateSerializer.Meta): read_only_fields = ('user',) + + +class ExperimentKeyValueSerializer(serializers.ModelSerializer): + class Meta(object): + model = ExperimentKeyValue + fields = ('id', 'experiment_id', 'key', 'value', 'created', 'modified',) diff --git a/lms/djangoapps/experiments/tests/test_views.py b/lms/djangoapps/experiments/tests/test_views.py index 379eddde96..b26f5f586c 100644 --- a/lms/djangoapps/experiments/tests/test_views.py +++ b/lms/djangoapps/experiments/tests/test_views.py @@ -3,8 +3,8 @@ import urllib from django.core.urlresolvers import reverse from rest_framework.test import APITestCase -from experiments.factories import ExperimentDataFactory -from experiments.models import ExperimentData +from experiments.factories import ExperimentDataFactory, ExperimentKeyValueFactory +from experiments.models import ExperimentData, ExperimentKeyValue from experiments.serializers import ExperimentDataSerializer from student.tests.factories import UserFactory @@ -208,3 +208,84 @@ class ExperimentDataViewSetTests(APITestCase): self.assertEqual(response.status_code, 200) ExperimentData.objects.get(user=user, **kwargs) ExperimentData.objects.get(user=other_user, **kwargs) + + +class ExperimentKeyValueViewSetTests(APITestCase): + def test_permissions(self): + """ Staff access is required for write operations. """ + url = reverse('api_experiments:v0:key_value-list') + + response = self.client.get(url) + self.assertEqual(response.status_code, 200) + + response = self.client.post(url, {}) + self.assertEqual(response.status_code, 401) + + instance = ExperimentKeyValueFactory() + url = reverse('api_experiments:v0:key_value-detail', kwargs={'pk': instance.id}) + + response = self.client.get(url) + self.assertEqual(response.status_code, 200) + + user = UserFactory(is_staff=False) + self.client.login(username=user.username, password=UserFactory._DEFAULT_PASSWORD) + + response = self.client.put(url, {}) + self.assertEqual(response.status_code, 403) + + response = self.client.patch(url, {}) + self.assertEqual(response.status_code, 403) + + response = self.client.delete(url) + self.assertEqual(response.status_code, 403) + + def test_bulk_upsert_permissions(self): + """ Non-staff users should not be allowed to access the endpoint. """ + data = [] + url = reverse('api_experiments:v0:key_value-bulk-upsert') + user = UserFactory(is_staff=False) + + # Authentication required + response = self.client.put(url, data, format='json') + self.assertEqual(response.status_code, 401) + + # Staff permission required + self.client.login(username=user.username, password=UserFactory._DEFAULT_PASSWORD) + response = self.client.put(url, data, format='json') + self.assertEqual(response.status_code, 403) + + def test_bulk_upsert(self): + """ The endpoint should support creating/updating multiple ExperimentData objects with a single call. """ + url = reverse('api_experiments:v0:key_value-bulk-upsert') + experiment_id = 1 + user = UserFactory(is_staff=True) + data = [ + { + 'experiment_id': experiment_id, + 'key': 'foo', + 'value': 'bar', + }, + { + 'experiment_id': experiment_id, + 'key': 'foo1', + 'value': 'bar', + }, + ] + + self.client.login(username=user.username, password=UserFactory._DEFAULT_PASSWORD) + + # New data should be created + response = self.client.put(url, data, format='json') + self.assertEqual(response.status_code, 200) + kwargs = { + 'experiment_id': experiment_id, + 'value': 'bar', + } + ExperimentKeyValue.objects.get(key='foo', **kwargs) + ExperimentKeyValue.objects.get(key='foo1', **kwargs) + + # Subsequent calls should update the existing data rather than create more + response = self.client.put(url, data, format='json') + self.assertEqual(response.status_code, 200) + ExperimentKeyValue.objects.get(key='foo', **kwargs) + ExperimentKeyValue.objects.get(key='foo1', **kwargs) diff --git a/lms/djangoapps/experiments/urls.py b/lms/djangoapps/experiments/urls.py index 635ea7b7e7..e4b30552ca 100644 --- a/lms/djangoapps/experiments/urls.py +++ b/lms/djangoapps/experiments/urls.py @@ -1,10 +1,10 @@ from django.conf.urls import include, url -from experiments import routers -from experiments.views import ExperimentDataViewSet +from experiments import routers, views router = routers.DefaultRouter() -router.register(r'data', ExperimentDataViewSet, base_name='data') +router.register(r'data', views.ExperimentDataViewSet, base_name='data') +router.register(r'key-value', views.ExperimentKeyValueViewSet, base_name='key_value') urlpatterns = [ url(r'^v0/', include(router.urls, namespace='v0')), diff --git a/lms/djangoapps/experiments/views.py b/lms/djangoapps/experiments/views.py index f48256b88e..e4df1978d5 100644 --- a/lms/djangoapps/experiments/views.py +++ b/lms/djangoapps/experiments/views.py @@ -6,10 +6,9 @@ from rest_framework.decorators import list_route from rest_framework.filters import DjangoFilterBackend from rest_framework.response import Response -from experiments import filters -from experiments.models import ExperimentData -from experiments.permissions import IsStaffOrOwner -from experiments.serializers import ExperimentDataCreateSerializer, ExperimentDataSerializer +from experiments import filters, serializers +from experiments.models import ExperimentData, ExperimentKeyValue +from experiments.permissions import IsStaffOrOwner, IsStaffOrReadOnly from openedx.core.lib.api.authentication import SessionAuthenticationAllowInactiveUser User = get_user_model() # pylint: disable=invalid-name @@ -21,7 +20,7 @@ class ExperimentDataViewSet(viewsets.ModelViewSet): filter_class = filters.ExperimentDataFilter permission_classes = (permissions.IsAuthenticated, IsStaffOrOwner,) queryset = ExperimentData.objects.all() - serializer_class = ExperimentDataSerializer + serializer_class = serializers.ExperimentDataSerializer _cached_users = {} def filter_queryset(self, queryset): @@ -30,8 +29,8 @@ class ExperimentDataViewSet(viewsets.ModelViewSet): def get_serializer_class(self): if self.action == 'create': - return ExperimentDataCreateSerializer - return ExperimentDataSerializer + return serializers.ExperimentDataCreateSerializer + return serializers.ExperimentDataSerializer def create_or_update(self, request, *args, **kwargs): # If we have a primary key, treat this as a regular update request @@ -82,3 +81,25 @@ class ExperimentDataViewSet(viewsets.ModelViewSet): serializer = self.get_serializer(upserted, many=True) return Response(serializer.data) + + +class ExperimentKeyValueViewSet(viewsets.ModelViewSet): + authentication_classes = (JwtAuthentication, SessionAuthenticationAllowInactiveUser,) + filter_backends = (DjangoFilterBackend,) + filter_class = filters.ExperimentKeyValueFilter + permission_classes = (IsStaffOrReadOnly,) + queryset = ExperimentKeyValue.objects.all() + serializer_class = serializers.ExperimentKeyValueSerializer + + @list_route(methods=['put'], permission_classes=[permissions.IsAdminUser]) + def bulk_upsert(self, request): + upserted = [] + + with transaction.atomic(): + for item in request.data: + datum, __ = ExperimentKeyValue.objects.update_or_create( + experiment_id=item['experiment_id'], key=item['key'], defaults={'value': item['value']}) + upserted.append(datum) + + serializer = self.get_serializer(upserted, many=True) + return Response(serializer.data)