From 6184362c5e6951e40bebdf5e521ca033c39096a3 Mon Sep 17 00:00:00 2001 From: "robinwilliam.hundt" <robinwilliam.hundt@stud.uni-goettingen.de> Date: Wed, 6 Mar 2019 18:43:13 +0100 Subject: [PATCH] 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 | 8 + .../subscriptions/SubscriptionList.vue | 10 +- frontend/src/main.ts | 6 +- frontend/src/models.ts | 9 +- frontend/src/pages/SubscriptionWorkPage.vue | 108 +++++++------ .../modules/feedback_list/feedback-table.ts | 2 +- frontend/src/store/modules/subscriptions.ts | 146 ++++++++---------- functional_tests/test_feedback_creation.py | 46 +++++- 8 files changed, 183 insertions(+), 152 deletions(-) create mode 100644 frontend/src/class-component-hooks.ts diff --git a/frontend/src/class-component-hooks.ts b/frontend/src/class-component-hooks.ts new file mode 100644 index 00000000..2518b8b6 --- /dev/null +++ b/frontend/src/class-component-hooks.ts @@ -0,0 +1,8 @@ +import Component from 'vue-class-component' + +// Register the router hooks with their names +Component.registerHooks([ + 'beforeRouteEnter', + 'beforeRouteLeave', + 'beforeRouteUpdate' // for vue-router 2.2+ +]) diff --git a/frontend/src/components/subscriptions/SubscriptionList.vue b/frontend/src/components/subscriptions/SubscriptionList.vue index 74f4383b..ebe70f12 100644 --- a/frontend/src/components/subscriptions/SubscriptionList.vue +++ b/frontend/src/components/subscriptions/SubscriptionList.vue @@ -44,11 +44,11 @@ import { Subscriptions } from '@/store/modules/subscriptions' components: { SubscriptionsForStage, SubscriptionForList, - SubscriptionCreation + SubscriptionCreation }, }) 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 @@ -57,8 +57,8 @@ export default class SubscriptionList extends Vue { get subscriptions () { return Subscriptions.state.subscriptions } get stages () { return Subscriptions.availableStages } get stagesReadable () { return Subscriptions.availableStagesReadable } - get showDetail () { - return !this.sidebar || (this.sidebar && !UI.state.sideBarCollapsed) + get showDetail () { + return !this.sidebar || (this.sidebar && !UI.state.sideBarCollapsed) } async getSubscriptions (silent: boolean) { @@ -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) }) } } diff --git a/frontend/src/main.ts b/frontend/src/main.ts index 1cd37bcf..9d0d5524 100644 --- a/frontend/src/main.ts +++ b/frontend/src/main.ts @@ -1,3 +1,6 @@ + +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' @@ -22,4 +26,4 @@ export default new Vue({ router: router, store, render: h => h(App) -}).$mount(el) \ No newline at end of file +}).$mount(el) diff --git a/frontend/src/models.ts b/frontend/src/models.ts index c94d46fa..a1d33255 100644 --- a/frontend/src/models.ts +++ b/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 diff --git a/frontend/src/pages/SubscriptionWorkPage.vue b/frontend/src/pages/SubscriptionWorkPage.vue index dc0eba45..7040ba24 100644 --- a/frontend/src/pages/SubscriptionWorkPage.vue +++ b/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 () { - 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] + } +}) +export default class SubscriptionWorkPage extends Vue { + + subscriptionActive = false + nextRoute = () => {} + + get subscription () { + return Subscriptions.state.subscriptions[this.$route.params['pk']] + } + + 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, from, next) { + } + + 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) { - const typePk = SubmissionNotes.state.submission.type + } + + 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> diff --git a/frontend/src/store/modules/feedback_list/feedback-table.ts b/frontend/src/store/modules/feedback_list/feedback-table.ts index 85c52287..ab18dbcf 100644 --- a/frontend/src/store/modules/feedback_list/feedback-table.ts +++ b/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]: { diff --git a/frontend/src/store/modules/subscriptions.ts b/frontend/src/store/modules/subscriptions.ts index 83c52ccf..366876e1 100644 --- a/frontend/src/store/modules/subscriptions.ts +++ b/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 })) + + +async function changeToSubscription({state}: BareActionContext<SubscriptionsState, RootState>, subscriptionPk: string) { + const currAssignment = state.currentAssignment + if (currAssignment && currAssignment.subscription == subscriptionPk) { + return } - return assignmentsPromises + + if (currAssignment) { + await api.deleteAssignment({assignment: currAssignment}) + } + + const newAssignment = await api.createAssignment({subscriptionPk}) + Subscriptions.SET_CURRENT_ASSIGNMENT(newAssignment) } -async function deleteAssignment -(context: BareActionContext<SubscriptionsState, RootState>, assignment: Assignment) { - return api.deleteAssignment({ assignment }) + +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,25 @@ 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 (_) {} - } - Subscriptions.SET_ASSIGNMENT_QUEUE(createdAssignments) + await api.deleteAssignment({assignment: state.currentAssignment }) + const newAssignment = await api.createAssignment({subscriptionPk: state.currentAssignment.subscription}) + + Subscriptions.SET_CURRENT_ASSIGNMENT(newAssignment) } -async function removeActiveSubscription () { - await Subscriptions.deleteActiveAssignments() - Subscriptions.SET_ASSIGNMENT_QUEUE([]) - Subscriptions.SET_ACTIVE_SUBSCRIPTION_PK('') + +async function deleteCurrentAssignment ({ state }: BareActionContext<SubscriptionsState, RootState>) { + if (!state.currentAssignment) { + throw new Error("No active assignment to delete") + } + 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 +254,7 @@ async function subscribeToType break } } + const subscribeToAll = once(async () => { return Promise.all(flatten(Subscriptions.availableTypes.map((type) => { return Subscriptions.subscribeToType(type) @@ -287,21 +274,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') } diff --git a/functional_tests/test_feedback_creation.py b/functional_tests/test_feedback_creation.py index bc40155d..667d7a48 100644 --- a/functional_tests/test_feedback_creation.py +++ b/functional_tests/test_feedback_creation.py @@ -211,11 +211,20 @@ class UntestedParent: def test_can_validate_submission(self): self._login() self._go_to_subscription() - 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() + + 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 ) -- GitLab