From 4a11a94abc32dcfaed43661c3d7ac64d4fd8ad53 Mon Sep 17 00:00:00 2001 From: nourshoreibah Date: Thu, 13 Aug 2026 21:34:18 -0400 Subject: [PATCH 1/2] feat(expenditures): count only approved expenditures in totals Every SUM, chart series, budget percentage and report total counted every expenditure row regardless of status, so a pending or denied request inflated spend the moment it was entered and could show a project as over budget on expenditures nobody had signed off on. Filter to status = 'approved' in the dashboard aggregates, the project list cards, the project overview stats and the report data. The raw lists are deliberately left alone: the project expense table and the admin review queue exist to show what is awaiting review. The seed now sets statuses explicitly, keeping one denied and one pending row so local dev exercises both the review queue and the filter. Seed spend is unchanged at 18000, of which 14000 is approved. Co-Authored-By: Claude Opus 5 (1M context) --- apps/backend/db/seed.sql | 18 ++- apps/backend/lambdas/projects/handler.ts | 19 ++- apps/backend/lambdas/projects/openapi.yaml | 8 + .../test/approved-expenditures.e2e.test.ts | 153 ++++++++++++++++++ .../projects/test/projects.e2e.test.ts | 6 +- .../lambdas/projects/validation-utils.ts | 7 + apps/backend/lambdas/reports/jest.config.js | 3 + apps/backend/lambdas/reports/openapi.yaml | 10 +- .../backend/lambdas/reports/report-service.ts | 8 + .../reports/test/report-service.e2e.test.ts | 68 ++++++++ 10 files changed, 284 insertions(+), 16 deletions(-) create mode 100644 apps/backend/lambdas/projects/test/approved-expenditures.e2e.test.ts create mode 100644 apps/backend/lambdas/reports/test/report-service.e2e.test.ts diff --git a/apps/backend/db/seed.sql b/apps/backend/db/seed.sql index ae73432e..794d708c 100644 --- a/apps/backend/db/seed.sql +++ b/apps/backend/db/seed.sql @@ -42,13 +42,17 @@ INSERT INTO branch.project_memberships (project_id, user_id, role, start_date, h (1, 2, 'Director', '2025-02-01', 80.00), (2, 3, 'Student', '2025-03-15', 60.00); -INSERT INTO branch.expenditures (project_id, entered_by, amount, category, description, spent_on) VALUES -(1, 1, 5000, 'Travel', 'Domestic conference attendance', '2025-02-10'), -(1, 1, 4200, 'Travel Foreign', 'International collaborator meeting in London', '2025-03-22'), -(2, 2, 3000, 'General', 'Recording device supplies', '2025-04-05'), -(2, 2, 1500, 'Visitor / Honorarium', 'Guest lecturer honorarium', '2025-05-18'), -(3, 3, 2500, 'General', 'Educational materials', '2025-07-12'), -(3, 3, 1800, 'Travel', 'Local outreach travel', '2025-08-03'); +-- Statuses are load-bearing too. Totals, charts and reports count 'approved' +-- rows only, so the seed keeps one denied and one pending row: they give the +-- admin review queue something to show, and they keep the dashboard honest +-- about what it excludes. 14000 of the 18000 below is approved spend. +INSERT INTO branch.expenditures (project_id, entered_by, amount, category, description, status, spent_on) VALUES +(1, 1, 5000, 'Travel', 'Domestic conference attendance', 'approved', '2025-02-10'), +(1, 1, 4200, 'Travel Foreign', 'International collaborator meeting in London', 'approved', '2025-03-22'), +(2, 2, 3000, 'General', 'Recording device supplies', 'approved', '2025-04-05'), +(2, 2, 1500, 'Visitor / Honorarium', 'Guest lecturer honorarium', 'denied', '2025-05-18'), +(3, 3, 2500, 'General', 'Educational materials', 'pending', '2025-07-12'), +(3, 3, 1800, 'Travel', 'Local outreach travel', 'approved', '2025-08-03'); INSERT INTO branch.reports (project_id, title, object_url) VALUES (1, 'Clinician Communication Study Report', 'https://s3.amazonaws.com/branch-reports/clinician_communication_study_report.pdf'), diff --git a/apps/backend/lambdas/projects/handler.ts b/apps/backend/lambdas/projects/handler.ts index ecbe9bae..6af5d66b 100644 --- a/apps/backend/lambdas/projects/handler.ts +++ b/apps/backend/lambdas/projects/handler.ts @@ -2,7 +2,11 @@ import { APIGatewayProxyEvent, APIGatewayProxyResult } from 'aws-lambda'; import { sql, Transaction } from 'kysely'; import type { DB } from '@branch/types'; import db from './db'; -import { MemberAssignment, ProjectValidationUtils } from './validation-utils'; +import { + APPROVED_EXPENDITURE_STATUS, + MemberAssignment, + ProjectValidationUtils, +} from './validation-utils'; import { authenticateRequest, canAccessProject, @@ -80,6 +84,7 @@ export const handler = async (event: any): Promise => { ] = await Promise.all([ db.selectFrom('branch.expenditures') .select(db.fn.sum('amount').as('total')) + .where('status', '=', APPROVED_EXPENDITURE_STATUS) .where('spent_on', '>=', yearStart as any) .where('spent_on', '<=', yearEnd as any) .executeTakeFirst(), @@ -89,6 +94,7 @@ export const handler = async (event: any): Promise => { .executeTakeFirst(), db.selectFrom('branch.expenditures') .select(['category', db.fn.sum('amount').as('total')]) + .where('status', '=', APPROVED_EXPENDITURE_STATUS) .where('category', 'is not', null) .where('spent_on', '>=', yearStart as any) .where('spent_on', '<=', yearEnd as any) @@ -102,6 +108,7 @@ export const handler = async (event: any): Promise => { db.selectFrom('branch.expenditures as e') .innerJoin('branch.projects as p', 'p.project_id', 'e.project_id') .select((eb) => eb.fn.sum('e.amount').as('total')) + .where('e.status', '=', APPROVED_EXPENDITURE_STATUS) .where('e.spent_on', '>=', yearStart as any) .where('e.spent_on', '<=', yearEnd as any) .where(isActive('p.end_date')) @@ -115,6 +122,7 @@ export const handler = async (event: any): Promise => { eb.selectFrom('branch.expenditures') .select('project_id') .select((sub) => sub.fn.sum('amount').as('total')) + .where('status', '=', APPROVED_EXPENDITURE_STATUS) .groupBy('project_id') .as('spend'), (join) => join.onRef('spend.project_id', '=', 'p.project_id'), @@ -140,6 +148,7 @@ export const handler = async (event: any): Promise => { .execute(), db.selectFrom('branch.expenditures') .select([monthExpr.as('month'), 'category', db.fn.sum('amount').as('total')]) + .where('status', '=', APPROVED_EXPENDITURE_STATUS) .where('category', 'is not', null) .where('spent_on', '>=', yearStart as any) .where('spent_on', '<=', yearEnd as any) @@ -325,7 +334,10 @@ export const handler = async (event: any): Promise => { ]); const totalBudget = project.total_budget !== null ? Number(project.total_budget) : 0; - const totalSpent = expenditures.reduce((sum, e) => sum + Number(e.amount), 0); + // The table below lists every expenditure, including the ones still in + // review; the stats beside it count only what was approved. + const approved = expenditures.filter((e) => e.status === APPROVED_EXPENDITURE_STATUS); + const totalSpent = approved.reduce((sum, e) => sum + Number(e.amount), 0); // Guarded because a project may legitimately have no budget set yet, and // 0/0 would render as NaN% in the donut. const spentPercentage = totalBudget > 0 ? (totalSpent / totalBudget) * 100 : 0; @@ -339,7 +351,7 @@ export const handler = async (event: any): Promise => { spentPercentage: Number(spentPercentage.toFixed(2)), totalDonated: Number(donationRow?.total ?? 0), memberCount: members.length, - expenditureCount: expenditures.length, + expenditureCount: approved.length, }, members, expenditures, @@ -683,6 +695,7 @@ async function loadProjectAggregates(projectIds: number[]): Promise<{ db .selectFrom('branch.expenditures') .select(['project_id', db.fn.sum('amount').as('total')]) + .where('status', '=', APPROVED_EXPENDITURE_STATUS) .where('project_id', 'in', projectIds) .groupBy('project_id') .execute(), diff --git a/apps/backend/lambdas/projects/openapi.yaml b/apps/backend/lambdas/projects/openapi.yaml index 417b8230..8e7d11b3 100644 --- a/apps/backend/lambdas/projects/openapi.yaml +++ b/apps/backend/lambdas/projects/openapi.yaml @@ -81,6 +81,7 @@ paths: properties: total_spent: type: number + description: Approved expenditures only. member_count: type: integer is_active: @@ -142,6 +143,9 @@ paths: $ref: '#/components/schemas/Project' stats: type: object + description: >- + Counts approved expenditures only; `expenditures` below + still lists every row, including those under review. properties: totalBudget: type: number @@ -285,6 +289,10 @@ paths: /dashboard: get: summary: GET /dashboard + description: >- + Admin-only roll-up: this year's totals, the per-project breakdown and + the monthly expense series. Every spend figure counts approved + expenditures only. responses: '200': description: OK diff --git a/apps/backend/lambdas/projects/test/approved-expenditures.e2e.test.ts b/apps/backend/lambdas/projects/test/approved-expenditures.e2e.test.ts new file mode 100644 index 00000000..f7a0d9bf --- /dev/null +++ b/apps/backend/lambdas/projects/test/approved-expenditures.e2e.test.ts @@ -0,0 +1,153 @@ +/** + * Every figure the app reports as money spent must count approved + * expenditures only. A pending or denied row is a request, not a spend, so + * letting it into a total overstates the budget burn and can show a project + * as over budget on the strength of expenditures nobody signed off on. + * + * The raw lists (the project expense table, the admin review queue) stay + * unfiltered — they exist to show what is awaiting review. + */ +import { describe, test, expect, beforeAll, beforeEach, afterAll, jest } from '@jest/globals'; +import { Pool } from 'pg'; +import { ensureSchema, resetData } from '../../../db/testkit'; + +jest.mock('../auth', () => ({ + ...jest.requireActual('../auth'), + authenticateRequest: jest.fn(), +})); + +import { handler } from '../handler'; +import db from '../db'; +import { authenticateRequest } from '../auth'; + +const mockAuthenticateRequest = authenticateRequest as jest.MockedFunction; + +const adminAuthResult = { + isAuthenticated: true as const, + user: { cognitoSub: 'admin-sub', userId: 1, email: 'ashley@branch.org', isAdmin: true }, +}; + +const pool = new Pool({ + host: 'localhost', + port: 5432, + user: 'branch_dev', + password: 'password', + database: 'branch_db', + ssl: false, +}); + +beforeAll(async () => { + const client = await pool.connect(); + try { + await ensureSchema(client); + } finally { + client.release(); + } +}); + +// One project, one budget, one of each status. Every assertion below reduces +// to "did the 1000 survive and the other 3000 stay out". +beforeEach(async () => { + jest.clearAllMocks(); + mockAuthenticateRequest.mockResolvedValue(adminAuthResult); + + const client = await pool.connect(); + try { + await resetData(client); + await client.query('DELETE FROM branch.expenditures'); + await client.query(` + INSERT INTO branch.expenditures + (project_id, entered_by, amount, category, description, status, spent_on) + VALUES + (1, 1, 1000, 'Travel', 'approved', 'approved', CURRENT_DATE), + (1, 1, 1000, 'Travel', 'pending', 'pending', CURRENT_DATE), + (1, 1, 1000, 'Travel', 'denied', 'denied', CURRENT_DATE), + (1, 1, 1000, 'Travel', 'needs info', 'needs_more_info', CURRENT_DATE) + `); + await client.query(`UPDATE branch.projects SET end_date = '2099-12-31' WHERE end_date IS NOT NULL`); + } finally { + client.release(); + } +}); + +afterAll(async () => { + await pool.end(); + await db.destroy(); +}); + +function getEvent(rawPath: string) { + return { + rawPath, + requestContext: { http: { method: 'GET' } }, + headers: { Authorization: 'Bearer fake-token' }, + queryStringParameters: {}, + } as any; +} + +async function get(rawPath: string) { + const res = await handler(getEvent(rawPath)); + expect(res.statusCode).toBe(200); + return JSON.parse(res.body); +} + +describe('GET /dashboard counts approved expenditures only', () => { + test('totalSpent leaves out pending, denied and needs_more_info', async () => { + const body = await get('/dashboard'); + expect(body.summary.totalSpent).toBe(1000); + }); + + test('topExpenseCategory sums approved rows only', async () => { + const body = await get('/dashboard'); + expect(body.summary.topExpenseCategory).toEqual({ + category: 'Travel', + amount: 1000, + percentage: 100, + }); + }); + + test('averageSpendPerProject divides approved spend across active projects', async () => { + const body = await get('/dashboard'); + // 1000 approved over the 4 active seed projects. + expect(body.summary.averageSpendPerProject).toBe(250); + }); + + test('per-project spend on the dashboard cards is approved-only', async () => { + const body = await get('/dashboard'); + const p1 = body.projects.find((p: any) => p.project_id === 1); + expect(p1.spent).toBe(1000); + }); + + test('the expenses bar chart series is approved-only', async () => { + const body = await get('/dashboard'); + const total = body.expensesByMonth.reduce((sum: number, r: any) => sum + r.amount, 0); + expect(total).toBe(1000); + }); +}); + +describe('GET /projects counts approved expenditures only', () => { + test('total_spent on the list cards leaves out unapproved rows', async () => { + const projects = await get('/'); + const p1 = projects.find((p: any) => p.project_id === 1); + expect(Number(p1.total_spent)).toBe(1000); + }); +}); + +describe('GET /projects/{id}/overview counts approved expenditures only', () => { + test('stats are computed from approved rows', async () => { + const body = await get('/1/overview'); + expect(body.stats.totalSpent).toBe(1000); + expect(body.stats.totalRemaining).toBe(body.stats.totalBudget - 1000); + expect(body.stats.expenditureCount).toBe(1); + }); + + test('the expenditures table still lists every row so reviewers can see them', async () => { + const body = await get('/1/overview'); + expect(body.expenditures).toHaveLength(4); + expect(body.expenditures.map((e: any) => e.status).sort()).toEqual([ + 'approved', + 'denied', + 'needs_more_info', + 'pending', + ]); + }); +}); diff --git a/apps/backend/lambdas/projects/test/projects.e2e.test.ts b/apps/backend/lambdas/projects/test/projects.e2e.test.ts index 4cbcea64..f48afb00 100644 --- a/apps/backend/lambdas/projects/test/projects.e2e.test.ts +++ b/apps/backend/lambdas/projects/test/projects.e2e.test.ts @@ -357,8 +357,10 @@ describe('GET /dashboard (e2e)', () => { const body = JSON.parse(res.body); expect(body.summary.totalProjects).toBe(4); - expect(body.summary.totalSpent).toBe(18000); - expect(body.summary.averageSpendPerProject).toBe(4500); + // 14000, not the 18000 the seed spends: one denied and one pending row are + // requests rather than spend and stay out of every total. + expect(body.summary.totalSpent).toBe(14000); + expect(body.summary.averageSpendPerProject).toBe(3500); }); test('topExpenseCategory is highest-summed category 🌞', async () => { diff --git a/apps/backend/lambdas/projects/validation-utils.ts b/apps/backend/lambdas/projects/validation-utils.ts index ba29e288..f212c0df 100644 --- a/apps/backend/lambdas/projects/validation-utils.ts +++ b/apps/backend/lambdas/projects/validation-utils.ts @@ -18,6 +18,13 @@ export const DEFAULT_PROJECT_ROLE: ProjectRole = 'Student'; export type MemberAssignment = { user_id: number; role: ProjectRole }; +/** + * A pending or denied expenditure is a request, not a spend. Every total, + * chart series and budget percentage filters on this; the raw lists do not, + * because their job is to show what is still awaiting review. + */ +export const APPROVED_EXPENDITURE_STATUS = 'approved'; + // Utility class for validating project-related input fields export class ProjectValidationUtils { // Parses numeric input (number or string), converts to fixed 2-decimal string for database storage diff --git a/apps/backend/lambdas/reports/jest.config.js b/apps/backend/lambdas/reports/jest.config.js index 3ed2e70d..cb28368c 100644 --- a/apps/backend/lambdas/reports/jest.config.js +++ b/apps/backend/lambdas/reports/jest.config.js @@ -2,6 +2,9 @@ module.exports = { preset: 'ts-jest', testEnvironment: 'node', testMatch: ['**/*.test.ts'], + // Two suites reseed the same database; in parallel they truncate each + // other's fixtures mid-test. Matches the projects lambda. + maxWorkers: 1, extensionsToTreatAsEsm: ['.ts'], moduleNameMapper: { '^(\\.{1,2}/.*)\\.js$': '$1', diff --git a/apps/backend/lambdas/reports/openapi.yaml b/apps/backend/lambdas/reports/openapi.yaml index 84ca41de..e7b69dc6 100644 --- a/apps/backend/lambdas/reports/openapi.yaml +++ b/apps/backend/lambdas/reports/openapi.yaml @@ -163,10 +163,12 @@ paths: summary: Auto-generate a PDF report from project data description: > Generates a report for the given project containing project info, - participants and roles, donations, and expenditures. Supports PDF and - DOCX output via the `file_type` field (defaults to `pdf`). Uploads the - file to S3 and records it in the database. Requires the caller to be a - member of the project or a global admin. + participants and roles, donations, and approved expenditures. Rows + still under review, or denied, appear in neither the expenditures + table nor the total beneath it. Supports PDF and DOCX output via the + `file_type` field (defaults to `pdf`). Uploads the file to S3 and + records it in the database. Requires the caller to be a member of the + project or a global admin. requestBody: required: true content: diff --git a/apps/backend/lambdas/reports/report-service.ts b/apps/backend/lambdas/reports/report-service.ts index b43c334d..26fa7657 100644 --- a/apps/backend/lambdas/reports/report-service.ts +++ b/apps/backend/lambdas/reports/report-service.ts @@ -24,6 +24,13 @@ const URLResolver = require('pdfmake/js/URLResolver').default; const s3 = new S3Client({ region: process.env.AWS_REGION ?? 'us-east-2' }); +/** + * A report states what a project spent. Expenditures still in review, or + * denied, are requests rather than spend, so they stay out of the table and + * out of the total under it. + */ +const APPROVED_EXPENDITURE_STATUS = 'approved'; + function getBucketName(): string { const bucket = process.env.REPORTS_BUCKET_NAME; if (!bucket) { @@ -178,6 +185,7 @@ export async function fetchReportData(projectId: number): Promise { + const client = await pool.connect(); + try { + await ensureSchema(client); + } finally { + client.release(); + } +}); + +beforeEach(async () => { + const client = await pool.connect(); + try { + await resetData(client); + await client.query('DELETE FROM branch.expenditures'); + await client.query(` + INSERT INTO branch.expenditures + (project_id, entered_by, amount, category, description, status, spent_on) + VALUES + (1, 1, 1000, 'Travel', 'approved', 'approved', '2025-02-10'), + (1, 1, 2000, 'Travel', 'pending', 'pending', '2025-02-11'), + (1, 1, 4000, 'Travel', 'denied', 'denied', '2025-02-12'), + (1, 1, 8000, 'Travel', 'needs info', 'needs_more_info', '2025-02-13') + `); + } finally { + client.release(); + } +}); + +afterAll(async () => { + await pool.end(); + await db.destroy(); +}); + +describe('fetchReportData', () => { + test('returns approved expenditures only', async () => { + const data = await fetchReportData(1); + expect(data).not.toBeNull(); + expect(data!.expenditures).toHaveLength(1); + expect(data!.expenditures[0].description).toBe('approved'); + }); + + test('the total the report prints covers approved expenditures only', async () => { + const data = await fetchReportData(1); + const total = data!.expenditures.reduce((sum, e) => sum + parseFloat(e.amount), 0); + expect(total).toBe(1000); + }); +}); From e7b5325c45ba3e7a17bdb747350d48df6a24669e Mon Sep 17 00:00:00 2001 From: nourshoreibah Date: Thu, 13 Aug 2026 23:10:29 -0400 Subject: [PATCH 2/2] test(expenditures): assert the status is unchanged, not that it is pending Three PATCH /expenditures/{id}/status rejection tests asserted the row was still 'pending' afterwards, encoding the seeded status rather than the thing they mean: that the write did not happen. Seeding expenditure 1 as approved broke them. Capture the status before the call and compare, and ask for a status the row does not already hold, so a rejection test cannot pass merely because the request matched what was already there. The approve and decline cases move to the seeded pending row so they still exercise a real transition. Co-Authored-By: Claude Opus 5 (1M context) --- .../test/expenditures.e2e.test.ts | 33 ++++++++++++++----- 1 file changed, 24 insertions(+), 9 deletions(-) diff --git a/apps/backend/lambdas/expenditures/test/expenditures.e2e.test.ts b/apps/backend/lambdas/expenditures/test/expenditures.e2e.test.ts index 0646134f..81a49aea 100644 --- a/apps/backend/lambdas/expenditures/test/expenditures.e2e.test.ts +++ b/apps/backend/lambdas/expenditures/test/expenditures.e2e.test.ts @@ -628,39 +628,53 @@ describe('Expenditures integration tests', () => { return result.rows[0]?.status; } + // Seeded pending row, so the two success cases below really do move a + // status. Expenditure 1 is seeded approved and is used for the rejection + // cases, where the point is that nothing changes. + const PENDING_ID = 5; + test('200: admin approves a pending expenditure', async () => { mockAuthenticateRequest.mockResolvedValue(adminUser); - const res = await handler(patchStatusEvent(1, { status: 'approved' })); + expect(await getStatus(PENDING_ID)).toBe('pending'); + const res = await handler(patchStatusEvent(PENDING_ID, { status: 'approved' })); expect(res.statusCode).toBe(200); expect(JSON.parse(res.body).body.status).toBe('approved'); // confirms it persisted to the database - expect(await getStatus(1)).toBe('approved'); + expect(await getStatus(PENDING_ID)).toBe('approved'); }); test('200: admin declines a pending expenditure', async () => { mockAuthenticateRequest.mockResolvedValue(adminUser); - const res = await handler(patchStatusEvent(1, { status: 'denied' })); + expect(await getStatus(PENDING_ID)).toBe('pending'); + const res = await handler(patchStatusEvent(PENDING_ID, { status: 'denied' })); expect(res.statusCode).toBe(200); expect(JSON.parse(res.body).body.status).toBe('denied'); - expect(await getStatus(1)).toBe('denied'); + expect(await getStatus(PENDING_ID)).toBe('denied'); }); + // The rejection cases each ask for a status the row does not already hold + // and assert the stored value is untouched, so they cannot pass merely + // because the request happened to match the seed. test('401: unauthenticated request is rejected', async () => { mockAuthenticateRequest.mockResolvedValue({ isAuthenticated: false }); - const res = await handler(patchStatusEvent(1, { status: 'approved' })); + const before = await getStatus(1); + const res = await handler(patchStatusEvent(1, { status: 'denied' })); expect(res.statusCode).toBe(401); - expect(await getStatus(1)).toBe('pending'); + expect(before).not.toBe('denied'); + expect(await getStatus(1)).toBe(before); }); test('403: non-admin user is rejected', async () => { mockAuthenticateRequest.mockResolvedValue(studentUser); - const res = await handler(patchStatusEvent(1, { status: 'approved' })); + const before = await getStatus(1); + const res = await handler(patchStatusEvent(1, { status: 'denied' })); expect(res.statusCode).toBe(403); - expect(await getStatus(1)).toBe('pending'); + expect(before).not.toBe('denied'); + expect(await getStatus(1)).toBe(before); }); test('404: expenditure not found', async () => { @@ -672,10 +686,11 @@ describe('Expenditures integration tests', () => { test('400: status not valid is rejected', async () => { mockAuthenticateRequest.mockResolvedValue(adminUser); + const before = await getStatus(1); const res = await handler(patchStatusEvent(1, { status: 'pend' })); expect(res.statusCode).toBe(400); - expect(await getStatus(1)).toBe('pending'); + expect(await getStatus(1)).toBe(before); }); test('400: invalid id is rejected', async () => {