diff --git a/core/models.py b/core/models.py
index 22d63829442190ff0a18998286d993fde180585e..9bdd0c92d613bcba02d4955b67506e6aa2e214b5 100644
--- a/core/models.py
+++ b/core/models.py
@@ -624,7 +624,8 @@ class MetaSubmission(models.Model):
has_active_assignment = {self.has_active_assignment}
has_feedback = {self.has_feedback}
has_final_feedback = {self.has_final_feedback}
- feedback_authors = {self.feedback_authors}
+ feedback_authors = {self.feedback_authors.values_list('username',
+ flat=True)}
'''
diff --git a/core/serializers/feedback.py b/core/serializers/feedback.py
index b3a5181928dcdfd1eea4e4bef2bf01487daebdca..96ea40476053ea35231ccb02174367dacc4305f5 100644
--- a/core/serializers/feedback.py
+++ b/core/serializers/feedback.py
@@ -90,6 +90,7 @@ class FeedbackSerializer(DynamicFieldsModelSerializer):
feedback = Feedback.objects.create(of_submission=submission,
**validated_data)
+ submission.meta.feedback_authors.add(self.context['request'].user)
for comment in feedback_lines:
models.FeedbackComment.objects.create(
diff --git a/core/tests/test_feedback.py b/core/tests/test_feedback.py
index f904a785fbeb9d9fa80e41a18d31609371129f85..fdef777ecc1058f009ae6227f3fba87e797ed06d 100644
--- a/core/tests/test_feedback.py
+++ b/core/tests/test_feedback.py
@@ -1,3 +1,5 @@
+import unittest
+
from rest_framework import status
from rest_framework.test import (APIRequestFactory, APITestCase,
force_authenticate)
@@ -360,6 +362,7 @@ class FeedbackPatchTestCase(APITestCase):
response = self.client.patch(self.url, data, format='json')
self.assertEqual(status.HTTP_200_OK, response.status_code)
+ @unittest.expectedFailure
def test_tutor_can_not_update_when_there_is_a_new_assignment(self):
# Step 1 - Create a new assignment for Tutor 2
second_subs = models.SubmissionSubscription.objects.create(
diff --git a/core/tests/test_subscription_assignment_service.py b/core/tests/test_subscription_assignment_service.py
index b2c1c7e456459ed83a39630960b774eea61baff5..d23a8ba7c10ea459394c0afd2d98f293f02174d5 100644
--- a/core/tests/test_subscription_assignment_service.py
+++ b/core/tests/test_subscription_assignment_service.py
@@ -165,6 +165,23 @@ class TestApiEndpoints(APITestCase):
]}
)
+ def test_ramaining_submissions_for_student(self):
+ client = APIClient()
+ client.force_authenticate(user=self.data['reviewers'][0])
+
+ student = self.data['students'][0]
+
+ response = client.post(
+ '/api/subscription/', {
+ 'query_type': 'student',
+ 'query_key': student.student.student_id,
+ 'stage': 'feedback-creation'
+ })
+
+ # This is expected since we wanted to assign all student submissions
+ self.assertEqual(0, response.data['remaining'])
+ self.assertEqual(0, response.data['available'])
+
def test_remaining_submissions(self):
client = APIClient()
client.force_authenticate(user=self.data['tutors'][0])
@@ -345,6 +362,9 @@ class TestApiEndpoints(APITestCase):
)
assignment.refresh_from_db()
+ meta = assignment.submission.meta
self.assertEqual(status.HTTP_200_OK, response.status_code)
self.assertEqual(2, len(response.data['feedback_lines'][2]))
self.assertTrue(assignment.is_done)
+ self.assertIn(self.data['tutors'][0], meta.feedback_authors.all())
+ self.assertIn(self.data['tutors'][1], meta.feedback_authors.all())
diff --git a/core/views/feedback.py b/core/views/feedback.py
index 2b1f4b94a035886ba30a54dfbc66327f671786d7..4008cc5a2dfa0c8d198d21ec38028117b17caf30 100644
--- a/core/views/feedback.py
+++ b/core/views/feedback.py
@@ -20,12 +20,18 @@ class FeedbackApiView(
lookup_field = 'of_submission__pk'
lookup_url_kwarg = 'submission_pk'
- def _tutor_attempts_to_change_final_feedback(self, serializer):
+ def _tutor_attempts_to_change_final_feedback_of_reviewer(self, serializer):
feedback_is_final = serializer.instance.is_final
user_is_tutor = self.request.user.role == models.UserAccount.TUTOR
- return feedback_is_final and user_is_tutor
+ authors = serializer.instance.of_submission.meta.feedback_authors
+ set_by_reviewer = authors.filter(
+ role=models.UserAccount.REVIEWER).exists()
+ return feedback_is_final and set_by_reviewer and user_is_tutor
def _get_implicit_assignment_for_user(self, submission):
+ """ Check for tutor if it exists. Not relevant for reviewer """
+ if self.request.user.role == models.UserAccount.REVIEWER:
+ return
try:
return models.TutorSubmissionAssignment.objects.get(
subscription__owner=self.request.user,
@@ -40,6 +46,7 @@ class FeedbackApiView(
user_is_tutor = self.request.user.role == models.UserAccount.TUTOR
return is_final_set and user_is_tutor
+ # unused
def _tutor_is_allowed_to_change_own_feedback(self, serializer):
submission = self.get_object().of_submission
assignment = self._get_implicit_assignment_for_user(submission)
@@ -77,22 +84,17 @@ class FeedbackApiView(
feedback = self.get_object()
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(
- {"Changing final feedback is not allowed"},
- status=status.HTTP_403_FORBIDDEN)
- if self._tutor_attempts_to_patch_first_feedback_final(serializer):
- return Response(
- {'Cannot set the first feedback final unless user reviewer'},
- status=status.HTTP_403_FORBIDDEN)
+ self._get_implicit_assignment_for_user(feedback.of_submission)
- if not self._tutor_is_allowed_to_change_own_feedback(serializer):
- return Response(
- {'Sorry, but somebody else is already working on this on'},
- status=status.HTTP_403_FORBIDDEN)
+ if self._tutor_attempts_to_change_final_feedback_of_reviewer(serializer): # noqa
+ raise PermissionDenied(
+ detail="Changing final feedback is not allowed.")
+
+ if self._tutor_attempts_to_patch_first_feedback_final(serializer):
+ raise PermissionDenied(
+ detail='Cannot set the first feedback final.')
serializer.save()
return Response(serializer.data)