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 15 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
72 changes: 72 additions & 0 deletions kolibri/plugins/coach/assets/src/composables/useQuizCreation.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,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 +269,14 @@ export default function useQuizCreation(DEBUG = false) {
_fetchChannels();
}

// // Method to initialize the working resource pool
function initializeWorkingResourcePool() {
// Get the active section
const currentActiveResourcePool = get(activeResourcePool);
// Set the value of _working_resource_pool to the resource_pool of the active section
set(_working_resource_pool, currentActiveResourcePool);
Copy link
Member

Choose a reason for hiding this comment

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

Non-blocking:

In a case like this where we aren't doing any other intermediary work on the value assigned to currentActiveResourcePool it is unnecessary to assign it before then passing it into set.

Here it could be simplified to set(_working_resource_pool, get(activeResourcePool)).

}

/**
* @returns {Promise<Quiz>}
* @throws {Error} if quiz is not valid
Expand Down Expand Up @@ -337,6 +349,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 +425,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 +465,40 @@ export default function useQuizCreation(DEBUG = false) {
}
});

/**
* Adds resources to _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.

Let's get the JSDoc types up-to-date here w/ @param and @affects entries - the type for the resources param ought to be QuizExercise[]

*/
function addToWorkingResourcePool(resources = []) {
set(_working_resource_pool, [...get(_working_resource_pool), ...resources]);
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 we should wrap the array here with lodash's uniqWith and isEqual alike to:

uniqWith([...get(_working_resource_pool), ...resources], isEqual) -- this will use the lodash isEqual function to check equality of the objects in the list (see the conveniently relevant example in the lodash docs).

}

/**
* 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);
}
/**
* Remove resource with the given id from _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.

The JSDoc for the param would be helpful here as well as this method doesn't expect the QuizExercise -- but rather, just the id for one

Copy link
Member

Choose a reason for hiding this comment

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

Noting that I've instead just revised this to accept content akin to it's related working resource pool functions.

*/
function removeFromWorkingResourcePool(id) {
set(
_working_resource_pool,
_working_resource_pool.value.filter(obj => obj.id !== 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 +508,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 +528,13 @@ export default function useQuizCreation(DEBUG = false) {
return {
// Methods
saveQuiz,
initializeWorkingResourcePool,
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

removeFromWorkingResourcePool,
addToWorkingResourcePool,
contentPresentInWorkingResourcePool,
updateSection,
replaceSelectedQuestions,
resetWorkingResourcePool,
addSection,
removeSection,
setActiveSection,
Expand All @@ -493,6 +549,8 @@ export default function useQuizCreation(DEBUG = false) {
allSections,
activeSection,
inactiveSections,
workingResourcePool,
activeResourcePool,
activeExercisePool,
activeQuestionsPool,
activeQuestions,
Expand All @@ -516,9 +574,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 +593,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 +607,14 @@ export function injectQuizCreation() {
return {
// Methods
saveQuiz,
initializeWorkingResourcePool,
addToWorkingResourcePool,
contentPresentInWorkingResourcePool,
removeFromWorkingResourcePool,
deleteActiveSelectedQuestions,
selectAllQuestions,
updateSection,
resetWorkingResourcePool,
replaceSelectedQuestions,
addSection,
removeSection,
Expand All @@ -561,6 +631,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,79 @@
<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';
import { injectQuizCreation } from '../../../composables/useQuizCreation';

export default {
name: 'ConfirmCancellationModal',
mixins: [commonCoreStrings],
setup() {
const {
//Computed
resetWorkingResourcePool,
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 it would be better to make this component emit an event rather than handle all state clearing and route changing.

} = injectQuizCreation();

return {
resetWorkingResourcePool,
};
},
props: {
closePanelRoute: {
type: Object,
required: true,
},
},
methods: {
handleContinueAction() {
this.resetWorkingResourcePool();
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 @@ -370,6 +372,7 @@
addSection,
removeSection,
setActiveSection,
initializeWorkingResourcePool,
initializeQuiz,
updateQuiz,
addQuestionToSelection,
Expand All @@ -382,6 +385,8 @@
allSections,
activeSection,
inactiveSections,
activeResourcePool,
Copy link
Member

Choose a reason for hiding this comment

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

Both of these are added, and neither are used.

workingResourcePool,
activeExercisePool,
activeQuestionsPool,
activeQuestions,
Expand Down Expand Up @@ -420,6 +425,7 @@
addSection,
removeSection,
setActiveSection,
initializeWorkingResourcePool,
initializeQuiz,
updateQuiz,
addQuestionToSelection,
Expand All @@ -431,6 +437,8 @@
allSections,
activeSection,
inactiveSections,
workingResourcePool,
activeResourcePool,
activeExercisePool,
activeQuestionsPool,
activeQuestions,
Expand Down Expand Up @@ -515,6 +523,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 Expand Up @@ -558,6 +567,8 @@
set(this.dragActive, true);
},
openSelectResources(section_id) {
//initialize workingResourcePool
this.initializeWorkingResourcePool();
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason this couldn't happen within the ResourceSelection component instead?

this.$router.replace({ path: 'new/' + section_id + '/select-resources' });
},
},
Expand Down
Loading