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.

Sign up for GitHub

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

Update folder selection logic to handle deep folders. #12381

Merged
merged 1 commit into from
Jul 3, 2024
Merged

Update folder selection logic to handle deep folders. #12381

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 0 additions & 2 deletions kolibri/core/assets/src/constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,5 +198,3 @@ export const Presets = Object.freeze({
// This should be kept in sync with the value in
// kolibri/core/exams/constants.py
export const MAX_QUESTIONS_PER_QUIZ_SECTION = 50;
// this is not used for the actual exam model
export const MAX_QUESTION_OPTIONS_PER_QUIZ_SECTION = 500;
17 changes: 0 additions & 17 deletions kolibri/plugins/coach/assets/src/composables/useQuizResources.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,22 +128,6 @@ export default function useQuizResources({ topicId, practiceQuiz = false } = {})
});
}

/** @returns {Boolean} Whether the given node should be displayed with a checkbox
* @description Returns true for exercises and for topics that have no topic children and no
* more children to load
*/
function hasCheckbox(node) {
return (
node.kind === ContentNodeKinds.EXERCISE ||
// Has children, no more to load, and no children are topics
(!practiceQuiz &&
node.children &&
!node.children.more &&
!node.children.results.some(c => c.kind === ContentNodeKinds.TOPIC) &&
node.children.results.length <= 12)
);
}

function setResources(r) {
set(_resources, r);
}
Expand All @@ -155,7 +139,6 @@ export default function useQuizResources({ topicId, practiceQuiz = false } = {})
loadingMore: computed(() => get(_loadingMore)),
fetchQuizResources,
fetchMoreQuizResources,
hasCheckbox,
hasMore,
topic,
annotateTopicsWithDescendantCounts,
Expand Down
215 changes: 143 additions & 72 deletions kolibri/plugins/coach/assets/src/views/plan/CreateExamPage/ResourceSelection.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@
backgroundColor: $themePalette.grey.v_100,
}"
>
{{ cannotSelectSomeTopicWarning$({ count: 500 }) }}
{{ cannotSelectSomeTopicWarning$({ count: maxSectionQuestionOptions }) }}
</div>

<ContentCardList
Expand All @@ -123,7 +123,9 @@
:selectAllChecked="selectAllChecked"
:selectAllIndeterminate="selectAllIndeterminate"
:contentIsChecked="contentPresentInWorkingResourcePool"
:contentHasCheckbox="folderDoesNotHaveTooManyQuestions || hasCheckbox"
:contentIsIndeterminate="contentPartlyPresentInWorkingResourcePool"
:contentHasCheckbox="showCheckbox"
:contentCheckboxDisabled="c => !nodeIsSelectableOrUnselectable(c)"
:contentCardMessage="selectionMetadata"
:contentCardLink="contentLink"
:loadingMoreState="loadingMore"
Expand Down Expand Up @@ -197,18 +199,15 @@
import get from 'lodash/get';
import uniqWith from 'lodash/uniqWith';
import isEqual from 'lodash/isEqual';
import { useMemoize } from '@vueuse/core';
import {
displaySectionTitle,
enhancedQuizManagementStrings,
} from 'kolibri-common/strings/enhancedQuizManagementStrings';
import { computed, ref, getCurrentInstance, watch } from 'kolibri.lib.vueCompositionApi';
import commonCoreStrings from 'kolibri.coreVue.mixins.commonCoreStrings';
import { ContentNodeResource, ChannelResource } from 'kolibri.resources';
import {
ContentNodeKinds,
MAX_QUESTIONS_PER_QUIZ_SECTION,
MAX_QUESTION_OPTIONS_PER_QUIZ_SECTION,
} from 'kolibri.coreVue.vuex.constants';
import { ContentNodeKinds, MAX_QUESTIONS_PER_QUIZ_SECTION } from 'kolibri.coreVue.vuex.constants';
import useKResponsiveWindow from 'kolibri-design-system/lib/composables/useKResponsiveWindow';
import { exerciseToQuestionArray } from '../../../utils/selectQuestions';
import { PageNames, ViewMoreButtonStates } from '../../../constants/index';
Expand Down Expand Up @@ -252,10 +251,14 @@
const maxQuestions = computed(
() => MAX_QUESTIONS_PER_QUIZ_SECTION - activeQuestions.value.length,
);
const maxSectionQuestionOptions = computed(() => MAX_QUESTION_OPTIONS_PER_QUIZ_SECTION);

const questionCount = ref(Math.min(10, maxQuestions.value));

// Make the maxSectionQuestionOptions a computed property based on the questionCount
// that the user has selected, so if they want to add 10 questions, only let them
// choose a total of 100 to select those from.
const maxSectionQuestionOptions = computed(() => questionCount.value * 10);
nucleogenesis marked this conversation as resolved.
Show resolved Hide resolved

const selectPracticeQuiz = computed(() => props.selectPracticeQuiz);

const {
Expand Down Expand Up @@ -309,25 +312,6 @@
);
}

/**
* @description Returns the list of Exercises which can possibly be selected from the current
* contentList taking into consideration the logic for whether a topic can be selected or not.
* @returns {QuizExercise[]} - All contents which can be selected
*/
function selectableContentList() {
return contentList.value.reduce((newList, content) => {
if (
content.kind === ContentNodeKinds.TOPIC &&
folderDoesNotHaveTooManyQuestions(content)
) {
newList = [...newList, ...content.children.results];
} else {
newList.push(content);
}
return newList;
}, []);
}

/**
* @param {QuizExercise} content
* @affects workingResourcePool - Remove given quiz exercise from workingResourcePool
Expand All @@ -343,16 +327,49 @@
workingResourcePool.value = [];
}

// Function to calculate the total number of questions currently selected for a node
// use this in order to do accurate counts for enabling/disabling checkboxes
function selectedQuestionsFromNode(content) {
if (content.kind === ContentNodeKinds.EXERCISE) {
return workingResourcePool.value.some(wr => wr.id === content.id)
? unusedQuestionsCount(content)
: 0;
}
return workingResourcePool.value.reduce((acc, wr) => {
return (
acc +
(wr.ancestors.some(ancestor => ancestor.id === content.id)
? unusedQuestionsCount(wr)
: 0)
);
}, 0);
}

/**
* @param {QuizExercise} content
* Check if the content is present in workingResourcePool
*/
function contentPresentInWorkingResourcePool(content) {
const workingResourceIds = workingResourcePool.value.map(wr => wr.id);
if (content.kind === ContentNodeKinds.TOPIC) {
return content.children.results.every(child => workingResourceIds.includes(child.id));
const selectedQuestionsFromTopic = selectedQuestionsFromNode(content);
return selectedQuestionsFromTopic >= unusedQuestionsCount(content);
}
return workingResourceIds.includes(content.id);
return workingResourcePool.value.some(wr => wr.id === content.id);
}

/**
* @param {QuizExercise} content
* Check if the content is partly present in workingResourcePool
* Only really useful for folders, as resources cannot be partially selected.
*/
function contentPartlyPresentInWorkingResourcePool(content) {
if (content.kind !== ContentNodeKinds.TOPIC) {
return false;
}
const selectedQuestionsFromTopic = selectedQuestionsFromNode(content);
return (
selectedQuestionsFromTopic > 0 && selectedQuestionsFromTopic < content.num_assessments
);
}

function fetchSearchResults() {
Expand Down Expand Up @@ -382,57 +399,69 @@
});
}

const _selectAllState = computed(() => {
return contentList.value.map(contentPresentInWorkingResourcePool);
});

const selectAllChecked = computed(() => {
// Returns true if all the resources in the topic are in the working resource pool
const workingResourceIds = workingResourcePool.value.map(wr => wr.id);
const selectableIds = selectableContentList().map(content => content.id);
return selectableIds.every(id => workingResourceIds.includes(id));
return _selectAllState.value.every(Boolean);
});

const selectAllIndeterminate = computed(() => {
// Returns true if some, but not all, of the resources in the topic are in the working
// resource
const workingResourceIds = workingResourcePool.value.map(wr => wr.id);
const selectableIds = selectableContentList().map(content => content.id);
return !selectAllChecked.value && selectableIds.some(id => workingResourceIds.includes(id));
return (
(!selectAllChecked.value && _selectAllState.value.some(Boolean)) ||
contentList.value.some(contentPartlyPresentInWorkingResourcePool)
);
});

const showSelectAll = computed(() => {
return (
!selectPracticeQuiz.value &&
contentList.value.every(content => folderDoesNotHaveTooManyQuestions(content))
contentList.value.every(content => nodeIsSelectableOrUnselectable(content)) &&
// We only show the select all button if both all the checkboxes are enabled,
// and adding all the currently unselected questions in resources/folders plus
// those that are already selected (both in this level of the topic tree and elsewhere)
// would not exceed the maxSectionQuestionOptions.
// Do this by taking away the selected questions from the total questions in the
// resource/folder and checking if the sum of all of these is less than or equal to
// the remaining number of questions that can be added.
contentList.value.reduce(
(acc, content) =>
unusedQuestionsCount(content) - selectedQuestionsFromNode(content) + acc,
0,
) <=
maxSectionQuestionOptions.value - workingPoolUnusedQuestions.value
);
});

function handleSelectAll(isChecked) {
if (isChecked) {
addToWorkingResourcePool(selectableContentList());
} else {
contentList.value.forEach(content => {
var contentToRemove = [];
if (content.kind === ContentNodeKinds.TOPIC) {
contentToRemove = content.children.results;
} else {
contentToRemove.push(content);
}
contentToRemove.forEach(c => {
removeFromWorkingResourcePool(c);
});
});
}
}

/**
* @param {Object} param
* @param {ContentNode} param.content
* @param {boolean} param.checked
* @affects workingResourcePool - Adds or removes the content from the workingResourcePool
* When given a topic, it adds or removes all the exercises in the topic from the
* workingResourcePool. This assumes that topics which should not be added are not able to
* be checked and does not do any additional checks.
* workingResourcePool.
*/
function toggleSelected({ content, checked }) {
content = content.kind === ContentNodeKinds.TOPIC ? content.children.results : [content];
async function toggleSelected({ content, checked }) {
if (content.is_leaf) {
content = [content];
} else {
// If we already have all of the children locally, and every child is a leaf, we can
// just add them all to the working resource pool
if (!content.children.more && !content.children.results.some(n => !n.is_leaf)) {
content = content.children.results;
} else {
// If we don't have all of the children locally, we need to fetch them
const children = await ContentNodeResource.fetchCollection({
Comment on lines +454 to +455
Copy link
Member

Choose a reason for hiding this comment

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

Haven't tested yet - but just wondering what this might look like on a slow connection and if we might want to add a loading state here.

Copy link
Member

Choose a reason for hiding this comment

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

With that said, we probably need to work on adding loading states in various places...

2024-07-01.15-05-33.mp4

getParams: {
descendant_of: content.id,
available: true,
kind: ContentNodeKinds.EXERCISE,
},
});
content = children;
}
}
if (checked) {
if (selectPracticeQuiz.value) {
resetWorkingResourcePool();
Expand All @@ -446,7 +475,6 @@
}

const {
hasCheckbox,
topic,
resources,
loading: quizResourcesLoading,
Expand All @@ -465,7 +493,7 @@
const searchResults = ref([]);
const moreSearchResults = ref(null);

function unusedQuestionsCount(content) {
const unusedQuestionsCount = useMemoize(content => {
if (content.kind === ContentNodeKinds.EXERCISE) {
const questionItems = content.assessmentmetadata.assessment_item_ids.map(
aid => `${content.id}:${aid}`,
Expand All @@ -490,9 +518,43 @@
return total - numberOfQuestionsSelected;
}
return -1;
});

/** @returns {Boolean} Whether the given node should be displayed with a checkbox
* @description Returns true for exercises or folders that have more than 0 unused questions
* and adding it would give us fewer than maxSectionQuestionOptions questions
*/
function nodeIsSelectableOrUnselectable(node) {
// For practice quizzes, we only want to allow selection of resources, not folders.
if (selectPracticeQuiz.value && node.kind === ContentNodeKinds.EXERCISE) {
return true;
}
if (selectPracticeQuiz.value) {
return false;
}
if (
contentPresentInWorkingResourcePool(node) ||
contentPartlyPresentInWorkingResourcePool(node)
) {
// If a node has been selected or partly selected, always allow it to be deselected.
return true;
}
Comment on lines +535 to +541
Copy link
Member

Choose a reason for hiding this comment

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

This situation is weird:

  • 12 questions "Number of questions"
  • Select QA Channel > Exercises > CK12 Exercises (111 questions)
  • Go back to QA Channel root, click the indeterminate checkbox on QA Channel
  • Now I've been allowed to select 500+ questions, and I get an error and I have to click to deselect everything
  • I shouldn't have been allowed to select that many questions with my current question count

I think maybe disabling the checkbox altogether might be better than this.

Or maybe we could instead just deselect the relevant selection when there are too many questions to add -- but this might be confusing in its own right (ie, "why does it select things in some cases but deselect in others?" might not have an obvious answer.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes - I think I missed an edge case here and didn't ensure that checkboxes are always disabled for channels (even if we are displaying to show the descendant selector state).

// If a node has not been selected, only allow it to be selected if it has unused questions
// and adding it would not exceed the remaining maxSectionQuestionOptions.
const count = unusedQuestionsCount(node);
return (
count > 0 && count + workingPoolUnusedQuestions.value <= maxSectionQuestionOptions.value
);
}
function folderDoesNotHaveTooManyQuestions(content) {
return content.kind == ContentNodeKinds.EXERCISE || content.num_assessments < 500;

function showCheckbox(node) {
// We only show checkboxes for exercises, not topics for practice quizzes
if (selectPracticeQuiz.value) {
return node.kind === ContentNodeKinds.EXERCISE;
}
// Otherwise we show checkboxes for exercises and topics
// but not channels.
return node.kind === ContentNodeKinds.EXERCISE || node.kind === ContentNodeKinds.TOPIC;
}

// Load up the channels
Expand Down Expand Up @@ -623,10 +685,18 @@
);
});

function handleSelectAll(isChecked) {
for (const content of contentList.value) {
if (nodeIsSelectableOrUnselectable(content)) {
toggleSelected({ content, checked: isChecked });
}
}
}

return {
folderDoesNotHaveTooManyQuestions,
nodeIsSelectableOrUnselectable,
showCheckbox,
displaySectionTitle,
hasCheckbox,
unusedQuestionsCount,
activeSection,
activeSectionIndex,
Expand All @@ -653,6 +723,7 @@
fetchMoreResources,
resetWorkingResourcePool,
contentPresentInWorkingResourcePool,
contentPartlyPresentInWorkingResourcePool,
questionCount,
maxQuestions,
maxSectionQuestionOptions,
Expand Down Expand Up @@ -719,7 +790,10 @@
};
},
showNumberOfQuestionsWarning() {
return this.contentList.some(this.showNumberOfQuestionsWarningCard);
if (this.selectPracticeQuiz) {
return false;
}
return !this.showSelectAll;
},
borderStyle() {
return `border: 1px solid ${this.$themeTokens.fineLine}`;
Expand Down Expand Up @@ -749,9 +823,6 @@
}
},
methods: {
showNumberOfQuestionsWarningCard(content) {
return !this.selectPracticeQuiz && content.num_assessments > 500;
},
/** @public */
focusFirstEl() {
this.$refs.textbox.focus();
Expand Down
Loading