From 4185241590b00f404f4e8e5bc625bd03814f1b89 Mon Sep 17 00:00:00 2001
From: janmax <j.michal@stud.uni-goettingen.de>
Date: Sun, 7 Jan 2018 11:33:34 +0100
Subject: [PATCH] Changed student query_key to a UUID. Closes #87
* Added stages to the subscription mechanism, meaning that
a subscription is distingushed between feedback-creation,
-validation and -conflict resolution
* Minor refactoring to the Feedback ViewSet and serializer
* Added a larger integration test
---
core/migrations/0001_initial.py | 17 ++--
core/models.py | 76 ++++++++++++-----
core/serializers.py | 25 +++++-
.../test_subscription_assignment_service.py | 82 +++++++++++++++++--
core/views.py | 17 +++-
5 files changed, 174 insertions(+), 43 deletions(-)
diff --git a/core/migrations/0001_initial.py b/core/migrations/0001_initial.py
index 9102abb2..014e8786 100644
--- a/core/migrations/0001_initial.py
+++ b/core/migrations/0001_initial.py
@@ -1,15 +1,13 @@
-# Generated by Django 2.0.1 on 2018-01-07 14:37
-
-import uuid
+# Generated by Django 2.0.1 on 2018-01-10 10:46
+import core.models
+from django.conf import settings
import django.contrib.auth.models
import django.contrib.auth.validators
+from django.db import migrations, models
import django.db.models.deletion
import django.utils.timezone
-from django.conf import settings
-from django.db import migrations, models
-
-import core.models
+import uuid
class Migration(migrations.Migration):
@@ -113,13 +111,14 @@ class Migration(migrations.Migration):
('subscription_id', models.UUIDField(default=uuid.uuid4, editable=False, primary_key=True, serialize=False)),
('query_key', models.CharField(blank=True, max_length=75)),
('query_type', models.CharField(choices=[('random', 'Query for any submission'), ('student', 'Query for submissions of student'), ('exam', 'Query for submissions of exam type'), ('submission_type', 'Query for submissions of submissions_type')], default='random', max_length=75)),
- ('owner', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='susbscriptions', to=settings.AUTH_USER_MODEL)),
+ ('feedback_stage', models.CharField(choices=[('feedback-creation', 'No feedback was ever assigned'), ('feedback-validation', 'Feedback exists but is not validated'), ('feedback-conflict-resolution', 'Previous correctors disagree')], default='feedback-creation', max_length=40)),
+ ('owner', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='subscriptions', to=settings.AUTH_USER_MODEL)),
],
),
migrations.CreateModel(
name='StudentInfo',
fields=[
- ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
+ ('student_id', models.UUIDField(default=uuid.uuid4, editable=False, primary_key=True, serialize=False)),
('has_logged_in', models.BooleanField(default=False)),
('matrikel_no', models.CharField(default=core.models.random_matrikel_no, max_length=8, unique=True)),
('exam', models.ForeignKey(null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='students', to='core.ExamType')),
diff --git a/core/models.py b/core/models.py
index 47da2b27..8bbce3f4 100644
--- a/core/models.py
+++ b/core/models.py
@@ -205,6 +205,9 @@ class StudentInfo(models.Model):
matrikel_no (CharField):
The matriculation number of the student
"""
+ student_id = models.UUIDField(primary_key=True,
+ default=uuid.uuid4,
+ editable=False)
has_logged_in = models.BooleanField(default=False)
matrikel_no = models.CharField(unique=True,
max_length=8,
@@ -446,7 +449,7 @@ class GeneralTaskSubscription(models.Model):
type_query_mapper = {
RANDOM: '__any',
- STUDENT_QUERY: 'student__user__username',
+ STUDENT_QUERY: 'student__student_id',
EXAM_TYPE_QUERY: 'student__examtype__module_reference',
SUBMISSION_TYPE_QUERY: 'type__title',
}
@@ -458,16 +461,35 @@ class GeneralTaskSubscription(models.Model):
(SUBMISSION_TYPE_QUERY, 'Query for submissions of submissions_type'),
)
+ FEEDBACK_CREATION = 'feedback-creation'
+ FEEDBACK_VALIDATION = 'feedback-validation'
+ FEEDBACK_CONFLICT_RESOLUTION = 'feedback-conflict-resolution'
+
+ type_to_assignment_count_map = {
+ FEEDBACK_CREATION: 0,
+ FEEDBACK_VALIDATION: 1,
+ FEEDBACK_CONFLICT_RESOLUTION: 2,
+ }
+
+ stages = (
+ (FEEDBACK_CREATION, 'No feedback was ever assigned'),
+ (FEEDBACK_VALIDATION, 'Feedback exists but is not validated'),
+ (FEEDBACK_CONFLICT_RESOLUTION, 'Previous correctors disagree'),
+ )
+
subscription_id = models.UUIDField(primary_key=True,
default=uuid.uuid4,
editable=False)
owner = models.ForeignKey(get_user_model(),
on_delete=models.CASCADE,
- related_name='susbscriptions')
+ related_name='subscriptions')
query_key = models.CharField(max_length=75, blank=True)
query_type = models.CharField(max_length=75,
choices=QUERY_CHOICE,
default=RANDOM)
+ feedback_stage = models.CharField(choices=stages,
+ max_length=40,
+ default=FEEDBACK_CREATION)
class Meta:
unique_together = ('owner', 'query_key', 'query_type')
@@ -479,6 +501,24 @@ class GeneralTaskSubscription(models.Model):
return Submission.objects.filter(
**{self.type_query_mapper[self.query_type]: self.query_key})
+ def _find_submissions_based_on_completed_assignments(
+ self, done_assignments_count):
+ base = self._get_submission_base_query().filter(
+ Q(feedback__isnull=False),
+ Q(feedback__is_final=False),
+ ~Q(feedback__of_tutor=self.owner)
+ ).annotate( # to display only manual
+ done_assignments_count=Count(
+ Case(When(assignments__is_done=True, then=Value(1)),
+ output_field=IntegerField(),
+ )
+ )
+ ).filter(done_assignments_count=self.type_to_assignment_count_map[
+ done_assignments_count])
+
+ log.debug('base %s', base)
+ return base
+
def _find_unassigned_non_final_submissions(self):
unassigned_non_final_submissions = \
self._get_submission_base_query().filter(
@@ -492,31 +532,25 @@ class GeneralTaskSubscription(models.Model):
return unassigned_non_final_submissions
def _find_unassigned_unapproved_non_final_submissions(self):
- unapproved_not_final_submissions = \
- self._get_submission_base_query().filter(
- Q(feedback__isnull=False),
- Q(feedback__is_final=False),
- ~Q(feedback__of_tutor=self.owner),
- # TODO: prevent reassigning to the same tutor
- )
-
- log.debug('unapproved not final submissions %s',
- unapproved_not_final_submissions)
+ return self._find_submissions_based_on_completed_assignments(
+ done_assignments_count=self.FEEDBACK_VALIDATION)
- return unapproved_not_final_submissions
+ def _find_submissions_with_confilcting_feedback(self):
+ return self._find_submissions_based_on_completed_assignments(
+ done_assignments_count=self.FEEDBACK_CONFLICT_RESOLUTION)
def _get_next_assignment_in_subscription(self):
- assignment_priority = (
+ candidates = (
self._find_unassigned_non_final_submissions,
- self._find_unassigned_unapproved_non_final_submissions
- )
+ self._find_unassigned_unapproved_non_final_submissions,
+ self._find_submissions_with_confilcting_feedback
+ )[self.type_to_assignment_count_map[self.feedback_stage]]()
- lazy_queries = (query_set() for query_set in assignment_priority)
- for query in (q for q in lazy_queries if len(q) > 0):
- return query.first()
+ if candidates.count() == 0:
+ raise SubscriptionEnded(
+ f'The task which user {self.owner} subscribed to is done')
- raise SubscriptionEnded(
- f'The task which user {self.owner} subscribed to is done')
+ return candidates.first()
@transaction.atomic
def get_or_create_work_assignment(self):
diff --git a/core/serializers.py b/core/serializers.py
index 778c1b90..5f9d7391 100644
--- a/core/serializers.py
+++ b/core/serializers.py
@@ -15,6 +15,14 @@ log = logging.getLogger(__name__)
user_factory = GradyUserFactory()
+class NoValidAssignmentForFeedbackCreation(Exception):
+ pass
+
+
+class CannotSetFirstFeedbackFinal(Exception):
+ pass
+
+
class DynamicFieldsModelSerializer(DynamicFieldsMixin,
serializers.ModelSerializer):
pass
@@ -29,6 +37,7 @@ class ExamSerializer(DynamicFieldsModelSerializer):
class FeedbackForSubmissionLineSerializer(serializers.BaseSerializer):
+
def to_representation(self, obj):
return {feedback.of_line:
FeedbackCommentSerializer(
@@ -44,7 +53,7 @@ class FeedbackSerializer(DynamicFieldsModelSerializer):
feedbacklines = FeedbackForSubmissionLineSerializer(
required=False
)
- isFinal = serializers.BooleanField(source="is_final")
+ isFinal = serializers.BooleanField(source="is_final", required=False)
ofSubmission = serializers.PrimaryKeyRelatedField(
source='of_submission',
required=False,
@@ -76,6 +85,7 @@ class FeedbackSerializer(DynamicFieldsModelSerializer):
log.debug(data)
assignment_id = data.pop('assignment_id')
score = data.get('score')
+ is_final = data.get('is_final', False)
try:
assignment = TutorSubmissionAssignment.objects.get(
@@ -83,6 +93,16 @@ class FeedbackSerializer(DynamicFieldsModelSerializer):
except ObjectDoesNotExist as err:
raise serializers.ValidationError('No assignment for given id.')
+ requesting_tutor = self.context['request'].user
+ if assignment.subscription.owner != requesting_tutor:
+ raise NoValidAssignmentForFeedbackCreation()
+
+ http_method = self.context['request'].method
+ if (is_final and
+ http_method == 'POST' and
+ requesting_tutor.role == models.UserAccount.TUTOR):
+ raise CannotSetFirstFeedbackFinal()
+
submission = assignment.submission
if not 0 <= score <= submission.type.full_score:
raise serializers.ValidationError(
@@ -251,4 +271,5 @@ class SubscriptionSerializer(DynamicFieldsModelSerializer):
'owner',
'query_type',
'query_key',
- 'assignments')
+ 'assignments',
+ 'feedback_stage')
diff --git a/core/tests/test_subscription_assignment_service.py b/core/tests/test_subscription_assignment_service.py
index ebdcd33d..1302fd52 100644
--- a/core/tests/test_subscription_assignment_service.py
+++ b/core/tests/test_subscription_assignment_service.py
@@ -1,6 +1,7 @@
from rest_framework import status
from rest_framework.test import APIClient, APITestCase
+from core import models
from core.models import (GeneralTaskSubscription, Submission, SubmissionType,
SubscriptionEnded)
from util.factories import GradyUserFactory, make_test_data
@@ -91,6 +92,9 @@ class TestApiEndpoints(APITestCase):
{'username': 'tutor01'},
{'username': 'tutor02'}
],
+ 'reviewers': [
+ {'username': 'reviewer'}
+ ],
'submissions': [
{
'text': 'function blabl\n'
@@ -145,7 +149,7 @@ class TestApiEndpoints(APITestCase):
self.assertEqual('tutor01', response.data['owner'])
- def test_subscription_has_next_assignment(self):
+ def test_subscription_has_next_and_current_assignment(self):
client = APIClient()
client.force_authenticate(user=self.data['tutors'][0])
@@ -154,22 +158,86 @@ class TestApiEndpoints(APITestCase):
subscription_id = response_subs.data['subscription_id']
assignment_id = response_subs.data['assignments'][0]['assignment_id']
- response_current = client.get(
+ response = client.get(
f'/api/subscription/{subscription_id}/assignments/current/')
self.assertEqual(1, len(response_subs.data['assignments']))
self.assertEqual(assignment_id,
- response_current.data['assignment_id'])
+ response.data['assignment_id'])
+
+ response_next = client.get(
+ f'/api/subscription/{subscription_id}/assignments/next/')
+ response_detail_subs = \
+ client.get(f'/api/subscription/{subscription_id}/')
+
+ self.assertEqual(2, len(response_detail_subs.data['assignments']))
+ self.assertNotEqual(assignment_id, response_next.data['assignment_id'])
def test_subscription_can_assign_to_student(self):
client = APIClient()
- client.force_authenticate(user=self.data['tutors'][0])
+ client.force_authenticate(user=self.data['reviewers'][0])
- response_subs = client.post(
+ student01 = self.data['students'][0]
+
+ response = client.post(
'/api/subscription/', {
'query_type': 'student',
- 'query_key': 'student01'
+ 'query_key': student01.student.student_id,
+ 'stage': 'feedback-creation'
})
- assignments = response_subs.data['assignments']
+ assignments = response.data['assignments']
self.assertEqual(2, len(assignments))
+
+ def test_all_stages_of_the_subscription(self):
+ client = APIClient()
+ client.force_authenticate(user=self.data['tutors'][0])
+
+ # The tutor corrects something
+ response = client.post(
+ '/api/subscription/', {
+ 'query_type': 'random',
+ 'feedback_stage': 'feedback-creation'
+ })
+
+ self.assertEqual(response.status_code, status.HTTP_201_CREATED)
+
+ assignment_id = response.data['assignments'][0]['assignment_id']
+ response = client.post(
+ f'/api/feedback/', {
+ "score": 23,
+ "assignment_id": assignment_id,
+ "feedbacklines": {
+ 2: {"text": "< some string >"},
+ 3: {"text": "< some string >"}
+ }
+ }
+ )
+ print(response, response.data)
+ self.assertEqual(response.status_code, status.HTTP_201_CREATED)
+
+ # some other tutor reviews it
+ client.force_authenticate(user=self.data['tutors'][1])
+
+ response = client.post(
+ '/api/subscription/', {
+ 'query_type': 'random',
+ 'feedback_stage': 'feedback-validation'
+ })
+
+ self.assertEqual(response.status_code, status.HTTP_201_CREATED)
+
+ subscription_id = response.data['subscription_id']
+ response = client.get(
+ f'/api/subscription/{subscription_id}/assignments/current/')
+
+ self.assertEqual(response.status_code, status.HTTP_200_OK)
+
+ submissioin_id_in_database = models.Feedback.objects.filter(
+ is_final=False).first().of_submission.submission_id
+ submissioin_id_in_response = \
+ response.data['submission']['submission_id']
+
+ self.assertEqual(
+ str(submissioin_id_in_database),
+ submissioin_id_in_response)
diff --git a/core/views.py b/core/views.py
index 886d41c5..165498d7 100644
--- a/core/views.py
+++ b/core/views.py
@@ -6,7 +6,7 @@ from rest_framework import generics, mixins, status, viewsets
from rest_framework.decorators import api_view, detail_route
from rest_framework.response import Response
-from core import models
+from core import models, serializers
from core.models import (ExamType, Feedback, GeneralTaskSubscription,
StudentInfo, SubmissionType,
TutorSubmissionAssignment)
@@ -67,9 +67,18 @@ class FeedbackApiView(
lookup_url_kwarg = 'submission_id'
def create(self, request, *args, **kwargs):
- serializer = self.get_serializer(
- data={**request.data, 'of_tutor': request.user})
- serializer.is_valid(raise_exception=True)
+ serializer = self.get_serializer(data=request.data)
+
+ try:
+ serializer.is_valid(raise_exception=True)
+ except serializers.NoValidAssignmentForFeedbackCreation:
+ return Response(
+ {'This user has not permission to create this feedback'},
+ status=status.HTTP_403_FORBIDDEN)
+ except serializers.CannotSetFirstFeedbackFinal:
+ return Response(
+ {'Cannot set the first feedback final unless user reviewer'},
+ status=status.HTTP_403_FORBIDDEN)
self.perform_create(serializer)
headers = self.get_success_headers(serializer.data)
--
GitLab