Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Synchronise user selections with Quiz Creation State #11783

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
a44adac
add confirm model
AllanOXDi Jan 17, 2024
a4608ac
add helper function realted with working_resource_pool
ozer550 Jan 22, 2024
c696280
complete set setters and getters related with workingResourcePool
ozer550 Jan 23, 2024
bc0b264
Merge pull request #1 from ozer550/add-working-resource-pool-logic
AllanOXDi Jan 23, 2024
8ebc4ab
section questions get updated
AllanOXDi Jan 23, 2024
4ed3c5a
select all adds and removes resources from the pool
AllanOXDi Jan 24, 2024
90eb803
select all adds and removes resources from the pool
AllanOXDi Jan 24, 2024
42d85c2
working pool reset on saving changes
AllanOXDi Jan 24, 2024
534b431
only checked resources are added to the working resource pool
AllanOXDi Jan 24, 2024
98e88d6
removed double addition of resource to the working pool
AllanOXDi Jan 24, 2024
6cfff4d
removed unused component
AllanOXDi Jan 24, 2024
d29b873
make ConfirmationalModal work
ozer550 Jan 24, 2024
595d92a
integrate ConfirmationalModal working with closing sidePanel
ozer550 Jan 25, 2024
960d29b
fix small bugs for select all checkboxes
ozer550 Jan 25, 2024
7e5dd31
fix linting
ozer550 Jan 25, 2024
775434c
handle @cancel event from ConfirmCancellationModal
nucleogenesis Jan 25, 2024
5628303
revert unused injection of quiz creation stuff into ContentCardList
nucleogenesis Jan 25, 2024
1e88a7a
make add/remove/exists? resource selection API consistent; add JSDocs
nucleogenesis Jan 25, 2024
90f1b6f
minor code simplification
nucleogenesis Jan 25, 2024
8149baa
comments update; remove search-related unused methods
nucleogenesis Jan 25, 2024
76eb986
remove unused borrowed code
nucleogenesis Jan 25, 2024
2379859
remove reference to saveQuiz in ResourceSelection as it is not used
nucleogenesis Jan 25, 2024
f9e66da
remove -'s from uuid's generated in quiz creation for consistency
nucleogenesis Jan 25, 2024
2cbd234
only pass IDs to API in resource_pool
nucleogenesis Jan 26, 2024
97ca355
move resource pool init to ResourceSelection
nucleogenesis Jan 29, 2024
618fe47
remove unused import/exporst
nucleogenesis Jan 29, 2024
9f64cc0
emit continue event from cancellation modal, reset in event handler
nucleogenesis Jan 29, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 94 additions & 2 deletions kolibri/plugins/coach/assets/src/composables/useQuizCreation.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { v4 as uuidv4 } from 'uuid';
import { v4 } from 'uuid';
import isEqual from 'lodash/isEqual';
import uniqWith from 'lodash/uniqWith';
import range from 'lodash/range';
import shuffle from 'lodash/shuffle';
import { enhancedQuizManagementStrings } from 'kolibri-common/strings/enhancedQuizManagementStrings';
Expand All @@ -16,6 +17,10 @@ import { Quiz, QuizSection, QuizQuestion, QuizExercise } from './quizCreationSpe

const logger = logging.getLogger(__filename);

function uuidv4() {
return v4().replace(/-/g, '');
}

/** Validators **/
/* objectSpecs expects every property to be available -- but we don't want to have to make an
* object with every property just to validate it. So we use these functions to validate subsets
Expand All @@ -41,6 +46,10 @@ export default function useQuizCreation(DEBUG = false) {
// Local state
// -----------

/** @type {ComputedRef<QuizExercise[]>} Currently selected resource_pool
* from the side_panel*/
const _working_resource_pool = ref([]);

/** @type {ref<Quiz>}
* The "source of truth" quiz object from which all reactive properties should derive */
const _quiz = ref(objectWithDefaults({}, Quiz));
Expand Down Expand Up @@ -265,6 +274,12 @@ export default function useQuizCreation(DEBUG = false) {
_fetchChannels();
}

// // Method to initialize the working resource pool
function initializeWorkingResourcePool() {
// Set the value of _working_resource_pool to the resource_pool of the active section
set(_working_resource_pool, get(activeResourcePool));
}

/**
* @returns {Promise<Quiz>}
* @throws {Error} if quiz is not valid
Expand All @@ -282,7 +297,19 @@ export default function useQuizCreation(DEBUG = false) {
if (!validateQuiz(get(_quiz))) {
throw new Error(`Quiz is not valid: ${JSON.stringify(get(_quiz))}`);
}
return ExamResource.saveModel({ data: get(_quiz) });

// Here we update each section's `resource_pool` to only be the IDs of the resources
const sectionsWithResourcePoolAsIDs = get(allSections).map(section => {
const resourcePoolAsIds = get(section).resource_pool.map(content => content.id);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't we just always maintain the frontend representation of this the same as the backend, and have a separate representation of the resources that map to these IDs that we populate when we start editing a specific section? (in the proposed separate state management within ResourceSelection that I propose below).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I specced it this way so that we have access to all of the Exercise metadata that we've already fetched at hand so we can do the ancestry calculations and get the assessmentids for question enumeration.

This is related to the issue I made and closed yesterday re: updating the serializer to take the object rather than the IDs - but since that's only a concern in the case that we edit a saved quiz (in which case we'd need to fetch the metadata).

I figured that this way reduces our needing to fetch the metadata wherever it is needed here while keeping things roughly in the shape of the backend data - except on the front end we keep the whole object whereas we just send the IDs.

To be honest I'm not entirely sure this needs to be saved at all except for in the case that we permit editing / duplication of quizzes as the 'questions' property has everything needed to render the quiz. The resource_pool is solely used for enumerating the total possible pool of questions the coach can choose from to add to the quiz.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coming back to this after the weekend, I think that it makes sense to maintain a separate map of the metadata and to keep the list of IDs in the section's data withing the useQuizCreation module.

This would then be easy to extend to accommodate duplication/editing of a quiz or section as we can then populate the initial map with a single API call for the list of node IDs there.

section.resource_pool = resourcePoolAsIds;
return section;
});

const finalQuiz = get(_quiz);

finalQuiz.question_sources = sectionsWithResourcePoolAsIDs;

return ExamResource.saveModel({ data: finalQuiz });
}

/**
Expand Down Expand Up @@ -337,6 +364,11 @@ export default function useQuizCreation(DEBUG = false) {
}
}

function resetWorkingResourcePool() {
// Set the WorkingResource to empty array again!
set(_working_resource_pool, []);
}

/**
* @affects _channels - Fetches all channels with exercises and sets them to _channels */
function _fetchChannels() {
Expand Down Expand Up @@ -408,6 +440,9 @@ export default function useQuizCreation(DEBUG = false) {
/** @type {ComputedRef<Array>} A list of all channels available which have exercises */
const channels = computed(() => get(_channels));

// /** @type {ComputedRef<QuizExercise[]>} The current value of _working_resource_pool */
const workingResourcePool = computed(() => get(_working_resource_pool));

/** Handling the Select All Checkbox
* See: remove/toggleQuestionFromSelection() & selectAllQuestions() for more */

Expand Down Expand Up @@ -445,12 +480,45 @@ export default function useQuizCreation(DEBUG = false) {
}
});

/**
* @param {QuizExercise[]} resources
* @affects _working_resource_pool -- Updates it with the given resources and is ensured to have
* a list of unique resources to avoid unnecessary duplication
*/
function addToWorkingResourcePool(resources = []) {
set(_working_resource_pool, uniqWith([...get(_working_resource_pool), ...resources], isEqual));
}

/**
* @param {QuizExercise} content
* @affects _working_resource_pool - Remove given quiz exercise from _working_resource_pool
*/
function removeFromWorkingResourcePool(content) {
set(
_working_resource_pool,
_working_resource_pool.value.filter(obj => obj.id !== content.id)
);
}

/**
* @param {QuizExercise} content
* Check if the content is present in working_resource_pool
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JSDoc suggestion:
@param {QuizExercise} content - Check if the content is present...
@returns Boolean

function contentPresentInWorkingResourcePool(content) {
const workingResourceIds = get(workingResourcePool).map(wr => wr.id);
return workingResourceIds.includes(content.id);
}

/** @type {ComputedRef<Boolean>} Whether the select all checkbox should be indeterminate */
const selectAllIsIndeterminate = computed(() => {
return !get(allQuestionsSelected) && !get(noQuestionsSelected);
});

provide('saveQuiz', saveQuiz);
provide('initializeWorkingResourcePool', initializeWorkingResourcePool);
provide('addToWorkingResourcePool', addToWorkingResourcePool);
provide('removeFromWorkingResourcePool', removeFromWorkingResourcePool);
provide('contentPresentInWorkingResourcePool', contentPresentInWorkingResourcePool);
provide('updateSection', updateSection);
provide('replaceSelectedQuestions', replaceSelectedQuestions);
provide('addSection', addSection);
Expand All @@ -460,11 +528,14 @@ export default function useQuizCreation(DEBUG = false) {
provide('updateQuiz', updateQuiz);
provide('addQuestionToSelection', addQuestionToSelection);
provide('removeQuestionFromSelection', removeQuestionFromSelection);
provide('resetWorkingResourcePool', resetWorkingResourcePool);
provide('channels', channels);
provide('quiz', quiz);
provide('allSections', allSections);
provide('activeSection', activeSection);
provide('inactiveSections', inactiveSections);
provide('activeResourcePool', activeResourcePool);
provide('workingResourcePool', workingResourcePool);
provide('activeExercisePool', activeExercisePool);
provide('activeQuestionsPool', activeQuestionsPool);
provide('activeQuestions', activeQuestions);
Expand All @@ -477,8 +548,13 @@ export default function useQuizCreation(DEBUG = false) {
return {
// Methods
saveQuiz,
initializeWorkingResourcePool,
removeFromWorkingResourcePool,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is turning the useQuizCreation composable into a bit of a catch all again - I think all the state and methods here are at a smaller level of granularity than the useQuizCreation composable is handling.

I think with some judicious work, this could all be instantiated entirely within the ResourceSelection component, or in a separate composable that is only used inside the ResourceSelection component.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the main issue is handling that the component re-setups each navigation, hence it living in then composable. I think I initially suggested that it live in ResourceSelection but foresaw it being a problem until we fix the coach routing issues

addToWorkingResourcePool,
contentPresentInWorkingResourcePool,
updateSection,
replaceSelectedQuestions,
resetWorkingResourcePool,
addSection,
removeSection,
setActiveSection,
Expand All @@ -493,6 +569,8 @@ export default function useQuizCreation(DEBUG = false) {
allSections,
activeSection,
inactiveSections,
workingResourcePool,
activeResourcePool,
activeExercisePool,
activeQuestionsPool,
activeQuestions,
Expand All @@ -516,9 +594,14 @@ export default function useQuizCreation(DEBUG = false) {

export function injectQuizCreation() {
const saveQuiz = inject('saveQuiz');
const initializeWorkingResourcePool = inject('initializeWorkingResourcePool');
const removeFromWorkingResourcePool = inject('removeFromWorkingResourcePool');
const contentPresentInWorkingResourcePool = inject('contentPresentInWorkingResourcePool');
const addToWorkingResourcePool = inject('addToWorkingResourcePool');
const updateSection = inject('updateSection');
const replaceSelectedQuestions = inject('replaceSelectedQuestions');
const addSection = inject('addSection');
const resetWorkingResourcePool = inject('resetWorkingResourcePool');
const removeSection = inject('removeSection');
const setActiveSection = inject('setActiveSection');
const initializeQuiz = inject('initializeQuiz');
Expand All @@ -530,6 +613,8 @@ export function injectQuizCreation() {
const allSections = inject('allSections');
const activeSection = inject('activeSection');
const inactiveSections = inject('inactiveSections');
const activeResourcePool = inject('activeResourcePool');
const workingResourcePool = inject('workingResourcePool');
const activeExercisePool = inject('activeExercisePool');
const activeQuestionsPool = inject('activeQuestionsPool');
const activeQuestions = inject('activeQuestions');
Expand All @@ -542,9 +627,14 @@ export function injectQuizCreation() {
return {
// Methods
saveQuiz,
initializeWorkingResourcePool,
addToWorkingResourcePool,
contentPresentInWorkingResourcePool,
removeFromWorkingResourcePool,
deleteActiveSelectedQuestions,
selectAllQuestions,
updateSection,
resetWorkingResourcePool,
replaceSelectedQuestions,
addSection,
removeSection,
Expand All @@ -561,6 +651,8 @@ export function injectQuizCreation() {
allSections,
activeSection,
inactiveSections,
workingResourcePool,
activeResourcePool,
activeExercisePool,
activeQuestionsPool,
activeQuestions,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
<template>

<div>
<KModal
title="Are you sure you want to exit ?"
>
<p>All the changes made on selected resource will be lost</p>

<template #actions>
<KGrid>
<KGridItem
:layout12="{ span: 6 }"
:layout8="{ span: 4 }"
:layout4="{ span: 2 }"
>
<KButton
:text="coreString('cancelAction')"
class="kbuttons"
style="width:100%;"
@click="closeModal()"
/>
</KGridItem>
<KGridItem
:layout12="{ span: 6 }"
:layout8="{ span: 4 }"
:layout4="{ span: 2 }"
>
<KButton
:text="coreString('continueAction')"
appearance="raised-button"
style="width:100%;"
primary
@click="handleContinueAction"
/>
</KGridItem>
</KGrid>
</template>
</KModal>
</div>

</template>


<script>

import commonCoreStrings from 'kolibri.coreVue.mixins.commonCoreStrings';

export default {
name: 'ConfirmCancellationModal',
mixins: [commonCoreStrings],
props: {
closePanelRoute: {
type: Object,
required: true,
},
},
methods: {
handleContinueAction() {
this.$emit('continue');
this.$router.replace(this.closePanelRoute);
},
closeModal() {
this.$emit('cancel');
},
},
};

</script>
Original file line number Diff line number Diff line change
Expand Up @@ -304,6 +304,8 @@
</KTabsPanel>

<SectionSidePanel @closePanel="focusActiveSectionTab()" />


</div>

</template>
Expand Down Expand Up @@ -382,6 +384,7 @@
allSections,
activeSection,
inactiveSections,
workingResourcePool,
activeExercisePool,
activeQuestionsPool,
activeQuestions,
Expand Down Expand Up @@ -431,6 +434,7 @@
allSections,
activeSection,
inactiveSections,
workingResourcePool,
activeExercisePool,
activeQuestionsPool,
activeQuestions,
Expand Down Expand Up @@ -515,6 +519,7 @@
focusActiveSectionTab() {
const label = this.tabRefLabel(this.activeSection.section_id);
const tabRef = this.$refs[label];

// TODO Consider the "Delete section" button on the side panel; maybe we need to await
// nextTick if we're getting the error
if (tabRef) {
Expand Down
Loading
Loading