From b005149994e0f2002cc3bbc6a7fdfc96e2d0a0e9 Mon Sep 17 00:00:00 2001 From: Hongwei Date: Mon, 17 Aug 2026 15:57:25 +0200 Subject: [PATCH 1/2] fix: three calls to endpoints OBP-API does not serve Each was found by resolving every call site in this repository against OBP-API's published resource-docs, and each was confirmed against a live develop-obp (3df73fe11) with the verb held constant, so none of them is the 404-for-a-wrong-verb ambiguity. Portal, accepting a user invitation (#5) obp_requests.put -> post. /obp/v4.0.0/banks/{id}/user-invitation is POST-only at v4.0.0 and v6.0.0 alike; PUT answers 404 OBP-10404 while POST answers 401. The same file already POSTs the sibling /user-invitations path correctly, and API-Manager POSTs this one correctly, so the convention was established in two other places and only this call site diverged. Fixing the verb alone did not make the flow work, which the new test caught: the redirect was thrown inside the try, and the catch-all below it turns anything that is not an OBPRequestError into "Failed to create account: Unknown error". redirect() signals by throwing, so a successful account creation reported failure. The redirect now returns after the try/catch, matching what the register action already does. Portal, removing a chat reaction (#6) The emoji moves from the DELETE body into the path: OBP-API serves .../reactions/{emoji} for DELETE and only POST/GET on .../reactions. The response is now checked with res.ok, mirroring the add branch immediately below it. fetch does not reject on 4xx, so the old try/catch could not see the failure -- the reaction vanished from the UI, stayed in the database, and returned on reload with nothing logged. The add path was already correct and is untouched. API-Manager, creating a custom view (#7) routes/backend/obp/banks/[bank_id]/views/+server.ts is deleted rather than repaired. It targets /obp/v6.0.0/banks/{id}/views, which OBP-API does not serve at any verb -- every view endpoint is account-scoped -- and nothing calls it: the custom-view page posts to the account-scoped route next to it, which builds the correct URL. It was a copy of a working route with the account segment dropped. Adds apps/portal/src/routes/user-invitation/page.server.test.ts, following the pattern of the register action's test. It asserts the verb rather than only the URL, which is what lets it fail if this regresses, and it is what surfaced the swallowed redirect. Verified: portal 86/86 unit tests pass, api-manager 15/15. svelte-check is unchanged against the base commit on both apps (portal 21 errors/70 warnings, api-manager 22/332), so no new type errors. Both apps build. --- .../obp/banks/[bank_id]/views/+server.ts | 129 ------------------ .../user/chat/[chatRoomId]/+page.svelte | 14 +- .../routes/user-invitation/+page.server.ts | 14 +- .../user-invitation/page.server.test.ts | 88 ++++++++++++ 4 files changed, 107 insertions(+), 138 deletions(-) delete mode 100644 apps/api-manager/src/routes/backend/obp/banks/[bank_id]/views/+server.ts create mode 100644 apps/portal/src/routes/user-invitation/page.server.test.ts diff --git a/apps/api-manager/src/routes/backend/obp/banks/[bank_id]/views/+server.ts b/apps/api-manager/src/routes/backend/obp/banks/[bank_id]/views/+server.ts deleted file mode 100644 index aaf0e280..00000000 --- a/apps/api-manager/src/routes/backend/obp/banks/[bank_id]/views/+server.ts +++ /dev/null @@ -1,129 +0,0 @@ -import { json } from "@sveltejs/kit"; -import type { RequestHandler } from "./$types"; -import { obp_requests } from "$lib/obp/requests"; -import { obpErrorResponse } from "$lib/obp/errors"; -import { SessionOAuthHelper } from "$lib/oauth/sessionHelper"; -import { createLogger } from '@obp/shared/utils'; - -const logger = createLogger("CustomViewsAPI"); - -export const POST: RequestHandler = async ({ locals, params, request }) => { - const session = locals.session; - - if (!session?.data?.user) { - return json({ message: "Unauthorized", code: 401 }, { status: 401 }); - } - - // Get the OAuth session data - const sessionOAuth = SessionOAuthHelper.getSessionOAuth(session); - const accessToken = sessionOAuth?.accessToken; - - if (!accessToken) { - logger.warn("No access token available for custom view creation"); - return json({ message: "No API access token available", code: 401 }, { status: 401 }); - } - - const { bank_id } = params; - - if (!bank_id) { - return json({ message: "Bank ID is required", code: 400 }, { status: 400 }); - } - - try { - const body = await request.json(); - - logger.info("=== CREATE CUSTOM VIEW API CALL ==="); - logger.info(`Bank ID: ${bank_id}`); - logger.info(`View Name: ${body.name}`); - - // Build the payload for the OBP API - const payload: any = { - name: body.name, - description: body.description, - is_public: body.is_public || false, - metadata_view: body.metadata_view || "_0", - which_alias_to_use: body.which_alias_to_use || "", - hide_metadata_if_alias_used: body.hide_metadata_if_alias_used || false, - allowed_actions: body.allowed_actions || [], - }; - - // Add all permission fields that are enabled - const permissionFields = [ - // Transaction permissions - "can_see_transaction_this_bank_account", - "can_see_transaction_other_bank_account", - "can_see_transaction_metadata", - "can_see_transaction_label", - "can_see_transaction_amount", - "can_see_transaction_type", - "can_see_transaction_currency", - "can_see_transaction_start_date", - "can_see_transaction_finish_date", - "can_see_transaction_balance", - // Account permissions - "can_see_bank_account_owners", - "can_see_bank_account_type", - "can_see_bank_account_balance", - "can_see_bank_account_currency", - "can_see_bank_account_label", - "can_see_bank_account_national_identifier", - "can_see_bank_account_swift_bic", - "can_see_bank_account_iban", - "can_see_bank_account_number", - "can_see_bank_account_bank_name", - "can_see_bank_account_credit_limit", - // Counterparty permissions - "can_see_other_account_national_identifier", - "can_see_other_account_swift_bic", - "can_see_other_account_iban", - "can_see_other_account_bank_name", - "can_see_other_account_number", - "can_see_other_account_metadata", - "can_see_other_account_kind", - "can_see_public_alias", - "can_see_private_alias", - // Other permissions - "can_see_comments", - "can_see_narrative", - "can_see_tags", - "can_see_images", - "can_see_more_info", - "can_see_url", - "can_see_image_url", - "can_see_where_tag", - // Write permissions - "can_add_comment", - "can_delete_comment", - "can_add_tag", - "can_delete_tag", - "can_add_image", - "can_delete_image", - "can_edit_narrative", - "can_create_counterparty", - "can_add_transaction_request_to_own_account", - "can_add_transaction_request_to_any_account", - ]; - - // Add each permission field if it exists in the request body - permissionFields.forEach((field) => { - if (body[field] !== undefined) { - payload[field] = body[field]; - } - }); - - // Make the API call to create the custom view - const endpoint = `/obp/v6.0.0/banks/${bank_id}/views`; - logger.info(`Creating custom view at: ${endpoint}`); - - const response = await obp_requests.post(endpoint, payload, accessToken); - - logger.info(`Custom view created successfully: ${response.id}`); - - return json(response, { status: 201 }); - } catch (err) { - logger.error("Error creating custom view:", err); - - const { body, status } = obpErrorResponse(err); - return json(body, { status }); - } -}; diff --git a/apps/portal/src/routes/(protected)/user/chat/[chatRoomId]/+page.svelte b/apps/portal/src/routes/(protected)/user/chat/[chatRoomId]/+page.svelte index f7562088..200e81c7 100644 --- a/apps/portal/src/routes/(protected)/user/chat/[chatRoomId]/+page.svelte +++ b/apps/portal/src/routes/(protected)/user/chat/[chatRoomId]/+page.svelte @@ -177,11 +177,17 @@ // Optimistic remove reactions[messageId] = existing.filter(r => !(r.emoji === emoji && r.user_id === data.currentUserId)); try { - await fetch(`/proxy/obp/v6.0.0/chat-rooms/${data.chatRoom.chat_room_id}/messages/${messageId}/reactions`, { - method: 'DELETE', - headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ emoji }) + // The emoji is the last path segment on DELETE, not a body field -- the API + // serves .../reactions/{emoji}, and .../reactions is only POST and GET. + const res = await fetch(`/proxy/obp/v6.0.0/chat-rooms/${data.chatRoom.chat_room_id}/messages/${messageId}/reactions/${encodeURIComponent(emoji)}`, { + method: 'DELETE' }); + if (!res.ok) { + // Revert on failure. fetch only rejects on a network error, so without + // this check a 4xx left the reaction gone from the UI, still in the + // database, and back again on reload -- with nothing logged anywhere. + reactions[messageId] = [...(reactions[messageId] || []), myReaction]; + } } catch { // Revert on failure reactions[messageId] = [...(reactions[messageId] || []), myReaction]; diff --git a/apps/portal/src/routes/user-invitation/+page.server.ts b/apps/portal/src/routes/user-invitation/+page.server.ts index fb757471..38f27650 100644 --- a/apps/portal/src/routes/user-invitation/+page.server.ts +++ b/apps/portal/src/routes/user-invitation/+page.server.ts @@ -187,7 +187,7 @@ export const actions = { try { // Option B: Create user account directly via REST API - // PUT /obp/v4.0.0/banks/{BANK_ID}/user-invitation + // POST /obp/v4.0.0/banks/{BANK_ID}/user-invitation const requestBody: OBPUserInvitationAcceptRequestBody = { secret_key: parseInt(secretKey), username: username.trim(), @@ -205,16 +205,13 @@ export const actions = { email: email }); - const response = await obp_requests.put( + const response = await obp_requests.post( `/obp/v4.0.0/banks/${effectiveBankId}/user-invitation`, requestBody ); logger.info("User account created successfully via invitation acceptance:", response); - // Redirect to login page with success message - throw redirect(303, '/login?invitation_accepted=true'); - } catch (err) { if (err instanceof OBPRequestError) { logger.error("OBP API error during invitation acceptance:", err.message, err.code); @@ -260,5 +257,12 @@ export const actions = { success: false }; } + + // Outside the try on purpose. `redirect()` signals by throwing, so a redirect + // raised inside the block above is caught by the same catch, falls through to its + // catch-all, and turns a successful account creation into + // "Failed to create account: Unknown error". The register action already returns + // its redirect after the try for this reason. + return redirect(303, '/login?invitation_accepted=true'); } } satisfies Actions; \ No newline at end of file diff --git a/apps/portal/src/routes/user-invitation/page.server.test.ts b/apps/portal/src/routes/user-invitation/page.server.test.ts new file mode 100644 index 00000000..38d9c50f --- /dev/null +++ b/apps/portal/src/routes/user-invitation/page.server.test.ts @@ -0,0 +1,88 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { actions } from './+page.server.js'; + +// Mock the obp_requests module the action calls through. Both verbs are stubbed so the +// test can assert which one was used -- that is the whole point of the first case. +vi.mock('$lib/obp/requests', () => ({ + obp_requests: { + post: vi.fn(), + put: vi.fn() + } +})); + +vi.mock('$env/dynamic/public', () => ({ env: { PUBLIC_DEFAULT_BANK_ID: 'gh.29.uk' } })); +vi.mock('$env/dynamic/private', () => ({ env: {} })); + +import { obp_requests } from '$lib/obp/requests'; + +describe('user-invitation accept action', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + const createMockRequest = (formData: Record) => { + const mockFormData = new FormData(); + Object.entries(formData).forEach(([key, value]) => { + mockFormData.append(key, value); + }); + return { formData: () => Promise.resolve(mockFormData) }; + }; + + const validForm = { + secret_key: '12345', + username: 'inviteduser', + password: 'password123', + confirm_password: 'password123', + first_name: 'Jane', + last_name: 'Doe', + email: 'jane@example.com', + company: 'Example Ltd', + country: 'DE', + privacy_policy: 'on', + terms_conditions: 'on', + personal_data: 'on', + bank_id: 'gh.29.uk' + }; + + const run = (form: Record) => + actions.accept({ request: createMockRequest(form) } as never); + + // The regression this file exists for. OBP-API serves + // /obp/v4.0.0/banks/{BANK_ID}/user-invitation as POST only -- there is no PUT on that + // path at any version, so the previous `obp_requests.put` could only ever 404 and + // accepting an invitation could never complete. Asserting the verb, not just the URL, + // is what makes this test able to fail if it regresses. + it('accepts an invitation with POST, not PUT', async () => { + vi.mocked(obp_requests.post).mockResolvedValue({ user_id: 'user-123' }); + + // On success the action throws a SvelteKit redirect to the login page. + await expect(run(validForm)).rejects.toMatchObject({ + status: 303, + location: '/login?invitation_accepted=true' + }); + + expect(obp_requests.put).not.toHaveBeenCalled(); + expect(obp_requests.post).toHaveBeenCalledWith( + '/obp/v4.0.0/banks/gh.29.uk/user-invitation', + expect.objectContaining({ + secret_key: 12345, + username: 'inviteduser', + email: 'jane@example.com' + }) + ); + }); + + it('rejects a password shorter than the minimum without calling the API', async () => { + const result = await run({ ...validForm, password: 'short', confirm_password: 'short' }); + + expect(result).toMatchObject({ success: false }); + expect(obp_requests.post).not.toHaveBeenCalled(); + }); + + it('rejects mismatched password confirmation without calling the API', async () => { + const result = await run({ ...validForm, confirm_password: 'different123' }); + + expect(result).toMatchObject({ success: false }); + expect(obp_requests.post).not.toHaveBeenCalled(); + }); +}); From 9c8132f8a1d40b806aa98555ba08f5c99eb8350f Mon Sep 17 00:00:00 2001 From: Hongwei Date: Mon, 17 Aug 2026 16:30:31 +0200 Subject: [PATCH 2/2] test: cover the chat-reaction removal, the one fix that had none The reaction fix shipped without a test, which left the least visible of the three defects the least protected: fetch resolves on a 4xx, so the failure it guards against produces no error anywhere -- the reaction vanishes from the UI, stays in the database, and returns on reload. Three cases, rendered against the real component: * removal sends the emoji as the last path segment and no body; * a failed removal restores the badge, which is the res.ok check; * addition still POSTs with the emoji in the body, unchanged -- that path was already correct and the test exists to keep it that way. Both new cases were run against the pre-fix code and fail there: the first on the URL, the second on the badge staying gone. The third passes either way, as it should, since the add path was not touched. A test that has not been shown to fail does not establish anything when it passes. $env/dynamic/public is mocked because the component pulls in $lib/avatar/generate, which reads it at module scope and throws under vitest otherwise. portal is now 89/89 across 11 files. --- .../unit/routes/chat.reactions.svelte.test.ts | 116 ++++++++++++++++++ 1 file changed, 116 insertions(+) create mode 100644 apps/portal/test/unit/routes/chat.reactions.svelte.test.ts diff --git a/apps/portal/test/unit/routes/chat.reactions.svelte.test.ts b/apps/portal/test/unit/routes/chat.reactions.svelte.test.ts new file mode 100644 index 00000000..8d6ae6bf --- /dev/null +++ b/apps/portal/test/unit/routes/chat.reactions.svelte.test.ts @@ -0,0 +1,116 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { render, screen } from '@testing-library/svelte'; + +// The component pulls in $lib/avatar/generate, which reads $env/dynamic/public at module +// scope. Unmocked, that is undefined under vitest and the import throws before any test +// runs. +vi.mock('$env/dynamic/public', () => ({ env: {} })); + +import ChatRoomPage from '$lib/../routes/(protected)/user/chat/[chatRoomId]/+page.svelte'; + +// The component opens an EventSource and a poll interval on mount; jsdom has neither. +class FakeEventSource { + onmessage: ((e: MessageEvent) => void) | null = null; + onerror: ((e: Event) => void) | null = null; + close() {} + addEventListener() {} +} + +const CURRENT_USER = 'user-me'; +const MESSAGE_ID = 'msg-1'; + +function makeData() { + return { + chatRoom: { chat_room_id: 'room-1', name: 'Test room' }, + currentUserId: CURRENT_USER, + participants: [{ user_id: CURRENT_USER, username: 'me' }], + messages: [ + { + chat_message_id: MESSAGE_ID, + message: 'hello', + from_user_id: CURRENT_USER, + created_at: '2026-01-01T00:00:00Z', + // A reaction this user already left, so the badge click removes it. + reactions: [{ emoji: '👍', user_ids: [CURRENT_USER] }] + } + ] + } as never; +} + +describe('chat reactions', () => { + let fetchMock: ReturnType; + + beforeEach(() => { + document.body.innerHTML = ''; + vi.stubGlobal('EventSource', FakeEventSource); + fetchMock = vi.fn().mockResolvedValue({ ok: true, json: async () => ({}) }); + vi.stubGlobal('fetch', fetchMock); + }); + + afterEach(() => { + vi.unstubAllGlobals(); + vi.restoreAllMocks(); + document.body.innerHTML = ''; + }); + + const clickBadge = async () => { + const badge = await screen.findByTestId(`reaction-badge-${MESSAGE_ID}-👍`); + badge.click(); + // let the click handler's await settle + await new Promise((r) => setTimeout(r, 0)); + }; + + const reactionCalls = () => + fetchMock.mock.calls.filter(([url]) => String(url).includes('/reactions')); + + // The regression this file exists for. OBP-API serves the emoji as the last PATH + // segment on DELETE -- `.../reactions/{emoji}` -- and only POST/GET on + // `.../reactions`. Sending it in the body meant the request 404'd every time. + it('removes a reaction with the emoji in the path, not the body', async () => { + render(ChatRoomPage, { data: makeData() }); + await clickBadge(); + + const calls = reactionCalls(); + expect(calls.length).toBe(1); + + const [url, init] = calls[0]; + expect(init.method).toBe('DELETE'); + expect(String(url)).toBe( + `/proxy/obp/v6.0.0/chat-rooms/room-1/messages/${MESSAGE_ID}/reactions/${encodeURIComponent('👍')}` + ); + // A body on this DELETE is what the endpoint does not read. + expect(init.body).toBeUndefined(); + }); + + // fetch resolves on a 4xx rather than rejecting, so a try/catch alone cannot see the + // failure. Before res.ok was checked, a rejected removal left the reaction gone from + // the UI, still in the database, and back again on reload -- with nothing logged. + it('restores the reaction in the UI when the request fails', async () => { + fetchMock.mockResolvedValue({ ok: false, status: 404, json: async () => ({}) }); + render(ChatRoomPage, { data: makeData() }); + + await clickBadge(); + await new Promise((r) => setTimeout(r, 0)); + + expect(screen.queryByTestId(`reaction-badge-${MESSAGE_ID}-👍`)).not.toBeNull(); + }); + + // The add path was already correct and must stay that way: POST to `.../reactions` + // with the emoji in the body is exactly what OBP-API serves. + it('adds a reaction with the emoji in the body, unchanged', async () => { + const data = makeData(); + // Nobody has reacted yet, so the same badge click adds rather than removes. + (data as never as { messages: Array<{ reactions: unknown[] }> }).messages[0].reactions = [ + { emoji: '👍', user_ids: ['someone-else'] } + ]; + render(ChatRoomPage, { data }); + await clickBadge(); + + const [url, init] = reactionCalls()[0]; + expect(init.method).toBe('POST'); + expect(String(url)).toBe( + `/proxy/obp/v6.0.0/chat-rooms/room-1/messages/${MESSAGE_ID}/reactions` + ); + expect(JSON.parse(init.body)).toEqual({ emoji: '👍' }); + }); +});