diff --git a/src/__tests__/controllers/UserController.test.ts b/src/__tests__/controllers/UserController.test.ts new file mode 100644 index 0000000..f609712 --- /dev/null +++ b/src/__tests__/controllers/UserController.test.ts @@ -0,0 +1,81 @@ +import { describe, it, expect, beforeEach, jest } from '@jest/globals'; +import { UserController } from '../../controllers/UserController'; +import { UserEventEnum } from '../../types/user'; + +const DAY = 24 * 60 * 60 * 1000; + +function makeRes() { + const res: any = {}; + res.status = jest.fn().mockReturnValue(res); + res.json = jest.fn().mockReturnValue(res); + return res; +} + +describe('UserController.secondOnboarding', () => { + let userService: any; + let subscriptionService: any; + let controller: UserController; + + beforeEach(() => { + userService = { + getLastUserEvent: jest.fn(), + getUserEventCount: jest.fn(), + getSecondOnboardings: jest.fn(), + }; + subscriptionService = { + hasInAppPurchase: jest.fn(), + }; + controller = new UserController(userService, subscriptionService); + }); + + /// USA user, first_seen ~200 days ago, so we land in the post-skip + /// (support_paywall) branch of handleDefaultOnboarding. + const baseBody = () => ({ + rc_id: 'rc_123', + region: 'USA', + app_version: '5.5.1', + first_seen: Math.floor((Date.now() - 200 * DAY) / 1000), + onboarding_name: 'first_seen', + }); + + it('does not 500 when the user skipped but has no START event (regression: BOOKPLAYER-TSZ)', async () => { + // Reproduces the production crash: a SKIP exists but its START was never + // recorded (dropped client event), so getLastUserEvent(START) is null. + const oldSkip = { created_at: new Date(Date.now() - 200 * DAY) }; + userService.getLastUserEvent.mockImplementation(async ({ event_name }: any) => + event_name === UserEventEnum.SECOND_ONBOARDING_SKIP ? oldSkip : null, + ); + userService.getUserEventCount.mockResolvedValue(0); + userService.getSecondOnboardings.mockResolvedValue([{ title: 'support' }]); + subscriptionService.hasInAppPurchase.mockResolvedValue(false); + + const res = makeRes(); + + // Before the null-guard this threw and surfaced as a 500. + await expect( + controller.secondOnboarding({ body: baseBody() } as any, res), + ).resolves.toBeDefined(); + + expect(userService.getSecondOnboardings).toHaveBeenCalledWith({ + onboarding_name: 'support_paywall', + }); + expect(res.json).toHaveBeenCalledWith([{ title: 'support' }]); + }); + + it('still interrupts (returns {}) when a START was shown within the last 2 days', async () => { + // Guards that adding `lastEvent != null` did not weaken the original + // "shown too recently" short-circuit when a START does exist. + const oldSkip = { created_at: new Date(Date.now() - 200 * DAY) }; + const recentStart = { created_at: new Date() }; + userService.getLastUserEvent.mockImplementation(async ({ event_name }: any) => + event_name === UserEventEnum.SECOND_ONBOARDING_SKIP ? oldSkip : recentStart, + ); + subscriptionService.hasInAppPurchase.mockResolvedValue(false); + + const res = makeRes(); + await controller.secondOnboarding({ body: baseBody() } as any, res); + + expect(res.json).toHaveBeenCalledWith({}); + expect(userService.getSecondOnboardings).not.toHaveBeenCalled(); + }); +}); diff --git a/src/api/RouterHttp.ts b/src/api/RouterHttp.ts index 1f5263c..d8e9b81 100644 --- a/src/api/RouterHttp.ts +++ b/src/api/RouterHttp.ts @@ -1,7 +1,6 @@ import express from 'express'; import UserRouter from './UserRouter'; import LibraryRouter from './LibraryRouter'; -import StorageRouter from './StorageRouter'; import RetentionMessagingRouter from './RetentionMessagingRouter'; import PasskeyRouter from './PasskeyRouter'; @@ -11,7 +10,6 @@ router.get('/status', (req, res) => res.send('OK')); router.use('/user', UserRouter); router.use('/passkey', PasskeyRouter); router.use('/library', LibraryRouter); -router.use('/storage', StorageRouter); router.use('/retention', RetentionMessagingRouter); export default router; diff --git a/src/api/StorageRouter.ts b/src/api/StorageRouter.ts deleted file mode 100644 index 08eb110..0000000 --- a/src/api/StorageRouter.ts +++ /dev/null @@ -1,12 +0,0 @@ -import express from 'express'; -import { StorageController } from '../controllers/StorageController'; -import { checkSubscription } from './middlewares/subscription'; - -const StorageRouter = express.Router(); -const controller = new StorageController(); - -StorageRouter.get('/*', checkSubscription, (req, res, next) => - controller.getProxyLibrary(req, res).catch(next), -); - -export default StorageRouter; diff --git a/src/api/middlewares/error.ts b/src/api/middlewares/error.ts index a5d7f4a..fdeb11b 100644 --- a/src/api/middlewares/error.ts +++ b/src/api/middlewares/error.ts @@ -1,5 +1,6 @@ /* eslint-disable @typescript-eslint/no-unused-vars */ import { IRequest, IResponse, INext } from '../../types/http'; +import { logger } from '../../services/LoggerService'; class HttpException extends Error { status: number; @@ -13,12 +14,25 @@ class HttpException extends Error { export const handleError = ( error: HttpException, - _req: IResponse, - res: IRequest, + req: IRequest, + res: IResponse, _next: INext, ) => { const status = error.status || 500; const message = error.message || 'Something went wrong'; + /// Uncaught errors reach here with no status (defaulted to 500). Log them — + /// otherwise handlers that rely on `.catch(next)` (e.g. UserController) 500 + /// silently. Intentional 4xx HttpExceptions are left unlogged. + if (status >= 500) { + logger.log( + { + origin: 'handleError', + message, + data: { method: req.method, url: req.originalUrl, stack: error.stack }, + }, + 'error', + ); + } return res.status(status).send({ status, message, diff --git a/src/controllers/StorageController.ts b/src/controllers/StorageController.ts deleted file mode 100644 index 61eae3c..0000000 --- a/src/controllers/StorageController.ts +++ /dev/null @@ -1,95 +0,0 @@ -import { IRequest, IResponse } from '../types/http'; -import { StorageService } from '../services/StorageService'; -import { StoragePrefixService } from '../services/StoragePrefixService'; -import { LibraryDB } from '../services/db/LibraryDB'; -import { Writable } from 'stream'; -import { S3ClientHeaders, S3ValidHeader } from '../types/user'; -import { logger } from '../services/LoggerService'; - -export class StorageController { - private readonly _loggerService = logger; - - constructor( - private _storageService: StorageService = new StorageService(), - private _libraryDB: LibraryDB = new LibraryDB(), - private _prefix: StoragePrefixService = new StoragePrefixService(), - ) {} - - public async getProxyLibrary( - req: IRequest, - res: IResponse, - ): Promise { - const user = req.user; - const key = req.params[0]; - const pathArray = key?.split('/') || []; - const rootFolder = pathArray[0]; - let filepath: string; - switch (rootFolder) { - case '_thumbnail': - const itemThumbnail = await this._libraryDB.getItemByThumbnail( - user.id_user, - pathArray.slice(1)?.join('/'), - ); - filepath = !!itemThumbnail - ? `${rootFolder}/${itemThumbnail.thumbnail}` - : null; - break; - default: - const userItemDB = await this._libraryDB.getLibrary( - user.id_user, - key, - { exactly: true }, - ); - filepath = userItemDB?.length - ? `/${userItemDB[0].source_path || userItemDB[0].key}` - : null; - break; - } - if (!filepath) { - return res.status(400).json({ - message: 'Invalid request', - }); - } - try { - const clientHeaders: S3ClientHeaders = {}; - const reqHeaders = Object.keys(req.headers).reduce( - (headers: { [k: string]: string | Date }, headerKey: string) => { - headers[headerKey.toLowerCase()] = req.headers[headerKey]; - return headers; - }, - {}, - ); - for (const [key, value] of Object.entries(S3ValidHeader)) { - const validValue = reqHeaders[key]; - if (validValue) { - clientHeaders[value] = validValue; - } - } - const storagePrefix = await this._prefix.getPrefix(user); - const userKey = `${storagePrefix}${filepath}`; - const s3Stream = await this._storageService.getObjectStream({ - key: userKey, - clientHeaders, - }); - if (s3Stream?.body) { - Object.keys(s3Stream.headers)?.forEach((headerKey: string) => { - res.setHeader(headerKey, s3Stream.headers[headerKey]); - }); - res.setHeader('Cross-Origin-Resource-Policy', 'cross-origin'); - res.statusCode = s3Stream.statusCode || 200; - s3Stream?.body.pipe(res as Writable); - } else { - return res.status(s3Stream.statusCode).end(); - } - } catch (error) { - this._loggerService.log({ - origin: 'getProxyLibrary', - error: { - user, - message: error.message, - }, - }); - return res.status(500).json({ error: 'Error fetching the file' }); - } - } -} diff --git a/src/controllers/UserController.ts b/src/controllers/UserController.ts index 6f482b7..419118f 100644 --- a/src/controllers/UserController.ts +++ b/src/controllers/UserController.ts @@ -291,6 +291,7 @@ export class UserController { /// if the last time it was shown was less than two days ago, interrupt process if ( + lastEvent != null && !moment(lastEvent.created_at).isBefore(moment().subtract(2, 'days')) ) { return res.json({}); diff --git a/src/services/UserServices.ts b/src/services/UserServices.ts index 18cb74e..f0323db 100644 --- a/src/services/UserServices.ts +++ b/src/services/UserServices.ts @@ -184,7 +184,7 @@ export class UserServices { event_name: UserEventEnum; user_id?: number; external_id?: string; - }): Promise { + }): Promise { return this._userDB.getLastUserEvent(params); } diff --git a/src/services/db/UserDB.ts b/src/services/db/UserDB.ts index c72ec65..c9a18a6 100644 --- a/src/services/db/UserDB.ts +++ b/src/services/db/UserDB.ts @@ -242,7 +242,7 @@ export class UserDB { external_id?: string; }, trx?: Knex.Transaction, - ): Promise { + ): Promise { try { const db = trx || this.db; const filter: { @@ -257,7 +257,7 @@ export class UserDB { .where(filter) .orderBy('created_at', 'desc') .first(); - return event; + return event || null; } catch (err) { this._logger.log({ origin: 'UserDB.getLastUserEvent',