From 7ee8521ffa04b3bf9c74d699cb73c0cae6fb8bb5 Mon Sep 17 00:00:00 2001 From: Dan LaManna Date: Mon, 4 Apr 2022 15:50:27 -0400 Subject: [PATCH] Add set-tasks endpoint --- isic/core/views.py | 8 -- isic/studies/api.py | 76 +++++++++++++++- isic/studies/models.py | 23 ++++- isic/studies/serializers.py | 19 +++- isic/studies/tests/conftest.py | 60 +++++++++++++ isic/studies/tests/test_api.py | 44 ++++++++++ isic/studies/tests/test_permissions.py | 115 ++++++++++++------------- 7 files changed, 269 insertions(+), 76 deletions(-) create mode 100644 isic/studies/tests/test_api.py diff --git a/isic/core/views.py b/isic/core/views.py index 64d76f34..aa8aa1f3 100644 --- a/isic/core/views.py +++ b/isic/core/views.py @@ -1,4 +1,3 @@ -from collections import defaultdict import json from django.conf import settings @@ -27,13 +26,6 @@ from isic.studies.models import Annotation, Study, StudyTask -def key_by(sequence, f): - r = defaultdict(list) - for item in sequence: - r[f(item)].append(item) - return dict(r) - - @needs_object_permission('auth.view_staff') def staff_list(request): users = User.objects.filter(is_staff=True).order_by('email') diff --git a/isic/studies/api.py b/isic/studies/api.py index 9ee633bd..eca6a712 100644 --- a/isic/studies/api.py +++ b/isic/studies/api.py @@ -1,29 +1,99 @@ +from django.contrib.auth.models import User +from django.db import transaction +from django.db.models.query_utils import Q +from django.http.response import JsonResponse from rest_framework import viewsets +from rest_framework.decorators import action +from rest_framework.exceptions import PermissionDenied from rest_framework.permissions import IsAdminUser +from isic.core.api import Conflict +from isic.core.permissions import IsicObjectPermissionsFilter, get_visible_objects from isic.studies.models import Annotation, Study, StudyTask -from isic.studies.serializers import AnnotationSerializer, StudySerializer, StudyTaskSerializer +from isic.studies.serializers import ( + AnnotationSerializer, + StudySerializer, + StudyTaskAssignmentSerializer, + StudyTaskSerializer, +) class StudyTaskViewSet(viewsets.ReadOnlyModelViewSet): serializer_class = StudyTaskSerializer queryset = StudyTask.objects.all() permission_classes = [IsAdminUser] + filter_backends = [IsicObjectPermissionsFilter] swagger_schema = None class StudyViewSet(viewsets.ReadOnlyModelViewSet): serializer_class = StudySerializer - queryset = Study.objects.all() - permission_classes = [IsAdminUser] + queryset = Study.objects.distinct() + filter_backends = [IsicObjectPermissionsFilter] swagger_schema = None + @action(detail=True, methods=['post'], pagination_class=None, url_path='set-tasks') + def set_tasks(self, request, *args, **kwargs): + study: Study = self.get_object() + if not request.user.has_perm('studies.modify_study', study): + raise PermissionDenied + elif study.tasks.filter(annotation__isnull=False).exists(): + raise Conflict('Study has answered questions, tasks cannot be overwritten.') + + serializer = StudyTaskAssignmentSerializer(data=request.data, many=True, max_length=100) + serializer.is_valid(raise_exception=True) + + isic_ids = [x['isic_id'] for x in serializer.validated_data] + identifier_filter = Q() + for data in serializer.validated_data: + identifier_filter |= Q(profile__hash_id__iexact=data['user_hash_id_or_email']) + identifier_filter |= Q(email__iexact=data['user_hash_id_or_email']) + + requested_users = ( + User.objects.select_related('profile').filter(is_active=True).filter(identifier_filter) + ) + # create a lookup dictionary that keys users by their hash id and email + requested_users_lookup = {} + for user in requested_users: + requested_users_lookup[user.profile.hash_id] = user + requested_users_lookup[user.email] = user + + requested_images = study.collection.images.filter(isic_id__in=isic_ids) + visible_images = get_visible_objects( + request.user, 'core.view_image', requested_images + ).in_bulk(field_name='isic_id') + + summary = { + 'image_no_perms_or_does_not_exist': [], + 'user_does_not_exist': [], + 'succeeded': [], + } + + with transaction.atomic(): + for task_assignment in serializer.validated_data: + if task_assignment['user_hash_id_or_email'] not in requested_users_lookup: + summary['user_does_not_exist'].append(task_assignment['user_hash_id_or_email']) + elif task_assignment['isic_id'] not in visible_images: + summary['image_no_perms_or_does_not_exist'].append(task_assignment['isic_id']) + else: + StudyTask.objects.create( + study=study, + annotator=requested_users_lookup[task_assignment['user_hash_id_or_email']], + image=visible_images[task_assignment['isic_id']], + ) + summary['succeeded'].append( + f'{task_assignment["isic_id"]}/{task_assignment["user_hash_id_or_email"]}' + ) + + return JsonResponse(summary) + class AnnotationViewSet(viewsets.ReadOnlyModelViewSet): serializer_class = AnnotationSerializer queryset = Annotation.objects.all() permission_classes = [IsAdminUser] + filter_backends = [IsicObjectPermissionsFilter] swagger_schema = None diff --git a/isic/studies/models.py b/isic/studies/models.py index 67fb01f4..5cd88951 100644 --- a/isic/studies/models.py +++ b/isic/studies/models.py @@ -193,8 +193,27 @@ class Meta: class StudyPermissions: model = Study - perms = ['view_study', 'view_study_results'] - filters = {'view_study': 'view_study_list', 'view_study_results': 'view_study_results_list'} + perms = ['view_study', 'view_study_results', 'modify_study'] + filters = { + 'view_study': 'view_study_list', + 'view_study_results': 'view_study_results_list', + 'modify_study': 'modify_study_list', + } + + @staticmethod + def modify_study_list(user_obj: User, qs: QuerySet[Study] | None = None) -> QuerySet[Study]: + qs: QuerySet[Study] = qs if qs is not None else Study._default_manager.all() + + if user_obj.is_staff: + return qs + elif user_obj.is_authenticated: + return qs.filter(creator=user_obj) + else: + return qs.none() + + @staticmethod + def modify_study(user_obj, obj): + return StudyPermissions.modify_study_list(user_obj).contains(obj) @staticmethod def view_study_results_list( diff --git a/isic/studies/serializers.py b/isic/studies/serializers.py index d9bf71fa..a1887a9b 100644 --- a/isic/studies/serializers.py +++ b/isic/studies/serializers.py @@ -1,5 +1,6 @@ from rest_framework import serializers +from isic.core.constants import ISIC_ID_REGEX from isic.studies.models import Annotation, Feature, Question, QuestionChoice, Study, StudyTask @@ -32,7 +33,7 @@ class QuestionSerializer(serializers.ModelSerializer): class Meta: model = Question - fields = ['id', 'required', 'type', 'prompt', 'official', 'choices'] + fields = ['id', 'type', 'prompt', 'official', 'choices'] class StudySerializer(serializers.ModelSerializer): @@ -41,4 +42,18 @@ class StudySerializer(serializers.ModelSerializer): class Meta: model = Study - fields = ['id', 'created', 'creator', 'name', 'description', 'features', 'questions'] + fields = [ + 'id', + 'created', + 'creator', + 'name', + 'description', + 'public', + 'features', + 'questions', + ] + + +class StudyTaskAssignmentSerializer(serializers.Serializer): + isic_id = serializers.RegexField(ISIC_ID_REGEX) + user_hash_id_or_email = serializers.CharField() diff --git a/isic/studies/tests/conftest.py b/isic/studies/tests/conftest.py index e69de29b..7ddc11dd 100644 --- a/isic/studies/tests/conftest.py +++ b/isic/studies/tests/conftest.py @@ -0,0 +1,60 @@ +import pytest + + +@pytest.fixture +def public_study(study_factory, user_factory): + creator = user_factory() + owners = [creator] + [user_factory() for _ in range(2)] + return study_factory(public=True, creator=creator, owners=owners) + + +@pytest.fixture +def private_study(study_factory, user_factory): + creator = user_factory() + owners = [creator] + [user_factory() for _ in range(2)] + return study_factory(public=False, creator=creator, owners=owners) + + +@pytest.fixture +def private_study_and_guest(private_study, user_factory): + return private_study, user_factory() + + +@pytest.fixture +def private_study_and_annotator(private_study, user_factory, study_task_factory): + u = user_factory() + study_task_factory(annotator=u, study=private_study) + return private_study, u + + +@pytest.fixture +def private_study_and_owner(private_study): + return private_study, private_study.owners.first() + + +@pytest.fixture +def private_study_with_responses(study_factory, user_factory, response_factory): + # create a scenario for testing that a user can only see their responses and + # not another annotators. + study = study_factory(public=False) + u1, u2 = user_factory(), user_factory() + response_factory( + annotation__annotator=u1, + annotation__study=study, + annotation__task__annotator=u1, + annotation__task__study=study, + ) + response_factory( + annotation__annotator=u2, + annotation__study=study, + annotation__task__annotator=u2, + annotation__task__study=study, + ) + return study, u1, u2 + + +@pytest.fixture +def study_task_with_user(study_task_factory, user_factory): + u = user_factory() + study_task = study_task_factory(annotator=u) + return study_task diff --git a/isic/studies/tests/test_api.py b/isic/studies/tests/test_api.py new file mode 100644 index 00000000..1fd2e1f0 --- /dev/null +++ b/isic/studies/tests/test_api.py @@ -0,0 +1,44 @@ +from unittest import TestCase + +import pytest + +from isic.studies.models import StudyTask + + +@pytest.fixture +def study_with_images(user_factory, image_factory, collection_factory, study_factory): + user = user_factory() + collection = collection_factory(creator=user, public=True) + images = [image_factory(public=True) for _ in range(2)] + collection.images.add(*images) + study = study_factory(creator=user, collection=collection) + return study, images + + +@pytest.mark.django_db +def test_set_tasks(api_client, study_with_images, user_factory): + users = [user_factory() for _ in range(2)] + study, images = study_with_images + api_client.force_login(study.creator) + + r = api_client.post( + f'/api/v2/studies/{study.pk}/set-tasks/', + [ + {'isic_id': images[0].isic_id, 'user_hash_id_or_email': users[0].profile.hash_id}, + {'isic_id': images[1].isic_id, 'user_hash_id_or_email': users[1].email}, + # bad image + {'isic_id': 'ISIC_9999999', 'user_hash_id_or_email': users[1].email}, + # bad user + {'isic_id': images[1].isic_id, 'user_hash_id_or_email': 'FAKEUSER'}, + ], + ) + assert r.status_code == 200, r.json() + assert StudyTask.objects.count() == 2 + + tasks_actual = list(StudyTask.objects.all().values('study', 'annotator', 'image')) + tasks_expected = [ + {'study': study.pk, 'annotator': users[0].pk, 'image': images[0].pk}, + {'study': study.pk, 'annotator': users[1].pk, 'image': images[1].pk}, + ] + for task_actual, task_expected in zip(tasks_actual, tasks_expected): + TestCase().assertDictEqual(task_actual, task_expected) diff --git a/isic/studies/tests/test_permissions.py b/isic/studies/tests/test_permissions.py index 08038345..985ae7c3 100644 --- a/isic/studies/tests/test_permissions.py +++ b/isic/studies/tests/test_permissions.py @@ -6,65 +6,6 @@ from isic.studies.models import StudyTask -@pytest.fixture -def public_study(study_factory, user_factory): - creator = user_factory() - owners = [creator] + [user_factory() for _ in range(2)] - return study_factory(public=True, creator=creator, owners=owners) - - -@pytest.fixture -def private_study(study_factory, user_factory): - creator = user_factory() - owners = [creator] + [user_factory() for _ in range(2)] - return study_factory(public=False, creator=creator, owners=owners) - - -@pytest.fixture -def private_study_and_guest(private_study, user_factory): - return private_study, user_factory() - - -@pytest.fixture -def private_study_and_annotator(private_study, user_factory, study_task_factory): - u = user_factory() - study_task_factory(annotator=u, study=private_study) - return private_study, u - - -@pytest.fixture -def private_study_and_owner(private_study): - return private_study, private_study.owners.first() - - -@pytest.fixture -def private_study_with_responses(study_factory, user_factory, response_factory): - # create a scenario for testing that a user can only see their responses and - # not another annotators. - study = study_factory(public=False) - u1, u2 = user_factory(), user_factory() - response_factory( - annotation__annotator=u1, - annotation__study=study, - annotation__task__annotator=u1, - annotation__task__study=study, - ) - response_factory( - annotation__annotator=u2, - annotation__study=study, - annotation__task__annotator=u2, - annotation__task__study=study, - ) - return study, u1, u2 - - -@pytest.fixture -def study_task_with_user(study_task_factory, user_factory): - u = user_factory() - study_task = study_task_factory(annotator=u) - return study_task - - @pytest.mark.django_db @pytest.mark.parametrize( 'client_', @@ -305,8 +246,9 @@ def test_annotation_detail_permissions(client, authenticated_client, staff_clien @pytest.fixture -def study_scenario(study_factory, question_factory, question_choice_factory): - study = study_factory() +def study_scenario(user_factory, study_factory, question_factory, question_choice_factory): + user = user_factory() + study = study_factory(creator=user, public=False) question = question_factory() choice = question_choice_factory(question=question) question.choices.add(choice) @@ -330,3 +272,54 @@ def test_study_task_detail_post(client, study_scenario, study_task_factory, user assert response.annotation.annotator == user assert response.question == question assert response.choice == choice + + +@pytest.mark.django_db +def test_study_api_list_creator_permissions( + api_client, study_scenario, study_factory, user_factory +): + study_factory(creator=user_factory(), public=False) + api_client.force_login(study_scenario[0].creator) + r = api_client.get('/api/v2/studies/') + assert r.data['count'] == 1 + assert r.data['results'][0]['id'] == study_scenario[0].id + + +@pytest.mark.django_db +def test_study_api_list_owners_permissions(api_client, study_scenario, study_factory, user_factory): + study_factory(creator=user_factory(), public=False, owners=[study_scenario[0].creator]) + api_client.force_login(study_scenario[0].creator) + r = api_client.get('/api/v2/studies/') + assert r.data['count'] == 2 + + +# @pytest.mark.django_db +# @pytest.mark.parametrize( +# 'client_,url,', +# [ +# lazy_fixture('client'), +# lazy_fixture('authenticated_client'), +# lazy_fixture('staff_client'), +# ], +# ) +# def test_study_api_detail_permissions(client, study_scenario, study_task_factory, user_factory): +# pass + + +@pytest.mark.django_db +def test_study_api_set_tasks_on_study_with_responses_permissions( + api_client, private_study_with_responses +): + study = private_study_with_responses[0] + api_client.force_login(study.creator) + r = api_client.post(f'/api/v2/studies/{study.pk}/set-tasks/') + assert r.status_code == 409 + + +@pytest.mark.django_db +def test_study_api_set_tasks_on_someone_elses_study_permissions( + api_client, user_factory, public_study +): + api_client.force_login(user_factory()) + r = api_client.post(f'/api/v2/studies/{public_study.pk}/set-tasks/') + assert r.status_code == 403