fixed labels being removed when not changing anything, introduced new type for updated feedback

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/serializers/feedback.py

@@ -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
@@ -151,13 +153,15 @@ class FeedbackSerializer(DynamicFieldsModelSerializer):
             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)
 

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/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

@@ -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

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()