diff --git a/core/migrations/0002_auto_20180114_1057.py b/core/migrations/0002_auto_20180114_1057.py
new file mode 100644
index 0000000000000000000000000000000000000000..e10404b1a3d7a77f77cf9a36430681574c2a882e
--- /dev/null
+++ b/core/migrations/0002_auto_20180114_1057.py
@@ -0,0 +1,35 @@
+# Generated by Django 2.0.1 on 2018-01-14 10:57
+
+from django.db import migrations, models
+import django.db.models.deletion
+
+
+class Migration(migrations.Migration):
+
+ dependencies = [
+ ('core', '0001_initial'),
+ ]
+
+ operations = [
+ migrations.AlterModelOptions(
+ name='feedbackforsubmissionline',
+ options={'verbose_name': 'Feedback for Submission line', 'verbose_name_plural': 'Feedback Submission lines'},
+ ),
+ migrations.RemoveField(
+ model_name='feedback',
+ name='modified',
+ ),
+ migrations.RemoveField(
+ model_name='feedback',
+ name='of_tutor',
+ ),
+ migrations.RemoveField(
+ model_name='feedback',
+ name='text',
+ ),
+ migrations.AlterField(
+ model_name='feedbackforsubmissionline',
+ name='of_feedback',
+ field=models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='feedback_lines', to='core.Feedback'),
+ ),
+ ]
diff --git a/core/models.py b/core/models.py
index 8bbce3f43e366899792f4451e801bf31383a2a96..4ffd5bcf9f92c6ef6534aceaaaf1b8208b3f6749 100644
--- a/core/models.py
+++ b/core/models.py
@@ -170,8 +170,10 @@ class UserAccount(AbstractUser):
def is_reviewer(self):
return self.role == 'Reviewer'
- def get_feedback_count(self) -> int:
- return self.feedback_list.count()
+ def done_assignments_count(self) -> int:
+ # TODO optimize this query
+ return sum([subscription.assignments.filter(done=True).count()
+ for subscription in self.subscriptions.all()])
@classmethod
def get_students(cls):
@@ -360,43 +362,25 @@ class Feedback(models.Model):
"""
Attributes
----------
+ score : PositiveIntegerField
+ A score that has been assigned to he submission. Is final if it was
+ accepted.
created : DateTimeField
When the feedback was initially created
- modified : DateTimeField
- The last time this feedback was modified
- of_reviewer : ForeignKey
- The reviewer that accepted/corrected a feedback
of_submission : OneToOneField
The submission this feedback belongs to. It finally determines how many
points a student receives for his submission.
- of_tutor : ForeignKey
- The tutor/reviewer how last edited the feedback
origin : IntegerField
Of whom was this feedback originally created. She below for the choices
- score : PositiveIntegerField
- A score that has been assigned to he submission. Is final if it was
- accepted.
- text : TextField
- Detailed description by the tutor about what went wrong.
- Every line in the feedback should correspond with a line in the
- students submission, maybe with additional comments appended.
"""
- text = models.TextField()
score = models.PositiveIntegerField(default=0)
created = models.DateTimeField(auto_now_add=True)
- modified = models.DateTimeField(auto_now=True)
is_final = models.BooleanField(default=False)
of_submission = models.OneToOneField(
Submission,
on_delete=models.CASCADE,
related_name='feedback')
- of_tutor = models.ForeignKey(
- get_user_model(),
- on_delete=models.SET_NULL,
- related_name='feedback_list',
- blank=True,
- null=True)
# how was this feedback created
(
@@ -506,7 +490,7 @@ class GeneralTaskSubscription(models.Model):
base = self._get_submission_base_query().filter(
Q(feedback__isnull=False),
Q(feedback__is_final=False),
- ~Q(feedback__of_tutor=self.owner)
+ ~Q(assignments__subscription__owner=self.owner)
).annotate( # to display only manual
done_assignments_count=Count(
Case(When(assignments__is_done=True, then=Value(1)),
@@ -535,7 +519,7 @@ class GeneralTaskSubscription(models.Model):
return self._find_submissions_based_on_completed_assignments(
done_assignments_count=self.FEEDBACK_VALIDATION)
- def _find_submissions_with_confilcting_feedback(self):
+ def _find_submissions_with_conflicting_feedback(self):
return self._find_submissions_based_on_completed_assignments(
done_assignments_count=self.FEEDBACK_CONFLICT_RESOLUTION)
@@ -543,7 +527,7 @@ class GeneralTaskSubscription(models.Model):
candidates = (
self._find_unassigned_non_final_submissions,
self._find_unassigned_unapproved_non_final_submissions,
- self._find_submissions_with_confilcting_feedback
+ self._find_submissions_with_conflicting_feedback
)[self.type_to_assignment_count_map[self.feedback_stage]]()
if candidates.count() == 0:
@@ -622,13 +606,13 @@ class FeedbackForSubmissionLine(models.Model):
of_line = models.PositiveIntegerField()
of_feedback = models.ForeignKey(
Feedback,
- related_name="feedbacklines",
+ related_name="feedback_lines",
on_delete=models.CASCADE
)
class Meta:
- verbose_name = "Feedback for Submissionline"
- verbose_name_plural = "Feedback Submissionlines"
+ verbose_name = "Feedback for Submission line"
+ verbose_name_plural = "Feedback Submission lines"
class FeedbackComment(models.Model):
diff --git a/core/serializers.py b/core/serializers.py
index c04238896d9fc0dc71f96f82ccd7b2f29ec5c1b9..5dd096095b358040d9c8d2ecf57da30de27f39a4 100644
--- a/core/serializers.py
+++ b/core/serializers.py
@@ -63,7 +63,7 @@ class FeedbackForSubmissionLineSerializer(serializers.BaseSerializer):
class FeedbackSerializer(DynamicFieldsModelSerializer):
assignment_id = serializers.UUIDField(write_only=True)
- feedbacklines = FeedbackForSubmissionLineSerializer(
+ feedback_lines = FeedbackForSubmissionLineSerializer(
required=False
)
isFinal = serializers.BooleanField(source="is_final", required=False)
@@ -80,8 +80,8 @@ class FeedbackSerializer(DynamicFieldsModelSerializer):
of_submission=validated_data['of_submission']
)
assignment = validated_data.pop('assignment')
- if 'feedbacklines' in validated_data:
- for line_no, comment in validated_data['feedbacklines'].items():
+ if 'feedback_lines' in validated_data:
+ for line_no, comment in validated_data['feedback_lines'].items():
line_obj = models.FeedbackForSubmissionLine.objects.create(
of_line=line_no,
of_feedback=feedback
@@ -134,7 +134,7 @@ class FeedbackSerializer(DynamicFieldsModelSerializer):
class Meta:
model = Feedback
fields = ('assignment_id', 'isFinal', 'score',
- 'ofSubmission', 'feedbacklines')
+ 'ofSubmission', 'feedback_lines')
class FeedbackCommentSerializer(serializers.ModelSerializer):
@@ -226,8 +226,8 @@ class StudentInfoSerializerForListView(DynamicFieldsModelSerializer):
class TutorSerializer(DynamicFieldsModelSerializer):
- feedback_count = serializers.IntegerField(source='get_feedback_count',
- read_only=True)
+ done_assignments_count = serializers.IntegerField(
+ read_only=True)
def create(self, validated_data) -> UserAccount:
log.info("Crating tutor from data %s", validated_data)
@@ -236,7 +236,7 @@ class TutorSerializer(DynamicFieldsModelSerializer):
class Meta:
model = UserAccount
- fields = ('username', 'feedback_count')
+ fields = ('username', 'done_assignments_count')
class AssignmentSerializer(DynamicFieldsModelSerializer):
diff --git a/core/tests/test_feedback.py b/core/tests/test_feedback.py
index d7e52b516b2e41620a1808feeec2fcf5e45921a3..75c22ab9b42d88106894b0eb74c64750589dcce1 100644
--- a/core/tests/test_feedback.py
+++ b/core/tests/test_feedback.py
@@ -26,8 +26,7 @@ class FeedbackRetrieveTestCase(APITestCase):
name='Cooking some crystal with Jesse')
cls.sub = Submission.objects.create(student=cls.student.student,
type=cls.submission_type)
- cls.feedback = Feedback.objects.create(of_tutor=cls.tutor,
- score=23, is_final=False,
+ cls.feedback = Feedback.objects.create(score=23, is_final=False,
of_submission=cls.sub)
cls.linelist = []
for line in range(2):
@@ -65,33 +64,33 @@ class FeedbackRetrieveTestCase(APITestCase):
self.assertEqual(self.data.get('score'), self.EXPECTED_SCORE)
def test_if_feedback_contains_linekeys(self):
- self.assertIn('feedbacklines', self.data)
- self.assertIn(1, self.data['feedbacklines'])
- self.assertIn(2, self.data['feedbacklines'])
+ self.assertIn('feedback_lines', self.data)
+ self.assertIn(1, self.data['feedback_lines'])
+ self.assertIn(2, self.data['feedback_lines'])
def test_if_feedback_contains_final(self):
self.assertIn('isFinal', self.data)
self.assertIsNotNone(self.data['isFinal'])
def test_if_comment_contains_text(self):
- self.assertIn('text', self.data['feedbacklines'][1][0])
+ self.assertIn('text', self.data['feedback_lines'][1][0])
self.assertEqual(
- 'fortytwo', self.data['feedbacklines'][1][0]['text'])
+ 'fortytwo', self.data['feedback_lines'][1][0]['text'])
def test_if_comment_contains_created(self):
- self.assertIn('created', self.data['feedbacklines'][1][0])
- self.assertIsNotNone(self.data['feedbacklines'][1][0]['created'])
+ self.assertIn('created', self.data['feedback_lines'][1][0])
+ self.assertIsNotNone(self.data['feedback_lines'][1][0]['created'])
def test_if_comment_has_tutor(self):
- self.assertIn('ofTutor', self.data['feedbacklines'][1][0])
+ self.assertIn('ofTutor', self.data['feedback_lines'][1][0])
self.assertEqual(
self.tutor.username,
- self.data['feedbacklines'][1][0]['ofTutor'])
+ self.data['feedback_lines'][1][0]['ofTutor'])
def test_if_comment_has_final(self):
- self.assertIn('isFinalComment', self.data['feedbacklines'][1][0])
+ self.assertIn('isFinalComment', self.data['feedback_lines'][1][0])
self.assertIsNotNone(
- self.data['feedbacklines'][1][0]['isFinalComment'])
+ self.data['feedback_lines'][1][0]['isFinalComment'])
class FeedbackCreateTestCase(APITestCase):
@@ -170,7 +169,7 @@ class FeedbackCreateTestCase(APITestCase):
'score': 0,
'isFinal': False,
'assignment_id': self.assignment.assignment_id,
- 'feedbacklines': {
+ 'feedback_lines': {
'5': {
'text': 'Nice meth!'
}
@@ -186,7 +185,7 @@ class FeedbackCreateTestCase(APITestCase):
'score': 0,
'isFinal': False,
'assignment_id': self.assignment.assignment_id,
- 'feedbacklines': {
+ 'feedback_lines': {
'5': {
'text': 'Nice meth!'
}
@@ -205,7 +204,7 @@ class FeedbackCreateTestCase(APITestCase):
'score': 0,
'isFinal': False,
'assignment_id': self.assignment.assignment_id,
- 'feedbacklines': {
+ 'feedback_lines': {
'5': {
'text': 'Nice meth!'
},
diff --git a/core/tests/test_student_page.py b/core/tests/test_student_page.py
index 455dfd58f09271c184f244924c6dce94076cd998..a4a86a37e9f77c853b4555bedc061634002f4178 100644
--- a/core/tests/test_student_page.py
+++ b/core/tests/test_student_page.py
@@ -42,9 +42,14 @@ class StudentPageTests(APITestCase):
'type': 'problem01',
'text': 'Too hard for me ;-(',
'feedback': {
- 'of_tutor': 'tutor01',
'text': 'Very bad!',
- 'score': 3
+ 'score': 3,
+ 'feedback_lines': {
+ '1': [{
+ 'text': 'This is very bad!',
+ 'of_tutor': 'tutor01'
+ }],
+ }
}
}]
})
@@ -142,9 +147,14 @@ class StudentSelfSubmissionsTests(APITestCase):
'type': 'problem01',
'text': 'Too hard for me ;-(',
'feedback': {
- 'of_tutor': 'tutor01',
'text': 'Very bad!',
- 'score': 3
+ 'score': 3,
+ 'feedback_lines': {
+ '1': [{
+ 'text': 'This is very bad!',
+ 'of_tutor': 'tutor01'
+ }],
+ }
}
}]
})
@@ -200,6 +210,12 @@ class StudentSelfSubmissionsTests(APITestCase):
self.submission_list_first_entry['feedback']['score'],
self.student_info.submissions.first().feedback.score)
+ def submssion_feedback_contains_submission_lines(self):
+ self.assertIn(
+ 'feedback_lines',
+ self.submission_list_first_entry['feedback']
+ )
+
# We don't want a matriculation number here
def test_matriculation_number_is_not_send(self):
self.assertNotIn('matrikel_no', self.submission_list_first_entry)
diff --git a/core/tests/test_student_reviewer_viewset.py b/core/tests/test_student_reviewer_viewset.py
index cffccfa3e8f80777200d39e10a7b94a01e5a2b7f..ddc317546e63aad954f6dea37a24a93c9b53a71f 100644
--- a/core/tests/test_student_reviewer_viewset.py
+++ b/core/tests/test_student_reviewer_viewset.py
@@ -43,7 +43,12 @@ class StudentPageTests(APITestCase):
'text': 'Too hard for me ;-(',
'feedback': {
'score': 3,
- 'of_tutor': 'tutor'
+ 'feedback_lines': {
+ '1': [{
+ 'text': 'This is very bad!',
+ 'of_tutor': 'tutor'
+ }],
+ }
}
}]
})
diff --git a/core/tests/test_subscription_assignment_service.py b/core/tests/test_subscription_assignment_service.py
index 1302fd52f02b3488d23d57636c441a26e6e5e3eb..f11b23194a803973949878ed3f33b66c6d8e24fb 100644
--- a/core/tests/test_subscription_assignment_service.py
+++ b/core/tests/test_subscription_assignment_service.py
@@ -98,30 +98,35 @@ class TestApiEndpoints(APITestCase):
'submissions': [
{
'text': 'function blabl\n'
- ' on multi lines\n'
- ' for blabla in bla:\n'
- ' lorem ipsum und so\n',
+ ' on multi lines\n'
+ ' for blabla in bla:\n'
+ ' lorem ipsum und so\n',
'type': '01. Sort this or that',
'user': 'student01',
'feedback': {
- 'text': 'Not good!',
- 'score': 5,
- 'of_tutor': 'tutor01',
- 'is_final': True
+ 'score': 5,
+ 'is_final': True,
+ 'feedback_lines': {
+ '1': [{
+ 'text': 'This is very bad!',
+ 'of_tutor': 'tutor01'
+ }],
+ }
+
}
},
{
'text': 'function blabl\n'
- ' asasxasx\n'
- ' lorem ipsum und so\n',
+ ' asasxasx\n'
+ ' lorem ipsum und so\n',
'type': '02. Merge this or that or maybe even this',
'user': 'student01'
},
{
'text': 'function blabl\n'
- ' on multi lines\n'
- ' asasxasx\n'
- ' lorem ipsum und so\n',
+ ' on multi lines\n'
+ ' asasxasx\n'
+ ' lorem ipsum und so\n',
'type': '03. This one exists for the sole purpose to test',
'user': 'student01'
},
@@ -207,7 +212,7 @@ class TestApiEndpoints(APITestCase):
f'/api/feedback/', {
"score": 23,
"assignment_id": assignment_id,
- "feedbacklines": {
+ "feedback_lines": {
2: {"text": "< some string >"},
3: {"text": "< some string >"}
}
@@ -232,12 +237,11 @@ class TestApiEndpoints(APITestCase):
f'/api/subscription/{subscription_id}/assignments/current/')
self.assertEqual(response.status_code, status.HTTP_200_OK)
-
- submissioin_id_in_database = models.Feedback.objects.filter(
+ submission_id_in_database = models.Feedback.objects.filter(
is_final=False).first().of_submission.submission_id
- submissioin_id_in_response = \
+ submission_id_in_response = \
response.data['submission']['submission_id']
self.assertEqual(
- str(submissioin_id_in_database),
- submissioin_id_in_response)
+ str(submission_id_in_database),
+ submission_id_in_response)
diff --git a/core/tests/test_tutor_api_endpoints.py b/core/tests/test_tutor_api_endpoints.py
index 133b23826e0219086fe911a42f26a7fd4191017f..adbed0b9001e95f016cbcf3706d9c53e019955f6 100644
--- a/core/tests/test_tutor_api_endpoints.py
+++ b/core/tests/test_tutor_api_endpoints.py
@@ -10,7 +10,7 @@ from rest_framework.reverse import reverse
from rest_framework.test import (APIClient, APIRequestFactory, APITestCase,
force_authenticate)
-from core.models import Feedback
+from core.models import TutorSubmissionAssignment
from core.views import TutorApiViewSet
from util.factories import GradyUserFactory
@@ -69,14 +69,17 @@ class TutorListTests(APITestCase):
def test_feedback_count_matches_database(self):
def verify_fields(tutor_obj):
t = get_user_model().objects.get(username=tutor_obj['username'])
- return t.get_feedback_count() == tutor_obj['feedback_count']
+ return t.done_assignments_count() == \
+ tutor_obj['done_assignments_count']
self.assertTrue(all(map(verify_fields, self.response.data)))
- def test_sum_of_feedback_count(self):
- self.assertEqual(sum(obj['feedback_count']
- for obj in self.response.data),
- Feedback.objects.count())
+ def test_sum_of_done_assignments(self):
+ self.assertEqual(
+ sum(obj['done_assignments_count']
+ for obj in self.response.data),
+ TutorSubmissionAssignment.objects.filter(is_done=True).count()
+ )
class TutorCreateTests(APITestCase):
diff --git a/util/factories.py b/util/factories.py
index 7e4711de1a89e06ef469deda8678fee20053e9f3..d8e7c77a236dad106c153cc54476edf8f6ab904f 100644
--- a/util/factories.py
+++ b/util/factories.py
@@ -2,6 +2,7 @@ import configparser
import secrets
import string
+from core import models
from core.models import UserAccount as User
from core.models import (ExamType, Feedback, StudentInfo, Submission,
SubmissionType)
@@ -93,12 +94,12 @@ class GradyUserFactory:
""" Creates a student. Defaults can be passed via kwargs like in
relation managers objects.update method. """
user = self._make_base_user(username, 'Student', **kwargs)
- studentInfo = StudentInfo.objects.get_or_create(user=user)[0]
+ student_info = StudentInfo.objects.get_or_create(user=user)[0]
if matrikel_no:
- studentInfo.matrikel_no = matrikel_no
+ student_info.matrikel_no = matrikel_no
if exam:
- studentInfo.exam = exam
- studentInfo.save()
+ student_info.exam = exam
+ student_info.save()
return user
def make_tutor(self, username=None, **kwargs):
@@ -142,11 +143,24 @@ def make_reviewers(reviewers=[], **kwargs):
def make_feedback(feedback, submission_object):
- feedback['of_tutor'] = User.objects.get(
- username=feedback['of_tutor'])
- return Feedback.objects.update_or_create(
+ feedback_obj = Feedback.objects.update_or_create(
of_submission=submission_object,
- defaults=feedback)[0]
+ score=feedback['score'],
+ is_final=feedback.get('is_final', False)
+ )[0]
+ for line_index, comment_list in feedback['feedback_lines'].items():
+ line = models.FeedbackForSubmissionLine.objects.get_or_create(
+ of_line=int(line_index),
+ of_feedback=feedback_obj
+ )[0]
+ for comment in comment_list:
+ tutor = models.UserAccount.objects.get(
+ username=comment.pop('of_tutor'))
+ models.FeedbackComment.objects.update_or_create(
+ of_line=line,
+ of_tutor=tutor,
+ defaults=comment
+ )
def make_submissions(submissions=[], **kwargs):
@@ -207,17 +221,28 @@ def init_test_instance():
'solution': 'Trivial!'
}
],
- 'students': [{
- 'username': 'student01',
- 'exam': 'Test Exam 01',
- },
+ 'students': [
{
- 'username': 'student02',
- 'exam': 'Test Exam 01',
- }],
- 'tutors': [{
- 'username': 'tutor01'
- }],
+ 'username': 'student01',
+ 'exam': 'Test Exam 01',
+ 'password': 'p'
+ },
+ {
+ 'username': 'student02',
+ 'exam': 'Test Exam 01',
+ 'password': 'p'
+ },
+ ],
+ 'tutors': [
+ {
+ 'username': 'tutor01',
+ 'password': 'p'
+ },
+ {
+ 'username': 'tutor02',
+ 'password': 'p'
+ }
+ ],
'reviewers': [{
'username': 'reviewer01',
'password': 'p'
@@ -233,10 +258,26 @@ def init_test_instance():
'type': '01. Sort this or that',
'user': 'student01',
'feedback': {
- 'text': 'Not good!',
'score': 5,
- 'of_tutor': 'tutor01',
- 'is_final': True
+ 'is_final': True,
+ 'feedback_lines': {
+ '1': [
+ {
+ 'text': 'This is very bad!',
+ 'of_tutor': 'tutor01'
+ },
+ {
+ 'text': 'I agree',
+ 'of_tutor': 'tutor02'
+ }
+ ],
+ '3': [
+ {
+ 'text': 'Even worse!',
+ 'of_tutor': 'tutor02'
+ }
+ ]
+ }
}
},
{
@@ -249,10 +290,26 @@ def init_test_instance():
'type': '02. Merge this or that or maybe even this',
'user': 'student01',
'feedback': {
- 'text': 'Not good!',
'score': 5,
- 'of_tutor': 'tutor01',
- 'is_final': True
+ 'is_final': True,
+ 'feedback_lines': {
+ '1': [
+ {
+ 'text': 'This is very bad!',
+ 'of_tutor': 'tutor01'
+ },
+ {
+ 'text': 'I agree',
+ 'of_tutor': 'tutor02'
+ }
+ ],
+ '3': [
+ {
+ 'text': 'Even worse!',
+ 'of_tutor': 'tutor02'
+ }
+ ]
+ }
}
},
{
@@ -265,10 +322,26 @@ def init_test_instance():
'type': '03. This one exists for the sole purpose to test',
'user': 'student01',
'feedback': {
- 'text': 'Not good!',
'score': 5,
- 'of_tutor': 'tutor01',
- 'is_final': True
+ 'is_final': True,
+ 'feedback_lines': {
+ '1': [
+ {
+ 'text': 'This is very bad!',
+ 'of_tutor': 'tutor01'
+ },
+ {
+ 'text': 'I agree',
+ 'of_tutor': 'tutor02'
+ }
+ ],
+ '3': [
+ {
+ 'text': 'Even worse!',
+ 'of_tutor': 'tutor02'
+ }
+ ]
+ }
}
},
]}