Resolve "Updating own past feedback sets other tutors assignment to is_done=True"

.gitignore

@@ -38,6 +38,7 @@ coverage_html/
 anon-export/
 public/
 geckodriver.log
+.screenshots
 
 # node
 node_modules

.gitlab-ci.yml

@@ -25,7 +25,7 @@ build_test_env:
   artifacts:
     paths:
       - .venv/
-    expire_in: 20 minutes
+    expire_in: 1 days
   cache:
     key: "$CI_JOB_NAME"
     paths:
@@ -44,7 +44,7 @@ build_frontend:
     paths:
       - frontend/dist
       - frontend/node_modules/
-    expire_in: 20 minutes
+    expire_in: 1 days
   cache:
     key: "$CI_JOB_NAME"
     paths:
@@ -106,6 +106,11 @@ test_frontend:
     - python util/format_index.py
     - python manage.py collectstatic --no-input
     - HEADLESS_TESTS=True pytest --ds=grady.settings.test functional_tests
+  artifacts:
+    paths:
+      - functional_tests/screenshots/
+    when: on_failure
+    expire_in: 30 days
 
 test_frontend_unit:
   image: node:carbon

Makefile

@@ -28,13 +28,13 @@ install:
 	pip install -Ur requirements.dev.txt
 
 test:
-	DJANGO_SETTINGS_MODULE=grady.settings pytest
+	pytest --ds=grady.settings core/tests
 
 teste2e:
-	cd frontend && yarn build && cp dist/index.html ../core/templates && cd .. && python util/format_index.py && python manage.py collectstatic --no-input && HEADLESS_TESTS=$(headless) pytest --ds=grady.settings $(path); git checkout core/templates/index.html
+	cd frontend && yarn build && cp dist/index.html ../core/templates && cd .. && python util/format_index.py && python manage.py collectstatic --no-input && HEADLESS_TESTS=$(headless) pytest  --ds=grady.settings $(path); git checkout core/templates/index.html
 
 teste2e-nc:
-	cp frontend/dist/index.html ./core/templates && python util/format_index.py && python manage.py collectstatic --no-input && HEADLESS_TESTS=$(headless) pytest --ds=grady.settings $(path); git checkout core/templates/index.html
+	cp frontend/dist/index.html ./core/templates && python util/format_index.py && python manage.py collectstatic --no-input && HEADLESS_TESTS=$(headless) pytest -n 4 --ds=grady.settings $(path); git checkout core/templates/index.html
 
 
 coverage:

core/models/assignment.py

@@ -14,6 +14,10 @@ class DeletionOfDoneAssignmentsNotPermitted(Exception):
     pass
 
 
+class CanOnlyCallFinishOnUnfinishedAssignments(Exception):
+    pass
+
+
 class TutorSubmissionAssignment(models.Model):
 
     assignment_id = models.UUIDField(primary_key=True,
@@ -32,6 +36,19 @@ class TutorSubmissionAssignment(models.Model):
         return (f'{self.subscription.owner} assigned to {self.submission}'
                 f' (done={self.is_done})')
 
+    def finish(self):
+        self.refresh_from_db()
+        if self.is_done:
+            raise CanOnlyCallFinishOnUnfinishedAssignments()
+
+        meta = self.submission.meta
+        meta.feedback_authors.add(self.subscription.owner)
+        meta.done_assignments += 1
+        meta.has_active_assignment = False
+        self.is_done = True
+        self.save()
+        meta.save()
+
     def delete(self, *args, **kwargs):
         if self.is_done:
             raise DeletionOfDoneAssignmentsNotPermitted()

core/models/submission.py

@@ -63,7 +63,9 @@ class MetaSubmission(models.Model):
     done_assignments = models.PositiveIntegerField(default=0)
     has_active_assignment = models.BooleanField(default=False)
 
+    # Managed by signal!
     has_feedback = models.BooleanField(default=False)
+    # Managed by signal!
     has_final_feedback = models.BooleanField(default=False)
 
     feedback_authors = models.ManyToManyField(get_user_model())

core/serializers/feedback.py

@@ -124,11 +124,11 @@ class FeedbackSerializer(DynamicFieldsModelSerializer):
         feedback_lines = validated_data.pop('feedback_lines', [])
         labels = validated_data.pop('labels', [])
         user = self.context['request'].user
+        final_by_reviewer = validated_data.get('is_final', False) and \
+            user.role == UserAccount.REVIEWER
         feedback = Feedback.objects.create(of_submission=submission,
+                                           final_by_reviewer=final_by_reviewer,
                                            **validated_data)
-        if user.role == UserAccount.REVIEWER:
-            feedback.final_by_reviewer = self.context['request'].data['is_final']
-
         for label in labels:
             feedback.labels.add(label)
 
@@ -143,7 +143,7 @@ class FeedbackSerializer(DynamicFieldsModelSerializer):
             )
             comment_instance.labels.set(labels)
 
-        return Feedback.objects.get(of_submission=submission)
+        return feedback
 
     @transaction.atomic
     def update(self, feedback, validated_data):
@@ -205,8 +205,7 @@ class FeedbackSerializer(DynamicFieldsModelSerializer):
             raise serializers.ValidationError(
                 'Sorry, you have to explain why this does not get full score')
 
-        http_method = self.context['request'].method
-        if hasattr(submission, 'feedback') and http_method == 'POST':
+        if hasattr(submission, 'feedback') and not self.instance:
             raise serializers.ValidationError(
                 'Feedback for this submission already exists')
 

core/signals.py

@@ -49,17 +49,7 @@ def update_after_feedback_save(sender, instance, created, **kwargs):
     log.debug('SIGNAL -- update_after_feedback_save')
     meta = instance.of_submission.meta
     meta.has_feedback = True
-    meta.has_final_feedback = instance.is_final
-
-    undone_assignment = meta.submission.assignments.filter(is_done=False)
-    assert undone_assignment.count() <= 1
-    if undone_assignment.count() > 0:
-        log.debug('SIGNAL -- Completed: %s' % undone_assignment.first())
-        meta.feedback_authors.add(undone_assignment.first().subscription.owner)
-        meta.done_assignments += 1
-        meta.has_active_assignment = False
-        undone_assignment.update(is_done=True)
-
+    meta.has_final_feedback = instance.is_final or instance.final_by_reviewer
     meta.save()
 
 

core/tests/test_custom_subscription_filter.py

@@ -140,6 +140,7 @@ class StopOnPass(APITestCase):
         self.assertEqual(35, self.data['students'][0].student.total_score)
         self.assertTrue(self.data['students'][0].student.passes_exam)
 
+    # TODO why is this code commented?!?
     # def test_submissions_left_after_not_pass_only_student_passed_exam(self):
     #     Feedback.objects.create(
     #         of_submission=self.data['submissions'][3], score=20)
@@ -164,8 +165,10 @@ class StopOnPass(APITestCase):
         # signals recognize the open assignments
         Feedback.objects.create(
             of_submission=a1.submission, score=20)
+        a1.finish()
         Feedback.objects.create(
             of_submission=a2.submission, score=15)
+        a2.finish()
 
         subscription_other_tutor = SubmissionSubscription.objects.create(
             owner=self.tutor02,
@@ -188,8 +191,10 @@ class StopOnPass(APITestCase):
         # signals recognize the open assignments
         Feedback.objects.create(
             of_submission=a1.submission, score=20)
+        a1.finish()
         Feedback.objects.create(
             of_submission=a2.submission, score=15)
+        a2.finish()
 
         subscription_other_tutor = SubmissionSubscription.objects.create(
             owner=self.tutor02,

core/tests/test_feedback.py

@@ -93,7 +93,7 @@ class FeedbackCreateTestCase(APITestCase):
 
     @classmethod
     def setUpTestData(cls):
-        cls.url = '/api/feedback/'
+        cls.url = lambda self: f'/api/assignment/{self.assignment.pk}/finish/'
         cls.user_factory = GradyUserFactory()
         cls.tutor = cls.user_factory.make_tutor(password='p')
         cls.exam = make_exams(exams=[{
@@ -138,7 +138,7 @@ class FeedbackCreateTestCase(APITestCase):
 
         }
         self.assertEqual(Feedback.objects.count(), 0)
-        response = self.client.post(self.url, data, format='json')
+        response = self.client.post(self.url(), data, format='json')
         self.assertEqual(status.HTTP_400_BAD_REQUEST, response.status_code)
         self.assertEqual(Feedback.objects.count(), 0)
 
@@ -149,7 +149,7 @@ class FeedbackCreateTestCase(APITestCase):
             'of_submission': self.assignment.submission.pk
         }
         self.assertEqual(Feedback.objects.count(), 0)
-        response = self.client.post(self.url, data, format='json')
+        response = self.client.post(self.url(), data, format='json')
         self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
         self.assertEqual(Feedback.objects.count(), 0)
 
@@ -159,7 +159,7 @@ class FeedbackCreateTestCase(APITestCase):
             'is_final': True,
             'of_submission': self.assignment.submission.pk
         }
-        response = self.client.post(self.url, data, format='json')
+        response = self.client.post(self.url(), data, format='json')
         self.assertEqual(status.HTTP_403_FORBIDDEN, response.status_code)
         self.assertEqual(Feedback.objects.count(), 0)
 
@@ -169,7 +169,7 @@ class FeedbackCreateTestCase(APITestCase):
             'is_final': False,
             'of_submission': self.assignment.submission.pk
         }
-        response = self.client.post(self.url, data, format='json')
+        response = self.client.post(self.url(), data, format='json')
         self.assertEqual(status.HTTP_400_BAD_REQUEST, response.status_code)
         self.assertEqual(Feedback.objects.count(), 0)
 
@@ -180,7 +180,7 @@ class FeedbackCreateTestCase(APITestCase):
             'of_submission': self.assignment.submission.pk
         }
         self.assertEqual(Feedback.objects.count(), 0)
-        response = self.client.post(self.url, data, format='json')
+        response = self.client.post(self.url(), data, format='json')
         self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
         self.assertEqual(Feedback.objects.count(), 0)
 
@@ -191,7 +191,7 @@ class FeedbackCreateTestCase(APITestCase):
             'of_submission': self.assignment.submission.pk
         }
         self.assertEqual(Feedback.objects.count(), 0)
-        response = self.client.post(self.url, data, format='json')
+        response = self.client.post(self.url(), data, format='json')
         self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
         self.assertEqual(Feedback.objects.count(), 0)
 
@@ -209,7 +209,7 @@ class FeedbackCreateTestCase(APITestCase):
             }
         }
         self.assertEqual(self.fst_label.feedback.count(), 0)
-        response = self.client.post(self.url, data, format='json')
+        response = self.client.post(self.url(), data, format='json')
         self.assertEqual(response.status_code, status.HTTP_201_CREATED)
         self.fst_label.refresh_from_db()
         self.snd_label.refresh_from_db()
@@ -229,7 +229,7 @@ class FeedbackCreateTestCase(APITestCase):
                 }
             }
         }
-        self.client.post(self.url, data, format='json')
+        self.client.post(self.url(), data, format='json')
         object_score = self.sub.feedback.score
         self.assertEqual(object_score, 0.5)
 
@@ -245,7 +245,7 @@ class FeedbackCreateTestCase(APITestCase):
                 }
             }
         }
-        self.client.post(self.url, data, format='json')
+        self.client.post(self.url(), data, format='json')
         object_score = self.sub.feedback.score
         self.assertEqual(object_score, 5)
 
@@ -262,7 +262,7 @@ class FeedbackCreateTestCase(APITestCase):
             }
         }
         self.assertEqual(FeedbackComment.objects.count(), 0)
-        response = self.client.post(self.url, data, format='json')
+        response = self.client.post(self.url(), data, format='json')
         self.assertEqual(response.status_code, status.HTTP_201_CREATED)
         self.assertEqual(FeedbackComment.objects.count(), 1)
 
@@ -278,7 +278,7 @@ class FeedbackCreateTestCase(APITestCase):
                 }
             }
         }
-        self.client.post(self.url, data, format='json')
+        self.client.post(self.url(), data, format='json')
         comment = FeedbackComment.objects.first()
         self.assertEqual(comment.of_tutor, self.tutor)
         self.assertEqual(comment.text, 'Nice meth!')
@@ -298,8 +298,8 @@ class FeedbackCreateTestCase(APITestCase):
             }
         }
         self.assignment.delete()
-        response = self.client.post(self.url, data, format='json')
-        self.assertEqual(status.HTTP_403_FORBIDDEN, response.status_code)
+        response = self.client.post(self.url(), data, format='json')
+        self.assertEqual(status.HTTP_404_NOT_FOUND, response.status_code)
 
     def test_cannot_create_with_someoneelses_assignment(self):
         data = {
@@ -314,8 +314,9 @@ class FeedbackCreateTestCase(APITestCase):
         }
         other_tutor = self.user_factory.make_tutor('Berta')
         self.client.force_authenticate(other_tutor)
-        response = self.client.post(self.url, data, format='json')
-        self.assertEqual(status.HTTP_403_FORBIDDEN, response.status_code)
+        response = self.client.post(self.url(), data, format='json')
+        # returns 404 since the other users assignment is not visible to this one
+        self.assertEqual(status.HTTP_404_NOT_FOUND, response.status_code)
 
     def test_can_create_multiple_feedback_comments(self):
         data = {
@@ -333,7 +334,7 @@ class FeedbackCreateTestCase(APITestCase):
                 }
             }
         }
-        self.client.post(self.url, data, format='json')
+        self.client.post(self.url(), data, format='json')
         first_comment = FeedbackComment.objects.get(text='Nice meth!')
         self.assertEqual(first_comment.of_tutor, self.tutor)
         self.assertIsNotNone(first_comment.created)

core/tests/test_subscription_assignment_service.py

@@ -235,7 +235,6 @@ class TestApiEndpoints(APITestCase):
 
         response_subscription_create = self.client.post(
             '/api/subscription/', {'query_type': 'random'})
-        subscription_pk = response_subscription_create.data['pk']
 
         subscription_pk = response_subscription_create.data['pk']
         response_assignment = self.client.post(
@@ -374,7 +373,7 @@ class TestApiEndpoints(APITestCase):
             })
         self.assertEqual(status.HTTP_201_CREATED, response.status_code)
         response = self.client.post(
-            f'/api/feedback/', {
+            f'/api/assignment/{response.data["pk"]}/finish/', {
                 "score": 23,
                 "of_submission": response.data['submission']['pk'],
                 "feedback_lines": {
@@ -415,8 +414,8 @@ class TestApiEndpoints(APITestCase):
         assignment = models.TutorSubmissionAssignment.objects.get(
             pk=response.data['pk'])
         self.assertFalse(assignment.is_done)
-        response = self.client.patch(
-            '/api/feedback/%s/' % submission_id_in_response, {
+        response = self.client.post(
+            f'/api/assignment/{assignment.pk}/finish/', {
                 "score": 20,
                 "is_final": True,
                 "feedback_lines": {

core/tests/test_tutor_api_endpoints.py

@@ -107,6 +107,7 @@ class TutorListTests(APITestCase):
             Feedback.objects.update_or_create(
                 of_submission=assignment.submission,
                 score=35)
+            assignment.finish()
 
         tutor01 = data['tutors'][0]
         tutor02 = data['tutors'][1]

core/urls.py

@@ -7,6 +7,7 @@ from rest_framework.permissions import AllowAny
 from core import views
 
 # Create a router and register our viewsets with it.
+
 router = DefaultRouter()
 router.register('student', views.StudentReviewerApiViewSet,
                 basename='student')

core/views/feedback.py

@@ -4,8 +4,11 @@ from multiprocessing import Lock
 from rest_framework import mixins, status, viewsets
 from rest_framework.exceptions import PermissionDenied
 from rest_framework.response import Response
+from rest_framework import decorators
 
 from core import models, permissions, serializers
+from core.views.util import tutor_attempts_to_patch_first_feedback_final, \
+    get_implicit_assignment_for_user
 
 log = logging.getLogger(__name__)
 
@@ -17,11 +20,11 @@ class FeedbackApiView(
         viewsets.GenericViewSet):
     """ Gets a list of an individual exam by Id if provided """
     permission_classes = (permissions.IsTutorOrReviewer,)
-    queryset = models.Feedback.objects\
-        .select_related('of_submission')\
-        .select_related('of_submission__type')\
-        .select_related('of_submission__student')\
-        .select_related('of_submission__student__user')\
+    queryset = models.Feedback.objects \
+        .select_related('of_submission') \
+        .select_related('of_submission__type') \
+        .select_related('of_submission__student') \
+        .select_related('of_submission__student__user') \
         .all()
     serializer_class = serializers.FeedbackSerializer
     lookup_field = 'of_submission__pk'
@@ -32,44 +35,11 @@ class FeedbackApiView(
         user_is_tutor = self.request.user.role == models.UserAccount.TUTOR
         return feedback_final_by_reviewer and user_is_tutor
 
-    def _get_implicit_assignment_for_user(self, submission):
-        """ Check for tutor if it exists. Not relevant for reviewer """
-        try:
-            return models.TutorSubmissionAssignment.objects.get(
-                subscription__owner=self.request.user,
-                submission=submission
-            )
-        except models.TutorSubmissionAssignment.DoesNotExist:
-            if self.request.user.role == models.UserAccount.REVIEWER:
-                return None
-            raise PermissionDenied(
-                detail='This user has no permission to create this feedback')
-
     def _tutor_attempts_to_set_first_feedback_final(self, serializer):
         is_final_set = serializer.validated_data.get('is_final', False)
         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)
-        youngest = models.TutorSubmissionAssignment.objects \
-            .filter(submission=submission) \
-            .order_by('-created') \
-            .first()
-
-        return assignment == youngest
-
-    def _tutor_attempts_to_patch_first_feedback_final(self, serializer):
-        if self.request.user.role == models.UserAccount.REVIEWER:
-            return False
-        is_final_set = serializer.validated_data.get('is_final', False)
-        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 get_queryset(self):
         if self.request.user.is_reviewer():
             return self.queryset \
@@ -81,19 +51,15 @@ class FeedbackApiView(
             of_submission__assignments__subscription__owner=self.request.user
         )
 
+    @decorators.permission_classes((permissions.IsReviewer,))
     def create(self, request, *args, **kwargs):
         serializer = self.get_serializer(data=request.data)
         serializer.is_valid(raise_exception=True)
-
-        self._get_implicit_assignment_for_user(
-            serializer.validated_data['of_submission'])
-
-        if self._tutor_attempts_to_set_first_feedback_final(serializer):
-            return Response(
-                {'For tutors it is not allowed to create feedback final.'},
-                status=status.HTTP_403_FORBIDDEN)
-
         self.perform_create(serializer)
+
+        # update MetaSubmission information
+        meta = serializer.validated_data.get('of_submission').meta
+        meta.feedback_authors.add(self.request.user)
         return Response(serializer.data,
                         status=status.HTTP_201_CREATED)
 
@@ -103,16 +69,15 @@ class FeedbackApiView(
                                          partial=True)
         serializer.is_valid(raise_exception=True)
 
-        self._get_implicit_assignment_for_user(feedback.of_submission)
+        assignment = get_implicit_assignment_for_user(feedback.of_submission, self.request.user)
 
         if self._tutor_attempts_to_change_final_feedback_of_reviewer(serializer):  # noqa
             raise PermissionDenied(
-                detail="Changing final feedback is not allowed.")
+                detail="Changing feedback set to final by a reviewer is not allowed.")
 
-        if self._tutor_attempts_to_patch_first_feedback_final(serializer):
+        if tutor_attempts_to_patch_first_feedback_final(serializer, self.request.user, assignment):
             raise PermissionDenied(
                 detail='Cannot set the first feedback final.')
-
         serializer.save()
         return Response(serializer.data)
 

core/views/subscription.py

@@ -2,7 +2,8 @@ import logging
 
 from django.core.exceptions import ObjectDoesNotExist
 from rest_framework import mixins, status, viewsets
-from rest_framework.decorators import action, permission_classes
+from rest_framework import decorators
+from rest_framework.exceptions import PermissionDenied
 from rest_framework.response import Response
 
 from core import models, permissions, serializers
@@ -12,6 +13,8 @@ from core.serializers import AssignmentDetailSerializer, AssignmentSerializer
 
 from multiprocessing import Lock
 
+from core.views.util import tutor_attempts_to_patch_first_feedback_final
+
 log = logging.getLogger(__name__)
 
 
@@ -76,6 +79,7 @@ class AssignmentApiViewSet(
     queryset = TutorSubmissionAssignment.objects\
         .select_related('subscription').all()
     serializer_class = AssignmentSerializer
+    permission_classes = (IsTutorOrReviewer, )
 
     def get_queryset(self):
         if self.action in ['list', 'active', 'destroy']:
@@ -97,11 +101,11 @@ class AssignmentApiViewSet(
                             status=status.HTTP_403_FORBIDDEN)
         return Response(serializer.data, status=status.HTTP_201_CREATED)
 
-    @permission_classes((IsReviewer,))
+    @decorators.permission_classes((IsReviewer,))
     def list(self, *args, **kwargs):
         return super().list(*args, **kwargs)
 
-    @action(detail=False, permission_classes=(IsReviewer,), methods=['get', 'delete'])
+    @decorators.action(detail=False, permission_classes=(IsReviewer,), methods=['get', 'delete'])
     def active(self, request):
         if request.method == 'GET':
             queryset = self.get_queryset().filter(is_done=False)
@@ -111,7 +115,39 @@ class AssignmentApiViewSet(
             self.get_queryset().filter(is_done=False).delete()
             return Response(status=status.HTTP_204_NO_CONTENT)
 
-    @permission_classes((IsTutorOrReviewer,))
+    @decorators.action(detail=True, methods=['post'])
+    def finish(self, request, *args, **kwargs):
+        context = self.get_serializer_context()
+        instance = self.get_object()
+        if instance.is_done or (instance.subscription.owner != request.user):
+            return Response(status=status.HTTP_403_FORBIDDEN)
+        try:
+            orig_feedback = instance.submission.feedback
+            serializer = serializers.FeedbackSerializer(
+                orig_feedback,
+                data=request.data,
+                context=context,
+                partial=True)
+            if orig_feedback.final_by_reviewer and request.user.role == models.UserAccount.TUTOR:
+                raise PermissionDenied(detail="Unfortunately you won't be able to finish this"
+                                              "assignment since a reviewer has marked it as "
+                                              "final while you were assigned.")
+        except models.Feedback.DoesNotExist:
+            serializer = serializers.FeedbackSerializer(
+                data=request.data,
+                context=context)
+
+        serializer.is_valid(raise_exception=True)
+        if tutor_attempts_to_patch_first_feedback_final(serializer, self.request.user, instance):
+            raise PermissionDenied(
+                detail='Cannot set the first feedback final.')
+        serializer.save()
+        instance.finish()
+        response_status = status.HTTP_201_CREATED if \
+            instance.subscription.feedback_stage == \
+            models.SubmissionSubscription.FEEDBACK_CREATION else status.HTTP_200_OK
+        return Response(serializer.data, status=response_status)
+
     def destroy(self, request, pk=None):
         """ Stop working on the assignment before it is finished """
         instance = self.get_object()
@@ -123,7 +159,6 @@ class AssignmentApiViewSet(
         instance.delete()
         return Response(status=status.HTTP_204_NO_CONTENT)
 
-    @permission_classes((IsTutorOrReviewer,))
     def create(self, request, *args, **kwargs):
         with Lock():
             context = self.get_serializer_context()
@@ -133,7 +168,6 @@ class AssignmentApiViewSet(
             assignment = self._fetch_assignment(serializer)
         return assignment
 
-    @permission_classes((IsTutorOrReviewer,))
     def retrieve(self, request, *args, **kwargs):
         assignment = self.get_object()
         if assignment.subscription.owner != request.user:

core/views/util.py

+from rest_framework.exceptions import PermissionDenied
+
+from core import models
+
+
+class NoAssignmentForTutor(Exception):
+    pass
+
+
+def tutor_attempts_to_patch_first_feedback_final(feedback_serializer,
+                                                 user,
+                                                 assignment=None):
+    if user.role == models.UserAccount.REVIEWER:
+        return False
+    if user.role == models.UserAccount.TUTOR and assignment is None:
+        raise NoAssignmentForTutor()
+    is_final_set = feedback_serializer.validated_data.get('is_final', False)
+    in_creation = assignment.subscription.feedback_stage == models.SubmissionSubscription.FEEDBACK_CREATION  # noqa
+    return is_final_set and in_creation
+
+
+def get_implicit_assignment_for_user(submission, user):
+    """ Check for tutor if it exists. Not relevant for reviewer """
+    try:
+        return models.TutorSubmissionAssignment.objects.get(
+            subscription__owner=user,
+            submission=submission
+        )
+    except models.TutorSubmissionAssignment.DoesNotExist:
+        if user.role == models.UserAccount.REVIEWER:
+            return None
+        raise PermissionDenied(
+            detail='This user has no permission to create this feedback')

frontend/src/api.ts

@@ -115,7 +115,7 @@ export async function fetchStatistics (): Promise<Statistics> {
   return (await ax.get(url)).data
 }
 
-export async function fetchLabelStatistics (): Promise<LabelStatisticsForSubType  []> {
+export async function fetchLabelStatistics (): Promise<LabelStatisticsForSubType[]> {
   const url = '/api/label-statistics'
   return (await ax.get(url)).data
 }
@@ -152,16 +152,20 @@ export async function createAssignment (
   return (await ax.post(`/api/assignment/`, data)).data
 }
 
-export async function submitFeedbackForAssignment ({ feedback }: 
-  { feedback: Partial<CreateUpdateFeedback>}): Promise<CreateUpdateFeedback> {
-  return (await ax.post('/api/feedback/', feedback)).data
+export async function submitFeedbackForAssignment ({ feedback, assignment }:
+  { feedback: Partial<CreateUpdateFeedback>, assignment: Assignment}): Promise<CreateUpdateFeedback> {
+  return (await ax.post(`/api/assignment/${assignment.pk}/finish/`, feedback)).data
 }
 
-export async function submitUpdatedFeedback ({ feedback }: 
+export async function submitUpdatedFeedback ({ feedback }:
   {feedback: CreateUpdateFeedback}): Promise<CreateUpdateFeedback> {
   return (await ax.patch(`/api/feedback/${feedback.ofSubmission}/`, feedback)).data
 }
 
+export async function submitFeedback ({ feedback }: {feedback: CreateUpdateFeedback}): Promise<Feedback> {
+  return (await ax.post(`/api/feedback/`, feedback)).data
+}
+
 export async function fetchSubmissionTypes (): Promise<Array<SubmissionType>> {
   const url = '/api/submissiontype/'
   return (await ax.get(url)).data
@@ -221,7 +225,7 @@ export async function createLabel (payload: Partial<FeedbackLabel>) {
 }
 
 export async function updateLabel (payload: FeedbackLabel) {
-  return (await ax.put('/api/label/' + payload.pk + '/', payload))
+  return (await ax.put('/api/label/' + payload.pk + '/', payload)).data
 }
 
 export interface StudentExportOptions { setPasswords?: boolean }
@@ -250,6 +254,6 @@ export async function fetchInstanceExportData (): Promise<InstanceExportData> {
   return (await ax.get('/api/instance/export')).data
 }
 
-ax.interceptors.response.use(undefined, errorInterceptor);
+ax.interceptors.response.use(undefined, errorInterceptor)
 
 export default ax

frontend/src/components/submission_notes/SubmissionCorrection.vue

@@ -141,10 +141,11 @@ export default {
       this.loading = true
       SubmissionNotes.submitFeedback({
         isFinal: isFinal
-      }).then(feedback => {
+      }).then(_ => {
         SubmissionNotes.RESET_UPDATED_FEEDBACK()
-        SubmissionNotes.SET_ORIG_FEEDBACK(feedback)
         this.$emit('feedbackCreated')
+        this.$emit('feedbackChanged')
+        SubmissionNotes.RESET_MARKED_COMMENTS_FOR_DELETE()
       }).catch(err => {
         // ignore trivial errors as those are handled
         // by an interceptor
@@ -157,8 +158,6 @@ export default {
           duration: -1
         })
       }).finally(() => {
-        this.$emit('feedbackChanged')
-        SubmissionNotes.RESET_MARKED_COMMENTS_FOR_DELETE()
         this.loading = false
       })
     },

frontend/src/store/modules/submission-notes.ts

@@ -4,8 +4,9 @@ import * as api from '@/api'
 import { Feedback, FeedbackComment, SubmissionNoType, CreateUpdateFeedback } from '@/models'
 import { RootState } from '@/store/store'
 import { getStoreBuilder, BareActionContext } from 'vuex-typex'
-import { syntaxPostProcess } from '@/util/helpers';
-import { AxiosResponse } from 'axios';
+import { syntaxPostProcess } from '@/util/helpers'
+import { AxiosResponse } from 'axios'
+import { Subscriptions } from './subscriptions';
 
 export interface SubmissionNotesState {
   submission: SubmissionNoType
@@ -21,7 +22,7 @@ export interface SubmissionNotesState {
   changedLabels: boolean
 }
 
-function initialState(): SubmissionNotesState {
+function initialState (): SubmissionNotesState {
   return {
     submission: {
       text: '',
@@ -194,11 +195,15 @@ Promise<AxiosResponse<void>[]> {
   } else if (feedback.score! < SubmissionNotes.submissionType.fullScore! && !state.hasOrigFeedback) {
     throw new Error('You need to add or change a comment when setting a non full score.')
   }
-  if (!state.hasOrigFeedback) {
-    await api.submitFeedbackForAssignment({ feedback })
-  } else {
+
+  const assignment = Subscriptions.state.currentAssignment
+  if (assignment) {
+    await api.submitFeedbackForAssignment({ feedback , assignment})
+  } else if (state.hasOrigFeedback) {
     feedback.pk = state.origFeedback.pk
     await api.submitUpdatedFeedback(<{ feedback: CreateUpdateFeedback }>{ feedback })
+  } else {
+    await api.submitFeedback(<{ feedback: CreateUpdateFeedback }>{feedback})
   }
   // delete those comments that have been marked for deletion
   return SubmissionNotes.deleteComments()

functional_tests/.gitignore

 export.json
 export.csv
-
+screenshots

functional_tests/test_export_modal.py

 import json
 import os
+from pathlib import Path
 
 from django.test import LiveServerTestCase
 from selenium import webdriver
@@ -10,18 +11,22 @@ from functional_tests.util import login, create_browser, reset_browser_after_tes
 from util import factory_boys as fact
 
 
-def expect_file_to_be_present(path):
+def expect_file_to_be_downloaded(path):
+    """
+    Checks if a file has finished downloading by checking if a file exists at the path and
+    no accompanying `<path>.part` file.
+    :param path: path to check
+    :return:
+    """
     def condition(*args):
-        try:
-            with open(path):
-                pass
-        except FileNotFoundError:
-            return False
-        return True
+        file_present = Path(path).is_file()
+        partial_file_present = Path(os.path.join(path, '.part')).is_file()
+        return file_present and not partial_file_present
     return condition
 
 
 JSON_EXPORT_FILE = os.path.join(os.path.dirname(__file__), 'export.json')
+SCREENSHOTS = os.path.join(os.path.dirname(__file__), 'screenshots')
 
 
 class ExportTestModal(LiveServerTestCase):
@@ -51,6 +56,15 @@ class ExportTestModal(LiveServerTestCase):
         )
 
     def tearDown(self):
+        try:
+            os.mkdir(SCREENSHOTS)
+        except FileExistsError:
+            pass
+        for method, error in self._outcome.errors:
+            if error:
+                self.browser.get_screenshot_as_file(
+                    os.path.join(SCREENSHOTS, self.id() + ".png")
+                )
         reset_browser_after_test(self.browser, self.live_server_url)
 
     def _login(self):
@@ -97,8 +111,8 @@ class ExportTestModal(LiveServerTestCase):
                          list_elements[1].find_element_by_tag_name('a').text)
 
     def test_export_student_scores_as_json(self):
-        self._login()
         fact.StudentInfoFactory()
+        self._login()
         export_btn = self.browser.find_element_by_id('export-btn')
         export_btn.click()
         export_scores = self.browser.find_element_by_id('export-list0')
@@ -110,21 +124,29 @@ class ExportTestModal(LiveServerTestCase):
         export_type_json.click()
         data_export_btn = data_export_modal.find_element_by_id('export-data-download-btn')
         data_export_btn.click()
-        WebDriverWait(self.browser, 10).until(expect_file_to_be_present(JSON_EXPORT_FILE))
-        with open(JSON_EXPORT_FILE) as f:
-            data = json.load(f)
-        self.assertEqual('B.Inf.4242 Test Module', data[0]['Exam'])
-        os.remove(JSON_EXPORT_FILE)
+        WebDriverWait(self.browser, 10).until(expect_file_to_be_downloaded(JSON_EXPORT_FILE))
+        try:
+            with open(JSON_EXPORT_FILE) as f:
+                data = json.load(f)
+            self.assertEqual('B.Inf.4242 Test Module', data[0]['Exam'])
+        except Exception as e:
+            raise e
+        finally:
+            os.remove(JSON_EXPORT_FILE)
 
     def test_export_instance(self):
-        self._login()
         fact.SubmissionFactory()
+        self._login()
         self.browser.find_element_by_id('export-btn').click()
         self.browser.find_element_by_id('export-list1').click()
         instance_export_modal = self.browser.find_element_by_id('instance-export-modal')
         instance_export_modal.find_element_by_id('instance-export-dl').click()
-        WebDriverWait(self.browser, 10).until(expect_file_to_be_present(JSON_EXPORT_FILE))
-        with open(JSON_EXPORT_FILE) as f:
-            data = json.load(f)
-        self.assertEqual('B.Inf.4242 Test Module', data['examTypes'][0]['moduleReference'])
-        os.remove(JSON_EXPORT_FILE)
+        WebDriverWait(self.browser, 10).until(expect_file_to_be_downloaded(JSON_EXPORT_FILE))
+        try:
+            with open(JSON_EXPORT_FILE) as f:
+                data = json.load(f)
+            self.assertEqual('B.Inf.4242 Test Module', data['examTypes'][0]['moduleReference'])
+        except Exception as e:
+            raise e
+        finally:
+            os.remove(JSON_EXPORT_FILE)