core/migrations/0018_auto_20190709_1526.py

+# Generated by Django 2.2 on 2019-07-09 15:26
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('core', '0017_auto_20190604_1631'),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name='feedback',
+            name='final_by_reviewer',
+            field=models.BooleanField(default=False),
+        ),
+        migrations.AlterField(
+            model_name='feedbacklabel',
+            name='name',
+            field=models.CharField(max_length=50, unique=True),
+        ),
+    ]

core/migrations/0018_auto_20190814_1324.py

+# Generated by Django 2.1.11 on 2019-08-14 13:24
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('core', '0017_auto_20190604_1631'),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name='feedbacklabel',
+            name='name',
+            field=models.CharField(max_length=50, unique=True),
+        ),
+    ]

core/migrations/0019_merge_20190814_1437.py

+# Generated by Django 2.1.11 on 2019-08-14 14:37
+
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('core', '0018_auto_20190709_1526'),
+        ('core', '0018_auto_20190814_1324'),
+    ]
+
+    operations = [
+    ]

core/models/feedback.py

@@ -25,10 +25,13 @@ class Feedback(models.Model):
         points a student receives for his submission.
     origin : IntegerField
         Of whom was this feedback originally created. She below for the choices
+    final_by_reviewer: BooleanField
+        Whether or not this feedback was set to final by a reviewer once
     """
     score = models.DecimalField(max_digits=5, decimal_places=2, default=0)
     created = models.DateTimeField(auto_now_add=True)
     is_final = models.BooleanField(default=False)
+    final_by_reviewer = models.BooleanField(default=False)
 
     of_submission = models.OneToOneField(
         Submission,

core/serializers/feedback.py

@@ -7,7 +7,7 @@ from rest_framework import serializers
 from rest_framework.utils import html
 
 from core import models
-from core.models import Feedback
+from core.models import Feedback, UserAccount
 from util.factories import GradyUserFactory
 
 from .generic import DynamicFieldsModelSerializer
@@ -64,6 +64,8 @@ class FeedbackCommentDictionarySerializer(serializers.ListSerializer):
 
 class FeedbackCommentSerializer(DynamicFieldsModelSerializer):
     of_tutor = serializers.StringRelatedField(source='of_tutor.username')
+    labels = serializers.PrimaryKeyRelatedField(many=True, required=False,
+                                                queryset=models.FeedbackLabel.objects.all())
 
     class Meta:
         model = models.FeedbackComment
@@ -121,8 +123,12 @@ class FeedbackSerializer(DynamicFieldsModelSerializer):
         submission = validated_data.pop('of_submission')
         feedback_lines = validated_data.pop('feedback_lines', [])
         labels = validated_data.pop('labels', [])
+        user = self.context['request'].user
         feedback = Feedback.objects.create(of_submission=submission,
                                            **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)
 
@@ -141,14 +147,21 @@ class FeedbackSerializer(DynamicFieldsModelSerializer):
 
     @transaction.atomic
     def update(self, feedback, validated_data):
+        user = self.context['request'].user
+
+        if user.role == UserAccount.REVIEWER:
+            feedback.final_by_reviewer = self.context['request'].data['is_final']
+
         for comment in validated_data.pop('feedback_lines', []):
-            labels = comment.pop('labels', [])
+            labels = comment.pop('labels', None)
             comment_instance, _ = models.FeedbackComment.objects.update_or_create(
                 of_feedback=feedback,
                 of_tutor=self.context['request'].user,
                 of_line=comment.get('of_line'),
                 defaults={'text': comment.get('text')})
-            comment_instance.labels.set(labels)
+
+            if labels is not None:
+                comment_instance.labels.set(labels)
 
         return super().update(feedback, validated_data)
 

core/views/feedback.py

@@ -28,12 +28,9 @@ class FeedbackApiView(
     lookup_url_kwarg = 'submission_pk'
 
     def _tutor_attempts_to_change_final_feedback_of_reviewer(self, serializer):
-        feedback_is_final = serializer.instance.is_final
+        feedback_final_by_reviewer = serializer.instance.final_by_reviewer
         user_is_tutor = self.request.user.role == models.UserAccount.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
+        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 """

frontend/src/api.ts

@@ -12,7 +12,8 @@ import {
   SubmissionNoType, SubmissionType,
   Subscription,
   Tutor, UserAccount, LabelStatisticsForSubType,
-  FeedbackLabel
+  FeedbackLabel,
+  CreateUpdateFeedback
 } from '@/models'
 
 function getInstanceBaseUrl (): string {
@@ -151,11 +152,13 @@ export async function createAssignment (
   return (await ax.post(`/api/assignment/`, data)).data
 }
 
-export async function submitFeedbackForAssignment ({ feedback }: {feedback: Partial<Feedback>}): Promise<Feedback> {
+export async function submitFeedbackForAssignment ({ feedback }: 
+  { feedback: Partial<CreateUpdateFeedback>}): Promise<CreateUpdateFeedback> {
   return (await ax.post('/api/feedback/', feedback)).data
 }
 
-export async function submitUpdatedFeedback ({ feedback }: {feedback: Feedback}): Promise<Feedback> {
+export async function submitUpdatedFeedback ({ feedback }: 
+  {feedback: CreateUpdateFeedback}): Promise<CreateUpdateFeedback> {
   return (await ax.patch(`/api/feedback/${feedback.ofSubmission}/`, feedback)).data
 }
 

frontend/src/components/mixins/commentLabelSelector.ts

@@ -5,6 +5,11 @@ import { SubmissionNotes } from "@/store/modules/submission-notes"
 import { FeedbackComment, FeedbackLabel } from "@/models"
 import { FeedbackLabels } from "@/store/modules/feedback-labels"
 
+enum FeedbackType {
+  original = "origFeedback",
+  updated = "updatedFeedback",
+}
+
 @Component
 export default class commentLabelSelector extends Vue {
   @Prop({ type: String, required: true }) readonly lineNo!: string
@@ -12,35 +17,51 @@ export default class commentLabelSelector extends Vue {
   /**
    * Returns array of label pk's where feedbackType is
    * either "origFeedback" or "updatedFeedback"
+   * 
+   * Will return null when labels property does not exist on the requested state's comment
+   * This is the case when the labels have not been updated, as we don't want to have
+   * the labels field in the object if the labels have not changed.
    */
-  copyStateLabels(feedbackType: string): number[] {
-    if (feedbackType !== "origFeedback" && feedbackType !== "updatedFeedback") return new Array()
-    
+  copyStateLabels(feedbackType: FeedbackType): number[] | null {    
     const currentLine = this.getFeedbackLine(feedbackType)
-    return currentLine ? currentLine.labels : new Array()
+    if (currentLine && currentLine.labels) {
+      return currentLine.labels
+    } else {
+      return null
+    }
   }
 
-  getFeedbackLine (feedbackType: string): FeedbackComment | undefined {
-    if (feedbackType !== "origFeedback" && feedbackType !== "updatedFeedback") return undefined
+  /**
+   * Gets the latest feedback line object for the current lineNo and the given feedbackType
+   * @param feedbackType  The type to get the latest line from
+   */
+  getFeedbackLine (feedbackType: FeedbackType): FeedbackComment | undefined {
+
+    // helper used to determine the correct type to reduce redundancy
+    function isArray(val: FeedbackComment | FeedbackComment[]): val is FeedbackComment[] {
+      return (<FeedbackComment[]>val).length !== undefined
+    }
 
     const stateLines = SubmissionNotes.state[feedbackType].feedbackLines
     if (stateLines && Object.keys(stateLines).length > 0) {
       let lines = stateLines[Number(this.lineNo)]
-      if (lines === undefined) return undefined
+      if (!lines) return undefined
 
-      // always copy latest comment
-      if (lines.length > 0) {
-        return lines[lines.length-1]
+      if (isArray(lines)) {
+        return lines.length > 0 ? lines[lines.length-1] : undefined
       } else {
-        // @ts-ignore
         return lines
       }
     }
+
+    return undefined
   }
 
   getUnchangedLabels() {
-    const labelsOrig = this.copyStateLabels("origFeedback")
-    if (labelsOrig === undefined) return new Array()
+    const labelsOrig = this.copyStateLabels(FeedbackType.original)
+    if (labelsOrig === null || labelsOrig.length === 0) {
+      return new Array ()
+    }
 
     const removedLabels = this.getRemovedLabels()
     const addedLabels = this.getAddedLabels()
@@ -51,13 +72,15 @@ export default class commentLabelSelector extends Vue {
   }
 
   getRemovedLabels() {
-    const currentLine = this.getFeedbackLine("updatedFeedback")
+    const currentLine = this.getFeedbackLine(FeedbackType.updated)
     if (currentLine === undefined) return new Array()
 
-    const labelsOrig = this.copyStateLabels("origFeedback")
-    const labelsUpdated = this.copyStateLabels("updatedFeedback")
+    const labelsOrig = this.copyStateLabels(FeedbackType.original)
+    const labelsUpdated = this.copyStateLabels(FeedbackType.updated)
 
-    if (labelsOrig == undefined) return new Array()
+    if (labelsOrig === null || labelsUpdated === null) {
+      return new Array()
+    }
 
     return labelsOrig.filter((val) => {
       return !labelsUpdated.includes(val)
@@ -65,10 +88,16 @@ export default class commentLabelSelector extends Vue {
   }
 
   getAddedLabels() {
-    const labelsOrig = this.copyStateLabels("origFeedback")
-    const labelsUpdated = this.copyStateLabels("updatedFeedback")
+    const labelsOrig = this.copyStateLabels(FeedbackType.original)
+    const labelsUpdated = this.copyStateLabels(FeedbackType.updated)
 
-    if (labelsOrig === undefined) return new Array()
+    if (labelsUpdated === null) {
+      return new Array()
+    }
+
+    if (labelsOrig === null) {
+      return labelsUpdated ? labelsUpdated : new Array()
+    }
 
     return labelsUpdated.filter((val) => {
       return !labelsOrig.includes(val)
@@ -84,15 +113,22 @@ export default class commentLabelSelector extends Vue {
         return label.pk === val
       })
 
-      if (!label) return
+      if (!label) return undefined
       return { 
         pk: val,
         name: label.name,
         description: label.description,
         colour: label.colour
       }
+    }).filter((val): val is FeedbackLabel => {
+      if (!val) {
+        console.error("Encountered invalid label pk. This should be investigated.")
+        return false
+      }
+
+      return true
     })
 
-    return mappedLabels ? mappedLabels : new Array()
+    return mappedLabels;
   }
 }
\ No newline at end of file

frontend/src/components/submission_notes/SubmissionCorrection.vue

@@ -15,7 +15,7 @@
             <template v-if="showFeedback">
               <div v-if="origFeedback[lineNo]">
                 <feedback-comment
-                  v-for="(comment, index) in origFeedback[lineNo]"
+                  v-for="(comment, index) in getSortedComments(lineNo)"
                   v-bind="comment"
                   :visibleToStudent="updatedFeedback[lineNo] ? false : comment.visibleToStudent"
                   :line-no="lineNo"
@@ -170,6 +170,16 @@ export default {
         }
       }, 5e3)
     },
+    getSortedComments (lineNo) {
+      if (!this.origFeedback || (!this.origFeedback && !this.origFeedback[lineNo])) return new Array()
+      let feedback = [...this.origFeedback[lineNo]]
+
+      return feedback.sort((a, b) => {
+        const da = new Date(a.modified)
+        const db = new Date(b.modified)
+        return da.getTime() - db.getTime()
+      })
+    },
     init () {
       SubmissionNotes.RESET_STATE()
       SubmissionNotes.SET_SUBMISSION(this.submissionObj)

frontend/src/components/submission_notes/base/CommentForm.vue

@@ -104,13 +104,19 @@ export default class CommentForm extends mixins(commentLabelSelector) {
   }
   
   submitFeedback () {
-    SubmissionNotes.UPDATE_FEEDBACK_LINE({
+    const payload = {
       lineNo: Number(this.lineNo),
       comment: {
         text: this.currentFeedback,
         labels: this.labelsUnchanged.concat(this.labelsAdded),
       }
-    })
+    }
+
+    if (this.labelsAdded.length === 0 && this.labelsRemoved.length === 0) {
+      delete payload.comment.labels
+    }
+
+    SubmissionNotes.UPDATE_FEEDBACK_LINE(payload)
     this.collapseTextField()
   }
 }

frontend/src/components/submission_notes/base/FeedbackComment.vue

@@ -98,7 +98,7 @@ export default {
       type: String,
       required: true
     },
-    created: {
+    modified: {
       type: String,
       required: false
     },
@@ -133,8 +133,8 @@ export default {
     },
     markedForDeletion () { return SubmissionNotes.state.commentsMarkedForDeletion },
     parsedCreated () {
-      if (this.created) {
-        return new Date(this.created).toLocaleString()
+      if (this.modified) {
+        return new Date(this.modified).toLocaleString()
       } else {
         return 'Just now'
       }
@@ -148,22 +148,6 @@ export default {
     backgroundColor () {
       return UI.state.darkMode ? 'grey' : '#F3F3F3'
     },
-    labelsToShow () {
-      const mappedLabels = this.labels.map((val) => {
-      const label = Labels.availableLabels.find((label) => {
-        return label.pk === val
-      })
-
-      if (!label) return new Array()
-        return { 
-          pk: val,
-          name: label.name,
-          description: label.description,
-          colour: label.colour
-        }
-      })
-      return mappedLabels ? mappedLabels : new Array()
-    },
     unchangedLabels() {
       return this.mapPksToLabelObj(this.getUnchangedLabels())
     },

frontend/src/models.ts

@@ -142,6 +142,63 @@ export interface Feedback {
     labels: number[],
 }
 
+/**
+ *
+ * @export
+ * @interface CreateUpdateFeedback
+ */
+export interface CreateUpdateFeedback {
+    /**
+     *
+     * @type {number}
+     * @memberof Feedback
+     */
+    pk: number
+    /**
+     *
+     * @type {string}
+     * @memberof Feedback
+     */
+    ofSubmission?: string
+    /**
+     *
+     * @type {boolean}
+     * @memberof Feedback
+     */
+    isFinal?: boolean
+    /**
+     *
+     * @type {number}
+     * @memberof Feedback
+     */
+    score?: number
+    /**
+     *
+     * @type {Array<FeedbackComment>}
+     * @memberof Feedback
+     */
+    feedbackLines?: {[lineNo: number]: FeedbackComment}
+    /**
+     *
+     * @type {Date}
+     * @memberof Feedback
+     */
+    created?: string
+    /**
+     *
+     * @type {string}
+     * @memberof Feedback
+     */
+    ofSubmissionType?: string
+    /**
+     *
+     * @type {string}
+     * @memberof Feedback
+     */
+    feedbackStageForUser?: string,
+    labels: number[],
+}
+
 /**
  *
  * @export
@@ -165,7 +222,7 @@ export interface FeedbackComment {
      * @type {Date}
      * @memberof FeedbackComment
      */
-    created?: string
+    modified?: string
     /**
      *
      * @type {string}

frontend/src/pages/base/TutorReviewerBaseLayout.vue

@@ -12,7 +12,7 @@
             <v-icon>{{ item.icon }}</v-icon>
           </v-list-tile-action>
           <v-list-tile-content>
-            <v-list-tile-title>
+            <v-list-tile-title :id="item.tagId">
               {{ item.name }}
             </v-list-tile-title>
           </v-list-tile-content>
@@ -45,17 +45,20 @@ export default {
         {
           name: 'Overview',
           icon: 'home',
-          route: '/home'
+          route: '/home',
+          tagId: 'overview'
         },
         {
           name: 'Feedback History',
           icon: 'feedback',
-          route: '/feedback'
+          route: '/feedback',
+          tagId: 'feedback'
         },
         {
           name: 'Statistics',
           icon: 'bar_chart',
-          route: '/statistics'
+          route: '/statistics',
+          tagId: 'statistics'
         }
       ]
     }

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

 import Vue from 'vue'
 import * as hljs from 'highlight.js'
 import * as api from '@/api'
-import { Feedback, FeedbackComment, SubmissionNoType, FeedbackLabel } from '@/models'
+import { Feedback, FeedbackComment, SubmissionNoType, CreateUpdateFeedback } from '@/models'
 import { RootState } from '@/store/store'
 import { getStoreBuilder, BareActionContext } from 'vuex-typex'
 import { syntaxPostProcess } from '@/util/helpers';
@@ -16,7 +16,7 @@ export interface SubmissionNotesState {
   },
   hasOrigFeedback: boolean
   origFeedback: Feedback
-  updatedFeedback: Feedback
+  updatedFeedback: CreateUpdateFeedback
   commentsMarkedForDeletion: { [pk: string]: FeedbackComment }
   changedLabels: boolean
 }
@@ -163,11 +163,18 @@ async function submitFeedback(
 { isFinal = false}):
 Promise<AxiosResponse<void>[]> {
 
-  let feedback: Partial<Feedback> = {
+  let feedback: Partial<CreateUpdateFeedback> = {
     isFinal: isFinal,
     ofSubmission: state.submission.pk,
+    feedbackLines: {},
     labels: state.updatedFeedback.labels
   }
+
+  // omit labels for the request
+  if (!state.changedLabels) {
+    delete feedback.labels
+  }
+
   if (state.origFeedback.score === undefined && state.updatedFeedback.score === undefined) {
     throw new Error('You need to give a score.')
   } else if (state.updatedFeedback.score !== undefined) {
@@ -176,8 +183,14 @@ Promise<AxiosResponse<void>[]> {
     feedback.score = state.origFeedback.score
   }
 
-  if (Object.keys(state.updatedFeedback.feedbackLines || {}).length > 0) {
-    feedback.feedbackLines = state.updatedFeedback.feedbackLines
+  if (state.updatedFeedback.feedbackLines && Object.keys(state.updatedFeedback.feedbackLines).length > 0) {
+    // set the comments for the feedback lines accordingly
+    for (const key of Object.keys(state.updatedFeedback.feedbackLines)) {
+      const numKey = Number(key)
+
+      numKey && feedback.feedbackLines
+        && (feedback.feedbackLines[numKey] = state.updatedFeedback.feedbackLines[numKey])
+    }
   } 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.')
   }
@@ -185,7 +198,7 @@ Promise<AxiosResponse<void>[]> {
     await api.submitFeedbackForAssignment({ feedback })
   } else {
     feedback.pk = state.origFeedback.pk
-    await api.submitUpdatedFeedback(<{ feedback: Feedback }>{ feedback })
+    await api.submitUpdatedFeedback(<{ feedback: CreateUpdateFeedback }>{ feedback })
   }
   // delete those comments that have been marked for deletion
   return SubmissionNotes.deleteComments()

functional_tests/test_feedback_creation.py

 from django.test import LiveServerTestCase
 from selenium import webdriver
+from selenium.webdriver import ActionChains
 from selenium.webdriver.support.ui import WebDriverWait
 from selenium.webdriver.support import expected_conditions as ec
+from selenium.webdriver.common.by import By
 
 from core.models import UserAccount, Submission, FeedbackComment
 from functional_tests.util import (login, create_browser, reset_browser_after_test,
                                    go_to_subscription, wait_until_code_changes,
-                                   reconstruct_submission_code)
+                                   reconstruct_submission_code, wait_until_element_count_equals)
 from util import factory_boys as fact
 
 
@@ -223,6 +225,74 @@ class UntestedParent:
             self.assertEqual(0, submission_for_code.feedback.score)
             self.assertEqual(1, submission_for_code.feedback.feedback_lines.count())
 
+        def test_comments_are_sorted_by_last_updated(self):
+            self._login()
+            go_to_subscription(self)
+
+            code = reconstruct_submission_code(self)
+            self.browser.find_element_by_id('score-full').click()
+
+            # give feedback on first line
+            self.write_comments_on_lines([(1, 'first ever comment')])
+
+            submit_btn = self.browser.find_element_by_id('submit-feedback')
+            submit_btn.click()
+
+            WebDriverWait(self.browser, 10).until(
+                wait_until_code_changes(self, code)
+            )
+
+            reset_browser_after_test(self.browser, self.live_server_url)  # logs out user
+
+            user_snd = 'tutor_snd'
+            password = 'p'
+            fact.UserAccountFactory(username=user_snd, password=password)
+
+            login(self.browser, self.live_server_url, user_snd, password)
+            go_to_subscription(self, stage='validate')
+
+            self.write_comments_on_lines([(1, 'the second comment')])
+            self.browser.find_element_by_id('score-full').click()
+            self.browser.find_element_by_class_name('final-checkbox').click()
+            self.browser.find_element_by_id('submit-feedback').click()
+
+            sub_url = 'subscription/' + str(self.sub_type.pk) + '/ended'
+            WebDriverWait(self.browser, 10).until(ec.url_contains(sub_url))
+
+            reset_browser_after_test(self.browser, self.live_server_url)  # logs out user
+            self._login()
+
+            # goto history
+            self.browser.find_element_by_id('feedback').click()
+            feedback_entry = self.browser.find_element_by_class_name('feedback-row')
+            ActionChains(self.browser).move_to_element(feedback_entry).click().perform()
+
+            # validate that second comment is under the first comment
+            comments = self.browser.find_elements_by_class_name('dialog-box')
+            first_text = comments[0].find_element_by_class_name('message')
+            second_text = comments[1].find_element_by_class_name('message')
+
+            self.assertEqual(len(comments), 2)
+            self.assertEqual(first_text.text, 'first ever comment')
+            self.assertEqual(second_text.text, 'the second comment')
+
+            # give feedback on first line
+            self.write_comments_on_lines([(1, 'first comment updated')])
+            self.browser.find_element_by_id('score-full').click()
+            self.browser.find_element_by_id('submit-feedback').click()
+
+            WebDriverWait(self.browser, 5).until(
+                wait_until_element_count_equals(self, By.CLASS_NAME, "dialog-box", 2)
+            )
+
+            # validate that the edited first comment is under the second comment
+            comments = self.browser.find_elements_by_class_name('dialog-box')
+            first_text = comments[0].find_element_by_class_name('message')
+            second_text = comments[1].find_element_by_class_name('message')
+
+            self.assertEqual(first_text.text, 'the second comment')
+            self.assertEqual(second_text.text, 'first comment updated')
+
 
 class TestFeedbackCreationTutor(UntestedParent.TestFeedbackCreationGeneric):
     def setUp(self):

functional_tests/util.py

@@ -130,3 +130,13 @@ def wait_until_code_changes(test_class_instance, code):
             return False
         return code != new_code
     return condition
+
+
+def wait_until_element_count_equals(test_class_instance, by, selector, count):
+    def condition(*args):
+        try:
+            elements = test_class_instance.browser.find_elements(by, selector)
+        except Exception:
+            return False
+        return len(elements) == count
+    return condition

util/importer.py

@@ -134,7 +134,7 @@ def load_hektor_json():
             log.warning(f"Updated submission type {submission_type}")
 
     for student in exam_data['students']:
-        student_obj = user_factory.make_student(exam=exam,
+        student_obj = user_factory.make_student(exam=exam, is_active=False,
                                                 **student).student
         for submission_obj in student['submissions']:
             add_submission(student_obj, **submission_obj)