From 744df9529e6c6a531449d5a7008e29ad67b53436 Mon Sep 17 00:00:00 2001
From: janmax <j.michal@stud.uni-goettingen.de>
Date: Sat, 17 Feb 2018 15:34:26 +0100
Subject: [PATCH] Fixes a nasty race condition
---
core/apps.py | 3 +++
.../0003_submissiondoneassignmentscount.py | 22 +++++++++++++++++++
core/models.py | 19 +++++++++++-----
core/signals.py | 13 +++++++++++
.../test_subscription_assignment_service.py | 18 +++++----------
util/factories.py | 4 ++--
util/importer.py | 2 +-
7 files changed, 60 insertions(+), 21 deletions(-)
create mode 100644 core/migrations/0003_submissiondoneassignmentscount.py
create mode 100644 core/signals.py
diff --git a/core/apps.py b/core/apps.py
index 089650b7..17b18064 100644
--- a/core/apps.py
+++ b/core/apps.py
@@ -4,3 +4,6 @@ from django.apps import AppConfig
class CoreConfig(AppConfig):
name = 'core'
verbose_name = 'where everything comes together'
+
+ def ready(self):
+ import core.signals # noqa
diff --git a/core/migrations/0003_submissiondoneassignmentscount.py b/core/migrations/0003_submissiondoneassignmentscount.py
new file mode 100644
index 00000000..0d1f0b23
--- /dev/null
+++ b/core/migrations/0003_submissiondoneassignmentscount.py
@@ -0,0 +1,22 @@
+# Generated by Django 2.0.2 on 2018-02-17 12:01
+
+from django.db import migrations, models
+import django.db.models.deletion
+
+
+class Migration(migrations.Migration):
+
+ dependencies = [
+ ('core', '0002_auto_20180210_1727'),
+ ]
+
+ operations = [
+ migrations.CreateModel(
+ name='SubmissionDoneAssignmentsCount',
+ fields=[
+ ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
+ ('done_assignments', models.PositiveIntegerField(default=0)),
+ ('submission', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, related_name='counter', to='core.Submission')),
+ ],
+ ),
+ ]
diff --git a/core/models.py b/core/models.py
index 7e680a27..6b823bdc 100644
--- a/core/models.py
+++ b/core/models.py
@@ -502,6 +502,7 @@ class SubmissionSubscription(models.Model):
if self.query_type == self.RANDOM:
return Submission.objects.all()
+ TutorSubmissionAssignment.objects.select_for_update()
return Submission.objects.filter(
**{self.type_query_mapper[self.query_type]: self.query_key})
@@ -521,11 +522,6 @@ class SubmissionSubscription(models.Model):
Q(assignments__subscription__owner=self.owner)
).exclude(
assignments__is_done=False
- ).annotate(
- done_assignments_count=Count(
- Case(When(assignments__is_done=True, then=Value(1)),
- output_field=IntegerField(),)
- )
)
def _get_available_submissions_in_subscription_stage(self) -> QuerySet:
@@ -546,7 +542,7 @@ class SubmissionSubscription(models.Model):
done_assignments_count = self.assignment_count_on_stage[self.feedback_stage] # noqa
stage_candiates = candidates.filter(
- done_assignments_count=done_assignments_count,
+ counter__done_assignments=done_assignments_count,
)
if stage_candiates.count() == 0:
@@ -607,6 +603,14 @@ class DeletionOfDoneAssignmentsNotPermitted(Exception):
pass
+class SubmissionDoneAssignmentsCount(models.Model):
+
+ submission = models.OneToOneField('submission',
+ related_name='counter',
+ on_delete=models.CASCADE)
+ done_assignments = models.PositiveIntegerField(default=0)
+
+
class TutorSubmissionAssignment(models.Model):
assignment_id = models.UUIDField(primary_key=True,
@@ -623,6 +627,9 @@ class TutorSubmissionAssignment(models.Model):
@transaction.atomic
def set_done(self):
+ if not self.is_done:
+ self.submission.counter.done_assignments += 1
+ self.submission.counter.save()
self.is_done = True
self.save()
diff --git a/core/signals.py b/core/signals.py
new file mode 100644
index 00000000..57173f12
--- /dev/null
+++ b/core/signals.py
@@ -0,0 +1,13 @@
+from core.models import Submission, SubmissionDoneAssignmentsCount
+
+from django.db.models.signals import post_save
+from django.dispatch import receiver
+
+
+@receiver(post_save, sender=Submission)
+def create_counter_after_submission_create(sender,
+ instance,
+ created,
+ **kwargs):
+ if created:
+ SubmissionDoneAssignmentsCount.objects.create(submission=instance)
diff --git a/core/tests/test_subscription_assignment_service.py b/core/tests/test_subscription_assignment_service.py
index ea2533b8..b2c1c7e4 100644
--- a/core/tests/test_subscription_assignment_service.py
+++ b/core/tests/test_subscription_assignment_service.py
@@ -109,12 +109,6 @@ class TestApiEndpoints(APITestCase):
'full_score': 35,
'description': 'Very complicated',
'solution': 'Trivial!'
- },
- {
- 'name': '03. This one exists for the sole purpose to test',
- 'full_score': 30,
- 'description': 'Very complicated',
- 'solution': 'Trivial!'
}
],
'students': [
@@ -160,12 +154,12 @@ class TestApiEndpoints(APITestCase):
' on multi lines\n'
' asasxasx\n'
' lorem ipsum und so\n',
- 'type': '03. This one exists for the sole purpose to test',
- 'user': 'student01'
+ 'type': '01. Sort this or that',
+ 'user': 'student02'
},
{
'text': 'function lorem ipsum etc\n',
- 'type': '03. This one exists for the sole purpose to test',
+ 'type': '02. Merge this or that or maybe even this',
'user': 'student02'
},
]}
@@ -242,17 +236,17 @@ class TestApiEndpoints(APITestCase):
client = APIClient()
client.force_authenticate(user=self.data['reviewers'][0])
- student01 = self.data['students'][0]
+ student = self.data['students'][0]
response = client.post(
'/api/subscription/', {
'query_type': 'student',
- 'query_key': student01.student.student_id,
+ 'query_key': student.student.student_id,
'stage': 'feedback-creation'
})
assignments = response.data['assignments']
- self.assertEqual(2, len(assignments))
+ self.assertEqual(1, len(assignments))
def test_two_tutors_cant_have_assignments_for_same_submission(self):
client = APIClient()
diff --git a/util/factories.py b/util/factories.py
index 89957819..db42ebcb 100644
--- a/util/factories.py
+++ b/util/factories.py
@@ -53,7 +53,7 @@ class GradyUserFactory:
}[role]
def _make_base_user(self, username, role, password=None,
- store_pw=False, name='', **kwargs):
+ store_pw=False, fullname='', **kwargs):
""" This is a specific wrapper for the django update_or_create method of
objects.
* If now username is passed, a generic one will be generated
@@ -72,7 +72,7 @@ class GradyUserFactory:
user, created = UserAccount.objects.update_or_create(
username=username,
- fullname=name,
+ fullname=fullname,
role=role,
defaults=kwargs)
diff --git a/util/importer.py b/util/importer.py
index 683dccc7..9f60042b 100644
--- a/util/importer.py
+++ b/util/importer.py
@@ -110,7 +110,7 @@ def add_tests(submission_obj, tests):
for name, test_data in ((name, tests[name]) for name in TEST_ORDER):
test_obj, created = Test.objects.update_or_create(
- name=test_data['fullname'],
+ name=test_data['name'],
submission=submission_obj,
defaults={
'label': test_data['label'],
--
GitLab