diff --git a/kolibri/plugins/coach/frontend/composables/useResourceSelection.js b/kolibri/plugins/coach/frontend/composables/useResourceSelection.js index 6a87df4bf3e..de307e1b7d3 100644 --- a/kolibri/plugins/coach/frontend/composables/useResourceSelection.js +++ b/kolibri/plugins/coach/frontend/composables/useResourceSelection.js @@ -4,10 +4,10 @@ import { useRoute } from 'vue-router/composables'; import ContentNodeResource from 'kolibri-common/apiResources/ContentNodeResource'; import ChannelResource from 'kolibri-common/apiResources/ChannelResource'; import useBaseSearch from 'kolibri-common/composables/useBaseSearch'; -import useFetch from 'kolibri-common/composables/useFetch.js'; +import useFetch from 'kolibri/composables/useFetch'; /** - * @typedef {import('kolibri-common/composables/useFetch.js').FetchObject} FetchObject + * @typedef {import('kolibri/composables/useFetch').FetchObject} FetchObject */ /** diff --git a/kolibri/plugins/coach/frontend/views/common/resourceSelection/subPages/SelectFromBookmarks.vue b/kolibri/plugins/coach/frontend/views/common/resourceSelection/subPages/SelectFromBookmarks.vue index d5e670dcb81..4b6dbb9ced0 100644 --- a/kolibri/plugins/coach/frontend/views/common/resourceSelection/subPages/SelectFromBookmarks.vue +++ b/kolibri/plugins/coach/frontend/views/common/resourceSelection/subPages/SelectFromBookmarks.vue @@ -44,7 +44,7 @@ import QuizResourceSelectionHeader from '../QuizResourceSelectionHeader.vue'; /** - * @typedef {import('kolibri-common/composables/useFetch.js').FetchObject} FetchObject + * @typedef {import('kolibri/composables/useFetch').FetchObject} FetchObject */ export default { diff --git a/kolibri/plugins/coach/frontend/views/common/resourceSelection/subPages/SelectFromTopicTree.vue b/kolibri/plugins/coach/frontend/views/common/resourceSelection/subPages/SelectFromTopicTree.vue index 4090da2c56c..30ad84d4e20 100644 --- a/kolibri/plugins/coach/frontend/views/common/resourceSelection/subPages/SelectFromTopicTree.vue +++ b/kolibri/plugins/coach/frontend/views/common/resourceSelection/subPages/SelectFromTopicTree.vue @@ -88,7 +88,7 @@ import { SelectionTarget } from '../contants'; /** - * @typedef {import('kolibri-common/composables/useFetch.js').FetchObject} FetchObject + * @typedef {import('kolibri/composables/useFetch').FetchObject} FetchObject */ export default { diff --git a/kolibri/plugins/coach/frontend/views/common/resourceSelection/subPages/SelectionIndex.vue b/kolibri/plugins/coach/frontend/views/common/resourceSelection/subPages/SelectionIndex.vue index a76540c5704..3baecbafba7 100644 --- a/kolibri/plugins/coach/frontend/views/common/resourceSelection/subPages/SelectionIndex.vue +++ b/kolibri/plugins/coach/frontend/views/common/resourceSelection/subPages/SelectionIndex.vue @@ -124,7 +124,7 @@ import QuizResourceSelectionHeader from '../QuizResourceSelectionHeader.vue'; /** - * @typedef {import('kolibri-common/composables/useFetch.js').FetchObject} FetchObject + * @typedef {import('kolibri/composables/useFetch').FetchObject} FetchObject */ export default { diff --git a/kolibri/plugins/coach/frontend/views/courses/composables/useAssignCourse.js b/kolibri/plugins/coach/frontend/views/courses/composables/useAssignCourse.js index 61185bc36c7..ba4deca2a1c 100644 --- a/kolibri/plugins/coach/frontend/views/courses/composables/useAssignCourse.js +++ b/kolibri/plugins/coach/frontend/views/courses/composables/useAssignCourse.js @@ -2,7 +2,7 @@ import Modalities from 'kolibri-constants/Modalities'; import ContentNodeResource from 'kolibri-common/apiResources/ContentNodeResource'; import CourseSessionResource from 'kolibri-common/apiResources/CourseSessionResource'; import { ref, computed, provide, inject, watch } from 'vue'; -import useFetch from 'kolibri-common/composables/useFetch.js'; +import useFetch from 'kolibri/composables/useFetch'; import { useCourses } from '../../../composables/useCourses'; /** @@ -161,7 +161,7 @@ export default function useAssignCourse({ classId }) { } /** - * @typedef {import('kolibri-common/composables/useFetch.js').FetchObject} FetchObject + * @typedef {import('kolibri/composables/useFetch').FetchObject} FetchObject */ /** diff --git a/kolibri/plugins/coach/frontend/views/lessons/LessonSummaryPage/sidePanels/LessonResourceSelection/subPages/SearchFilters.vue b/kolibri/plugins/coach/frontend/views/lessons/LessonSummaryPage/sidePanels/LessonResourceSelection/subPages/SearchFilters.vue index dbf8e68d461..d35d660bd4f 100644 --- a/kolibri/plugins/coach/frontend/views/lessons/LessonSummaryPage/sidePanels/LessonResourceSelection/subPages/SearchFilters.vue +++ b/kolibri/plugins/coach/frontend/views/lessons/LessonSummaryPage/sidePanels/LessonResourceSelection/subPages/SearchFilters.vue @@ -22,7 +22,7 @@ import { PageNames } from '../../../../../../constants'; /** - * @typedef {import('kolibri-common/composables/useFetch.js').FetchObject} FetchObject + * @typedef {import('kolibri/composables/useFetch').FetchObject} FetchObject */ export default { diff --git a/kolibri/plugins/coach/frontend/views/lessons/LessonSummaryPage/sidePanels/LessonResourceSelection/subPages/SelectFromSearchResults.vue b/kolibri/plugins/coach/frontend/views/lessons/LessonSummaryPage/sidePanels/LessonResourceSelection/subPages/SelectFromSearchResults.vue index 146eb2e1716..2408a137651 100644 --- a/kolibri/plugins/coach/frontend/views/lessons/LessonSummaryPage/sidePanels/LessonResourceSelection/subPages/SelectFromSearchResults.vue +++ b/kolibri/plugins/coach/frontend/views/lessons/LessonSummaryPage/sidePanels/LessonResourceSelection/subPages/SelectFromSearchResults.vue @@ -58,7 +58,7 @@ import { SelectionTarget } from '../../../../../common/resourceSelection/contants'; /** - * @typedef {import('kolibri-common/composables/useFetch.js').FetchObject} FetchObject + * @typedef {import('kolibri/composables/useFetch').FetchObject} FetchObject */ export default { diff --git a/kolibri/plugins/coach/frontend/views/quizzes/CreateExamPage/sidePanels/QuizResourceSelection/subPages/QuestionsSettings.vue b/kolibri/plugins/coach/frontend/views/quizzes/CreateExamPage/sidePanels/QuizResourceSelection/subPages/QuestionsSettings.vue index 2cf0a48f6e4..b6ec77702fa 100644 --- a/kolibri/plugins/coach/frontend/views/quizzes/CreateExamPage/sidePanels/QuizResourceSelection/subPages/QuestionsSettings.vue +++ b/kolibri/plugins/coach/frontend/views/quizzes/CreateExamPage/sidePanels/QuizResourceSelection/subPages/QuestionsSettings.vue @@ -75,7 +75,7 @@ import { injectQuizCreation } from '../../../../../../composables/useQuizCreation'; /** - * @typedef {import('kolibri-common/composables/useFetch.js').FetchObject} FetchObject + * @typedef {import('kolibri/composables/useFetch').FetchObject} FetchObject */ export default { diff --git a/kolibri/plugins/coach/frontend/views/quizzes/CreateExamPage/sidePanels/QuizResourceSelection/subPages/SearchQuizFilters.vue b/kolibri/plugins/coach/frontend/views/quizzes/CreateExamPage/sidePanels/QuizResourceSelection/subPages/SearchQuizFilters.vue index bc7017469e0..5799e9c9da3 100644 --- a/kolibri/plugins/coach/frontend/views/quizzes/CreateExamPage/sidePanels/QuizResourceSelection/subPages/SearchQuizFilters.vue +++ b/kolibri/plugins/coach/frontend/views/quizzes/CreateExamPage/sidePanels/QuizResourceSelection/subPages/SearchQuizFilters.vue @@ -21,7 +21,7 @@ import { PageNames } from '../../../../../../constants'; /** - * @typedef {import('kolibri-common/composables/useFetch.js').FetchObject} FetchObject + * @typedef {import('kolibri/composables/useFetch').FetchObject} FetchObject */ export default { diff --git a/kolibri/plugins/coach/frontend/views/quizzes/CreateExamPage/sidePanels/QuizResourceSelection/subPages/SelectFromQuizSearchResults.vue b/kolibri/plugins/coach/frontend/views/quizzes/CreateExamPage/sidePanels/QuizResourceSelection/subPages/SelectFromQuizSearchResults.vue index 4e32de43e70..01bd4d22f19 100644 --- a/kolibri/plugins/coach/frontend/views/quizzes/CreateExamPage/sidePanels/QuizResourceSelection/subPages/SelectFromQuizSearchResults.vue +++ b/kolibri/plugins/coach/frontend/views/quizzes/CreateExamPage/sidePanels/QuizResourceSelection/subPages/SelectFromQuizSearchResults.vue @@ -65,7 +65,7 @@ import QuizResourceSelectionHeader from '../../../../../common/resourceSelection/QuizResourceSelectionHeader.vue'; /** - * @typedef {import('kolibri-common/composables/useFetch.js').FetchObject} FetchObject + * @typedef {import('kolibri/composables/useFetch').FetchObject} FetchObject */ export default { diff --git a/kolibri/plugins/learn/frontend/views/CourseUnitView/index.vue b/kolibri/plugins/learn/frontend/views/CourseUnitView/index.vue index 84525b13262..566da262d8f 100644 --- a/kolibri/plugins/learn/frontend/views/CourseUnitView/index.vue +++ b/kolibri/plugins/learn/frontend/views/CourseUnitView/index.vue @@ -107,7 +107,7 @@ import { computed, nextTick, ref, toRef, watch } from 'vue'; import { coursesStrings } from 'kolibri-common/strings/coursesStrings.js'; import Modalities from 'kolibri-constants/Modalities'; - import useFetch from 'kolibri-common/composables/useFetch.js'; + import useFetch from 'kolibri/composables/useFetch'; import { injectPreviousRoute } from 'kolibri-common/composables/usePreviousRoute'; import { LearnerCourseResource } from '../../apiResources'; import ResourceLayout from '../ResourceLayout/index.vue'; diff --git a/packages/kolibri/__tests__/api-resource.spec.js b/packages/kolibri/__tests__/api-resource.spec.js index 2eac6224b78..62a7ebf6edd 100644 --- a/packages/kolibri/__tests__/api-resource.spec.js +++ b/packages/kolibri/__tests__/api-resource.spec.js @@ -1,7 +1,37 @@ +import { ref } from 'vue'; import * as Resources from '../apiResource'; jest.mock('kolibri/urls'); +// Build a URL function of the shape the URL resolver produces. Route params - whether passed +// as named kwargs (an object) or positional args - are substituted into the path, mirroring +// how the real reverse() fills a route pattern. They never appear as a query string; query +// params are a separate concern handled by the request `params` option. +const urlFunctionFor = + action => + (...args) => { + let segments; + if (args.length === 1 && args[0] !== null && typeof args[0] === 'object') { + // Named kwargs: use the values, ordered by key so the path is deterministic. + segments = Object.keys(args[0]) + .sort() + .map(key => args[0][key]); + } else { + segments = args; + } + return `/api/${action}/${segments.join('/')}`; + }; + +// A promise whose resolution is controlled by the test. +const deferred = () => { + let resolve, reject; + const promise = new Promise((res, rej) => { + resolve = res; + reject = rej; + }); + return { promise, resolve, reject }; +}; + describe('Resource', function () { let resource, modelData; const testName = 'test'; @@ -1342,3 +1372,827 @@ describe('Model', function () { }); }); }); + +describe('Resource REST methods', function () { + let resource, client; + + beforeEach(function () { + resource = new Resources.Resource({ name: 'test' }); + client = jest.fn().mockResolvedValue({ data: {} }); + resource.client = client; + resource.getUrlFunction = jest.fn(action => + action === 'missing' ? undefined : urlFunctionFor(action), + ); + }); + + describe('request method', function () { + it('should resolve the URL from the action and named route params', async function () { + await resource.request({ action: 'detail', routeParams: { pk: 'abc' } }); + expect(client).toHaveBeenCalledWith(expect.objectContaining({ url: '/api/detail/abc' })); + }); + + it('should spread array route params as positional arguments', async function () { + await resource.request({ action: 'nested', routeParams: ['session', 'unit'] }); + expect(client).toHaveBeenCalledWith( + expect.objectContaining({ url: '/api/nested/session/unit' }), + ); + }); + + it('should pass a scalar route param as a single positional argument', async function () { + await resource.request({ action: 'detail', routeParams: 'abc' }); + expect(client).toHaveBeenCalledWith(expect.objectContaining({ url: '/api/detail/abc' })); + }); + + it('should resolve a URL with no route params', async function () { + await resource.request({ action: 'list' }); + expect(client).toHaveBeenCalledWith(expect.objectContaining({ url: '/api/list/' })); + }); + + it('should reject when no URL is registered for the action', async function () { + await expect(resource.request({ action: 'missing' })).rejects.toThrow(ReferenceError); + }); + + it('should send query params and no body for a GET', async function () { + await resource.request({ params: { queue: 'content' } }); + expect(client).toHaveBeenCalledWith({ + url: '/api/list/', + method: 'GET', + params: { queue: 'content' }, + }); + }); + + it('should send a body for a write', async function () { + await resource.request({ method: 'POST', data: { name: 'test' } }); + expect(client).toHaveBeenCalledWith( + expect.objectContaining({ method: 'POST', data: { name: 'test' } }), + ); + }); + + it('should forward the multipart flag for a write', async function () { + await resource.request({ method: 'POST', data: {}, multipart: true }); + expect(client).toHaveBeenCalledWith(expect.objectContaining({ multipart: true })); + }); + }); + + describe('request de-duplication', function () { + it('should share a single in-flight request between identical concurrent GETs', async function () { + const { promise, resolve } = deferred(); + client.mockReturnValue(promise); + + const first = resource.request({ params: { queue: 'content' } }); + const second = resource.request({ params: { queue: 'content' } }); + + expect(client).toHaveBeenCalledTimes(1); + resolve({ data: [{ id: 'task' }] }); + const [firstResponse, secondResponse] = await Promise.all([first, second]); + expect(firstResponse).toEqual(secondResponse); + }); + + it('should log the error and re-raise it when a request fails', async function () { + const failure = new Error('boom'); + client.mockRejectedValue(failure); + resource.logError = jest.fn(); + + await expect(resource.request({ action: 'detail', routeParams: 'abc' })).rejects.toBe( + failure, + ); + expect(resource.logError).toHaveBeenCalledWith(failure); + }); + + it('should log a failed coalesced GET only once', async function () { + const failure = new Error('boom'); + const { promise, reject } = deferred(); + client.mockReturnValue(promise); + resource.logError = jest.fn(); + + const first = resource.request({ action: 'detail', routeParams: 'abc' }); + const second = resource.request({ action: 'detail', routeParams: 'abc' }); + reject(failure); + + await expect(first).rejects.toBe(failure); + await expect(second).rejects.toBe(failure); + expect(resource.logError).toHaveBeenCalledTimes(1); + }); + + it('should re-raise the original error on a network failure with no response', async function () { + // Axios network failures carry `config` but no `response`; the real logError must not throw + // reading `response.statusText`, which would mask the request error with a TypeError. + const networkError = Object.assign(new Error('Network Error'), { config: { url: '/x' } }); + client.mockRejectedValue(networkError); + + await expect(resource.request({ action: 'detail', routeParams: 'abc' })).rejects.toBe( + networkError, + ); + }); + + it('should give coalesced callers independent copies of response.data', async function () { + const { promise, resolve } = deferred(); + client.mockReturnValue(promise); + + // Two custom-action-style GETs coalesce onto one request but must not share `data`: the + // originating caller owns the raw response, the one that attaches gets a deep copy. + const first = resource.request({ action: 'detail', routeParams: 'abc' }); + const second = resource.request({ action: 'detail', routeParams: 'abc' }); + + expect(client).toHaveBeenCalledTimes(1); + resolve({ data: { id: 'abc', tags: [] } }); + const [a, b] = await Promise.all([first, second]); + + expect(a.data).not.toBe(b.data); + b.data.tags.push('mutated'); + expect(a.data.tags).toEqual([]); + }); + + it('should ignore query param ordering when matching in-flight requests', async function () { + const { promise, resolve } = deferred(); + client.mockReturnValue(promise); + + const first = resource.request({ params: { a: 1, b: 2 } }); + const second = resource.request({ params: { b: 2, a: 1 } }); + + expect(client).toHaveBeenCalledTimes(1); + resolve({ data: [] }); + await Promise.all([first, second]); + }); + + it('should not de-duplicate GETs with different query params', async function () { + const { promise, resolve } = deferred(); + client.mockReturnValue(promise); + + const requests = Promise.all([ + resource.request({ params: { queue: 'content' } }), + resource.request({ params: { queue: 'facility' } }), + ]); + + expect(client).toHaveBeenCalledTimes(2); + resolve({ data: [] }); + await requests; + }); + + it('should not de-duplicate GETs against different URLs', async function () { + const { promise, resolve } = deferred(); + client.mockReturnValue(promise); + + const requests = Promise.all([ + resource.request({ action: 'detail', routeParams: 'one' }), + resource.request({ action: 'detail', routeParams: 'two' }), + ]); + + expect(client).toHaveBeenCalledTimes(2); + resolve({ data: {} }); + await requests; + }); + + it('should not de-duplicate writes', async function () { + const { promise, resolve } = deferred(); + client.mockReturnValue(promise); + + const requests = Promise.all([ + resource.request({ method: 'POST', data: { name: 'test' } }), + resource.request({ method: 'POST', data: { name: 'test' } }), + ]); + + expect(client).toHaveBeenCalledTimes(2); + resolve({ data: {} }); + await requests; + }); + + it('should fire a new request once the previous one has resolved', async function () { + await resource.request({ params: { queue: 'content' } }); + await resource.request({ params: { queue: 'content' } }); + expect(client).toHaveBeenCalledTimes(2); + }); + + it('should fire a new request once the previous one has failed', async function () { + client.mockRejectedValue(new Error('nope')); + await expect(resource.request({ params: { queue: 'content' } })).rejects.toThrow(); + await expect(resource.request({ params: { queue: 'content' } })).rejects.toThrow(); + expect(client).toHaveBeenCalledTimes(2); + }); + }); + + describe('retrieve method', function () { + it('should GET the detail URL and resolve with the response data', async function () { + client.mockResolvedValue({ data: { id: 'abc', name: 'test' } }); + const data = await resource.retrieve('abc'); + expect(client).toHaveBeenCalledWith({ + url: '/api/detail/abc', + method: 'GET', + params: undefined, + }); + expect(data).toEqual({ id: 'abc', name: 'test' }); + }); + + it('should pass query params through', async function () { + await resource.retrieve('abc', { params: { fields: 'name' } }); + expect(client).toHaveBeenCalledWith(expect.objectContaining({ params: { fields: 'name' } })); + }); + + it('should reject if no id is specified', async function () { + await expect(resource.retrieve()).rejects.toThrow(TypeError); + }); + }); + + describe('list method', function () { + it('should resolve with an array from an unpaginated endpoint', async function () { + client.mockResolvedValue({ data: [{ id: 'one' }, { id: 'two' }] }); + const data = await resource.list(); + expect(data).toEqual([{ id: 'one' }, { id: 'two' }]); + }); + + it('should resolve with the pagination object from a paginated endpoint', async function () { + const paginated = { results: [{ id: 'one' }], more: { offset: 25 }, count: 90 }; + client.mockResolvedValue({ data: paginated }); + const data = await resource.list(); + expect(data).toEqual(paginated); + }); + + it('should pass query params through', async function () { + client.mockResolvedValue({ data: [] }); + await resource.list({ queue: 'content' }); + expect(client).toHaveBeenCalledWith( + expect.objectContaining({ url: '/api/list/', params: { queue: 'content' } }), + ); + }); + + it('should hand each coalesced caller an independent copy of the data', async function () { + const { promise, resolve } = deferred(); + client.mockReturnValue(promise); + + // Both fire before the request settles, so they share one in-flight response. + const first = resource.list(); + const second = resource.list(); + resolve({ data: [{ id: 'one', tags: [] }] }); + const [a, b] = await Promise.all([first, second]); + + expect(a).not.toBe(b); + expect(a[0]).not.toBe(b[0]); + a[0].tags.push('mutated'); + expect(b[0].tags).toEqual([]); + }); + }); + + describe('create method', function () { + it('should POST the data and resolve with the created object', async function () { + client.mockResolvedValue({ data: { id: 'abc', name: 'test' } }); + const data = await resource.create({ name: 'test' }); + expect(client).toHaveBeenCalledWith({ + url: '/api/list/', + method: 'POST', + params: undefined, + data: { name: 'test' }, + multipart: false, + }); + expect(data).toEqual({ id: 'abc', name: 'test' }); + }); + + it('should forward the multipart flag', async function () { + await resource.create({ name: 'test' }, true); + expect(client).toHaveBeenCalledWith(expect.objectContaining({ multipart: true })); + }); + }); + + describe('update method', function () { + it('should PATCH the given fields as-is when there is no baseline', async function () { + client.mockResolvedValue({ data: { id: 'abc', name: 'new' } }); + const data = await resource.update('abc', { name: 'new' }); + expect(client).toHaveBeenCalledWith( + expect.objectContaining({ + url: '/api/detail/abc', + method: 'PATCH', + data: { name: 'new' }, + }), + ); + expect(data).toEqual({ id: 'abc', name: 'new' }); + }); + + it('should PATCH only the changed fields when a baseline is provided', async function () { + client.mockResolvedValue({ data: { id: 'abc', name: 'new', description: 'same' } }); + await resource.update( + 'abc', + { name: 'new', description: 'same' }, + { baseline: { id: 'abc', name: 'old', description: 'same' } }, + ); + + expect(client).toHaveBeenCalledWith( + expect.objectContaining({ method: 'PATCH', data: { name: 'new' } }), + ); + }); + + it('should not make a request when nothing changed against the baseline', async function () { + const data = await resource.update( + 'abc', + { name: 'old' }, + { baseline: { id: 'abc', name: 'old' } }, + ); + + expect(client).not.toHaveBeenCalled(); + expect(data).toEqual({ id: 'abc', name: 'old' }); + }); + + it('should reject if no id is specified', async function () { + await expect(resource.update()).rejects.toThrow(TypeError); + }); + }); + + describe('delete method', function () { + it('should DELETE the detail URL and resolve with the id', async function () { + const id = await resource.delete('abc'); + expect(client).toHaveBeenCalledWith( + expect.objectContaining({ url: '/api/detail/abc', method: 'DELETE' }), + ); + expect(id).toEqual('abc'); + }); + + it('should reject if no id is specified', async function () { + await expect(resource.delete()).rejects.toThrow(TypeError); + }); + }); + + describe('bulkCreate method', function () { + it('should POST the array and resolve with the created objects', async function () { + const created = [{ id: 'one' }, { id: 'two' }]; + client.mockResolvedValue({ data: created }); + const data = await resource.bulkCreate([{ name: 'one' }, { name: 'two' }]); + expect(client).toHaveBeenCalledWith( + expect.objectContaining({ + url: '/api/list/', + method: 'POST', + data: [{ name: 'one' }, { name: 'two' }], + }), + ); + expect(data).toEqual(created); + }); + + it('should reject if not given an array', async function () { + await expect(resource.bulkCreate({ name: 'one' })).rejects.toThrow(TypeError); + }); + }); + + describe('bulkDelete method', function () { + it('should DELETE with the given query params', async function () { + await resource.bulkDelete({ collection: 'abc' }); + expect(client).toHaveBeenCalledWith( + expect.objectContaining({ + url: '/api/list/', + method: 'DELETE', + params: { collection: 'abc' }, + }), + ); + }); + + it('should reject if no params are specified, to prevent an unfiltered delete', async function () { + await expect(resource.bulkDelete()).rejects.toThrow(TypeError); + await expect(resource.bulkDelete({})).rejects.toThrow(TypeError); + }); + }); +}); + +describe('Resource.useRetrieve', () => { + let resource, client; + + beforeEach(() => { + resource = new Resources.Resource({ name: 'test' }); + client = jest.fn().mockResolvedValue({ data: { id: 'abc', name: 'test' } }); + resource.client = client; + resource.getUrlFunction = jest.fn(action => urlFunctionFor(action)); + }); + + it('should not fetch until fetchData is called', () => { + const { data, loading } = resource.useRetrieve('abc'); + + expect(client).not.toHaveBeenCalled(); + expect(data.value).toBe(null); + expect(loading.value).toBe(false); + }); + + it('should retrieve the object by id and expose it as data', async () => { + const { data, fetchData } = resource.useRetrieve('abc'); + + await fetchData(); + + expect(client).toHaveBeenCalledWith( + expect.objectContaining({ url: '/api/detail/abc', method: 'GET' }), + ); + expect(data.value).toEqual({ id: 'abc', name: 'test' }); + }); + + it('should pass params through to retrieve', async () => { + const { fetchData } = resource.useRetrieve('abc', { params: { fields: 'name' } }); + + await fetchData(); + + expect(client).toHaveBeenCalledWith(expect.objectContaining({ params: { fields: 'name' } })); + }); + + it('should call onSuccess with the retrieved object after each fetch', async () => { + const onSuccess = jest.fn(); + const { fetchData } = resource.useRetrieve('abc', { onSuccess }); + + await fetchData(); + + expect(onSuccess).toHaveBeenCalledWith({ id: 'abc', name: 'test' }); + }); + + it('should read the current value of a ref id at fetch time', async () => { + const id = ref('abc'); + const { fetchData } = resource.useRetrieve(id); + + await fetchData(); + expect(client).toHaveBeenLastCalledWith(expect.objectContaining({ url: '/api/detail/abc' })); + + id.value = 'def'; + await fetchData(); + expect(client).toHaveBeenLastCalledWith(expect.objectContaining({ url: '/api/detail/def' })); + }); + + it('should read the current value of a getter id at fetch time', async () => { + let id = 'abc'; + const { fetchData } = resource.useRetrieve(() => id); + + await fetchData(); + expect(client).toHaveBeenLastCalledWith(expect.objectContaining({ url: '/api/detail/abc' })); + + id = 'def'; + await fetchData(); + expect(client).toHaveBeenLastCalledWith(expect.objectContaining({ url: '/api/detail/def' })); + }); + + it('should expose a failure as error', async () => { + const failure = new Error('nope'); + client.mockRejectedValue(failure); + const { data, error, fetchData } = resource.useRetrieve('abc'); + + await fetchData(); + + expect(error.value).toBe(failure); + expect(data.value).toBe(null); + }); + + it('should expose only the single-object subset, not the list-only fields', () => { + const result = resource.useRetrieve('abc'); + + expect(Object.keys(result).sort()).toEqual(['data', 'error', 'fetchData', 'loading']); + expect(result.count).toBeUndefined(); + expect(result.hasMore).toBeUndefined(); + expect(result.fetchMore).toBeUndefined(); + }); +}); + +describe('Resource.useList', () => { + let resource, client; + + beforeEach(() => { + resource = new Resources.Resource({ name: 'test' }); + client = jest.fn().mockResolvedValue({ data: [{ id: 'one' }, { id: 'two' }] }); + resource.client = client; + resource.getUrlFunction = jest.fn(action => urlFunctionFor(action)); + }); + + it('should not fetch until fetchData is called', () => { + const { data, loading } = resource.useList(); + + expect(client).not.toHaveBeenCalled(); + expect(data.value).toBe(null); + expect(loading.value).toBe(false); + }); + + it('should expose an unpaginated array response as data', async () => { + const { data, hasMore, fetchData } = resource.useList(); + + await fetchData(); + + expect(client).toHaveBeenCalledWith( + expect.objectContaining({ url: '/api/list/', method: 'GET' }), + ); + expect(data.value).toEqual([{ id: 'one' }, { id: 'two' }]); + expect(hasMore.value).toBe(false); + }); + + it('should pass params through to list', async () => { + const { fetchData } = resource.useList({ member_of: 'facility' }); + + await fetchData(); + + expect(client).toHaveBeenCalledWith( + expect.objectContaining({ params: { member_of: 'facility' } }), + ); + }); + + it('should call onSuccess with the list response after each fetch', async () => { + const onSuccess = jest.fn(); + const { fetchData } = resource.useList(undefined, { onSuccess }); + + await fetchData(); + + expect(onSuccess).toHaveBeenCalledWith([{ id: 'one' }, { id: 'two' }]); + }); + + it('should expose page and totalPages for a page-number paginated response', async () => { + client.mockResolvedValue({ + data: { results: [{ id: 'one' }], page: 1, total_pages: 3, count: 60 }, + }); + const { data, page, totalPages, count, hasMore, fetchData } = resource.useList(); + + await fetchData(); + + expect(data.value).toEqual([{ id: 'one' }]); + expect(page.value).toBe(1); + expect(totalPages.value).toBe(3); + expect(count.value).toBe(60); + expect(hasMore.value).toBe(false); + }); + + it('should read the current value of ref params at fetch time', async () => { + const params = ref({ member_of: 'one' }); + const { fetchData } = resource.useList(params); + + await fetchData(); + expect(client).toHaveBeenLastCalledWith( + expect.objectContaining({ params: { member_of: 'one' } }), + ); + + params.value = { member_of: 'two' }; + await fetchData(); + expect(client).toHaveBeenLastCalledWith( + expect.objectContaining({ params: { member_of: 'two' } }), + ); + }); + + describe('pagination', () => { + beforeEach(() => { + client.mockResolvedValue({ + data: { results: [{ id: 'one' }], more: { limit: 1, offset: 1 }, count: 2 }, + }); + }); + + it('should expose the results, count and hasMore of a paginated response', async () => { + const { data, count, hasMore, fetchData } = resource.useList(); + + await fetchData(); + + expect(data.value).toEqual([{ id: 'one' }]); + expect(count.value).toBe(2); + expect(hasMore.value).toBe(true); + }); + + it('should fetch the next page using the servers more params and append the results', async () => { + const { data, hasMore, fetchData, fetchMore } = resource.useList({ limit: 1 }); + + await fetchData(); + + client.mockResolvedValue({ data: { results: [{ id: 'two' }], more: null, count: 2 } }); + await fetchMore(); + + expect(client).toHaveBeenLastCalledWith( + expect.objectContaining({ params: { limit: 1, offset: 1 } }), + ); + expect(data.value).toEqual([{ id: 'one' }, { id: 'two' }]); + expect(hasMore.value).toBe(false); + }); + + it('should not fetch more when there is no more data', async () => { + client.mockResolvedValue({ data: { results: [{ id: 'one' }], more: null, count: 1 } }); + const { hasMore, fetchData, fetchMore } = resource.useList(); + + await fetchData(); + client.mockClear(); + await fetchMore(); + + expect(hasMore.value).toBe(false); + expect(client).not.toHaveBeenCalled(); + }); + }); +}); + +describe('Resource.useUpdate', () => { + let resource, client; + + beforeEach(() => { + resource = new Resources.Resource({ name: 'test' }); + client = jest.fn(); + resource.client = client; + resource.getUrlFunction = jest.fn(action => urlFunctionFor(action)); + }); + + it('should create the bound data, adopt it as the baseline, then diff a later update', async () => { + // A ref-driven id/data pair, as a component would bind them: no id yet, so create() is valid. + const id = ref(null); + const data = ref({ name: 'created', description: 'keep' }); + const { create, update } = resource.useUpdate(id, data); + + client.mockResolvedValueOnce({ data: { id: 'abc', name: 'created', description: 'keep' } }); + const created = await create(); + + expect(created).toEqual({ id: 'abc', name: 'created', description: 'keep' }); + expect(client).toHaveBeenCalledWith( + expect.objectContaining({ url: '/api/list/', method: 'POST' }), + ); + + // The consumer adopts the new id and edits the working data; create() set the baseline, so + // this update sends only the changed field. + id.value = 'abc'; + data.value = { name: 'renamed', description: 'keep' }; + client.mockClear(); + client.mockResolvedValueOnce({ data: { id: 'abc', name: 'renamed', description: 'keep' } }); + await update(); + + expect(client).toHaveBeenCalledWith( + expect.objectContaining({ + url: '/api/detail/abc', + method: 'PATCH', + data: { name: 'renamed' }, + }), + ); + }); + + it('should reject update() when there is no id, and create() when there is one', async () => { + const withoutId = resource.useUpdate(ref(null), ref({ name: 'x' })); + await expect(withoutId.update()).rejects.toThrow(TypeError); + + const withId = resource.useUpdate(ref('abc'), ref({ name: 'x' })); + await expect(withId.create()).rejects.toThrow(TypeError); + + // Neither misuse should have hit the network. + expect(client).not.toHaveBeenCalled(); + }); + + it('should diff update against a baseline set via setBaseline', async () => { + const data = ref({ name: 'changed', description: 'keep' }); + const { setBaseline, update } = resource.useUpdate(ref('abc'), data); + setBaseline({ id: 'abc', name: 'original', description: 'keep' }); + + client.mockResolvedValueOnce({ data: { id: 'abc', name: 'changed', description: 'keep' } }); + await update(); + + expect(client).toHaveBeenCalledWith( + expect.objectContaining({ + url: '/api/detail/abc', + method: 'PATCH', + data: { name: 'changed' }, + }), + ); + }); + + it('should deep-copy the baseline so later mutation cannot corrupt the diff', async () => { + const { setBaseline, update } = resource.useUpdate(ref('abc'), ref({ name: 'changed' })); + const original = { id: 'abc', name: 'original' }; + setBaseline(original); + // Mutating the caller's object after handing it over must not change what we diff against. + original.name = 'mutated'; + + client.mockResolvedValueOnce({ data: { id: 'abc', name: 'changed' } }); + await update(); + + expect(client).toHaveBeenCalledWith(expect.objectContaining({ data: { name: 'changed' } })); + }); + + it('should toggle isSaving around a write and surface errors', async () => { + const failure = new Error('nope'); + client.mockRejectedValueOnce(failure); + const { update, isSaving, error, setBaseline } = resource.useUpdate( + ref('abc'), + ref({ name: 'changed' }), + ); + setBaseline({ id: 'abc', name: 'original' }); + + await expect(update()).rejects.toBe(failure); + + expect(error.value).toBe(failure); + expect(isSaving.value).toBe(false); + }); + + it('should keep the baseline from the last-issued write when responses arrive out of order', async () => { + const { promise: first, resolve: resolveFirst } = deferred(); + const { promise: second, resolve: resolveSecond } = deferred(); + + const data = ref({ name: 'A' }); + const { update, setBaseline } = resource.useUpdate(ref('abc'), data); + setBaseline({ id: 'abc', name: 'original' }); + + client.mockReturnValueOnce(first); + const u1 = update(); + data.value = { name: 'B' }; + client.mockReturnValueOnce(second); + const u2 = update(); + + // Responses arrive reversed: the superseded first write settles last and must not overwrite + // the baseline recorded by the second. + resolveSecond({ data: { id: 'abc', name: 'B' } }); + resolveFirst({ data: { id: 'abc', name: 'A' } }); + await Promise.all([u1, u2]); + + // Baseline is 'B' (last issued), so re-saving 'B' is an empty diff and issues no request. + // Were it the last-settled 'A', this would PATCH and silently drop nothing - the bug. + client.mockClear(); + data.value = { name: 'B' }; + await update(); + expect(client).not.toHaveBeenCalled(); + }); + + it('should keep isSaving true until the last-issued write settles', async () => { + const { promise: first, resolve: resolveFirst } = deferred(); + const { promise: second, resolve: resolveSecond } = deferred(); + + const data = ref({ name: 'A' }); + const { update, isSaving, setBaseline } = resource.useUpdate(ref('abc'), data); + setBaseline({ id: 'abc', name: 'original' }); + + client.mockReturnValueOnce(first); + const u1 = update(); + data.value = { name: 'B' }; + client.mockReturnValueOnce(second); + const u2 = update(); + + expect(isSaving.value).toBe(true); + // The first (now superseded) write settles - isSaving must stay true while the second is + // still in flight. + resolveFirst({ data: { id: 'abc', name: 'A' } }); + await u1; + expect(isSaving.value).toBe(true); + + resolveSecond({ data: { id: 'abc', name: 'B' } }); + await u2; + expect(isSaving.value).toBe(false); + }); + + it('save() should create without an id and update once an id is set', async () => { + const id = ref(null); + const data = ref({ name: 'x' }); + const { save } = resource.useUpdate(id, data); + + client.mockResolvedValueOnce({ data: { id: 'abc', name: 'x' } }); + await save(); + expect(client).toHaveBeenLastCalledWith( + expect.objectContaining({ url: '/api/list/', method: 'POST' }), + ); + + // The consumer adopts the created id; save() now dispatches to update, diffing against the + // baseline that create() recorded, so only the changed field is sent. + id.value = 'abc'; + data.value = { name: 'y' }; + client.mockResolvedValueOnce({ data: { id: 'abc', name: 'y' } }); + await save(); + expect(client).toHaveBeenLastCalledWith( + expect.objectContaining({ url: '/api/detail/abc', method: 'PATCH', data: { name: 'y' } }), + ); + }); + + it('should feed a retrieved object into the baseline via useRetrieve onSuccess', async () => { + // The headline pairing: useRetrieve loads the object and hands it straight to setBaseline, + // then a later update() diffs the edited working data against that retrieved snapshot. + const id = ref('abc'); + const data = ref(null); + const { setBaseline, update } = resource.useUpdate(id, data); + const { fetchData } = resource.useRetrieve(id, { onSuccess: setBaseline }); + + client.mockResolvedValueOnce({ data: { id: 'abc', name: 'original', description: 'keep' } }); + await fetchData(); + + // Consumer edits a single field of the working copy. + data.value = { name: 'edited', description: 'keep' }; + client.mockResolvedValueOnce({ data: { id: 'abc', name: 'edited', description: 'keep' } }); + await update(); + + expect(client).toHaveBeenLastCalledWith( + expect.objectContaining({ + url: '/api/detail/abc', + method: 'PATCH', + data: { name: 'edited' }, + }), + ); + }); + + it('should not diff against a baseline recorded for a different id', async () => { + const id = ref('a'); + const data = ref({ name: 'x' }); + const { setBaseline, update } = resource.useUpdate(id, data); + setBaseline({ id: 'a', name: 'x' }); + + // Rebind to a different object before a fresh baseline is installed; the edited value + // happens to coincide with object A's. + id.value = 'b'; + client.mockResolvedValueOnce({ data: { id: 'b', name: 'x' } }); + await update(); + + // The stale baseline belongs to 'a', so it must be ignored: send the field as-is rather than + // diffing it away and skipping the write. + expect(client).toHaveBeenCalledWith( + expect.objectContaining({ url: '/api/detail/b', method: 'PATCH', data: { name: 'x' } }), + ); + }); + + it('should expose isDirty comparing the working data against the baseline', () => { + const data = ref({ id: 'abc', name: 'original' }); + const { setBaseline, isDirty } = resource.useUpdate(ref('abc'), data); + + // No baseline yet - nothing to be dirty against. + expect(isDirty.value).toBe(false); + + setBaseline({ id: 'abc', name: 'original' }); + expect(isDirty.value).toBe(false); + + data.value = { id: 'abc', name: 'changed' }; + expect(isDirty.value).toBe(true); + }); +}); diff --git a/packages/kolibri/apiResource.js b/packages/kolibri/apiResource.js index 876f0fac2d1..08c0a03f494 100644 --- a/packages/kolibri/apiResource.js +++ b/packages/kolibri/apiResource.js @@ -3,11 +3,19 @@ import cloneDeep from 'lodash/cloneDeep'; import find from 'lodash/find'; import matches from 'lodash/matches'; import isEqual from 'lodash/isEqual'; +import qs from 'qs'; +import { ref, shallowRef, computed } from 'vue'; +import { toValue } from '@vueuse/core'; import urls from 'kolibri/urls'; +import useFetch from 'kolibri/composables/useFetch'; import sanitizeError from './utils/sanitizeError'; export const logging = logger.getLogger(__filename); +// Serialize query parameters into a string that is stable regardless of key insertion order, +// so that two equivalent parameter objects produce the same in-flight request key. +const stableSerialize = params => qs.stringify(params, { sort: (a, b) => a.localeCompare(b) }); + /** Class representing a single API resource object */ export class Model { /** @@ -598,6 +606,9 @@ export class Resource { Object.defineProperty(this, key, optionsDefinitions[key]); }); this.clearCache(); + // Currently pending GET requests, keyed by resolved URL and query params. This is request + // coalescing, not a cache - an entry is removed as soon as its request settles. + this._inFlight = new Map(); } __cacheKey(...params) { @@ -955,14 +966,407 @@ export class Resource { return this.accessDetailEndpoint('get', 'detail', id).then(response => response.data); } - list(params = {}) { - return this.accessListEndpoint('get', 'list', params).then(response => response.data); + /** + * Resolve the URL for a named action, applying any route parameters. + * @param {string} action - The name of the endpoint, as registered with the URL resolver + * @param {object | Array | string | number} [routeParams] - An object is passed as named + * kwargs, an array is spread as positional arguments, and any other value is passed as a + * single positional argument. Omit for a route that takes no parameters. + * @returns {string} - The resolved URL + * @throws {ReferenceError} - When no URL is registered for this action + */ + __resolveUrl(action, routeParams) { + const urlFunction = this.getUrlFunction(action); + if (!urlFunction) { + throw ReferenceError(`No URL found for the ${action} action of the ${this.name} resource`); + } + if (routeParams === undefined || routeParams === null) { + return urlFunction(); + } + if (Array.isArray(routeParams)) { + return urlFunction(...routeParams); + } + return urlFunction(routeParams); + } + + /** + * The single low-level primitive that every read, write, and custom action goes through. + * + * Concurrent identical GET requests share a single in-flight request. Writes are never + * de-duplicated. + * @param {object} [options] - The request definition + * @param {string} [options.method=GET] - A valid HTTP method name + * @param {string} [options.action=list] - The name of the endpoint to target + * @param {object | Array | string | number} [options.routeParams] - Parameters for the URL + * itself + * @param {object} [options.params] - Query parameters + * @param {object | Array} [options.data] - The request body, for writes + * @param {boolean} [options.multipart=false] - Whether to encode the body as multipart form + * data + * @returns {Promise} - Promise that resolves with the full response object. A caller that + * coalesces onto an in-flight GET receives a deep copy of `data` unique to it. + */ + async request({ + method = 'GET', + action = 'list', + routeParams, + params, + data, + multipart = false, + } = {}) { + const url = this.__resolveUrl(action, routeParams); + // Log any request failure before re-raising it, so it surfaces with request context the way + // the legacy Model/Collection layer did. This only intercepts to log - the rejection still + // propagates to the caller unchanged. + const logAndRethrow = error => { + this.logError(error); + throw error; + }; + if (method.toUpperCase() !== 'GET') { + // General case (writes): no dedup, as two writes are two distinct intents. + return this.client({ url, method, params, data, multipart }).catch(logAndRethrow); + } + const key = `${url}?${stableSerialize(params)}`; + const existing = this._inFlight.get(key); + if (existing) { + // Attached to an in-flight request another caller started - clone `data` so the two + // callers don't share one mutable payload. + return existing.then(response => ({ ...response, data: cloneDeep(response.data) })); + } + // Clear the entry once the request settles, whether it succeeded or failed - a lingering + // rejected promise would poison every subsequent request for this key. `logError` runs on the + // shared promise, so coalesced callers do not each re-log the same failure. + const promise = this.client({ url, method, params }) + .catch(logAndRethrow) + .finally(() => { + this._inFlight.delete(key); + }); + this._inFlight.set(key, promise); + // The originating caller's payload is unshared by construction, so it needs no copy. + return promise; + } + + /** + * Fetch a single object by id from the resource's default detail endpoint. + * @param {string} id - The id of the object to fetch + * @param {object} [options] - Additional request options + * @param {object} [options.params] - Query parameters + * @returns {Promise} - Promise that resolves with the object + * @throws {TypeError} - When `id` is missing + */ + async retrieve(id, { params } = {}) { + if (!id) { + throw TypeError('An id must be specified'); + } + const response = await this.request({ action: 'detail', routeParams: id, params }); + return response.data; + } + + /** + * Fetch a collection of objects from the resource's default list endpoint. A custom + * list-shaped endpoint (e.g. a detail route that returns a collection) is authored as its + * own method over `request`, not through here. + * @param {object} [params] - Query parameters + * @returns {Promise} - Promise that resolves with the server's response data - either an + * array of objects, or a `{ results, more, count }` object when the endpoint is paginated + */ + async list(params = {}) { + const response = await this.request({ params }); + return response.data; } - create(params = {}, multipart = false) { - return this.accessListEndpoint('post', 'list', params, multipart).then( - response => response.data, + /** + * Create a single object against the resource's default list endpoint. + * @param {object} data - The object to create + * @param {boolean} [multipart=false] - Whether to encode the body as multipart form data + * @returns {Promise} - Promise that resolves with the created object + */ + async create(data, multipart = false) { + const response = await this.request({ method: 'POST', data, multipart }); + return response.data; + } + + /** + * Update an object. When a `baseline` snapshot is provided, only the fields that differ from + * it are sent, and an unchanged object issues no request at all. + * @param {string} id - The id of the object to update + * @param {object} data - The fields to update + * @param {object} [options] - Additional request options + * @param {object} [options.params] - Query parameters + * @param {object} [options.baseline] - The last server snapshot of this object. When present, + * `data` is diffed against it and only changed fields are sent; when omitted (or falsy), + * `data` is sent unchanged. + * @returns {Promise} - Promise that resolves with the updated object + * @throws {TypeError} - When `id` is missing + */ + async update(id, data = {}, { params, baseline } = {}) { + if (!id) { + throw TypeError('An id must be specified'); + } + let payload = data; + if (baseline) { + payload = {}; + for (const key of Object.keys(data)) { + if (!isEqual(data[key], baseline[key])) { + payload[key] = data[key]; + } + } + if (!Object.keys(payload).length) { + // Nothing changed, so there is nothing to send. + return cloneDeep(baseline); + } + } + const response = await this.request({ + method: 'PATCH', + action: 'detail', + routeParams: id, + params, + data: payload, + }); + return response.data; + } + + /** + * Delete a single object by id. + * @param {string} id - The id of the object to delete + * @param {object} [options] - Additional request options + * @param {object} [options.params] - Query parameters + * @returns {Promise} - Promise that resolves with the id of the deleted object + * @throws {TypeError} - When `id` is missing + */ + async delete(id, { params } = {}) { + if (!id) { + throw TypeError('An id must be specified'); + } + await this.request({ method: 'DELETE', action: 'detail', routeParams: id, params }); + return id; + } + + /** + * Create several objects in a single request against the resource's default list endpoint. + * Only works for resources whose endpoint accepts an array payload. + * @param {object[]} data - The objects to create + * @param {boolean} [multipart=false] - Whether to encode the body as multipart form data + * @returns {Promise} - Promise that resolves with the created objects + * @throws {TypeError} - When `data` is not an array + */ + async bulkCreate(data, multipart = false) { + if (!Array.isArray(data)) { + throw TypeError('An array of objects must be specified'); + } + const response = await this.request({ method: 'POST', data, multipart }); + return response.data; + } + + /** + * Delete every object matching the given query parameters, against the resource's default + * list endpoint. + * @param {object} params - Query parameters narrowing what will be deleted + * @returns {Promise} - Promise that resolves with the server's response data + * @throws {TypeError} - When no query parameters are given, to prevent an unfiltered + * bulk delete + */ + async bulkDelete(params = {}) { + if (!Object.keys(params).length) { + throw TypeError('Params must be specified to narrow what is being deleted'); + } + const response = await this.request({ method: 'DELETE', params }); + return response.data; + } + + /** + * @typedef {object} RetrieveObject + * @property {import('vue').Ref} data - The retrieved object, or `null` before the + * first successful fetch. + * @property {import('vue').Ref} loading - Whether a fetch is currently in flight. + * @property {import('vue').Ref} error - The error from the last fetch, or `null`. + * @property {() => Promise} fetchData - Trigger the fetch. Takes no arguments - reads the + * bound `id`/`params` at call time. + */ + + /** + * Reactive read of a single object by id, layered on `useFetch`. + * Data is not refetched when the params change, caller must call `fetchData` again. + * @param {string | import('vue').Ref | (() => string)} id - The id of the object to + * retrieve. A ref or getter is read at fetch time (not watched). + * @param {object} [options] - Additional options. + * @param {object | import('vue').Ref | (() => object)} [options.params] - Query + * parameters passed to `retrieve`. A ref or getter is read at fetch time (not watched). + * @param {(object: object) => void} [options.onSuccess] - Called with the retrieved object + * after each successful fetch. When pairing with `useUpdate`, seed both the working data and the + * baseline here - `onSuccess: obj => { form.value = { ...obj }; setBaseline(obj); }` - so they + * start in sync; setting the baseline alone leaves `isDirty` true until the working data is + * populated. + * @returns {RetrieveObject} The single-object fetch state and action. + */ + useRetrieve(id, { params, onSuccess } = {}) { + const { data, loading, error, fetchData } = useFetch({ + fetchMethod: () => this.retrieve(toValue(id), { params: toValue(params) }), + onSuccess, + }); + return { data, loading, error, fetchData }; + } + + /** + * Reactive read of a collection, layered on `useFetch`. + * Data is not refetched when the params change, caller must call `fetchData` again. + * + * Pagination is driven by the endpoint's shape, and the two backend styles map to two usage + * modes: + * - cursor / limit-offset endpoints emit a `more` cursor: append the next page with + * `fetchMore()`, gated by `hasMore` (infinite scroll). + * - page-number endpoints emit `page`/`total_pages`: show one page at a time by updating the + * page in `params` and re-calling `fetchData()`, reading `page`/`totalPages` for the pager. + * `hasMore`/`fetchMore` do not apply to these (`hasMore` stays `false`). + * @param {object | import('vue').Ref | (() => object)} [params] - Query parameters + * passed to `list`. A ref or getter is read at fetch time (not watched). + * @param {object} [options] - Additional options. + * @param {(response: object) => void} [options.onSuccess] - Called with the raw list response + * after each successful `fetchData` - an array, or a `{ results, more, count }` / + * `{ results, page, total_pages, count }` object when paginated. Not called for `fetchMore`, + * so a consumer mirroring results into a cache must handle appended pages separately. + * @returns {import('kolibri/composables/useFetch').FetchObject} The fetch state and actions. + * The returned `fetchData` takes no arguments. + */ + useList(params, { onSuccess } = {}) { + return useFetch({ + fetchMethod: () => this.list(toValue(params)), + onSuccess, + // `more` is the complete set of query parameters for the next page, so it replaces + // rather than extends the original params. + fetchMoreMethod: more => this.list(more), + }); + } + + /** + * @typedef {object} UpdateObject + * @property {import('vue').Ref} isSaving - Whether a write is in flight. Stays true + * until the latest-issued write settles. The latest-issued write only wins the local + * bookkeeping (baseline / `isSaving` / `error`); writes are not serialised on the wire, so a + * rapidly re-issued save can still race server-side. + * @property {import('vue').Ref} error - The error from the last write, or `null`. On + * failure `create()`/`update()` set this and also reject. + * @property {import('vue').ComputedRef} isDirty - Whether the working `data` differs + * from the last saved snapshot - the unsaved-changes signal an editor needs. Only meaningful + * when `data` has the same shape as the server object; a `data` that is a projection of a larger + * object reads dirty against the fuller baseline. On load, seed the working data alongside the + * baseline (see `useRetrieve`'s `onSuccess`), or this reads true before any edit. + * @property {(object: object|null) => void} setBaseline - Adopt a server snapshot as the + * baseline `update` diffs against (deep-copied); pass `null` to forget it. Typically wired to + * `useRetrieve`'s `onSuccess` to record the baseline on load. + * @property {() => Promise} create - POST the bound `data` and record the created object + * as the next baseline. Rejects if the bound `id` is already set. + * @property {() => Promise} update - PATCH only the fields of `data` that differ from + * the baseline. Rejects if the bound `id` is empty. + * @property {() => Promise} save - Dispatch on the bound `id`: `create()` when empty, + * `update()` otherwise, so call sites need not branch. + */ + + /** + * A write helper bound to a single object of this resource, scoped to the calling component. + * Rather than taking the object per call, the identity (`id`) and the working payload (`data`) + * are bound once here, so this composable is bound to a single object at a time, never arbitrary + * items. After `create()` resolves, the caller can set the new `id` to keep editing the same + * object. + * + * - when `id` is empty, `create()` is valid and `update()` rejects; + * - when `id` is set, `update()` is valid and `create()` rejects. + * + * `create()`, `update()`, and `save()` take no arguments - they read the bound refs at call + * time. + * @param {string | import('vue').Ref | (() => string)} id - The object's id, or empty + * for a not-yet-created object. A ref or getter is read at call time (not watched). + * @param {object | import('vue').Ref | (() => object)} data - The working payload to + * send. A ref or getter is read at call time (not watched). + * @param {object} [options] - Additional options. + * @param {object | import('vue').Ref | (() => object)} [options.params] - Query + * parameters for `update`. + * @param {boolean} [options.multipart] - Whether `create` should send multipart form data. + * @returns {UpdateObject} The write state and actions. + */ + useUpdate(id, data, { params, multipart } = {}) { + const isSaving = ref(false); + const error = ref(null); + // The last server snapshot. Written only through setBaseline / create / update; reactive so + // `isDirty` can derive from it (shallow is enough - it is replaced wholesale, never mutated). + const baseline = shallowRef(null); + + // Store a full copy, so later mutation of the caller's object cannot corrupt the snapshot + // we diff against. + const setBaseline = object => { + baseline.value = object == null ? null : cloneDeep(object); + }; + + // True when the working `data` differs from the last saved snapshot. Only meaningful when + // `data` has the same shape as the server object; a `data` that is a projection of a larger + // object (e.g. a subset of fields) will read dirty against the fuller baseline. + const isDirty = computed( + () => baseline.value != null && !isEqual(toValue(data), baseline.value), ); + + // A monotonic token, captured per call and re-checked before touching shared state, so a + // superseded write's out-of-order response cannot record a stale baseline (which would make + // the next diff short-circuit to a silent no-op) or flip isSaving/error for a call still in + // flight. + let writeCount = 0; + const run = async request => { + const current = ++writeCount; + isSaving.value = true; + error.value = null; + try { + const saved = await request(); + // The saved object is the freshest server truth, so it becomes the next baseline. + if (current === writeCount) { + setBaseline(saved); + } + return saved; + } catch (e) { + if (current === writeCount) { + error.value = e; + } + throw e; + } finally { + if (current === writeCount) { + isSaving.value = false; + } + } + }; + + const create = () => { + if (toValue(id)) { + return Promise.reject( + TypeError('create() called for an object that already has an id; use update()'), + ); + } + return run(() => this.create(toValue(data), multipart)); + }; + + const update = () => { + const currentId = toValue(id); + if (!currentId) { + return Promise.reject(TypeError('update() called without an id; use create()')); + } + // Only diff against the baseline when it belongs to the object currently bound. If `id` + // was rebound before a fresh baseline was installed, a stale baseline from the previous + // object could silently drop fields whose edited value coincides with the old one; a + // mismatched baseline degrades to a full PATCH, which is always correct. + const currentBaseline = + baseline.value != null && String(baseline.value[this.idKey]) === String(currentId) + ? baseline.value + : undefined; + return run(() => + this.update(currentId, toValue(data), { + params: toValue(params), + baseline: currentBaseline, + }), + ); + }; + + // Dispatch on the bound id so call sites need not branch: create a not-yet-persisted + // object, otherwise update the existing one. + const save = () => (toValue(id) ? update() : create()); + + return { isSaving, error, isDirty, setBaseline, create, update, save }; } /** @@ -1066,7 +1470,9 @@ export class Resource { logError(err) { // eslint-disable-next-line import-x/no-commonjs const router = require('kolibri/router').default; - if (!err.config) { + // Only log what we can describe. A network failure (or a cancellation) has no `response`, so + // reading `err.response.statusText` below would throw and mask the real error - bail early. + if (!err.config || !err.response) { return; } const sanitized = sanitizeError(err); diff --git a/packages/kolibri/apiResources/TaskResource.js b/packages/kolibri/apiResources/TaskResource.js index bac38a197e1..64205ee2854 100644 --- a/packages/kolibri/apiResources/TaskResource.js +++ b/packages/kolibri/apiResources/TaskResource.js @@ -8,7 +8,7 @@ export default new Resource({ }, startTasks(tasks, multipart = false) { - return this.create(tasks, multipart); + return this.bulkCreate(tasks, multipart); }, cancel(jobId) { diff --git a/packages/kolibri-common/composables/__tests__/useFetch.spec.js b/packages/kolibri/composables/__tests__/useFetch.spec.js similarity index 81% rename from packages/kolibri-common/composables/__tests__/useFetch.spec.js rename to packages/kolibri/composables/__tests__/useFetch.spec.js index 3469be5e1e7..2846af2ddcf 100644 --- a/packages/kolibri-common/composables/__tests__/useFetch.spec.js +++ b/packages/kolibri/composables/__tests__/useFetch.spec.js @@ -1,5 +1,5 @@ import Vue, { nextTick } from 'vue'; -import useFetch from 'kolibri-common/composables/useFetch.js'; +import useFetch from 'kolibri/composables/useFetch'; const _eventDispatcher = new Vue(); let _fetchCount = 0; @@ -218,6 +218,53 @@ describe('useFetch', () => { }); }); }); + describe('onSuccess', () => { + it('should call onSuccess with the response after a successful fetch', async () => { + const { fetchMethod, resolveFetch } = getSincronizableFetch(); + const onSuccess = jest.fn(); + + const { fetchData } = useFetch({ fetchMethod, onSuccess }); + + fetchData({ id: 'fetch1', response: 'response1' }); + resolveFetch('fetch1'); + await nextTick(); + + expect(onSuccess).toHaveBeenCalledWith('response1'); + }); + + it('should not call onSuccess for a fetch superseded by a newer one', async () => { + const { fetchMethod, resolveFetch } = getSincronizableFetch(); + const onSuccess = jest.fn(); + + const { fetchData } = useFetch({ fetchMethod, onSuccess }); + + fetchData({ id: 'fetch1', response: 'response1' }); + fetchData({ id: 'fetch2', response: 'response2' }); + + // The stale fetch resolves last, but must not fire onSuccess - only the latest wins, so a + // side effect like a baseline snapshot stays in sync with `data`. + resolveFetch('fetch2'); + resolveFetch('fetch1'); + await nextTick(); + + expect(onSuccess).toHaveBeenCalledTimes(1); + expect(onSuccess).toHaveBeenCalledWith('response2'); + }); + + it('should not call onSuccess when the fetch fails', async () => { + const { fetchMethod, resolveFetch } = getSincronizableFetch(); + const onSuccess = jest.fn(); + + const { fetchData } = useFetch({ fetchMethod, onSuccess }); + + fetchData({ id: 'fetch1', error: 'error1' }); + resolveFetch('fetch1'); + await nextTick(); + + expect(onSuccess).not.toHaveBeenCalled(); + }); + }); + describe('fetchMore', () => { it('data should be set to response.results of fetchData if fetchMoreMethod is passed', async () => { const { fetchMethod, resolveFetch } = getSincronizableFetch(); @@ -235,6 +282,27 @@ describe('useFetch', () => { expect(data.value).toBe(response.results); }); + it('data should be set to the array itself when fetchMoreMethod is passed but the response is a plain array', async () => { + // A list endpoint that is not paginated resolves a plain array even though a + // fetchMoreMethod is supplied; useFetch must handle that shape, not assume `.results`. + const { fetchMethod, resolveFetch } = getSincronizableFetch(); + const fetchMoreMethod = jest.fn(); + + const { data, hasMore, count, fetchData } = useFetch({ + fetchMethod, + fetchMoreMethod, + }); + + const response = [{ id: 'one' }, { id: 'two' }]; + fetchData({ id: 'fetch1', response }); + resolveFetch('fetch1'); + await nextTick(); + expect(data.value).toBe(response); + expect(hasMore.value).toBe(false); + // Contract note: `count` stays null for the array shape even though the length is known. + expect(count.value).toBe(null); + }); + it('should set hasMore to false if response.more is not defined', async () => { const { fetchMethod, resolveFetch } = getSincronizableFetch(); const fetchMoreMethod = jest.fn(); @@ -539,4 +607,47 @@ describe('useFetch', () => { }); }); }); + + describe('page-number pagination', () => { + it('should expose page and totalPages from a page-number response', async () => { + const { fetchMethod, resolveFetch } = getSincronizableFetch(); + const { data, page, totalPages, count, hasMore, fetchData } = useFetch({ + fetchMethod, + fetchMoreMethod: jest.fn(), + }); + + fetchData({ + id: 'fetch1', + response: { results: [{ id: 'a' }], page: 2, total_pages: 4, count: 90 }, + }); + resolveFetch('fetch1'); + await nextTick(); + + expect(data.value).toEqual([{ id: 'a' }]); + expect(page.value).toBe(2); + expect(totalPages.value).toBe(4); + expect(count.value).toBe(90); + // No `more` cursor, so the append-style pagination is inert for this shape. + expect(hasMore.value).toBe(false); + }); + + it('should leave page and totalPages null for a more-cursor response', async () => { + const { fetchMethod, resolveFetch } = getSincronizableFetch(); + const { page, totalPages, hasMore, fetchData } = useFetch({ + fetchMethod, + fetchMoreMethod: jest.fn(), + }); + + fetchData({ + id: 'fetch1', + response: { results: [{ id: 'a' }], more: { cursor: 'x' }, count: 5 }, + }); + resolveFetch('fetch1'); + await nextTick(); + + expect(page.value).toBe(null); + expect(totalPages.value).toBe(null); + expect(hasMore.value).toBe(true); + }); + }); }); diff --git a/packages/kolibri-common/composables/useFetch.js b/packages/kolibri/composables/useFetch.js similarity index 64% rename from packages/kolibri-common/composables/useFetch.js rename to packages/kolibri/composables/useFetch.js index 1bcd666ad15..403c1a6f37c 100644 --- a/packages/kolibri-common/composables/useFetch.js +++ b/packages/kolibri/composables/useFetch.js @@ -6,10 +6,16 @@ import { ref, computed } from 'vue'; * @property {import('vue').Ref} error - Error object if a fetch failed. * @property {import('vue').Ref} count - Count of the fetched data, e.g. the total * number of items. + * @property {import('vue').Ref} page - Current page number, for page-number paginated + * endpoints; `null` otherwise. + * @property {import('vue').Ref} totalPages - Total number of pages, for page-number + * paginated endpoints; `null` otherwise. * @property {import('vue').Ref} loading - Data loading state. This does not reflect * the loading state when fetching more data; refer to `loadingMore` for that. * @property {import('vue').Ref} loadingMore - Loading state when fetching more data. - * @property {import('vue').ComputedRef} hasMore - Whether there is more data to fetch. + * @property {import('vue').ComputedRef} hasMore - Whether `fetchMore` can append + * another page. True only for endpoints that emit a `more` cursor (limit-offset / cursor + * pagination); for page-number endpoints use `page`/`totalPages` with `fetchData` instead. * @property {(...args: unknown[]) => Promise} fetchData - Manually trigger the main fetch. * @property {(...args: unknown[]) => Promise} fetchMore - Manually trigger a fetch of * additional data. @@ -26,25 +32,32 @@ import { ref, computed } from 'vue'; * }); * ``` * - * `fetchMethod` should return either the fetched data, or an object of the form - * `{ results, more, count }` where `results` is the fetched data and `more` is the - * `moreParams` object passed to subsequent `fetchMoreMethod` calls. `fetchMore` only - * works when the fetched data is an array. + * `fetchMethod` should return either the fetched data (a plain array), or a paginated object. + * Two paginated shapes are understood: `{ results, more, count }` - where `more` is the + * `moreParams` passed to subsequent `fetchMoreMethod` calls to append the next page - and + * `{ results, page, total_pages, count }`, surfaced as `page`/`totalPages` for jump-to-page + * navigation by re-calling `fetchData` with a new page param. `fetchMore` only appends, and + * only for the `more` shape. * @param {object} options - Configuration options for the fetch operation. * @param {(...args: unknown[]) => Promise} options.fetchMethod - Function to fetch * the initial data. * @param {(more: unknown, ...args: unknown[]) => Promise} [options.fetchMoreMethod] * Function to fetch more data, called with the previous response's `more` object. + * @param {(response: unknown) => void} [options.onSuccess] - Called with the fetched response + * after a successful `fetchData`, but only once it has passed the staleness check - a fetch + * superseded by a newer one never invokes it. Not called for `fetchMore`. * @returns {FetchObject} An object exposing the fetch state and actions. */ export default function useFetch(options) { - const { fetchMethod, fetchMoreMethod } = options || {}; + const { fetchMethod, fetchMoreMethod, onSuccess } = options || {}; const loading = ref(false); const data = ref(null); const error = ref(null); const moreParams = ref(null); const count = ref(null); + const page = ref(null); + const totalPages = ref(null); const loadingMore = ref(false); // useFetch metadata to manage synchronization of fetches @@ -53,7 +66,10 @@ export default function useFetch(options) { const hasMore = computed(() => moreParams.value != null); const _setData = (response, loadingMore) => { - const responseData = fetchMoreMethod ? response.results : response; + // A list endpoint returns a plain array when it is not paginated, and a + // `{ results, more, count }` object when it is. Handle both, so that a fetchMoreMethod + // can be supplied without knowing up front which shape will come back. + const responseData = fetchMoreMethod && !Array.isArray(response) ? response.results : response; /** * For now, loading more just works if the data is an array. @@ -66,6 +82,10 @@ export default function useFetch(options) { moreParams.value = response.more || null; count.value = response.count || null; + // Page-number paginated endpoints emit `page`/`total_pages` instead of a `more` cursor; + // surface them so consumers can drive jump-to-page navigation via `fetchData`. + page.value = response.page ?? null; + totalPages.value = response.total_pages ?? null; }; const fetchData = async (...args) => { @@ -85,6 +105,11 @@ export default function useFetch(options) { return; } _setData(response); + // Runs only after the staleness check, so a superseded fetch cannot fire onSuccess - + // this is what keeps side effects like a baseline snapshot in sync with `data`. + if (onSuccess) { + onSuccess(response); + } } catch (err) { if (newFetchHasStarted()) { return; @@ -128,6 +153,8 @@ export default function useFetch(options) { data, error, count, + page, + totalPages, loading, hasMore, loadingMore, diff --git a/packages/kolibri/internal/apiSpec.js b/packages/kolibri/internal/apiSpec.js index 89ff6bbe366..fd4e7341f33 100644 --- a/packages/kolibri/internal/apiSpec.js +++ b/packages/kolibri/internal/apiSpec.js @@ -34,6 +34,7 @@ export default { 'kolibri/components/pages/NotificationsRoot': require('kolibri/components/pages/NotificationsRoot'), 'kolibri/components/pages/ScrollingHeader': require('kolibri/components/pages/ScrollingHeader'), 'kolibri/composables/useContentViewer': require('kolibri/composables/useContentViewer'), + 'kolibri/composables/useFetch': require('kolibri/composables/useFetch'), 'kolibri/composables/useMinimumKolibriVersion': require('kolibri/composables/useMinimumKolibriVersion'), 'kolibri/composables/useNav': require('kolibri/composables/useNav'), 'kolibri/composables/useNow': require('kolibri/composables/useNow'), diff --git a/packages/kolibri/package.json b/packages/kolibri/package.json index b96edeb92f2..2e278288446 100644 --- a/packages/kolibri/package.json +++ b/packages/kolibri/package.json @@ -40,6 +40,7 @@ "./components/pages/NotificationsRoot": "./components/pages/NotificationsRoot", "./components/pages/ScrollingHeader": "./components/pages/ScrollingHeader", "./composables/useContentViewer": "./composables/useContentViewer", + "./composables/useFetch": "./composables/useFetch", "./composables/useMinimumKolibriVersion": "./composables/useMinimumKolibriVersion", "./composables/useNav": "./composables/useNav", "./composables/useNow": "./composables/useNow",