routing on Feedback Creation component should be fixed

Also extended existing frontend tests for feedback creation.
In order to fix routing, partially refactored Subscription Vuex module #145

frontend/src/class-component-hooks.ts

+import Component from 'vue-class-component'
+
+// Register the router hooks with their names
+Component.registerHooks([
+  'beforeRouteEnter',
+  'beforeRouteLeave',
+  'beforeRouteUpdate' // for vue-router 2.2+
+])

frontend/src/components/subscriptions/SubscriptionList.vue

@@ -48,7 +48,7 @@ import { Subscriptions } from '@/store/modules/subscriptions'
   },
 })
 export default class SubscriptionList extends Vue {
-  @Prop({type: Boolean, required: true, default: false}) sidebar!: boolean
+  @Prop({type: Boolean, default: false}) sidebar!: boolean
 
   selectedStage = null
   updating = false
@@ -85,7 +85,7 @@ export default class SubscriptionList extends Vue {
     const subscriptions = Subscriptions.getSubscriptions()
     Promise.all([submissionTypes, subscriptions]).then(() => {
       Subscriptions.subscribeToAll()
-      Subscriptions.cleanAssignmentsFromSubscriptions(undefined)
+      Subscriptions.cleanAssignmentsFromSubscriptions(true)
     })
   }
 }

frontend/src/main.ts

+
+import './class-component-hooks'
+
 import Vue from 'vue'
 import store from './store/store'
 import App from './App.vue'
@@ -6,6 +9,7 @@ import Vuetify from 'vuetify'
 import Notifications from 'vue-notification'
 import Clipboard from 'v-clipboard'
 
+
 import 'vuetify/dist/vuetify.min.css'
 import 'highlight.js/styles/atom-one-light.css'
 

frontend/src/models.ts

@@ -15,7 +15,7 @@ export interface Assignment {
      * @type {string}
      * @memberof Assignment
      */
-    submission?: string
+    submission?: string | SubmissionAssignment
     /**
      *
      * @type {boolean}
@@ -40,6 +40,13 @@ export interface Assignment {
     subscription?: string
 }
 
+export interface SubmissionAssignment {
+    text: string,
+    type: string
+    full_score: number,
+    tests: Test[]
+}
+
 /**
  *
  * @export

frontend/src/pages/SubscriptionWorkPage.vue

@@ -31,89 +31,85 @@
   </v-layout>
 </template>
 
-<script>
-import SubmissionCorrection from '@/components/submission_notes/SubmissionCorrection'
-import { SubmissionNotes } from '@/store/modules/submission-notes'
-import SubmissionType from '@/components/SubmissionType'
+
+<script lang="ts">
+import { Vue, Component} from 'vue-property-decorator'
+import { Route, NavigationGuard } from 'vue-router'
+import SubmissionCorrection from '@/components/submission_notes/SubmissionCorrection.vue'
+import SubmissionType from '@/components/SubmissionType.vue'
 import store from '@/store/store'
-import SubmissionTests from '@/components/SubmissionTests'
+import { SubmissionNotes } from '@/store/modules/submission-notes'
+import SubmissionTests from '@/components/SubmissionTests.vue'
 import { Subscriptions } from '@/store/modules/subscriptions'
-import RouteChangeConfirmation from '@/components/submission_notes/RouteChangeConfirmation'
+import RouteChangeConfirmation from '@/components/submission_notes/RouteChangeConfirmation.vue'
 import { getters } from '@/store/getters'
+import { SubmissionAssignment } from '@/models'
 
-function onRouteEnterOrUpdate (to, from, next) {
-  Subscriptions.changeActiveSubscription(to.params['pk']).then(() => {
+const onRouteEnterOrUpdate: NavigationGuard =  function (to, from, next) {
+  Subscriptions.changeToSubscription(to.params['pk']).then(() => {
     next()
   })
 }
 
-export default {
+@Component({
   components: {
     RouteChangeConfirmation,
     SubmissionTests,
     SubmissionType,
     SubmissionCorrection
-  },
-  name: 'subscription-work-page',
-  data () {
-    return {
-      subscriptionActive: true,
-      nextRoute: null
-    }
-  },
-  computed: {
-    subscription () {
+  }
+})
+export default class SubscriptionWorkPage extends Vue {
+
+  subscriptionActive = false
+  nextRoute = () => {}
+
+  get subscription () {
     return Subscriptions.state.subscriptions[this.$route.params['pk']]
-    },
-    currentAssignment () {
-      return Subscriptions.state.assignmentQueue[0]
-    },
-    submission () {
-      return this.currentAssignment.submission
-    },
-    submissionType () {
-      return getters.state.submissionTypes[this.submission.type]
-    }
-  },
-  beforeRouteEnter (to, from, next) {
+  }
+
+  get currentAssignment () {
+    return Subscriptions.state.currentAssignment
+  }
+
+  get submission () {
+    return this.currentAssignment && this.currentAssignment.submission
+  }
+
+  get submissionType () {
+    if (this.submission && (<SubmissionAssignment> this.submission).type) {
+      return getters.state.submissionTypes[(<SubmissionAssignment>this.submission).type]
+    }
+  }
+
+  beforeRouteEnter (to: Route, from: Route, next: (to?: any) => void ) {
     onRouteEnterOrUpdate(to, from, next)
-  },
-  beforeRouteUpdate (to, from, next) {
+  }
+
+  beforeRouteUpdate (this: SubscriptionWorkPage, to: Route, from: Route, next: (to?: any) => void) {
     this.nextRoute = () => {
       onRouteEnterOrUpdate(to, from, next)
     }
-  },
-  beforeRouteLeave (to, from, next) {
+  }
+
+  beforeRouteLeave (this: SubscriptionWorkPage, to: Route, from: Route, next: (to?: any) => void) {
     if (to.name === 'subscription-ended') {
       next()
     } else {
       this.nextRoute = () => {
-        Subscriptions.removeActiveSubscription()
         next()
+        Subscriptions.deleteCurrentAssignment()
       }
     }
-  },
-  methods: {
-    startWorkOnNextAssignment () {
-      Subscriptions.getAssignmentsForActiveSubscription(1).then(([promise]) => {
-        promise.then(assignment => {
-          Subscriptions.ADD_ASSIGNMENT_TO_QUEUE(assignment)
-        }).finally(() => {
-          Subscriptions.POP_ASSIGNMENT_FROM_QUEUE()
-        })
-      })
   }
-  },
-  watch: {
-    currentAssignment (val) {
-      this.$vuetify.goTo(0, { duration: 200, easing: 'easeInOutCubic' })
-      if (val === undefined) {
+
+  startWorkOnNextAssignment () {
+    Subscriptions.createNextAssignment().catch(() => {
       const typePk = SubmissionNotes.state.submission.type
         this.$router.replace(typePk + '/ended')
-        Subscriptions.removeActiveSubscription()
+        Subscriptions.SET_CURRENT_ASSIGNMENT(undefined)
         Subscriptions.getSubscriptions()
-      }
-    }
+    })
   }
 }
 </script>

frontend/src/store/modules/feedback_list/feedback-table.ts

@@ -38,7 +38,7 @@ function ADD_ASSIGNMENTS_INFO (state: FeedbackTableState, assignments: Array<Ass
     if (!assignment.submission || !assignment.stage) {
       throw Error()
     }
-    const feedback = state.feedbackHist[assignment.submission]
+    const feedback = state.feedbackHist[<string> assignment.submission]
     feedback.history = {
       ...feedback.history,
       [assignment.stage]: {

frontend/src/store/modules/subscriptions.ts

@@ -9,24 +9,20 @@ import { getStoreBuilder, BareActionContext } from 'vuex-typex'
 
 export interface SubscriptionsState {
   subscriptions: {[pk: string]: Subscription}
-  assignmentQueue: Array<Assignment>
-  activeSubscriptionPk: string
+  currentAssignment?: Assignment
   loading: boolean
 }
 
 function initialState (): SubscriptionsState {
   return {
     subscriptions: {},
-    assignmentQueue: [],
-    activeSubscriptionPk: '',
+    currentAssignment: undefined,
     loading: false
   }
 }
 
 const mb = getStoreBuilder<RootState>().module('Subscriptions', initialState())
 
-const MAX_NUMBER_OF_ASSIGNMENTS = 2
-
 const stateGetter = mb.state()
 
 const availableTypesGetter = mb.read(function availableTypes (state, getters) {
@@ -36,6 +32,7 @@ const availableTypesGetter = mb.read(function availableTypes (state, getters) {
   }
   return types
 })
+
 const availableStagesGetter = mb.read(function availableStages (state, getters) {
   let stages = [Subscription.FeedbackStageEnum.Creation, Subscription.FeedbackStageEnum.Validation]
   if (Authentication.isReviewer) {
@@ -43,6 +40,7 @@ const availableStagesGetter = mb.read(function availableStages (state, getters)
   }
   return stages
 })
+
 const availableStagesReadableGetter = mb.read(function availableStagesReadable (state, getters) {
   let stages = ['initial', 'validate']
   if (Authentication.isReviewer) {
@@ -50,15 +48,23 @@ const availableStagesReadableGetter = mb.read(function availableStagesReadable (
   }
   return stages
 })
+
 const availableSubmissionTypeQueryKeysGetter = mb.read(function availableSubmissionTypeQueryKeys (state, getters, rootState) {
   return Object.values(rootState.submissionTypes).map((subType: any) => subType.pk)
 })
+
 const availableExamTypeQueryKeysGetter = mb.read(function availableExamTypeQueryKeys (state, getters, rootState) {
   return Object.values(rootState.examTypes).map((examType: any) => examType.pk)
 })
+
 const activeSubscriptionGetter = mb.read(function activeSubscription (state) {
-  return state.subscriptions[state.activeSubscriptionPk]
+  if (state.currentAssignment && state.currentAssignment.subscription) {
+    return state.subscriptions[state.currentAssignment.subscription]
+  }
+
+  return undefined
 })
+
 const resolveSubscriptionKeyToNameGetter = mb.read(function resolveSubscriptionKeyToName (state, getters, rootState) {
   return (subscription: {queryType: Subscription.QueryTypeEnum, queryKey: string}) => {
     switch (subscription.queryType) {
@@ -127,21 +133,15 @@ function SET_SUBSCRIPTIONS (state: SubscriptionsState, subscriptions: Array<Subs
     return acc
   }, {})
 }
+
 function SET_SUBSCRIPTION (state: SubscriptionsState, subscription: Subscription): void {
   Vue.set(state.subscriptions, subscription.pk, subscription)
 }
-function SET_ACTIVE_SUBSCRIPTION_PK (state: SubscriptionsState, subscriptionPk: string): void {
-  state.activeSubscriptionPk = subscriptionPk
-}
-function SET_ASSIGNMENT_QUEUE (state: SubscriptionsState, queue: Array<Assignment>): void {
-  state.assignmentQueue = queue
-}
-function ADD_ASSIGNMENT_TO_QUEUE (state: SubscriptionsState, assignment: Assignment): void {
-  state.assignmentQueue.push(assignment)
-}
-function POP_ASSIGNMENT_FROM_QUEUE (state: SubscriptionsState): void {
-  state.assignmentQueue.shift()
+
+function SET_CURRENT_ASSIGNMENT (state: SubscriptionsState, assignment?: Assignment): void {
+  state.currentAssignment = assignment
 }
+
 function RESET_STATE (state: SubscriptionsState): void {
   Object.assign(state, initialState())
   subscribeToAll.reset()
@@ -162,34 +162,43 @@ async function subscribeTo (
   Subscriptions.SET_SUBSCRIPTION(subscription)
   return subscription
 }
+
 async function getSubscriptions () {
   const subscriptions = await api.fetchSubscriptions()
   Subscriptions.SET_SUBSCRIPTIONS(subscriptions)
   return subscriptions
 }
-/**
- * Creates as many assignments as needed to reach MAX_NUMBER_OF_ASSIGNMENTS
- * @param numOfAssignments Use to override default behaviour of
- * creating MAX_NUMBER_OF_ASSIGNMENTS - assignmentQueue.length assignments
- */
-async function getAssignmentsForActiveSubscription
-(context: BareActionContext<SubscriptionsState, RootState>, numOfAssignments: number):
-  Promise<Promise<Assignment>[]> {
-  numOfAssignments = numOfAssignments || MAX_NUMBER_OF_ASSIGNMENTS - context.state.assignmentQueue.length
-  let assignmentsPromises = []
-  for (let i = 0; i < numOfAssignments; i++) {
-    assignmentsPromises.push(api.createAssignment({ subscription: Subscriptions.activeSubscription }))
-  }
-  return assignmentsPromises
-}
-async function deleteAssignment
-(context: BareActionContext<SubscriptionsState, RootState>, assignment: Assignment) {
-  return api.deleteAssignment({ assignment })
+
+
+async function changeToSubscription({state}: BareActionContext<SubscriptionsState, RootState>, subscriptionPk: string) {
+  const currAssignment = state.currentAssignment
+  if (currAssignment && currAssignment.subscription == subscriptionPk) {
+    return
   }
+
+  if (currAssignment) {
+    await api.deleteAssignment({assignment: currAssignment})
+  }
+
+  const newAssignment = await api.createAssignment({subscriptionPk})
+  Subscriptions.SET_CURRENT_ASSIGNMENT(newAssignment)
+}
+
+async function createNextAssignment() {
+  const activeSubscription = Subscriptions.activeSubscription
+  if (!activeSubscription) {
+    throw new Error("There must be an active Subscription before calling createNextAssignment")
+  }
+  const newAssignment = await api.createAssignment({subscription: activeSubscription})
+  Subscriptions.SET_CURRENT_ASSIGNMENT(newAssignment)
+}
+
 async function cleanAssignmentsFromSubscriptions
 ({ state }: BareActionContext<SubscriptionsState, RootState>, excludeActive = true) {
   Object.values(state.subscriptions).forEach(subscription => {
-    if (!excludeActive || subscription.pk !== state.activeSubscriptionPk) {
+    if (!excludeActive ||
+        !Subscriptions.activeSubscription ||
+        subscription.pk !== Subscriptions.activeSubscription.pk) {
       if (subscription.assignments) {
         subscription.assignments.forEach(assignment => {
           api.deleteAssignment({ assignment })
@@ -198,48 +207,26 @@ async function cleanAssignmentsFromSubscriptions
     }
   })
 }
+
 async function skipAssignment ({ state }: BareActionContext<SubscriptionsState, RootState>) {
-  Subscriptions.deleteAssignment(state.assignmentQueue[0])
-    .then(() => {
-      // pass numOfAssignments = 1 to create 1 new assignment although maybe two are already in the queue,
-      // this is needed because otherwise the current assignment in the comp. might be unknown for a period
-      // which will result get incorrectly interpreted as a an ended subscription
-      return Subscriptions.getAssignmentsForActiveSubscription(1)
-    }).then(([promise]) => {
-      promise.then((assignment: Assignment) => {
-        Subscriptions.ADD_ASSIGNMENT_TO_QUEUE(assignment)
-        Subscriptions.POP_ASSIGNMENT_FROM_QUEUE()
-      })
-    })
-}
-async function deleteActiveAssignments ({ state }: BareActionContext<SubscriptionsState, RootState>) {
-  Promise.all(state.assignmentQueue.map(assignment => {
-    Subscriptions.deleteAssignment(assignment)
-  }))
-}
-async function changeActiveSubscription ({ state }: BareActionContext<SubscriptionsState, RootState>, subscriptionPk = '') {
-  if (subscriptionPk === state.activeSubscriptionPk) {
-    return
+  if (!state.currentAssignment || !state.currentAssignment.subscription) {
+    throw new Error("skipAssignment can only be called with active assignment")
   }
-  await Subscriptions.deleteActiveAssignments()
-  Subscriptions.SET_ACTIVE_SUBSCRIPTION_PK(subscriptionPk)
-  let assignmentsPromises = await Subscriptions.getAssignmentsForActiveSubscription(MAX_NUMBER_OF_ASSIGNMENTS)
-  let createdAssignments = []
-  // TODO refactor this since it's very bad to await promises in for loops
 
-  for (let promise of assignmentsPromises) {
-    try {
-      createdAssignments.push(await promise)
-    } catch (_) {}
+  const newAssignment = await api.createAssignment({subscriptionPk: state.currentAssignment.subscription})
+  await api.deleteAssignment({assignment: state.currentAssignment })
+
+  Subscriptions.SET_CURRENT_ASSIGNMENT(newAssignment)
 }
-  Subscriptions.SET_ASSIGNMENT_QUEUE(createdAssignments)
+
+async function deleteCurrentAssignment ({ state }: BareActionContext<SubscriptionsState, RootState>) {
+  if (!state.currentAssignment) {
+    throw new Error("No active assignment to delete")
   }
-async function removeActiveSubscription () {
-  await Subscriptions.deleteActiveAssignments()
-  Subscriptions.SET_ASSIGNMENT_QUEUE([])
-  Subscriptions.SET_ACTIVE_SUBSCRIPTION_PK('')
+  await api.deleteAssignment({assignment: state.currentAssignment})
+  Subscriptions.SET_CURRENT_ASSIGNMENT(undefined)
 }
-// TODO use enums here
+
 async function subscribeToType
 (context: BareActionContext<SubscriptionsState, RootState>, type: Subscription.QueryTypeEnum) {
   switch (type) {
@@ -268,6 +255,7 @@ async function subscribeToType
       break
   }
 }
+
 const subscribeToAll = once(async () => {
   return Promise.all(flatten(Subscriptions.availableTypes.map((type) => {
     return Subscriptions.subscribeToType(type)
@@ -287,21 +275,16 @@ export const Subscriptions = {
 
   SET_SUBSCRIPTIONS: mb.commit(SET_SUBSCRIPTIONS),
   SET_SUBSCRIPTION: mb.commit(SET_SUBSCRIPTION),
-  SET_ACTIVE_SUBSCRIPTION_PK: mb.commit(SET_ACTIVE_SUBSCRIPTION_PK),
-  SET_ASSIGNMENT_QUEUE: mb.commit(SET_ASSIGNMENT_QUEUE),
-  ADD_ASSIGNMENT_TO_QUEUE: mb.commit(ADD_ASSIGNMENT_TO_QUEUE),
-  POP_ASSIGNMENT_FROM_QUEUE: mb.commit(POP_ASSIGNMENT_FROM_QUEUE),
+  SET_CURRENT_ASSIGNMENT: mb.commit(SET_CURRENT_ASSIGNMENT),
   RESET_STATE: mb.commit(RESET_STATE),
 
   subscribeTo: mb.dispatch(subscribeTo),
   getSubscriptions: mb.dispatch(getSubscriptions),
-  getAssignmentsForActiveSubscription: mb.dispatch(getAssignmentsForActiveSubscription),
-  deleteAssignment: mb.dispatch(deleteAssignment),
   cleanAssignmentsFromSubscriptions: mb.dispatch(cleanAssignmentsFromSubscriptions),
+  changeToSubscription: mb.dispatch(changeToSubscription),
+  createNextAssignment: mb.dispatch(createNextAssignment),
   skipAssignment: mb.dispatch(skipAssignment),
-  deleteActiveAssignments: mb.dispatch(deleteActiveAssignments),
-  changeActiveSubscription: mb.dispatch(changeActiveSubscription),
-  removeActiveSubscription: mb.dispatch(removeActiveSubscription),
+  deleteCurrentAssignment: mb.dispatch(deleteCurrentAssignment),
   subscribeToType: mb.dispatch(subscribeToType),
   subscribeToAll: mb.dispatch(subscribeToAll, 'subscribeToAll')
 }

functional_tests/test_feedback_creation.py

@@ -211,11 +211,20 @@ class UntestedParent:
         def test_can_validate_submission(self):
             self._login()
             self._go_to_subscription()
+
+            def correct():
                 code = self._reconstruct_submission_code()
                 self.write_comments_on_lines([(0, 'A comment by me')])
                 self.browser.find_element_by_id('score-zero').click()
                 self.browser.find_element_by_id('submit-feedback').click()
+                return code
+            code = correct()
             WebDriverWait(self.browser, 10).until(self.wait_until_code_changes(code))
+            correct()
+
+            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
 
             user_snd = 'tutor_snd'
@@ -225,14 +234,38 @@ class UntestedParent:
             login(self.browser, self.live_server_url, user_snd, password)
             self._go_to_subscription(stage='validate')
             self.write_comments_on_lines([(0, 'I disagree'), (1, 'Full points!')])
+            code_final = self._reconstruct_submission_code()
             self.browser.find_element_by_id('score-full').click()
             self.browser.find_element_by_id('submit-feedback').click()
+
+            WebDriverWait(self.browser, 10).until(self.wait_until_code_changes(code_final))
+            code_non_final = self._reconstruct_submission_code()
+            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))
-            submission_for_code = Submission.objects.get(text=code)
+
+            reset_browser_after_test(self.browser, self.live_server_url)
+
+            user_rev = 'rev'
+            password = 'p'
+            role = UserAccount.REVIEWER
+            fact.UserAccountFactory(username=user_rev, password=password, role=role)
+            login(self.browser, self.live_server_url, user_rev, password)
+
+            self._go_to_subscription('conflict')
+            code = self._reconstruct_submission_code()
+            self.assertEqual(code, code_non_final)
+
+            submission_for_code = Submission.objects.get(text=code_final)
             self.assertEqual(self.sub_type.full_score, submission_for_code.feedback.score)
             self.assertEqual(3, submission_for_code.feedback.feedback_lines.count())
 
+            submission_for_code = Submission.objects.get(text=code_non_final)
+            self.assertEqual(0, submission_for_code.feedback.score)
+            self.assertEqual(1, submission_for_code.feedback.feedback_lines.count())
+
 
 class TestFeedbackCreationTutor(UntestedParent.TestFeedbackCreationGeneric):
     def setUp(self):
@@ -242,5 +275,6 @@ class TestFeedbackCreationTutor(UntestedParent.TestFeedbackCreationGeneric):
         self.role = UserAccount.TUTOR
         fact.UserAccountFactory(
             username=self.username,
-            password=self.password
+            password=self.password,
+            role=self.role
         )