From 8a142a66104eb226b13e35184a44b3db76a62a4d Mon Sep 17 00:00:00 2001 From: GautamBytes Date: Fri, 14 Feb 2025 10:46:40 +0000 Subject: [PATCH 1/4] Add tests for useQuizResources and update functionality for annotation and loading states --- .../src/composables/useQuizResources.js | 117 ++++----- .../assets/test/UseQuizResources.spec.js | 224 ++++++++++++++++++ 2 files changed, 270 insertions(+), 71 deletions(-) create mode 100644 kolibri/plugins/coach/assets/test/UseQuizResources.spec.js diff --git a/kolibri/plugins/coach/assets/src/composables/useQuizResources.js b/kolibri/plugins/coach/assets/src/composables/useQuizResources.js index afeccb47a0..e8ad626af1 100644 --- a/kolibri/plugins/coach/assets/src/composables/useQuizResources.js +++ b/kolibri/plugins/coach/assets/src/composables/useQuizResources.js @@ -7,15 +7,12 @@ import useFetchTree from './useFetchTree'; const logger = logging.getLogger(__filename); const _loadingMore = ref(false); -/** - * @typedef {Object} QuizResourcesConfig - * @property { computed } topicId - The id of the root node to fetch the - * children for - */ /** * @module useQuizResources - * @param {QuizResourcesConfig} config + * @param {Object} config + * @param {computed} config.topicId - The id of the root node to fetch the children for + * @param {boolean} [config.practiceQuiz=false] */ export default function useQuizResources({ topicId, practiceQuiz = false } = {}) { const params = { @@ -27,7 +24,7 @@ export default function useQuizResources({ topicId, practiceQuiz = false } = {}) params.contains_quiz = true; } - // Initialize useFetchTree methods with the given topicId computed property and params + // Initialize useFetchTree methods with the given topicId and params const { topic, fetchTree, @@ -39,18 +36,13 @@ export default function useQuizResources({ topicId, practiceQuiz = false } = {}) params, }); - /** @type {ref} All resources which have been fetched that are the children of - * the given topicId annotated with assessment metadata */ const _resources = ref([]); - - /** @type {ref} Whether we are currently fetching/processing the child nodes */ const _loading = ref(false); /** * Annotates the child TOPIC nodes with the number of assessments that are contained within them * @param {ContentNode[]} results - The array of results from a content API call - * @returns {Promise} - A promise that resolves when the annotations have been - * made and returns the annotated results + * @returns {Promise} - A promise that resolves when the annotations have been made and returns the annotated results */ async function annotateTopicsWithDescendantCounts(results) { const topicIds = results @@ -62,85 +54,68 @@ export default function useQuizResources({ topicId, practiceQuiz = false } = {}) acc[topic.id] = topic.num_assessments; return acc; }, {}); - return ( - results - .map(node => { - // We'll map so that the topics are updated in place with the num_assessments, others - // are left as-is - if ([ContentNodeKinds.TOPIC, ContentNodeKinds.CHANNEL].includes(node.kind)) { - if (!topicsWithAssessmentCountsMap[node.id]) { - // If there are no assessment descendants, - // return null so we can easily filter after - return null; - } - if (node.kind === ContentNodeKinds.TOPIC && !node.children) { - // If the topic has no children, we can assume it has no assessments - // Only do this check for topics, because CHANNEL kinds are normally - // ChannelMetadata objects masquerading as ContentNode objects - // and so don't have children - return null; - } - node.num_assessments = topicsWithAssessmentCountsMap[node.id]; + return results + .map(node => { + if ([ContentNodeKinds.TOPIC, ContentNodeKinds.CHANNEL].includes(node.kind)) { + if (!topicsWithAssessmentCountsMap[node.id]) { + return null; } - return node; - }) - // Filter out any topics that have no assessments - // that we have already flagged as null above - .filter(Boolean) - ); + if (node.kind === ContentNodeKinds.TOPIC && !node.children) { + return null; + } + node.num_assessments = topicsWithAssessmentCountsMap[node.id]; + } + return node; + }) + .filter(Boolean); }) .catch(e => { - // TODO Work out best UX for this situation -- it may depend on if we're fetching more - // or the initial list of contents logger.error(e); }); } - /** - * @affects _resources - Sets the _resources to the results of the fetchTree call - * @affects _loading - * @returns {Promise} - A promise that resolves when the annotations have been made and - */ - async function fetchQuizResources() { + function setResources(r) { + set(_resources, r); + } + + // --- Create a public API object to hold and expose functions --- + const api = { + setResources, + resources: computed(() => get(_resources)), + loading: computed(() => get(_loading) || get(treeLoading)), + loadingMore: computed(() => get(_loadingMore)), + hasMore, + topic, + annotateTopicsWithDescendantCounts, // expose this function for testing + // We'll assign these next: + fetchQuizResources: undefined, + fetchMoreQuizResources: undefined, + }; + + // --- Define fetchQuizResources using the public API to call annotateTopicsWithDescendantCounts --- + api.fetchQuizResources = async function fetchQuizResources() { set(_loading, true); return fetchTree().then(async results => { - return annotateTopicsWithDescendantCounts(results).then(annotatedResults => { + return api.annotateTopicsWithDescendantCounts(results).then(annotatedResults => { setResources(annotatedResults); set(_loading, false); }); }); - } + }; - /** - * @affects _resources - Appends the results of the fetchMore call to the _resources - * and annotates any new topics with descendant counts - * @affects _loading - fetchMore & annotateTopicsWithDescendantCounts update the loading states - */ - async function fetchMoreQuizResources() { + // --- Define fetchMoreQuizResources similarly --- + api.fetchMoreQuizResources = async function fetchMoreQuizResources() { set(_loading, true); set(_loadingMore, true); return fetchMore().then(async results => { - return annotateTopicsWithDescendantCounts(results).then(annotatedResults => { + return api.annotateTopicsWithDescendantCounts(results).then(annotatedResults => { set(_resources, [...get(_resources), ...annotatedResults]); set(_loading, false); set(_loadingMore, false); }); }); - } - - function setResources(r) { - set(_resources, r); - } - - return { - setResources, - resources: computed(() => get(_resources)), - loading: computed(() => get(_loading) || get(treeLoading)), - loadingMore: computed(() => get(_loadingMore)), - fetchQuizResources, - fetchMoreQuizResources, - hasMore, - topic, - annotateTopicsWithDescendantCounts, }; + + return api; } + diff --git a/kolibri/plugins/coach/assets/test/UseQuizResources.spec.js b/kolibri/plugins/coach/assets/test/UseQuizResources.spec.js new file mode 100644 index 0000000000..a5afd78876 --- /dev/null +++ b/kolibri/plugins/coach/assets/test/UseQuizResources.spec.js @@ -0,0 +1,224 @@ +import { ref } from 'vue'; +import { get } from '@vueuse/core'; +import ContentNodeResource from 'kolibri-common/apiResources/ContentNodeResource'; +import { ContentNodeKinds } from 'kolibri/constants'; +import useQuizResources from '../src/composables/useQuizResources.js'; +import useFetchTree from '../src/composables/useFetchTree.js'; + +// Mock the useFetchTree module +jest.mock('../src/composables/useFetchTree.js'); +jest.mock('kolibri-common/apiResources/ContentNodeResource'); + +describe('useQuizResources', () => { + // Sample test data + const sampleResults = [ + { + id: 'topic1', + kind: ContentNodeKinds.TOPIC, + title: 'Topic 1', + children: ['exercise1', 'exercise2'] + }, + { + id: 'topic2', + kind: ContentNodeKinds.TOPIC, + title: 'Topic 2', + children: ['exercise3'] + }, + { + id: 'exercise1', + kind: ContentNodeKinds.EXERCISE, + title: 'Exercise 1' + } + ]; + + const annotatedResults = [ + { + ...sampleResults[0], + num_assessments: 2 + }, + { + ...sampleResults[1], + num_assessments: 1 + }, + sampleResults[2] + ]; + + const descendantsResponse = { + data: [ + { id: 'topic1', num_assessments: 2 }, + { id: 'topic2', num_assessments: 1 } + ] + }; + + beforeEach(() => { + // Reset all mocks before each test + jest.clearAllMocks(); + + // Mock useFetchTree implementation + useFetchTree.mockImplementation(() => ({ + topic: ref(null), + fetchTree: jest.fn().mockResolvedValue(sampleResults), + fetchMore: jest.fn().mockResolvedValue(sampleResults), + hasMore: ref(true), + loading: ref(false) + })); + + // Mock ContentNodeResource.fetchDescendantsAssessments + ContentNodeResource.fetchDescendantsAssessments.mockResolvedValue(descendantsResponse); + }); + + describe('initialization', () => { + it('should initialize with correct parameters for practice quiz', () => { + useQuizResources({ topicId: 'test-topic', practiceQuiz: true }); + + expect(useFetchTree).toHaveBeenCalledWith({ + topicId: 'test-topic', + params: { + kind_in: [ContentNodeKinds.EXERCISE, ContentNodeKinds.TOPIC], + include_coach_content: true, + contains_quiz: true + } + }); + }); + + it('should initialize with correct parameters for regular quiz', () => { + useQuizResources({ topicId: 'test-topic' }); + + expect(useFetchTree).toHaveBeenCalledWith({ + topicId: 'test-topic', + params: { + kind_in: [ContentNodeKinds.EXERCISE, ContentNodeKinds.TOPIC], + include_coach_content: true + } + }); + }); + }); + + describe('annotateTopicsWithDescendantCounts', () => { + it('should annotate topics with correct assessment counts', async () => { + const { annotateTopicsWithDescendantCounts } = useQuizResources(); + + const result = await annotateTopicsWithDescendantCounts(sampleResults); + + expect(result).toEqual(annotatedResults); + expect(ContentNodeResource.fetchDescendantsAssessments).toHaveBeenCalledWith([ + 'topic1', + 'topic2' + ]); + }); + + it('should filter out topics with no assessments', async () => { + ContentNodeResource.fetchDescendantsAssessments.mockResolvedValue({ + data: [ + { id: 'topic1', num_assessments: 0 }, + { id: 'topic2', num_assessments: 1 } + ] + }); + + const { annotateTopicsWithDescendantCounts } = useQuizResources(); + const result = await annotateTopicsWithDescendantCounts(sampleResults); + + expect(result).toEqual([ + { + ...sampleResults[1], + num_assessments: 1 + }, + sampleResults[2] + ]); + }); + + it('should handle API errors gracefully', async () => { + const error = new Error('API Error'); + ContentNodeResource.fetchDescendantsAssessments.mockRejectedValue(error); + + const { annotateTopicsWithDescendantCounts } = useQuizResources(); + const result = await annotateTopicsWithDescendantCounts(sampleResults); + + expect(result).toBeUndefined(); + }); + }); + + describe('integration with fetch methods', () => { + let quizResources, annotateTopicsSpy; + beforeEach(() => { + quizResources = useQuizResources({ topicId: 'test-topic' }); + // Spy on the public API property now that our fetch functions call it via "api.annotateTopicsWithDescendantCounts" + annotateTopicsSpy = jest.spyOn(quizResources, 'annotateTopicsWithDescendantCounts'); + }); + afterEach(() => { + annotateTopicsSpy.mockRestore(); + }); + + it('should call annotateTopicsWithDescendantCounts during fetchQuizResources', async () => { + await quizResources.fetchQuizResources(); + + expect(annotateTopicsSpy).toHaveBeenCalledTimes(1); + expect(annotateTopicsSpy).toHaveBeenCalledWith(sampleResults); + }); + + it('should call annotateTopicsWithDescendantCounts during fetchMoreQuizResources', async () => { + await quizResources.fetchMoreQuizResources(); + + expect(annotateTopicsSpy).toHaveBeenCalledTimes(1); + expect(annotateTopicsSpy).toHaveBeenCalledWith(sampleResults); + }); + }); + + describe('fetchQuizResources', () => { + it('should fetch and annotate resources', async () => { + const { fetchQuizResources, resources } = useQuizResources(); + + await fetchQuizResources(); + + expect(get(resources)).toEqual(annotatedResults); + }); + + it('should handle loading state correctly', async () => { + const { fetchQuizResources, loading } = useQuizResources(); + + const loadingStates = []; + // Use .value to get booleans from computed refs + loadingStates.push(loading.value); + + const fetchPromise = fetchQuizResources(); + loadingStates.push(loading.value); + + await fetchPromise; + loadingStates.push(loading.value); + + expect(loadingStates).toEqual([false, true, false]); + }); + }); + + describe('fetchMoreQuizResources', () => { + it('should fetch and append more resources', async () => { + const { fetchQuizResources, fetchMoreQuizResources, resources } = useQuizResources(); + + await fetchQuizResources(); + const initialResources = get(resources); + + await fetchMoreQuizResources(); + + expect(get(resources)).toEqual([...initialResources, ...annotatedResults]); + }); + + it('should handle loading states correctly', async () => { + const { fetchMoreQuizResources, loading, loadingMore } = useQuizResources(); + + const states = []; + states.push({ loading: loading.value, loadingMore: loadingMore.value }); + + const fetchPromise = fetchMoreQuizResources(); + states.push({ loading: loading.value, loadingMore: loadingMore.value }); + + await fetchPromise; + states.push({ loading: loading.value, loadingMore: loadingMore.value }); + + expect(states).toEqual([ + { loading: false, loadingMore: false }, + { loading: true, loadingMore: true }, + { loading: false, loadingMore: false } + ]); + }); + }); +}); From 9138704679c34e5af8c9bfea94ab27078dbe2c9d Mon Sep 17 00:00:00 2001 From: "pre-commit-ci-lite[bot]" <117423508+pre-commit-ci-lite[bot]@users.noreply.github.com> Date: Fri, 14 Feb 2025 10:57:36 +0000 Subject: [PATCH 2/4] [pre-commit.ci lite] apply automatic fixes --- .../src/composables/useQuizResources.js | 1 - .../assets/test/UseQuizResources.spec.js | 84 +++++++++---------- 2 files changed, 42 insertions(+), 43 deletions(-) diff --git a/kolibri/plugins/coach/assets/src/composables/useQuizResources.js b/kolibri/plugins/coach/assets/src/composables/useQuizResources.js index e8ad626af1..742cff1ded 100644 --- a/kolibri/plugins/coach/assets/src/composables/useQuizResources.js +++ b/kolibri/plugins/coach/assets/src/composables/useQuizResources.js @@ -118,4 +118,3 @@ export default function useQuizResources({ topicId, practiceQuiz = false } = {}) return api; } - diff --git a/kolibri/plugins/coach/assets/test/UseQuizResources.spec.js b/kolibri/plugins/coach/assets/test/UseQuizResources.spec.js index a5afd78876..3a2f47a505 100644 --- a/kolibri/plugins/coach/assets/test/UseQuizResources.spec.js +++ b/kolibri/plugins/coach/assets/test/UseQuizResources.spec.js @@ -16,51 +16,51 @@ describe('useQuizResources', () => { id: 'topic1', kind: ContentNodeKinds.TOPIC, title: 'Topic 1', - children: ['exercise1', 'exercise2'] + children: ['exercise1', 'exercise2'], }, { id: 'topic2', kind: ContentNodeKinds.TOPIC, title: 'Topic 2', - children: ['exercise3'] + children: ['exercise3'], }, { id: 'exercise1', kind: ContentNodeKinds.EXERCISE, - title: 'Exercise 1' - } + title: 'Exercise 1', + }, ]; const annotatedResults = [ { ...sampleResults[0], - num_assessments: 2 + num_assessments: 2, }, { ...sampleResults[1], - num_assessments: 1 + num_assessments: 1, }, - sampleResults[2] + sampleResults[2], ]; const descendantsResponse = { data: [ { id: 'topic1', num_assessments: 2 }, - { id: 'topic2', num_assessments: 1 } - ] + { id: 'topic2', num_assessments: 1 }, + ], }; beforeEach(() => { // Reset all mocks before each test jest.clearAllMocks(); - + // Mock useFetchTree implementation useFetchTree.mockImplementation(() => ({ topic: ref(null), fetchTree: jest.fn().mockResolvedValue(sampleResults), fetchMore: jest.fn().mockResolvedValue(sampleResults), hasMore: ref(true), - loading: ref(false) + loading: ref(false), })); // Mock ContentNodeResource.fetchDescendantsAssessments @@ -70,26 +70,26 @@ describe('useQuizResources', () => { describe('initialization', () => { it('should initialize with correct parameters for practice quiz', () => { useQuizResources({ topicId: 'test-topic', practiceQuiz: true }); - + expect(useFetchTree).toHaveBeenCalledWith({ topicId: 'test-topic', params: { kind_in: [ContentNodeKinds.EXERCISE, ContentNodeKinds.TOPIC], include_coach_content: true, - contains_quiz: true - } + contains_quiz: true, + }, }); }); it('should initialize with correct parameters for regular quiz', () => { useQuizResources({ topicId: 'test-topic' }); - + expect(useFetchTree).toHaveBeenCalledWith({ topicId: 'test-topic', params: { kind_in: [ContentNodeKinds.EXERCISE, ContentNodeKinds.TOPIC], - include_coach_content: true - } + include_coach_content: true, + }, }); }); }); @@ -97,13 +97,13 @@ describe('useQuizResources', () => { describe('annotateTopicsWithDescendantCounts', () => { it('should annotate topics with correct assessment counts', async () => { const { annotateTopicsWithDescendantCounts } = useQuizResources(); - + const result = await annotateTopicsWithDescendantCounts(sampleResults); - + expect(result).toEqual(annotatedResults); expect(ContentNodeResource.fetchDescendantsAssessments).toHaveBeenCalledWith([ 'topic1', - 'topic2' + 'topic2', ]); }); @@ -111,19 +111,19 @@ describe('useQuizResources', () => { ContentNodeResource.fetchDescendantsAssessments.mockResolvedValue({ data: [ { id: 'topic1', num_assessments: 0 }, - { id: 'topic2', num_assessments: 1 } - ] + { id: 'topic2', num_assessments: 1 }, + ], }); const { annotateTopicsWithDescendantCounts } = useQuizResources(); const result = await annotateTopicsWithDescendantCounts(sampleResults); - + expect(result).toEqual([ { ...sampleResults[1], - num_assessments: 1 + num_assessments: 1, }, - sampleResults[2] + sampleResults[2], ]); }); @@ -133,7 +133,7 @@ describe('useQuizResources', () => { const { annotateTopicsWithDescendantCounts } = useQuizResources(); const result = await annotateTopicsWithDescendantCounts(sampleResults); - + expect(result).toBeUndefined(); }); }); @@ -151,14 +151,14 @@ describe('useQuizResources', () => { it('should call annotateTopicsWithDescendantCounts during fetchQuizResources', async () => { await quizResources.fetchQuizResources(); - + expect(annotateTopicsSpy).toHaveBeenCalledTimes(1); expect(annotateTopicsSpy).toHaveBeenCalledWith(sampleResults); }); it('should call annotateTopicsWithDescendantCounts during fetchMoreQuizResources', async () => { await quizResources.fetchMoreQuizResources(); - + expect(annotateTopicsSpy).toHaveBeenCalledTimes(1); expect(annotateTopicsSpy).toHaveBeenCalledWith(sampleResults); }); @@ -167,25 +167,25 @@ describe('useQuizResources', () => { describe('fetchQuizResources', () => { it('should fetch and annotate resources', async () => { const { fetchQuizResources, resources } = useQuizResources(); - + await fetchQuizResources(); - + expect(get(resources)).toEqual(annotatedResults); }); it('should handle loading state correctly', async () => { const { fetchQuizResources, loading } = useQuizResources(); - + const loadingStates = []; // Use .value to get booleans from computed refs loadingStates.push(loading.value); - + const fetchPromise = fetchQuizResources(); loadingStates.push(loading.value); - + await fetchPromise; loadingStates.push(loading.value); - + expect(loadingStates).toEqual([false, true, false]); }); }); @@ -193,31 +193,31 @@ describe('useQuizResources', () => { describe('fetchMoreQuizResources', () => { it('should fetch and append more resources', async () => { const { fetchQuizResources, fetchMoreQuizResources, resources } = useQuizResources(); - + await fetchQuizResources(); const initialResources = get(resources); - + await fetchMoreQuizResources(); - + expect(get(resources)).toEqual([...initialResources, ...annotatedResults]); }); it('should handle loading states correctly', async () => { const { fetchMoreQuizResources, loading, loadingMore } = useQuizResources(); - + const states = []; states.push({ loading: loading.value, loadingMore: loadingMore.value }); - + const fetchPromise = fetchMoreQuizResources(); states.push({ loading: loading.value, loadingMore: loadingMore.value }); - + await fetchPromise; states.push({ loading: loading.value, loadingMore: loadingMore.value }); - + expect(states).toEqual([ { loading: false, loadingMore: false }, { loading: true, loadingMore: true }, - { loading: false, loadingMore: false } + { loading: false, loadingMore: false }, ]); }); }); From 92041ba1cee4407b0e6829867a0295205fbf83f7 Mon Sep 17 00:00:00 2001 From: GautamBytes Date: Fri, 14 Feb 2025 11:49:39 +0000 Subject: [PATCH 3/4] Apply lint auto-format fixes --- .../assets/src/composables/useQuizResources.js | 17 +++++++---------- .../coach/assets/test/UseQuizResources.spec.js | 5 +++-- 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/kolibri/plugins/coach/assets/src/composables/useQuizResources.js b/kolibri/plugins/coach/assets/src/composables/useQuizResources.js index 742cff1ded..1d0302e66c 100644 --- a/kolibri/plugins/coach/assets/src/composables/useQuizResources.js +++ b/kolibri/plugins/coach/assets/src/composables/useQuizResources.js @@ -11,7 +11,7 @@ const _loadingMore = ref(false); /** * @module useQuizResources * @param {Object} config - * @param {computed} config.topicId - The id of the root node to fetch the children for + * @param {computed} config.topicId - The id of the root node * @param {boolean} [config.practiceQuiz=false] */ export default function useQuizResources({ topicId, practiceQuiz = false } = {}) { @@ -24,7 +24,6 @@ export default function useQuizResources({ topicId, practiceQuiz = false } = {}) params.contains_quiz = true; } - // Initialize useFetchTree methods with the given topicId and params const { topic, fetchTree, @@ -40,20 +39,22 @@ export default function useQuizResources({ topicId, practiceQuiz = false } = {}) const _loading = ref(false); /** - * Annotates the child TOPIC nodes with the number of assessments that are contained within them - * @param {ContentNode[]} results - The array of results from a content API call - * @returns {Promise} - A promise that resolves when the annotations have been made and returns the annotated results + * Annotates the child TOPIC nodes with the number of assessments + * @param {ContentNode[]} results - The array of results from content API + * @returns {Promise} - Promise resolving to annotated results */ async function annotateTopicsWithDescendantCounts(results) { const topicIds = results .filter(({ kind }) => kind === ContentNodeKinds.TOPIC || kind === ContentNodeKinds.CHANNEL) .map(topic => topic.id); + return ContentNodeResource.fetchDescendantsAssessments(topicIds) .then(({ data: topicsWithAssessmentCounts }) => { const topicsWithAssessmentCountsMap = topicsWithAssessmentCounts.reduce((acc, topic) => { acc[topic.id] = topic.num_assessments; return acc; }, {}); + return results .map(node => { if ([ContentNodeKinds.TOPIC, ContentNodeKinds.CHANNEL].includes(node.kind)) { @@ -78,7 +79,6 @@ export default function useQuizResources({ topicId, practiceQuiz = false } = {}) set(_resources, r); } - // --- Create a public API object to hold and expose functions --- const api = { setResources, resources: computed(() => get(_resources)), @@ -86,13 +86,11 @@ export default function useQuizResources({ topicId, practiceQuiz = false } = {}) loadingMore: computed(() => get(_loadingMore)), hasMore, topic, - annotateTopicsWithDescendantCounts, // expose this function for testing - // We'll assign these next: + annotateTopicsWithDescendantCounts, fetchQuizResources: undefined, fetchMoreQuizResources: undefined, }; - // --- Define fetchQuizResources using the public API to call annotateTopicsWithDescendantCounts --- api.fetchQuizResources = async function fetchQuizResources() { set(_loading, true); return fetchTree().then(async results => { @@ -103,7 +101,6 @@ export default function useQuizResources({ topicId, practiceQuiz = false } = {}) }); }; - // --- Define fetchMoreQuizResources similarly --- api.fetchMoreQuizResources = async function fetchMoreQuizResources() { set(_loading, true); set(_loadingMore, true); diff --git a/kolibri/plugins/coach/assets/test/UseQuizResources.spec.js b/kolibri/plugins/coach/assets/test/UseQuizResources.spec.js index 3a2f47a505..d1ad70bcf5 100644 --- a/kolibri/plugins/coach/assets/test/UseQuizResources.spec.js +++ b/kolibri/plugins/coach/assets/test/UseQuizResources.spec.js @@ -140,11 +140,13 @@ describe('useQuizResources', () => { describe('integration with fetch methods', () => { let quizResources, annotateTopicsSpy; + beforeEach(() => { quizResources = useQuizResources({ topicId: 'test-topic' }); - // Spy on the public API property now that our fetch functions call it via "api.annotateTopicsWithDescendantCounts" + // Spy on the public API property annotateTopicsSpy = jest.spyOn(quizResources, 'annotateTopicsWithDescendantCounts'); }); + afterEach(() => { annotateTopicsSpy.mockRestore(); }); @@ -177,7 +179,6 @@ describe('useQuizResources', () => { const { fetchQuizResources, loading } = useQuizResources(); const loadingStates = []; - // Use .value to get booleans from computed refs loadingStates.push(loading.value); const fetchPromise = fetchQuizResources(); From 50e3ae5e507878fa97e987304da7491b4e960677 Mon Sep 17 00:00:00 2001 From: GautamBytes Date: Thu, 20 Feb 2025 15:49:09 +0000 Subject: [PATCH 4/4] Fix tests for useQuizResources and update specs --- .../src/composables/useQuizResources.js | 117 ++++++++++------- .../assets/test/UseQuizResources.spec.js | 124 ++++++++++++------ 2 files changed, 155 insertions(+), 86 deletions(-) diff --git a/kolibri/plugins/coach/assets/src/composables/useQuizResources.js b/kolibri/plugins/coach/assets/src/composables/useQuizResources.js index 1d0302e66c..afeccb47a0 100644 --- a/kolibri/plugins/coach/assets/src/composables/useQuizResources.js +++ b/kolibri/plugins/coach/assets/src/composables/useQuizResources.js @@ -7,12 +7,15 @@ import useFetchTree from './useFetchTree'; const logger = logging.getLogger(__filename); const _loadingMore = ref(false); +/** + * @typedef {Object} QuizResourcesConfig + * @property { computed } topicId - The id of the root node to fetch the + * children for + */ /** * @module useQuizResources - * @param {Object} config - * @param {computed} config.topicId - The id of the root node - * @param {boolean} [config.practiceQuiz=false] + * @param {QuizResourcesConfig} config */ export default function useQuizResources({ topicId, practiceQuiz = false } = {}) { const params = { @@ -24,6 +27,7 @@ export default function useQuizResources({ topicId, practiceQuiz = false } = {}) params.contains_quiz = true; } + // Initialize useFetchTree methods with the given topicId computed property and params const { topic, fetchTree, @@ -35,83 +39,108 @@ export default function useQuizResources({ topicId, practiceQuiz = false } = {}) params, }); + /** @type {ref} All resources which have been fetched that are the children of + * the given topicId annotated with assessment metadata */ const _resources = ref([]); + + /** @type {ref} Whether we are currently fetching/processing the child nodes */ const _loading = ref(false); /** - * Annotates the child TOPIC nodes with the number of assessments - * @param {ContentNode[]} results - The array of results from content API - * @returns {Promise} - Promise resolving to annotated results + * Annotates the child TOPIC nodes with the number of assessments that are contained within them + * @param {ContentNode[]} results - The array of results from a content API call + * @returns {Promise} - A promise that resolves when the annotations have been + * made and returns the annotated results */ async function annotateTopicsWithDescendantCounts(results) { const topicIds = results .filter(({ kind }) => kind === ContentNodeKinds.TOPIC || kind === ContentNodeKinds.CHANNEL) .map(topic => topic.id); - return ContentNodeResource.fetchDescendantsAssessments(topicIds) .then(({ data: topicsWithAssessmentCounts }) => { const topicsWithAssessmentCountsMap = topicsWithAssessmentCounts.reduce((acc, topic) => { acc[topic.id] = topic.num_assessments; return acc; }, {}); - - return results - .map(node => { - if ([ContentNodeKinds.TOPIC, ContentNodeKinds.CHANNEL].includes(node.kind)) { - if (!topicsWithAssessmentCountsMap[node.id]) { - return null; + return ( + results + .map(node => { + // We'll map so that the topics are updated in place with the num_assessments, others + // are left as-is + if ([ContentNodeKinds.TOPIC, ContentNodeKinds.CHANNEL].includes(node.kind)) { + if (!topicsWithAssessmentCountsMap[node.id]) { + // If there are no assessment descendants, + // return null so we can easily filter after + return null; + } + if (node.kind === ContentNodeKinds.TOPIC && !node.children) { + // If the topic has no children, we can assume it has no assessments + // Only do this check for topics, because CHANNEL kinds are normally + // ChannelMetadata objects masquerading as ContentNode objects + // and so don't have children + return null; + } + node.num_assessments = topicsWithAssessmentCountsMap[node.id]; } - if (node.kind === ContentNodeKinds.TOPIC && !node.children) { - return null; - } - node.num_assessments = topicsWithAssessmentCountsMap[node.id]; - } - return node; - }) - .filter(Boolean); + return node; + }) + // Filter out any topics that have no assessments + // that we have already flagged as null above + .filter(Boolean) + ); }) .catch(e => { + // TODO Work out best UX for this situation -- it may depend on if we're fetching more + // or the initial list of contents logger.error(e); }); } - function setResources(r) { - set(_resources, r); - } - - const api = { - setResources, - resources: computed(() => get(_resources)), - loading: computed(() => get(_loading) || get(treeLoading)), - loadingMore: computed(() => get(_loadingMore)), - hasMore, - topic, - annotateTopicsWithDescendantCounts, - fetchQuizResources: undefined, - fetchMoreQuizResources: undefined, - }; - - api.fetchQuizResources = async function fetchQuizResources() { + /** + * @affects _resources - Sets the _resources to the results of the fetchTree call + * @affects _loading + * @returns {Promise} - A promise that resolves when the annotations have been made and + */ + async function fetchQuizResources() { set(_loading, true); return fetchTree().then(async results => { - return api.annotateTopicsWithDescendantCounts(results).then(annotatedResults => { + return annotateTopicsWithDescendantCounts(results).then(annotatedResults => { setResources(annotatedResults); set(_loading, false); }); }); - }; + } - api.fetchMoreQuizResources = async function fetchMoreQuizResources() { + /** + * @affects _resources - Appends the results of the fetchMore call to the _resources + * and annotates any new topics with descendant counts + * @affects _loading - fetchMore & annotateTopicsWithDescendantCounts update the loading states + */ + async function fetchMoreQuizResources() { set(_loading, true); set(_loadingMore, true); return fetchMore().then(async results => { - return api.annotateTopicsWithDescendantCounts(results).then(annotatedResults => { + return annotateTopicsWithDescendantCounts(results).then(annotatedResults => { set(_resources, [...get(_resources), ...annotatedResults]); set(_loading, false); set(_loadingMore, false); }); }); - }; + } - return api; + function setResources(r) { + set(_resources, r); + } + + return { + setResources, + resources: computed(() => get(_resources)), + loading: computed(() => get(_loading) || get(treeLoading)), + loadingMore: computed(() => get(_loadingMore)), + fetchQuizResources, + fetchMoreQuizResources, + hasMore, + topic, + annotateTopicsWithDescendantCounts, + }; } diff --git a/kolibri/plugins/coach/assets/test/UseQuizResources.spec.js b/kolibri/plugins/coach/assets/test/UseQuizResources.spec.js index d1ad70bcf5..fa1722d93b 100644 --- a/kolibri/plugins/coach/assets/test/UseQuizResources.spec.js +++ b/kolibri/plugins/coach/assets/test/UseQuizResources.spec.js @@ -5,7 +5,6 @@ import { ContentNodeKinds } from 'kolibri/constants'; import useQuizResources from '../src/composables/useQuizResources.js'; import useFetchTree from '../src/composables/useFetchTree.js'; -// Mock the useFetchTree module jest.mock('../src/composables/useFetchTree.js'); jest.mock('kolibri-common/apiResources/ContentNodeResource'); @@ -31,18 +30,6 @@ describe('useQuizResources', () => { }, ]; - const annotatedResults = [ - { - ...sampleResults[0], - num_assessments: 2, - }, - { - ...sampleResults[1], - num_assessments: 1, - }, - sampleResults[2], - ]; - const descendantsResponse = { data: [ { id: 'topic1', num_assessments: 2 }, @@ -51,7 +38,7 @@ describe('useQuizResources', () => { }; beforeEach(() => { - // Reset all mocks before each test + // Reset mocks before each test jest.clearAllMocks(); // Mock useFetchTree implementation @@ -97,10 +84,21 @@ describe('useQuizResources', () => { describe('annotateTopicsWithDescendantCounts', () => { it('should annotate topics with correct assessment counts', async () => { const { annotateTopicsWithDescendantCounts } = useQuizResources(); - const result = await annotateTopicsWithDescendantCounts(sampleResults); - expect(result).toEqual(annotatedResults); + // Verify the topics are properly annotated + expect(result).toEqual([ + { + ...sampleResults[0], + num_assessments: 2, + }, + { + ...sampleResults[1], + num_assessments: 1, + }, + sampleResults[2], // Exercise remains unchanged + ]); + expect(ContentNodeResource.fetchDescendantsAssessments).toHaveBeenCalledWith([ 'topic1', 'topic2', @@ -110,7 +108,7 @@ describe('useQuizResources', () => { it('should filter out topics with no assessments', async () => { ContentNodeResource.fetchDescendantsAssessments.mockResolvedValue({ data: [ - { id: 'topic1', num_assessments: 0 }, + { id: 'topic1', num_assessments: 0 }, // No assessments { id: 'topic2', num_assessments: 1 }, ], }); @@ -123,7 +121,7 @@ describe('useQuizResources', () => { ...sampleResults[1], num_assessments: 1, }, - sampleResults[2], + sampleResults[2], // Exercise remains unchanged ]); }); @@ -139,30 +137,50 @@ describe('useQuizResources', () => { }); describe('integration with fetch methods', () => { - let quizResources, annotateTopicsSpy; + let quizResources; beforeEach(() => { quizResources = useQuizResources({ topicId: 'test-topic' }); - // Spy on the public API property - annotateTopicsSpy = jest.spyOn(quizResources, 'annotateTopicsWithDescendantCounts'); }); - afterEach(() => { - annotateTopicsSpy.mockRestore(); - }); - - it('should call annotateTopicsWithDescendantCounts during fetchQuizResources', async () => { + it('should annotate fetched resources in fetchQuizResources', async () => { await quizResources.fetchQuizResources(); - expect(annotateTopicsSpy).toHaveBeenCalledTimes(1); - expect(annotateTopicsSpy).toHaveBeenCalledWith(sampleResults); + // Check that resources have been annotated as expected + expect(get(quizResources.resources)).toEqual([ + { ...sampleResults[0], num_assessments: 2 }, + { ...sampleResults[1], num_assessments: 1 }, + sampleResults[2], + ]); + + // Verify that the API call to fetch descendant assessments was made with correct topic IDs + expect(ContentNodeResource.fetchDescendantsAssessments).toHaveBeenCalledWith([ + 'topic1', + 'topic2', + ]); }); - it('should call annotateTopicsWithDescendantCounts during fetchMoreQuizResources', async () => { + it('should annotate fetched resources in fetchMoreQuizResources', async () => { + // First, fetch the initial resources + await quizResources.fetchQuizResources(); + const initialResources = get(quizResources.resources); + + // Then, fetch more resources and append them await quizResources.fetchMoreQuizResources(); - expect(annotateTopicsSpy).toHaveBeenCalledTimes(1); - expect(annotateTopicsSpy).toHaveBeenCalledWith(sampleResults); + const expectedNewResources = [ + { ...sampleResults[0], num_assessments: 2 }, + { ...sampleResults[1], num_assessments: 1 }, + sampleResults[2], + ]; + + expect(get(quizResources.resources)).toEqual([...initialResources, ...expectedNewResources]); + + // Verify that the API call was made correctly during fetchMore as well + expect(ContentNodeResource.fetchDescendantsAssessments).toHaveBeenCalledWith([ + 'topic1', + 'topic2', + ]); }); }); @@ -172,27 +190,37 @@ describe('useQuizResources', () => { await fetchQuizResources(); - expect(get(resources)).toEqual(annotatedResults); + expect(get(resources)).toEqual([ + { + ...sampleResults[0], + num_assessments: 2, + }, + { + ...sampleResults[1], + num_assessments: 1, + }, + sampleResults[2], + ]); }); - it('should handle loading state correctly', async () => { + it('should manage loading state correctly', async () => { const { fetchQuizResources, loading } = useQuizResources(); const loadingStates = []; - loadingStates.push(loading.value); + loadingStates.push(get(loading)); const fetchPromise = fetchQuizResources(); - loadingStates.push(loading.value); + loadingStates.push(get(loading)); await fetchPromise; - loadingStates.push(loading.value); + loadingStates.push(get(loading)); expect(loadingStates).toEqual([false, true, false]); }); }); describe('fetchMoreQuizResources', () => { - it('should fetch and append more resources', async () => { + it('should fetch and append more annotated resources', async () => { const { fetchQuizResources, fetchMoreQuizResources, resources } = useQuizResources(); await fetchQuizResources(); @@ -200,20 +228,32 @@ describe('useQuizResources', () => { await fetchMoreQuizResources(); - expect(get(resources)).toEqual([...initialResources, ...annotatedResults]); + const expectedNewResources = [ + { + ...sampleResults[0], + num_assessments: 2, + }, + { + ...sampleResults[1], + num_assessments: 1, + }, + sampleResults[2], + ]; + + expect(get(resources)).toEqual([...initialResources, ...expectedNewResources]); }); - it('should handle loading states correctly', async () => { + it('should manage loading states correctly', async () => { const { fetchMoreQuizResources, loading, loadingMore } = useQuizResources(); const states = []; - states.push({ loading: loading.value, loadingMore: loadingMore.value }); + states.push({ loading: get(loading), loadingMore: get(loadingMore) }); const fetchPromise = fetchMoreQuizResources(); - states.push({ loading: loading.value, loadingMore: loadingMore.value }); + states.push({ loading: get(loading), loadingMore: get(loadingMore) }); await fetchPromise; - states.push({ loading: loading.value, loadingMore: loadingMore.value }); + states.push({ loading: get(loading), loadingMore: get(loadingMore) }); expect(states).toEqual([ { loading: false, loadingMore: false },