From d1cf3af2c55f3b6a3b1f01c7699f6527d97ada21 Mon Sep 17 00:00:00 2001
From: janmax <j.michal@stud.uni-goettingen.de>
Date: Thu, 8 Feb 2018 18:00:38 +0100
Subject: [PATCH] Subscription enhancements
* It is now possible to 'deactivate subscriptions via the delete
http verb
* This is not exactly what was specified in #92 but should achieve
the same result. Instead of introducing a depleted field,
subscriptions can distinguish if they are fully depleted or just
temporarily. The method does not involve any overhead.
* Refactorings in the subscription model to increase readability
* Creating a subscription does not have side effects (creates no
assignment)
Other minor changes
* Assignments are now implicitly checked if feedback is created
* using the assignment endpoint to create subscriptions instead of
subscription endpoint
Closes #93 and #92.
---
core/migrations/0001_initial.py | 141 +++++++-----------
core/migrations/0002_auto_20180202_1421.py | 17 ---
...0002_submissionsubscription_deactivated.py | 18 +++
core/migrations/0003_auto_20180207_1922.py | 18 ---
core/models.py | 123 ++++++++-------
core/serializers/feedback.py | 42 +++---
core/serializers/subscription.py | 55 +++----
core/tests/test_feedback.py | 35 ++---
.../test_subscription_assignment_service.py | 116 +++++++++-----
core/urls.py | 3 +-
core/views/feedback.py | 47 +++---
core/views/subscription.py | 97 ++++++------
12 files changed, 355 insertions(+), 357 deletions(-)
delete mode 100644 core/migrations/0002_auto_20180202_1421.py
create mode 100644 core/migrations/0002_submissionsubscription_deactivated.py
delete mode 100644 core/migrations/0003_auto_20180207_1922.py
diff --git a/core/migrations/0001_initial.py b/core/migrations/0001_initial.py
index 77e1634f..7f984f22 100644
--- a/core/migrations/0001_initial.py
+++ b/core/migrations/0001_initial.py
@@ -1,4 +1,4 @@
-# Generated by Django 2.0.1 on 2018-02-01 15:08
+# Generated by Django 2.0.2 on 2018-02-08 15:11
import uuid
@@ -24,37 +24,22 @@ class Migration(migrations.Migration):
migrations.CreateModel(
name='UserAccount',
fields=[
- ('id', models.AutoField(auto_created=True,
- primary_key=True, serialize=False, verbose_name='ID')),
- ('password', models.CharField(
- max_length=128, verbose_name='password')),
- ('last_login', models.DateTimeField(
- blank=True, null=True, verbose_name='last login')),
- ('is_superuser', models.BooleanField(default=False,
- help_text='Designates that this user has all permissions without explicitly assigning them.', verbose_name='superuser status')),
- ('username', models.CharField(error_messages={'unique': 'A user with that username already exists.'}, help_text='Required. 150 characters or fewer. Letters, digits and @/./+/-/_ only.',
- max_length=150, unique=True, validators=[django.contrib.auth.validators.UnicodeUsernameValidator()], verbose_name='username')),
- ('first_name', models.CharField(blank=True,
- max_length=30, verbose_name='first name')),
- ('last_name', models.CharField(blank=True,
- max_length=150, verbose_name='last name')),
- ('email', models.EmailField(blank=True,
- max_length=254, verbose_name='email address')),
- ('is_staff', models.BooleanField(default=False,
- help_text='Designates whether the user can log into this admin site.', verbose_name='staff status')),
- ('is_active', models.BooleanField(
- default=True, help_text='Designates whether this user should be treated as active. Unselect this instead of deleting accounts.', verbose_name='active')),
- ('date_joined', models.DateTimeField(
- default=django.utils.timezone.now, verbose_name='date joined')),
- ('fullname', models.CharField(blank=True,
- max_length=70, verbose_name='full name')),
+ ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
+ ('password', models.CharField(max_length=128, verbose_name='password')),
+ ('last_login', models.DateTimeField(blank=True, null=True, verbose_name='last login')),
+ ('is_superuser', models.BooleanField(default=False, help_text='Designates that this user has all permissions without explicitly assigning them.', verbose_name='superuser status')),
+ ('username', models.CharField(error_messages={'unique': 'A user with that username already exists.'}, help_text='Required. 150 characters or fewer. Letters, digits and @/./+/-/_ only.', max_length=150, unique=True, validators=[django.contrib.auth.validators.UnicodeUsernameValidator()], verbose_name='username')),
+ ('first_name', models.CharField(blank=True, max_length=30, verbose_name='first name')),
+ ('last_name', models.CharField(blank=True, max_length=150, verbose_name='last name')),
+ ('email', models.EmailField(blank=True, max_length=254, verbose_name='email address')),
+ ('is_staff', models.BooleanField(default=False, help_text='Designates whether the user can log into this admin site.', verbose_name='staff status')),
+ ('is_active', models.BooleanField(default=True, help_text='Designates whether this user should be treated as active. Unselect this instead of deleting accounts.', verbose_name='active')),
+ ('date_joined', models.DateTimeField(default=django.utils.timezone.now, verbose_name='date joined')),
+ ('fullname', models.CharField(blank=True, max_length=70, verbose_name='full name')),
('is_admin', models.BooleanField(default=False)),
- ('role', models.CharField(choices=[
- ('Student', 'student'), ('Tutor', 'tutor'), ('Reviewer', 'reviewer')], max_length=50)),
- ('groups', models.ManyToManyField(blank=True, help_text='The groups this user belongs to. A user will get all permissions granted to each of their groups.',
- related_name='user_set', related_query_name='user', to='auth.Group', verbose_name='groups')),
- ('user_permissions', models.ManyToManyField(blank=True, help_text='Specific permissions for this user.',
- related_name='user_set', related_query_name='user', to='auth.Permission', verbose_name='user permissions')),
+ ('role', models.CharField(choices=[('Student', 'student'), ('Tutor', 'tutor'), ('Reviewer', 'reviewer')], max_length=50)),
+ ('groups', models.ManyToManyField(blank=True, help_text='The groups this user belongs to. A user will get all permissions granted to each of their groups.', related_name='user_set', related_query_name='user', to='auth.Group', verbose_name='groups')),
+ ('user_permissions', models.ManyToManyField(blank=True, help_text='Specific permissions for this user.', related_name='user_set', related_query_name='user', to='auth.Permission', verbose_name='user permissions')),
],
options={
'verbose_name': 'user',
@@ -68,8 +53,7 @@ class Migration(migrations.Migration):
migrations.CreateModel(
name='ExamType',
fields=[
- ('id', models.AutoField(auto_created=True,
- primary_key=True, serialize=False, verbose_name='ID')),
+ ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
('module_reference', models.CharField(max_length=50, unique=True)),
('total_score', models.PositiveIntegerField()),
('pass_score', models.PositiveIntegerField()),
@@ -83,13 +67,11 @@ class Migration(migrations.Migration):
migrations.CreateModel(
name='Feedback',
fields=[
- ('id', models.AutoField(auto_created=True,
- primary_key=True, serialize=False, verbose_name='ID')),
+ ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
('score', models.PositiveIntegerField(default=0)),
('created', models.DateTimeField(auto_now_add=True)),
('is_final', models.BooleanField(default=False)),
- ('origin', models.IntegerField(choices=[(0, 'was empty'), (1, 'passed unittests'), (
- 2, 'did not compile'), (3, 'could not link'), (4, 'created by a human. yak!')], default=4)),
+ ('origin', models.IntegerField(choices=[(0, 'was empty'), (1, 'passed unittests'), (2, 'did not compile'), (3, 'could not link'), (4, 'created by a human. yak!')], default=4)),
],
options={
'verbose_name': 'Feedback',
@@ -99,17 +81,14 @@ class Migration(migrations.Migration):
migrations.CreateModel(
name='FeedbackComment',
fields=[
- ('id', models.AutoField(auto_created=True,
- primary_key=True, serialize=False, verbose_name='ID')),
+ ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
('text', models.TextField()),
('created', models.DateTimeField(auto_now_add=True)),
('modified', models.DateTimeField(auto_now=True)),
- ('is_final', models.BooleanField(default=True)),
+ ('visible_to_student', models.BooleanField(default=True)),
('of_line', models.PositiveIntegerField(default=0)),
- ('of_feedback', models.ForeignKey(null=True, on_delete=django.db.models.deletion.CASCADE,
- related_name='feedback_lines', to='core.Feedback')),
- ('of_tutor', models.ForeignKey(on_delete=django.db.models.deletion.PROTECT,
- related_name='comment_list', to=settings.AUTH_USER_MODEL)),
+ ('of_feedback', models.ForeignKey(null=True, on_delete=django.db.models.deletion.CASCADE, related_name='feedback_lines', to='core.Feedback')),
+ ('of_tutor', models.ForeignKey(on_delete=django.db.models.deletion.PROTECT, related_name='comment_list', to=settings.AUTH_USER_MODEL)),
],
options={
'verbose_name': 'Feedback Comment',
@@ -117,32 +96,14 @@ class Migration(migrations.Migration):
'ordering': ('created',),
},
),
- migrations.CreateModel(
- name='GeneralTaskSubscription',
- fields=[
- ('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)),
- ('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=[
- ('student_id', models.UUIDField(default=uuid.uuid4,
- editable=False, primary_key=True, serialize=False)),
+ ('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')),
- ('user', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE,
- related_name='student', to=settings.AUTH_USER_MODEL)),
+ ('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')),
+ ('user', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, related_name='student', to=settings.AUTH_USER_MODEL)),
],
options={
'verbose_name': 'Student',
@@ -152,12 +113,10 @@ class Migration(migrations.Migration):
migrations.CreateModel(
name='Submission',
fields=[
- ('submission_id', models.UUIDField(default=uuid.uuid4,
- editable=False, primary_key=True, serialize=False)),
+ ('submission_id', models.UUIDField(default=uuid.uuid4, editable=False, primary_key=True, serialize=False)),
('seen_by_student', models.BooleanField(default=False)),
('text', models.TextField(blank=True)),
- ('student', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE,
- related_name='submissions', to='core.StudentInfo')),
+ ('student', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='submissions', to='core.StudentInfo')),
],
options={
'verbose_name': 'Submission',
@@ -165,11 +124,20 @@ class Migration(migrations.Migration):
'ordering': ('type__name',),
},
),
+ migrations.CreateModel(
+ name='SubmissionSubscription',
+ fields=[
+ ('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)),
+ ('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='SubmissionType',
fields=[
- ('id', models.AutoField(auto_created=True,
- primary_key=True, serialize=False, verbose_name='ID')),
+ ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
('name', models.CharField(max_length=50, unique=True)),
('full_score', models.PositiveIntegerField(default=0)),
('description', models.TextField()),
@@ -183,13 +151,11 @@ class Migration(migrations.Migration):
migrations.CreateModel(
name='Test',
fields=[
- ('id', models.AutoField(auto_created=True,
- primary_key=True, serialize=False, verbose_name='ID')),
+ ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
('name', models.CharField(max_length=30)),
('label', models.CharField(max_length=50)),
('annotation', models.TextField()),
- ('submission', models.ForeignKey(
- on_delete=django.db.models.deletion.CASCADE, related_name='tests', to='core.Submission')),
+ ('submission', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='tests', to='core.Submission')),
],
options={
'verbose_name': 'Test',
@@ -199,39 +165,34 @@ class Migration(migrations.Migration):
migrations.CreateModel(
name='TutorSubmissionAssignment',
fields=[
- ('assignment_id', models.UUIDField(default=uuid.uuid4,
- editable=False, primary_key=True, serialize=False)),
+ ('assignment_id', models.UUIDField(default=uuid.uuid4, editable=False, primary_key=True, serialize=False)),
('is_done', models.BooleanField(default=False)),
('created', models.DateTimeField(auto_now_add=True)),
- ('submission', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE,
- related_name='assignments', to='core.Submission')),
- ('subscription', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE,
- related_name='assignments', to='core.GeneralTaskSubscription')),
+ ('submission', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='assignments', to='core.Submission')),
+ ('subscription', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='assignments', to='core.SubmissionSubscription')),
],
),
migrations.AddField(
model_name='submission',
name='type',
- field=models.ForeignKey(on_delete=django.db.models.deletion.PROTECT,
- related_name='submissions', to='core.SubmissionType'),
+ field=models.ForeignKey(on_delete=django.db.models.deletion.PROTECT, related_name='submissions', to='core.SubmissionType'),
),
migrations.AddField(
model_name='feedback',
name='of_submission',
- field=models.OneToOneField(
- on_delete=django.db.models.deletion.CASCADE, related_name='feedback', to='core.Submission'),
+ field=models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, related_name='feedback', to='core.Submission'),
),
migrations.AlterUniqueTogether(
name='test',
unique_together={('submission', 'name')},
),
migrations.AlterUniqueTogether(
- name='submission',
- unique_together={('type', 'student')},
+ name='submissionsubscription',
+ unique_together={('owner', 'query_key', 'query_type', 'feedback_stage')},
),
migrations.AlterUniqueTogether(
- name='generaltasksubscription',
- unique_together={('owner', 'query_key', 'query_type')},
+ name='submission',
+ unique_together={('type', 'student')},
),
migrations.AlterUniqueTogether(
name='feedbackcomment',
diff --git a/core/migrations/0002_auto_20180202_1421.py b/core/migrations/0002_auto_20180202_1421.py
deleted file mode 100644
index f06014c1..00000000
--- a/core/migrations/0002_auto_20180202_1421.py
+++ /dev/null
@@ -1,17 +0,0 @@
-# Generated by Django 2.0.2 on 2018-02-02 14:21
-
-from django.db import migrations
-
-
-class Migration(migrations.Migration):
-
- dependencies = [
- ('core', '0001_initial'),
- ]
-
- operations = [
- migrations.AlterUniqueTogether(
- name='generaltasksubscription',
- unique_together={('owner', 'query_key', 'query_type', 'feedback_stage')},
- ),
- ]
diff --git a/core/migrations/0002_submissionsubscription_deactivated.py b/core/migrations/0002_submissionsubscription_deactivated.py
new file mode 100644
index 00000000..d10cbe69
--- /dev/null
+++ b/core/migrations/0002_submissionsubscription_deactivated.py
@@ -0,0 +1,18 @@
+# Generated by Django 2.0.2 on 2018-02-08 15:50
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+ dependencies = [
+ ('core', '0001_initial'),
+ ]
+
+ operations = [
+ migrations.AddField(
+ model_name='submissionsubscription',
+ name='deactivated',
+ field=models.BooleanField(default=False),
+ ),
+ ]
diff --git a/core/migrations/0003_auto_20180207_1922.py b/core/migrations/0003_auto_20180207_1922.py
deleted file mode 100644
index b76df083..00000000
--- a/core/migrations/0003_auto_20180207_1922.py
+++ /dev/null
@@ -1,18 +0,0 @@
-# Generated by Django 2.0.2 on 2018-02-07 19:22
-
-from django.db import migrations
-
-
-class Migration(migrations.Migration):
-
- dependencies = [
- ('core', '0002_auto_20180202_1421'),
- ]
-
- operations = [
- migrations.RenameField(
- model_name='feedbackcomment',
- old_name='is_final',
- new_name='visible_to_student',
- ),
- ]
diff --git a/core/models.py b/core/models.py
index 08e852ee..77f8fcc0 100644
--- a/core/models.py
+++ b/core/models.py
@@ -420,11 +420,15 @@ class SubscriptionEnded(Exception):
pass
-class AssignmentError(Exception):
+class SubscriptionTemporarilyEnded(Exception):
pass
-class GeneralTaskSubscription(models.Model):
+class NotMoreThanTwoOpenAssignmentsAllowed(Exception):
+ pass
+
+
+class SubmissionSubscription(models.Model):
RANDOM = 'random'
STUDENT_QUERY = 'student'
@@ -449,7 +453,7 @@ class GeneralTaskSubscription(models.Model):
FEEDBACK_VALIDATION = 'feedback-validation'
FEEDBACK_CONFLICT_RESOLUTION = 'feedback-conflict-resolution'
- stage_to_assignment_count_map = {
+ assignment_count_on_stage = {
FEEDBACK_CREATION: 0,
FEEDBACK_VALIDATION: 1,
FEEDBACK_CONFLICT_RESOLUTION: 2,
@@ -461,6 +465,7 @@ class GeneralTaskSubscription(models.Model):
(FEEDBACK_CONFLICT_RESOLUTION, 'Previous correctors disagree'),
)
+ deactivated = models.BooleanField(default=False)
subscription_id = models.UUIDField(primary_key=True,
default=uuid.uuid4,
editable=False)
@@ -488,92 +493,77 @@ 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):
- return self._get_submission_base_query().filter(
+ def _get_submissions_that_do_not_have_final_feedback(self):
+ return self._get_submission_base_query().exclude(
Q(feedback__isnull=False),
- Q(feedback__is_final=False),
- ~Q(assignments__subscription__owner=self.owner)
- ).annotate( # to display only manual
+ Q(feedback__is_final=True)
+ ).exclude(
+ Q(assignments__subscription__owner=self.owner)
+ ).annotate(
done_assignments_count=Count(
Case(When(assignments__is_done=True, then=Value(1)),
output_field=IntegerField(),)
)
- ).filter(done_assignments_count=self.stage_to_assignment_count_map[
- done_assignments_count])
-
- def _find_unassigned_non_final_submissions(self):
- unassigned_non_final_submissions = \
- self._get_submission_base_query().filter(
- Q(assignments__isnull=True),
- Q(feedback__isnull=True)
- )
-
- log.debug('unassigned non final submissions %s',
- unassigned_non_final_submissions)
-
- return unassigned_non_final_submissions
-
- def _find_unassigned_unapproved_non_final_submissions(self):
- return self._find_submissions_based_on_completed_assignments(
- done_assignments_count=self.FEEDBACK_VALIDATION)
-
- def _find_submissions_with_conflicting_feedback(self):
- return self._find_submissions_based_on_completed_assignments(
- done_assignments_count=self.FEEDBACK_CONFLICT_RESOLUTION)
+ )
- def _get_next_assignment_in_subscription(self):
- candidates = (
- self._find_unassigned_non_final_submissions,
- self._find_unassigned_unapproved_non_final_submissions,
- self._find_submissions_with_conflicting_feedback
- )[self.stage_to_assignment_count_map[self.feedback_stage]]()
+ def _get_available_submissions_in_subscription_stage(self):
+ candidates = self._get_submissions_that_do_not_have_final_feedback()
if candidates.count() == 0:
raise SubscriptionEnded(
f'The task which user {self.owner} subscribed to is done')
- return candidates.first()
+ done_assignments_count = self.assignment_count_on_stage[self.feedback_stage] # noqa
+ stage_candiates = candidates.filter(
+ done_assignments_count=done_assignments_count
+ )
+
+ if stage_candiates.count() == 0:
+ raise SubscriptionTemporarilyEnded(
+ 'Currently unavailabe. Please check for more soon. '
+ 'Submissions remaining: %s' % stage_candiates.count())
+
+ return stage_candiates
@transaction.atomic
def get_or_create_work_assignment(self):
- task = self._get_next_assignment_in_subscription()
+ task = self._get_available_submissions_in_subscription_stage().first()
+ if self.assignments.filter(is_done=False).count() >= 2:
+ raise NotMoreThanTwoOpenAssignmentsAllowed(
+ 'Not more than 2 active assignments allowed.')
return TutorSubmissionAssignment.objects.get_or_create(
subscription=self,
submission=task)[0]
+ @transaction.atomic
def reserve_all_assignments_for_a_student(self):
assert self.query_type == self.STUDENT_QUERY
- try:
- while True:
- self.get_or_create_work_assignment()
- except SubscriptionEnded as err:
- log.info(f'Loaded all subscriptions of student {self.query_key}')
+ submissions = self._get_submissions_that_do_not_have_final_feedback()
- def _create_new_assignment_if_subscription_empty(self):
- if self.assignments.filter(is_done=False).count() < 1:
- self.get_or_create_work_assignment()
+ for submission in submissions:
+ if hasattr(submission, 'assignments'):
+ submission.assignments.filter(is_done=False).delete()
+ TutorSubmissionAssignment.objects.create(
+ subscription=self,
+ submission=submission
+ )
- def _eagerly_reserve_the_next_assignment(self):
- if self.assignments.filter(is_done=False).count() < 2:
- self.get_or_create_work_assignment()
+ log.info(f'Loaded all subscriptions of student {self.query_key}')
+
+ @transaction.atomic
+ def delete(self):
+ self.assignments.filter(is_done=False).delete()
+ if self.assignments.count() == 0:
+ super().delete()
+ else:
+ self.deactivated = True
+ self.save()
- def get_oldest_unfinished_assignment(self):
- self._create_new_assignment_if_subscription_empty()
- return self.assignments \
- .filter(is_done=False) \
- .order_by('created') \
- .first()
- def get_youngest_unfinished_assignment(self):
- self._create_new_assignment_if_subscription_empty()
- self._eagerly_reserve_the_next_assignment()
- return self.assignments \
- .filter(is_done=False) \
- .order_by('-created') \
- .first()
+class DeletionOfDoneAssignmentsNotPermitted(Exception):
+ pass
class TutorSubmissionAssignment(models.Model):
@@ -584,7 +574,7 @@ class TutorSubmissionAssignment(models.Model):
submission = models.ForeignKey(Submission,
on_delete=models.CASCADE,
related_name='assignments')
- subscription = models.ForeignKey(GeneralTaskSubscription,
+ subscription = models.ForeignKey(SubmissionSubscription,
on_delete=models.CASCADE,
related_name='assignments')
is_done = models.BooleanField(default=False)
@@ -599,6 +589,11 @@ class TutorSubmissionAssignment(models.Model):
return (f'{self.subscription.owner} assigned to {self.submission}'
f' (done={self.is_done})')
+ def delete(self, *args, **kwargs):
+ if self.is_done:
+ raise DeletionOfDoneAssignmentsNotPermitted()
+ super().delete(*args, **kwargs)
+
class FeedbackComment(models.Model):
diff --git a/core/serializers/feedback.py b/core/serializers/feedback.py
index 95e0e0d9..b57b21a6 100644
--- a/core/serializers/feedback.py
+++ b/core/serializers/feedback.py
@@ -1,14 +1,13 @@
import logging
from collections import defaultdict
-from django.core.exceptions import ObjectDoesNotExist
from django.db import transaction
from django.db.models.manager import Manager
from rest_framework import serializers
from rest_framework.utils import html
from core import models
-from core.models import Feedback, TutorSubmissionAssignment
+from core.models import Feedback
from util.factories import GradyUserFactory
from .generic import DynamicFieldsModelSerializer
@@ -82,16 +81,16 @@ class FeedbackCommentSerializer(serializers.ModelSerializer):
class FeedbackSerializer(DynamicFieldsModelSerializer):
- pk = serializers.ReadOnlyField(source='of_submission.pk')
- assignment_pk = serializers.UUIDField(write_only=True)
feedback_lines = FeedbackCommentSerializer(many=True, required=False)
@transaction.atomic
def create(self, validated_data) -> Feedback:
- assignment = validated_data.pop('assignment_pk')
+ submission = validated_data.pop('of_submission')
+ assignment = submission.assignments.get(
+ subscription__owner=self.context['request'].user)
feedback_lines = validated_data.pop('feedback_lines', [])
- feedback = Feedback.objects.create(of_submission=assignment.submission,
+ feedback = Feedback.objects.create(of_submission=submission,
**validated_data)
for comment in feedback_lines:
@@ -102,7 +101,7 @@ class FeedbackSerializer(DynamicFieldsModelSerializer):
)
assignment.set_done()
- return Feedback.objects.get(of_submission=assignment.submission)
+ return Feedback.objects.get(of_submission=submission)
@transaction.atomic
def update(self, instance, validated_data):
@@ -115,21 +114,26 @@ class FeedbackSerializer(DynamicFieldsModelSerializer):
return super().update(instance, validated_data)
- def validate_assignment_pk(self, assignment_pk):
- try:
- assignment = TutorSubmissionAssignment.objects.get(
- pk=assignment_pk)
- except ObjectDoesNotExist as err:
- raise serializers.ValidationError('No assignment for given id.')
+ def validate_of_submission(self, submission):
+ feedback = self.instance
+ if feedback is not None and feedback.submission is not submission:
+ raise serializers.ValidationError(
+ 'It is not allowed to update this field.')
- return assignment
+ return submission
def validate(self, data):
- log.debug("Validate feedback data: %s", data)
- score = data.get('score')
- assignment = data.get('assignment_pk')
+ if self.instance:
+ score = data.get('score', self.instance.score)
+ submission = data.get('of_submission', self.instance.of_submission)
+ else:
+ try:
+ score = data.get('score')
+ submission = data.get('of_submission')
+ except KeyError as err:
+ raise serializers.ValidationError(
+ 'You need a score and a submission.')
- submission = assignment.submission
if not 0 <= score <= submission.type.full_score:
raise serializers.ValidationError(
f'Score has to be in range [0..{submission.type.full_score}].')
@@ -157,4 +161,4 @@ class FeedbackSerializer(DynamicFieldsModelSerializer):
class Meta:
model = Feedback
- fields = ('pk', 'assignment_pk', 'is_final', 'score', 'feedback_lines')
+ fields = ('pk', 'of_submission', 'is_final', 'score', 'feedback_lines')
diff --git a/core/serializers/subscription.py b/core/serializers/subscription.py
index 9bbeaab5..ef9a63c8 100644
--- a/core/serializers/subscription.py
+++ b/core/serializers/subscription.py
@@ -1,18 +1,24 @@
-from django.core.exceptions import ObjectDoesNotExist
from rest_framework import serializers
-from core.models import (GeneralTaskSubscription, Submission,
+from core.models import (Submission, SubmissionSubscription,
TutorSubmissionAssignment)
from core.serializers import DynamicFieldsModelSerializer, FeedbackSerializer
class AssignmentSerializer(DynamicFieldsModelSerializer):
- submission_pk = serializers.ReadOnlyField(
- source='submission.pk')
+ submission_pk = serializers.ReadOnlyField(source='submission.pk')
class Meta:
model = TutorSubmissionAssignment
- fields = ('pk', 'submission_pk', 'is_done',)
+ fields = ('pk', 'submission_pk', 'is_done', 'subscription',)
+ read_only_fields = ('is_done',)
+ extra_kwargs = {
+ 'subscription': {'write_only': True},
+ }
+
+ def create(self, validated_data):
+ subscription = validated_data.get('subscription')
+ return subscription.get_or_create_work_assignment()
class SubmissionAssignmentSerializer(DynamicFieldsModelSerializer):
@@ -43,33 +49,28 @@ class SubscriptionSerializer(DynamicFieldsModelSerializer):
data['owner'] = self.context['request'].user
if 'query_key' in data != \
- data['query_type'] == GeneralTaskSubscription.RANDOM:
+ data['query_type'] == SubmissionSubscription.RANDOM:
raise serializers.ValidationError(
f'The {data["query_type"]} query_type does not work with the'
f'provided key')
- try:
- GeneralTaskSubscription.objects.get(
- owner=data['owner'],
- query_type=data['query_type'],
- query_key=data.get('query_key', None))
- except ObjectDoesNotExist:
- pass
- else:
- raise serializers.ValidationError(
- 'The user already has the subscription')
-
return data
- def create(self, validated_data) -> GeneralTaskSubscription:
- return GeneralTaskSubscription.objects.create(**validated_data)
+ def create(self, validated_data) -> SubmissionSubscription:
+ return SubmissionSubscription.objects.create(**validated_data)
+
+ def update(self, instance, validated_data):
+ instance.deactivated = False
+ instance.save()
+ return instance
class Meta:
- model = GeneralTaskSubscription
- fields = (
- 'pk',
- 'owner',
- 'query_type',
- 'query_key',
- 'assignments',
- 'feedback_stage')
+ model = SubmissionSubscription
+ fields = ('pk',
+ 'owner',
+ 'query_type',
+ 'query_key',
+ 'feedback_stage',
+ 'deactivated',
+ 'assignments')
+ read_only_fields = ('deactivated',)
diff --git a/core/tests/test_feedback.py b/core/tests/test_feedback.py
index 17b3cbfd..8397faf8 100644
--- a/core/tests/test_feedback.py
+++ b/core/tests/test_feedback.py
@@ -109,7 +109,7 @@ class FeedbackCreateTestCase(APITestCase):
def setUp(self):
self.client.force_authenticate(user=self.tutor)
- self.subscription = models.GeneralTaskSubscription.objects.create(
+ self.subscription = models.SubmissionSubscription.objects.create(
owner=self.tutor,
query_type='random',
query_key=''
@@ -123,7 +123,7 @@ class FeedbackCreateTestCase(APITestCase):
data = {
'score': 10,
'is_final': False,
- 'assignment_pk': self.assignment.pk
+ 'of_submission': self.assignment.submission.pk
}
self.assertEqual(Feedback.objects.count(), 0)
@@ -135,7 +135,7 @@ class FeedbackCreateTestCase(APITestCase):
data = {
'score': 101,
'is_final': False,
- 'assignment_pk': self.assignment.pk
+ 'of_submission': self.assignment.submission.pk
}
self.assertEqual(Feedback.objects.count(), 0)
response = self.client.post(self.url, data, format='json')
@@ -146,7 +146,7 @@ class FeedbackCreateTestCase(APITestCase):
data = {
'score': 100,
'is_final': True,
- 'assignment_pk': self.assignment.assignment_id
+ 'of_submission': self.assignment.submission.pk
}
response = self.client.post(self.url, data, format='json')
self.assertEqual(status.HTTP_403_FORBIDDEN, response.status_code)
@@ -156,7 +156,7 @@ class FeedbackCreateTestCase(APITestCase):
data = {
'score': 50,
'is_final': False,
- 'assignment_pk': self.assignment.assignment_id
+ 'of_submission': self.assignment.submission.pk
}
response = self.client.post(self.url, data, format='json')
self.assertEqual(status.HTTP_400_BAD_REQUEST, response.status_code)
@@ -166,7 +166,7 @@ class FeedbackCreateTestCase(APITestCase):
data = {
'score': -1,
'is_final': False,
- 'assignment_pk': self.assignment.pk
+ 'of_submission': self.assignment.submission.pk
}
self.assertEqual(Feedback.objects.count(), 0)
response = self.client.post(self.url, data, format='json')
@@ -177,7 +177,7 @@ class FeedbackCreateTestCase(APITestCase):
data = {
'score': 5,
'is_final': False,
- 'assignment_pk': self.assignment.pk,
+ 'of_submission': self.assignment.submission.pk,
'feedback_lines': {
'4': {
'text': 'Why you no learn how to code, man?'
@@ -192,7 +192,7 @@ class FeedbackCreateTestCase(APITestCase):
data = {
'score': 0,
'is_final': False,
- 'assignment_pk': self.assignment.pk,
+ 'of_submission': self.assignment.submission.pk,
'feedback_lines': {
'4': {
'text': 'Nice meth!'
@@ -208,7 +208,7 @@ class FeedbackCreateTestCase(APITestCase):
data = {
'score': 0,
'is_final': False,
- 'assignment_pk': self.assignment.pk,
+ 'of_submission': self.assignment.submission.pk,
'feedback_lines': {
'3': {
'text': 'Nice meth!'
@@ -226,7 +226,7 @@ class FeedbackCreateTestCase(APITestCase):
def test_cannot_create_without_assignment(self):
data = {
'score': 0,
- 'assignment_pk': self.assignment.assignment_id,
+ 'of_submission': self.assignment.submission.pk,
'feedback_lines': {
'2': {
'text': 'Well, at least you tried.'
@@ -235,12 +235,12 @@ class FeedbackCreateTestCase(APITestCase):
}
self.assignment.delete()
response = self.client.post(self.url, data, format='json')
- self.assertEqual(status.HTTP_400_BAD_REQUEST, response.status_code)
+ self.assertEqual(status.HTTP_403_FORBIDDEN, response.status_code)
def test_cannot_create_with_someoneelses_assignment(self):
data = {
'score': 0,
- 'assignment_pk': self.assignment.assignment_id,
+ 'of_submission': self.assignment.submission.pk,
'feedback_lines': {
'1': {
'text': 'Well, at least you tried.'
@@ -256,7 +256,7 @@ class FeedbackCreateTestCase(APITestCase):
data = {
'score': 0,
'is_final': False,
- 'assignment_pk': self.assignment.pk,
+ 'of_submission': self.assignment.submission.pk,
'feedback_lines': {
'1': {
'text': 'Nice meth!'
@@ -316,7 +316,7 @@ class FeedbackPatchTestCase(APITestCase):
self.tutor01 = self.data['tutors'][0]
self.tutor02 = self.data['tutors'][1]
self.client.force_authenticate(user=self.tutor01)
- self.subscription = models.GeneralTaskSubscription.objects.create(
+ self.subscription = models.SubmissionSubscription.objects.create(
owner=self.tutor01,
query_type='random',
query_key=''
@@ -325,13 +325,14 @@ class FeedbackPatchTestCase(APITestCase):
data = {
'score': 35,
'is_final': False,
- 'assignment_pk': self.assignment.assignment_id,
+ 'of_submission': self.assignment.submission.pk,
'feedback_lines': {
'2': {'text': 'Very good.'},
}
}
response = self.client.post(self.burl, data, format='json')
- self.feedback = Feedback.objects.get(of_submission=response.data['pk'])
+ self.feedback = Feedback.objects.get(
+ of_submission=response.data['of_submission'])
self.url = f'{self.burl}{self.feedback.of_submission.submission_id}/'
def test_can_patch_onto_the_own_feedback(self):
@@ -363,7 +364,7 @@ class FeedbackPatchTestCase(APITestCase):
def test_tutor_can_not_update_when_there_is_a_new_assignment(self):
# Step 1 - Create a new assignment
- second_subs = models.GeneralTaskSubscription.objects.create(
+ second_subs = models.SubmissionSubscription.objects.create(
owner=self.tutor02,
query_type='random',
feedback_stage='feedback-validation'
diff --git a/core/tests/test_subscription_assignment_service.py b/core/tests/test_subscription_assignment_service.py
index 01d418e0..24721706 100644
--- a/core/tests/test_subscription_assignment_service.py
+++ b/core/tests/test_subscription_assignment_service.py
@@ -2,12 +2,12 @@ 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 core.models import (Submission, SubmissionSubscription, SubmissionType,
+ SubscriptionEnded, SubscriptionTemporarilyEnded)
from util.factories import GradyUserFactory, make_test_data
-class GeneralTaskSubscriptionRandomTest(APITestCase):
+class SubmissionSubscriptionRandomTest(APITestCase):
@classmethod
def setUpTestData(cls):
@@ -27,36 +27,69 @@ class GeneralTaskSubscriptionRandomTest(APITestCase):
type=self.submission_type, student=self.s2.student,
text='I like apples')
- self.subscription = GeneralTaskSubscription.objects.create(
- owner=self.t, query_type=GeneralTaskSubscription.RANDOM)
+ self.subscription = SubmissionSubscription.objects.create(
+ owner=self.t, query_type=SubmissionSubscription.RANDOM)
def test_subscription_gets_an_assignment(self):
- self.subscription._create_new_assignment_if_subscription_empty()
+ self.subscription.get_or_create_work_assignment()
self.assertEqual(1, self.subscription.assignments.count())
def test_first_work_assignment_was_created_unfinished(self):
- self.subscription._create_new_assignment_if_subscription_empty()
+ self.subscription.get_or_create_work_assignment()
self.assertFalse(self.subscription.assignments.first().is_done)
def test_subscription_raises_error_when_depleted(self):
self.submission_01.delete()
self.submission_02.delete()
try:
- self.subscription._create_new_assignment_if_subscription_empty()
+ self.subscription.get_or_create_work_assignment()
except SubscriptionEnded as err:
self.assertFalse(False)
else:
self.assertTrue(False)
def test_can_prefetch(self):
- self.subscription._create_new_assignment_if_subscription_empty()
- self.subscription._eagerly_reserve_the_next_assignment()
+ self.subscription.get_or_create_work_assignment()
+ self.subscription.get_or_create_work_assignment()
self.assertEqual(2, self.subscription.assignments.count())
- def test_oldest_assignment_is_current(self):
- assignment = self.subscription.get_oldest_unfinished_assignment()
- self.assertEqual(assignment,
- self.subscription.get_oldest_unfinished_assignment())
+ def test_new_subscription_is_temporarily_unavailabe(self):
+ validation = SubmissionSubscription.objects.create(
+ owner=self.t, query_type=SubmissionSubscription.RANDOM,
+ feedback_stage=SubmissionSubscription.FEEDBACK_VALIDATION)
+ try:
+ validation.get_or_create_work_assignment()
+ except SubscriptionTemporarilyEnded as err:
+ self.assertTrue(True)
+ else:
+ self.assertTrue(False)
+
+ def test_delete_with_done_assignments_subscription_remains(self):
+ first = self.subscription.get_or_create_work_assignment()
+ self.subscription.get_or_create_work_assignment()
+ self.assertEqual(2, self.subscription.assignments.count())
+
+ first.is_done = True
+ first.save()
+ self.subscription.delete()
+ self.assertEqual(1, self.subscription.assignments.count())
+
+ def test_delete_without_done_assignments_subscription_is_deleted(self):
+ self.subscription.delete()
+ self.assertEqual(0, models.SubmissionSubscription.objects.count())
+
+ def test_assignment_delete_of_done_not_permitted(self):
+ first = self.subscription.get_or_create_work_assignment()
+ first.is_done = True
+ first.save()
+
+ self.assertRaises(models.DeletionOfDoneAssignmentsNotPermitted,
+ first.delete)
+
+ def test_assignment_delete_undone_permitted(self):
+ first = self.subscription.get_or_create_work_assignment()
+ first.delete()
+ self.assertEqual(0, self.subscription.assignments.count())
class TestApiEndpoints(APITestCase):
@@ -158,22 +191,31 @@ class TestApiEndpoints(APITestCase):
client = APIClient()
client.force_authenticate(user=self.data['tutors'][0])
- response_subs = client.post(
+ response_subscription_create = client.post(
'/api/subscription/', {'query_type': 'random'})
- subscription_id = response_subs.data['pk']
- assignment_pk = response_subs.data['assignments'][0]['pk']
+ subscription_pk = response_subscription_create.data['pk']
- response = client.get(
- f'/api/subscription/{subscription_id}/assignments/current/')
+ subscription_pk = response_subscription_create.data['pk']
+ response_assignment = client.post(
+ f'/api/assignment/', {
+ 'subscription': subscription_pk
+ })
+
+ assignment_pk = response_assignment.data['pk']
+ response_subscription = client.get(
+ f'/api/subscription/{subscription_pk}/')
- self.assertEqual(1, len(response_subs.data['assignments']))
- self.assertEqual(assignment_pk,
- response.data['pk'])
+ self.assertEqual(1, len(response_subscription.data['assignments']))
+ self.assertEqual(response_assignment.data['pk'],
+ response_subscription.data['assignments'][0]['pk'])
- response_next = client.get(
- f'/api/subscription/{subscription_id}/assignments/next/')
+ subscription_pk = response_subscription.data['pk']
+ response_next = client.post(
+ f'/api/assignment/', {
+ 'subscription': subscription_pk
+ })
response_detail_subs = \
- client.get(f'/api/subscription/{subscription_id}/')
+ client.get(f'/api/subscription/{subscription_pk}/')
self.assertEqual(2, len(response_detail_subs.data['assignments']))
self.assertNotEqual(assignment_pk, response_next.data['pk'])
@@ -205,13 +247,17 @@ class TestApiEndpoints(APITestCase):
'feedback_stage': 'feedback-creation'
})
- self.assertEqual(response.status_code, status.HTTP_201_CREATED)
+ self.assertEqual(status.HTTP_201_CREATED, response.status_code)
- assignment_pk = response.data['assignments'][0]['pk']
+ subscription_pk = response.data['pk']
+ response = client.post(
+ f'/api/assignment/', {
+ 'subscription': subscription_pk
+ })
response = client.post(
f'/api/feedback/', {
"score": 23,
- "assignment_pk": assignment_pk,
+ "of_submission": response.data['submission']['pk'],
"feedback_lines": {
2: {"text": "< some string >"},
3: {"text": "< some string >"}
@@ -219,7 +265,7 @@ class TestApiEndpoints(APITestCase):
}
)
print(response, response.data)
- self.assertEqual(response.status_code, status.HTTP_201_CREATED)
+ self.assertEqual(status.HTTP_201_CREATED, response.status_code)
# some other tutor reviews it
client.force_authenticate(user=self.data['tutors'][1])
@@ -230,13 +276,15 @@ class TestApiEndpoints(APITestCase):
'feedback_stage': 'feedback-validation'
})
- self.assertEqual(response.status_code, status.HTTP_201_CREATED)
+ self.assertEqual(status.HTTP_201_CREATED, response.status_code)
- subscription_id = response.data['pk']
- response = client.get(
- f'/api/subscription/{subscription_id}/assignments/current/')
+ subscription_pk = response.data['pk']
+ response = client.post(
+ f'/api/assignment/', {
+ 'subscription': subscription_pk
+ })
- self.assertEqual(response.status_code, status.HTTP_200_OK)
+ self.assertEqual(status.HTTP_201_CREATED, response.status_code)
submission_id_in_database = models.Feedback.objects.filter(
is_final=False).first().of_submission.submission_id
submission_id_in_response = \
diff --git a/core/urls.py b/core/urls.py
index e11beea8..3eaaa62d 100644
--- a/core/urls.py
+++ b/core/urls.py
@@ -12,7 +12,8 @@ router.register('examtype', views.ExamApiViewSet)
router.register('feedback', views.FeedbackApiView)
router.register('submissiontype', views.SubmissionTypeApiView)
router.register('tutor', views.TutorApiViewSet, base_name='tutor')
-router.register('subscription', views.SubscriptionApiViewSet)
+router.register('subscription', views.SubscriptionApiViewSet,
+ base_name='subscription')
router.register('assignment', views.AssignmentApiViewSet)
# regular views that are not viewsets
diff --git a/core/views/feedback.py b/core/views/feedback.py
index 4bdf36f6..f0af99b1 100644
--- a/core/views/feedback.py
+++ b/core/views/feedback.py
@@ -24,9 +24,19 @@ class FeedbackApiView(
user_is_tutor = self.request.user.role == models.UserAccount.TUTOR
return feedback_is_final and user_is_tutor
+ def _get_implicit_assignment_for_user(self, submission):
+ return models.TutorSubmissionAssignment.objects.get(
+ subscription__owner=self.request.user,
+ submission=submission
+ )
+
def _request_user_does_not_own_assignment(self, serializer):
- assignment = serializer.validated_data['assignment_pk']
- return assignment.subscription.owner != self.request.user
+ try:
+ submission = serializer.validated_data['of_submission']
+ assignment = self._get_implicit_assignment_for_user(submission)
+ return assignment.subscription.owner != self.request.user
+ except models.TutorSubmissionAssignment.DoesNotExist as err:
+ return True
def _tutor_attempts_to_set_first_feedback_final(self, serializer):
is_final_set = serializer.validated_data.get('is_final', False)
@@ -34,18 +44,20 @@ class FeedbackApiView(
return is_final_set and user_is_tutor
def _tutor_is_allowed_to_change_own_feedback(self, serializer):
- assignment = serializer.validated_data['assignment_pk']
- youngest = models.TutorSubmissionAssignment.objects\
- .filter(submission=assignment.submission)\
- .order_by('-created')\
+ submission = self.get_object().of_submission
+ assignment = self._get_implicit_assignment_for_user(submission)
+ youngest = models.TutorSubmissionAssignment.objects \
+ .filter(submission=submission) \
+ .order_by('-created') \
.first()
return assignment == youngest
def _tutor_attempts_to_patch_first_feedback_final(self, serializer):
is_final_set = serializer.validated_data.get('is_final', False)
- assignment = serializer.validated_data.get('assignment_pk')
- in_creation = assignment.subscription.feedback_stage == models.GeneralTaskSubscription.FEEDBACK_CREATION # noqa
+ submission = self.get_object().of_submission
+ assignment = self._get_implicit_assignment_for_user(submission)
+ in_creation = assignment.subscription.feedback_stage == models.SubmissionSubscription.FEEDBACK_CREATION # noqa
return is_final_set and in_creation
def create(self, request, *args, **kwargs):
@@ -54,12 +66,12 @@ class FeedbackApiView(
if self._tutor_attempts_to_set_first_feedback_final(serializer):
return Response(
- {'It is not allowed to create feedback final for tutors'},
+ {'For tutors it is not allowed to create feedback final.'},
status=status.HTTP_403_FORBIDDEN)
if self._request_user_does_not_own_assignment(serializer):
return Response(
- {'This user has not permission to create this feedback'},
+ {'This user has no permission to create this feedback'},
status=status.HTTP_403_FORBIDDEN)
self.perform_create(serializer)
@@ -68,22 +80,13 @@ class FeedbackApiView(
def partial_update(self, request, **kwargs):
feedback = self.get_object()
- assignment = models.TutorSubmissionAssignment.objects.get(
- subscription__owner=request.user,
- submission=feedback.of_submission
- )
- serializer = self.get_serializer(
- feedback,
- data={'assignment_pk': assignment.pk,
- 'score': feedback.score,
- **request.data},
- partial=True)
+ serializer = self.get_serializer(feedback, data=request.data,
+ partial=True)
serializer.is_valid(raise_exception=True)
-
if self._tutor_attempts_to_change_final_feedback(serializer):
return Response(
- {'Cannot set the first feedback final unless user reviewer'},
+ {"Changing final feedback is not allowed"},
status=status.HTTP_403_FORBIDDEN)
if self._tutor_attempts_to_patch_first_feedback_final(serializer):
diff --git a/core/views/subscription.py b/core/views/subscription.py
index e1920b3b..66908d69 100644
--- a/core/views/subscription.py
+++ b/core/views/subscription.py
@@ -1,13 +1,13 @@
import logging
+from django.core.exceptions import ObjectDoesNotExist
from rest_framework import mixins, status, viewsets
-from rest_framework.decorators import detail_route
from rest_framework.response import Response
from core import models, permissions, serializers
from core.models import TutorSubmissionAssignment
from core.permissions import IsTutorOrReviewer
-from core.serializers import AssignmentSerializer
+from core.serializers import AssignmentDetailSerializer, AssignmentSerializer
log = logging.getLogger(__name__)
@@ -15,64 +15,45 @@ log = logging.getLogger(__name__)
class SubscriptionApiViewSet(
mixins.RetrieveModelMixin,
mixins.CreateModelMixin,
+ mixins.DestroyModelMixin,
mixins.ListModelMixin,
viewsets.GenericViewSet):
permission_classes = (permissions.IsTutorOrReviewer,)
- queryset = models.GeneralTaskSubscription.objects.all()
serializer_class = serializers.SubscriptionSerializer
- def fetch_assignment(self,
- request,
- assignment_fetcher,
- error_message):
- try:
- assignment = assignment_fetcher()
- except models.SubscriptionEnded as err:
- return Response(
- {'Error': error_message},
- status=status.HTTP_410_GONE)
- serializer = serializers.AssignmentDetailSerializer(assignment)
- return Response(serializer.data)
-
- @detail_route(methods=['get'], url_path='assignments/current')
- def current_assignment(self, request, pk=None):
- subscription = self.get_object()
-
- return self.fetch_assignment(
- request,
- subscription.get_oldest_unfinished_assignment,
- 'Seems this subscription has ended')
-
- @detail_route(methods=['get'], url_path='assignments/next')
- def next_assignment(self, request, pk=None):
- subscription = self.get_object()
-
- return self.fetch_assignment(
- request,
- subscription.get_youngest_unfinished_assignment,
- 'Seems there is nothing left to prefetch')
-
def get_queryset(self):
- return models.GeneralTaskSubscription.objects.filter(
+ return models.SubmissionSubscription.objects.filter(
owner=self.request.user)
- def destroy(self, request, pk=None):
- instance = self.get_object()
-
- raise NotImplemented # todo: just impl. deactivate and reactivate
-
- instance.delete()
- return Response(status=status.HTTP_204_NO_CONTENT)
+ def _get_subscription_if_type_exists(self, data):
+ try:
+ return models.SubmissionSubscription.objects.get(
+ owner=self.request.user,
+ query_type=data.get('query_type', ''),
+ query_key=data.get('query_key', ''),
+ feedback_stage=data.get(
+ 'feedback_stage',
+ models.SubmissionSubscription.FEEDBACK_CREATION)
+ )
+ except ObjectDoesNotExist:
+ return None
def create(self, request, *args, **kwargs):
- serializer = self.get_serializer(data=request.data)
+ subscription = self._get_subscription_if_type_exists(request.data)
+ if subscription and not subscription.deactivated:
+ return Response({'Error': 'Subscriptions have to be unique.'},
+ status.HTTP_409_CONFLICT)
+ elif subscription and subscription.deactivated:
+ serializer = self.get_serializer(subscription,
+ data=request.data)
+ else:
+ serializer = self.get_serializer(data=request.data)
+
serializer.is_valid(raise_exception=True)
subscription = serializer.save()
- if subscription.query_type == models.GeneralTaskSubscription.STUDENT_QUERY: # noqa TODO refactor name of generaltasksubscription to SubmissionSubscription
+ if subscription.query_type == models.SubmissionSubscription.STUDENT_QUERY: # noqa TODO refactor name of generaltasksubscription to SubmissionSubscription
subscription.reserve_all_assignments_for_a_student()
- else:
- subscription.get_oldest_unfinished_assignment()
headers = self.get_success_headers(serializer.data)
return Response(serializer.data,
@@ -88,6 +69,21 @@ class AssignmentApiViewSet(
queryset = TutorSubmissionAssignment.objects.all()
serializer_class = AssignmentSerializer
+ def _fetch_assignment(self, serializer):
+ try:
+ serializer.save()
+ except models.SubscriptionEnded as err:
+ return Response({'Error': str(err)},
+ status=status.HTTP_410_GONE)
+ except models.SubscriptionTemporarilyEnded as err:
+ return Response({'Error': str(err)},
+ status=status.HTTP_404_NOT_FOUND)
+ except models.NotMoreThanTwoOpenAssignmentsAllowed as err:
+ return Response({'Error': str(err)},
+ status=status.HTTP_403_FORBIDDEN)
+ serializer = AssignmentDetailSerializer(serializer.instance)
+ return Response(serializer.data, status=status.HTTP_201_CREATED)
+
def get_queryset(self):
""" Get only assignments of that user """
return TutorSubmissionAssignment.objects.filter(
@@ -98,7 +94,12 @@ class AssignmentApiViewSet(
instance = self.get_object()
if instance.is_done:
- return Response(status=status.HTTP_403_FORBIDDEN) # test
+ return Response(status=status.HTTP_403_FORBIDDEN)
instance.delete()
- return Response(status=status.HTTP_204_NO_CONTENT) # test
+ return Response(status=status.HTTP_204_NO_CONTENT)
+
+ def create(self, request, *args, **kwargs):
+ serializer = self.get_serializer(data=request.data)
+ serializer.is_valid(raise_exception=True)
+ return self._fetch_assignment(serializer)
--
GitLab