From e6c65f852dea7187036417b8da5081171714d8c6 Mon Sep 17 00:00:00 2001 From: Mauko Quiroga Date: Thu, 23 Nov 2023 14:53:55 +0100 Subject: [PATCH] refactor(test): proper error handling (#128) --- .github/workflows/node.js.yml | 22 ++++----- src/model.ts | 39 ++++++++++++---- .../{DatagouvfrAPI.js => DatagouvfrAPI.ts} | 43 +++++++++++++---- .../api/__tests__/DatagouvfrAPI.test.js | 4 +- .../api/__tests__/DiscussionsAPI.test.js | 46 +++++++------------ src/services/api/resources/DiscussionsAPI.ts | 25 +++------- src/views/bouquets/BouquetEditView.vue | 8 ++-- 7 files changed, 103 insertions(+), 84 deletions(-) rename src/services/api/{DatagouvfrAPI.js => DatagouvfrAPI.ts} (77%) diff --git a/.github/workflows/node.js.yml b/.github/workflows/node.js.yml index e7c1cbca5..467234c44 100644 --- a/.github/workflows/node.js.yml +++ b/.github/workflows/node.js.yml @@ -5,20 +5,20 @@ name: Run tests on: push: - branches: [ "main" ] + branches: ['main'] pull_request: - branches: [ "main" ] + branches: ['main'] jobs: build: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v3 - - name: Use Node.js - uses: actions/setup-node@v3 - with: - node-version: 18.x - cache: 'npm' - - run: npm ci - - run: npm run build --if-present - - run: npm run test_single_run + - uses: actions/checkout@v3 + - name: Use Node.js + uses: actions/setup-node@v3 + with: + node-version: 18.x + cache: 'npm' + - run: npm ci + - run: npm run build --if-present + - run: npm run test_single_run diff --git a/src/model.ts b/src/model.ts index 5ee218347..60c7f486e 100644 --- a/src/model.ts +++ b/src/model.ts @@ -8,20 +8,39 @@ interface Subtheme { name: string } -interface DiscussionRequest { - title: string, - comment: string, - subject: DiscussionSubject, +interface Request { + method: string + url: string + data?: Record +} + +interface Response { + status: number + data?: Record + error?: { message: string } +} + +interface DiscussionParams { + title: string + comment: string + subject: DiscussionSubject } interface DiscussionSubject { - class: "Topic" - "id": string + class: 'Topic' + id: string } -interface DiscussionResponse { - status: 200 - data: DiscussionRequest +interface DiscussionResponse extends Response { + status: 200 | number + data?: Partial } -export type { Theme, Subtheme, DiscussionResponse } +export type { + Theme, + Subtheme, + Request, + Response, + DiscussionParams, + DiscussionResponse +} diff --git a/src/services/api/DatagouvfrAPI.js b/src/services/api/DatagouvfrAPI.ts similarity index 77% rename from src/services/api/DatagouvfrAPI.js rename to src/services/api/DatagouvfrAPI.ts index ef463def4..1fa4c2abf 100644 --- a/src/services/api/DatagouvfrAPI.js +++ b/src/services/api/DatagouvfrAPI.ts @@ -3,7 +3,9 @@ import { toast } from 'vue3-toastify' import { useLoading } from 'vue-loading-overlay' import config from '@/config' +import type { Response } from '@/model' +// FIXME: the client should not depend be aware of the store. import { useUserStore } from '../../store/UserStore' const $loading = useLoading() @@ -21,7 +23,7 @@ instance.interceptors.request.use( return config }, - (error) => Promise.reject(error) + async (error) => await Promise.reject(error) ) /** @@ -73,14 +75,16 @@ export default class DatagouvfrAPI { */ async makeRequestAndHandleResponse(url, method = 'get', params = {}) { const loader = $loading.show() - return this.request(url, method, params) + return await this.request(url, method, params) .catch((error) => { - if (error && error.message) { + if (error?.message) { toast(error.message, { type: 'error', autoClose: false }) // TODO: Refacto to handle the error return error.response } }) - .finally(() => loader.hide()) + .finally(() => { + loader.hide() + }) } /** @@ -141,6 +145,23 @@ export default class DatagouvfrAPI { ) } + /** + * Base function for HTTP calls (without error handling) + * + * @todo Remove this function once all API calls are all handled this way: + * leaving the error handling to the caller (store). + * + * @param {string} url + * @param {object} data + * @returns {Promise} + */ + async _create(url: string, data = {}): Promise { + return await this.httpClient.post(url, data).then( + (response) => response, + (error) => this.#handleError(error) + ) + } + /** * Update an entity (PUT) * @@ -160,17 +181,19 @@ export default class DatagouvfrAPI { * Delete an entity (DELETE) * * @param {string} entityId - A UUID entity id - * @returns {Promise} + * @returns {Promise} */ - async delete(entityId) { - return this.httpClient.delete(`${this.url()}/${entityId}/`).then( + async delete(entityId: string): Promise { + return await this.httpClient.delete(`${this.url()}/${entityId}/`).then( (response) => response, (error) => this.#handleError(error) ) } - #handleError({ response, message }) { - if (response) return { status: response.status } - return { message } + #handleError({ response, message }): Response { + return { + ...(response && { status: response.status }), + ...(message && { error: { message } }) + } } } diff --git a/src/services/api/__tests__/DatagouvfrAPI.test.js b/src/services/api/__tests__/DatagouvfrAPI.test.js index 74d751de4..fdafe271e 100644 --- a/src/services/api/__tests__/DatagouvfrAPI.test.js +++ b/src/services/api/__tests__/DatagouvfrAPI.test.js @@ -80,8 +80,8 @@ test('delete when 404', async ({ client }) => { }) test('delete something else', async ({ client }) => { - const { message } = await client.delete(networkError) - expect(message).toMatch(/network error/i) + const { error } = await client.delete(networkError) + expect(error.message).toMatch(/network error/i) }) test('raw list', async ({ client }) => { diff --git a/src/services/api/__tests__/DiscussionsAPI.test.js b/src/services/api/__tests__/DiscussionsAPI.test.js index a2296e173..9269d7bfb 100644 --- a/src/services/api/__tests__/DiscussionsAPI.test.js +++ b/src/services/api/__tests__/DiscussionsAPI.test.js @@ -11,23 +11,24 @@ import { test } from 'vitest' -import DiscussionsAPI from '@/services/api/DiscussionsAPI' +import DiscussionsAPI from '@/services/api/resources/DiscussionsAPI' const baseUrl = 'https://example.lol' const version = '1234' const endpoint = 'discussions' -const datasetId = 'your-dataset-id' -const page = 1 -const server = setupServer( - http.get(`${baseUrl}/${version}/${endpoint}/?for=${datasetId}&page=${page}`, () => { - return HttpResponse.json([], { status: 200 }) - }), +const discussionRequest = { + title: 'Title of the discussion', + comment: 'This is a discussion.', + subject: { + class: 'Topic', + id: 'id123' + } +} - http.post(`${baseUrl}/${version}/${endpoint}/`, (req, res, ctx) => { - const data = req.body - // Assuming you have some logic to validate the request and return a response - return res(ctx.json(data), ctx.status(201)) +const server = setupServer( + http.post(`${baseUrl}/${version}/${endpoint}/`, () => { + return HttpResponse.json(discussionRequest, { status: 200 }) }) ) @@ -35,14 +36,14 @@ beforeAll(() => { server.listen() }) -beforeEach(() => { +beforeEach(async (context) => { const httpClient = axios.create() httpClient.defaults.proxy = false setActivePinia(createPinia()) context.client = new DiscussionsAPI({ baseUrl, version, - httpClient, + httpClient }) }) @@ -54,22 +55,7 @@ afterAll(() => { server.close() }) -test('get a discussion', async ({ client }) => { - // TODO: Test on get a discussion -}) - test('create a discussion', async ({ client }) => { - const discussionData = { - title: 'Title of the discussion', - comment: 'This is a discussion.', - subject: { - class: 'Topic', - id: 'id123' - } - } - - expect(response.id).toBeDefined() - expect(response.title).toEqual(discussionData.title) - expect(response.comment).toEqual(discussionData.comment) - expect(response.subject).toEqual(discussionData.subject) + const { data } = await client.create(discussionRequest) + expect(data.title).toEqual(discussionRequest.title) }) diff --git a/src/services/api/resources/DiscussionsAPI.ts b/src/services/api/resources/DiscussionsAPI.ts index 92d1b22a5..fad5ada72 100644 --- a/src/services/api/resources/DiscussionsAPI.ts +++ b/src/services/api/resources/DiscussionsAPI.ts @@ -1,5 +1,6 @@ +import type { DiscussionParams, DiscussionResponse } from '@/model' + import DatagouvfrAPI from '../DatagouvfrAPI' -import type { DiscussionRequest } from '@/model' export default class DiscussionsAPI extends DatagouvfrAPI { endpoint = 'discussions' @@ -17,24 +18,12 @@ export default class DiscussionsAPI extends DatagouvfrAPI { } /** - * Base function for HTTP calls (without error handling) + * Create a discussion (POST) * - * @param {string} url - * @param {string} method - * @param {object} params - * @returns {Promise} - */ - async _request(url, method = 'get', params = {}) { - return await this.httpClient[method](url, params) - } - - /** - * Create an discussion (POST) - * - * @param {object} data - * @returns {object} + * @param {DiscussionParams} data + * @returns {Promise} */ - async create(data: DiscussionRequest): Promise { - return await this.request(`${this.url()}/`, 'post', data) + async create(data: DiscussionParams): Promise { + return await this._create(`${this.url()}/`, data) } } diff --git a/src/views/bouquets/BouquetEditView.vue b/src/views/bouquets/BouquetEditView.vue index c60e0dbac..663fe6eb8 100644 --- a/src/views/bouquets/BouquetEditView.vue +++ b/src/views/bouquets/BouquetEditView.vue @@ -109,7 +109,9 @@ const availabilityEnum = { } const goToDatasetPage = (id) => { - const url = config.website.menu_items.find((link) => link.linkPage === '/datasets') + const url = config.website.menu_items.find( + (link) => link.linkPage === '/datasets' + ) return `${url.linkPage}/${id}` } @@ -515,8 +517,8 @@ onMounted(() => { ]" > @@ -648,8 +650,8 @@ onMounted(() => { ]" >