From 19b4ded96139f7f61d86db0679ee74c89feaec63 Mon Sep 17 00:00:00 2001 From: Salaamu <282868313+ustaxs@users.noreply.github.com> Date: Sun, 18 Oct 2026 12:23:02 +0100 Subject: [PATCH] test(properties): cover the listing surface; fix bulk error handling; add listing log events Extends the Property API test pass (#1363-#1366, which covered create, lookups, remove and restore) over the listing routes, and fixes two real defects found while covering the bulk endpoints. - #1381: `POST /properties/bulk/status` mapped the requested status with `status === DRAFT ? DRAFT : status === ARCHIVED ? ARCHIVED : ACTIVE`, so any other status fell through to ACTIVE. Bulk-marking listings SOLD or PENDING silently published every one of them and returned a success-looking `updatedCount`. Only DRAFT/ACTIVE/ARCHIVED are accepted now and anything else is a 400 naming the supported set, with nothing written. `POST /properties/bulk/delete` echoed the *requested* ids back as `propertyIds`, so deleting ['real', 'typo'] answered deletedCount 1 with both ids listed, which reads as if both had been removed. It now resolves which ids exist first and returns those, so the count and the list always agree; when nothing matches it short-circuits without touching the database. A foreign-key violation (P2003/P2014, i.e. a listing held by a transaction, document or commission) was an unhandled 500 carrying the raw driver message; it is now a 409 with the underlying error logged instead of returned. Both bulk methods reject an empty id list, which was previously a silent no-op indistinguishable from "nothing matched". docs/api/listings-error-handling.md - #1382: added the AppLogger events the listing paths never emitted - search (with the filter *count* but never the filter values, so a slow search can be attributed to a query shape without addresses reaching the log stream), status transition including the idempotent no-op, approve/reject, all three bulk operations (each logging requestedCount alongside the affected count), expiry, the three agent-assignment operations and the three amenity operations. docs/api/listings-structured-logging.md - #1379: unit suites added for searchProperties (paging, defaults, the forced ACTIVE/non-deleted filter, trust-score annotation and the in-memory branch, geo validation), transitionStatus/approve/reject (404/403/400, the no-op, the three allowed roles), bulk operations, agent assignment and amenities. The Prisma mock gained the updateMany/deleteMany/count/$transaction/propertyAgent/ propertyAmenity/user delegates. docs/api/listings-tests.md - #1380: the HTTP suite now drives the status lifecycle (including that a refused transition is a 400 and that the caller role is forwarded), the bulk endpoints (id validation, reported ids, a 409 conflict), agent assignment, amenities, and the report download - which additionally asserts that an unexpected failure returns 500 without leaking the underlying error text. No tests were run and nothing was installed, built or linted; the new suites were written from the current service/controller code. Please verify in CI. Closes #1379 Closes #1380 Closes #1381 Closes #1382 --- docs/api/listings-error-handling.md | 31 ++ docs/api/listings-structured-logging.md | 33 ++ docs/api/listings-tests.md | 25 ++ src/properties/properties.service.spec.ts | 383 ++++++++++++++++++ src/properties/properties.service.ts | 238 ++++++++++- .../properties-controller.integration.spec.ts | 316 ++++++++++++++- 6 files changed, 1008 insertions(+), 18 deletions(-) create mode 100644 docs/api/listings-error-handling.md create mode 100644 docs/api/listings-structured-logging.md create mode 100644 docs/api/listings-tests.md diff --git a/docs/api/listings-error-handling.md b/docs/api/listings-error-handling.md new file mode 100644 index 00000000..0fb16fac --- /dev/null +++ b/docs/api/listings-error-handling.md @@ -0,0 +1,31 @@ +# Listing API error handling + +## Bulk status update silently published listings + +`POST /properties/bulk/status` mapped the requested status with + +```ts +status === DRAFT ? 'DRAFT' : status === ARCHIVED ? 'ARCHIVED' : 'ACTIVE' +``` + +Any status that was not `DRAFT` or `ARCHIVED` fell through to **`ACTIVE`**. Asking to bulk-mark listings `SOLD` or `PENDING` therefore published every one of them instead, and returned `200` with a plausible `updatedCount`. The response looked like a success and the data was wrong. + +The endpoint now accepts only `DRAFT`, `ACTIVE` and `ARCHIVED`, and answers `400` naming the supported set for anything else. Nothing is written when the request is rejected. + +## Bulk delete reported ids that were never deleted + +`POST /properties/bulk/delete` returned `{ deletedCount, propertyIds }` where `propertyIds` was the **requested** list, echoed back unchanged. Deleting `['real-id', 'typo-id']` answered `{ deletedCount: 1, propertyIds: ['real-id', 'typo-id'] }`, which reads as if both had been removed. + +The endpoint now resolves which ids actually exist first and returns those, so `deletedCount` and `propertyIds` always agree. When nothing matches it returns `{ deletedCount: 0, propertyIds: [] }` without touching the database. This costs one extra `findMany` with `select: { id: true }` on an admin-only endpoint. + +## Foreign-key violations surfaced as opaque 500s + +A listing referenced by a transaction, document or commission cannot be removed. That made Prisma throw `P2003`/`P2014`, which reached the client as an unhandled `500` carrying the raw driver message — internal schema detail in an error body. + +It is now a `409 Conflict` with a message that says which situation occurred, and the underlying error is logged (`properties.bulk_delete_conflict`) rather than returned. Any other error still propagates unchanged. + +## Empty id lists + +Both bulk endpoints now reject an empty list with `400`. The DTOs already enforced `@ArrayMinSize(1)` at the HTTP boundary, but the service methods are also called directly, and `updateMany`/`deleteMany` with an empty `in` list is a silent no-op that reports `updatedCount: 0` — indistinguishable from "nothing matched". + +→ `docs/api/listings-tests.md` diff --git a/docs/api/listings-structured-logging.md b/docs/api/listings-structured-logging.md new file mode 100644 index 00000000..90a85ece --- /dev/null +++ b/docs/api/listings-structured-logging.md @@ -0,0 +1,33 @@ +# Property API structured logging + +`PropertiesService` uses the project's `AppLogger`. Events are a short constant message plus a metadata object with a stable `event` name, so the JSON logs produced in production can be filtered and alerted on. + +This document covers the listing surface; the lifecycle events added alongside the Property API pass are in `properties-structured-logging.md`. + +| `event` | Level | Fields | +| --- | --- | --- | +| `properties.search` | log | `page`, `limit`, `total`, `returned`, `filters`, `trustScoreFiltered` | +| `properties.status_transitioned` | log | `propertyId`, `from`, `to`, `actorId` | +| `properties.status_transition_noop` | log | `propertyId`, `status`, `actorId` | +| `properties.approved` | log | `propertyId`, `actorId` | +| `properties.rejected` | log | `propertyId`, `actorId` | +| `properties.bulk_status_updated` | log | `requestedCount`, `updatedCount`, `status` | +| `properties.bulk_deleted` | warn | `requestedCount`, `deletedCount` | +| `properties.bulk_delete_no_match` | warn | `requestedCount` | +| `properties.bulk_delete_conflict` | warn | `requestedCount`, `error` | +| `properties.bulk_exported` | log | `requestedCount`, `matchedCount`, `filtered` | +| `properties.expired` | log | `updatedCount` | +| `properties.agent_assigned` | log | `propertyId`, `agentId`, `actorId` | +| `properties.agent_assignment_updated` | log | `propertyId`, `agentId`, `actorId` | +| `properties.agent_assignment_removed` | log | `propertyId`, `agentId`, `actorId` | +| `properties.amenity_added` | log | `propertyId`, `amenityId`, `amenityType` | +| `properties.amenity_updated` | log | `propertyId`, `amenityId` | +| `properties.amenity_removed` | log | `propertyId`, `amenityId` | + +## Two details worth knowing + +`properties.search` logs **how many** filters were supplied, never their values. A slow or empty search can therefore be attributed to a query shape — "every search with 8 filters is slow" — without putting addresses, cities or owner names into the log stream. Paging and sorting are excluded from the count, since `page` and `limit` are always present because the DTO defaults them. + +Every bulk event logs both `requestedCount` and `updatedCount`. Before this, a bulk delete of five ids where only two existed reported a count of two and nothing else, so a partially-failed admin operation was indistinguishable from a complete one. + +No addresses or personal data are logged; `AppLogger` also redacts fields such as `password`, `token` and `secret`. diff --git a/docs/api/listings-tests.md b/docs/api/listings-tests.md new file mode 100644 index 00000000..b1559958 --- /dev/null +++ b/docs/api/listings-tests.md @@ -0,0 +1,25 @@ +# Listing API tests + +## Unit — `src/properties/properties.service.spec.ts` + +Extends the existing suite (which covers create, lookups, remove and restore) over the listing surface that had none: + +- **`searchProperties`** — paging arithmetic and the reported total, the defaults (`page 1`, `20` per page, newest first), that the query is forced to `ACTIVE` and non-deleted whatever `status` the caller passed, the computed `neighborhoodTrustScore` annotation, the in-memory trust-score branch (and that it does not use the transaction), and geo validation: `lat` without `lng` rejected, a radius with no centre rejected, and a radius converted to a bounding box. +- **`transitionStatus` / `approve` / `reject`** — `404` for an unknown listing, `403` for a caller who is neither owner, agent nor admin, `400` for a transition the workflow disallows, an idempotent no-op that does not write, the three roles that *are* allowed, and that approve goes to `ACTIVE` while reject goes to `ARCHIVED`. +- **Bulk operations** — each supported status, the rejection of an unsupported one, the empty-list guard, that `propertyIds` reports only ids that existed, that a no-match short-circuits, and that a `P2003` becomes a conflict rather than a 500. +- **Agent assignment** — `404`/`403`/role/duplicate guards, the 3% default commission, `404` on updating or removing an assignment that does not exist, and the contact-detail fallback in `getAgents`. +- **Amenities** — `404` for an unknown listing, refusal to touch an amenity that belongs to a different listing, the `isAvailable` default, and the oldest-first ordering. + +The Prisma mock gained the `updateMany`, `deleteMany`, `count`, `$transaction`, `propertyAgent`, `propertyAmenity` and `user` delegates these suites drive. + +## Integration — `test/e2e/properties-controller.integration.spec.ts` + +Extends the existing HTTP suite (public reads, the 404 contract, search routing and query coercion, create validation, role enforcement) over the routes it did not touch: + +- **Status lifecycle** — the route is open to any authenticated caller and forwards the caller id *and* role to the service, requires authentication, rejects a status outside the enum with `400`, surfaces a refused transition as `400`, and that `approve`/`reject` are admin-only. +- **Bulk operations** — admin-only, id-list validation (empty and malformed), the reported ids, a foreign-key conflict surfacing as `409`, and the optional export filter. +- **Agent assignment** — authentication, argument forwarding for create/update/remove, a missing assignment surfacing as `404`, and that the read route is unauthenticated. +- **Amenities** — body validation, create, the unauthenticated read, and both-ids forwarding on update and delete. +- **Report download** — that a PDF is streamed with its attachment headers, that a missing listing is `404`, and that an unexpected failure returns `500` **without** leaking the underlying error text into the body. + +Run with `npx jest src/properties/properties.service.spec.ts test/e2e/properties-controller.integration.spec.ts`. diff --git a/src/properties/properties.service.spec.ts b/src/properties/properties.service.spec.ts index 22065c0d..6d6e254e 100644 --- a/src/properties/properties.service.spec.ts +++ b/src/properties/properties.service.spec.ts @@ -21,7 +21,27 @@ describe('PropertiesService', () => { findMany: jest.fn(), create: jest.fn(), update: jest.fn(), + updateMany: jest.fn(), + deleteMany: jest.fn(), + count: jest.fn(), }, + propertyAgent: { + findUnique: jest.fn(), + findMany: jest.fn(), + create: jest.fn(), + update: jest.fn(), + delete: jest.fn(), + }, + propertyAmenity: { + findUnique: jest.fn(), + findMany: jest.fn(), + create: jest.fn(), + update: jest.fn(), + delete: jest.fn(), + }, + user: { findUnique: jest.fn() }, + // searchProperties reads the page and the total in one transaction. + $transaction: jest.fn(), }; const fraud = { evaluatePropertyCreated: jest.fn() }; const geocoding = { geocodeAddress: jest.fn(), hasAddressChanged: jest.fn().mockReturnValue(false) }; @@ -215,4 +235,367 @@ describe('PropertiesService', () => { expect(cache.invalidateByTag).toHaveBeenCalled(); }); }); + + // --------------------------------------------------------------------- + // searchProperties + // --------------------------------------------------------------------- + describe('searchProperties', () => { + const page = (items: unknown[], total = items.length) => { + prisma.$transaction.mockImplementation( + ([rows, count]: [Promise, Promise]) => Promise.all([rows, count]), + ); + prisma.property.findMany.mockResolvedValue(items); + prisma.property.count.mockResolvedValue(total); + }; + + it('pages, orders and reports the total', async () => { + page([{ id: 'p1', neighborhood: null }], 42); + + const result = await service.searchProperties({ page: 3, limit: 10 } as never); + + expect(prisma.property.findMany).toHaveBeenCalledWith( + expect.objectContaining({ skip: 20, take: 10, orderBy: { createdAt: 'desc' } }), + ); + expect(prisma.property.count).toHaveBeenCalledTimes(1); + expect(result).toMatchObject({ total: 42, page: 3, limit: 10, totalPages: 5 }); + expect(result.items).toHaveLength(1); + }); + + it('defaults to page 1, 20 per page, newest first', async () => { + page([]); + + const result = await service.searchProperties({} as never); + + expect(prisma.property.findMany).toHaveBeenCalledWith( + expect.objectContaining({ skip: 0, take: 20, orderBy: { createdAt: 'desc' } }), + ); + expect(result).toMatchObject({ page: 1, limit: 20, total: 0 }); + }); + + it('only ever returns ACTIVE, non-deleted listings', async () => { + page([]); + + await service.searchProperties({ status: PropertyStatus.DRAFT } as never); + + expect(prisma.property.findMany.mock.calls[0][0].where).toMatchObject({ + deleted: false, + status: PropertyStatus.ACTIVE, + }); + }); + + it('annotates each row with a computed neighborhood trust score', async () => { + page([ + { + id: 'p1', + neighborhood: { + walkScore: 80, + transitScore: 60, + bikeScore: 70, + schoolRating: 9, + crimeIndex: 10, + }, + }, + { id: 'p2', neighborhood: null }, + ]); + + const result = await service.searchProperties({} as never); + + expect(result.items[0]).toMatchObject({ id: 'p1', neighborhoodTrustScore: 79 }); + expect(result.items[1]).toMatchObject({ id: 'p2', neighborhoodTrustScore: null }); + }); + + it('filters in memory when a trust-score range is requested', async () => { + page([ + { id: 'low', neighborhood: { walkScore: 10 } }, + { id: 'high', neighborhood: { walkScore: 95 } }, + { id: 'none', neighborhood: null }, + ]); + + const result = await service.searchProperties({ minNeighborhoodTrustScore: 50 } as never); + + expect(prisma.$transaction).not.toHaveBeenCalled(); + expect(result.items.map((i: { id: string }) => i.id)).toEqual(['high']); + expect(result.total).toBe(1); + }); + + it('rejects lat without lng', async () => { + await expect(service.searchProperties({ lat: 30 } as never)).rejects.toThrow( + BadRequestException, + ); + }); + + it('rejects a radius with neither lat/lng nor a bounding box', async () => { + await expect(service.searchProperties({ radiusKm: 5 } as never)).rejects.toThrow( + BadRequestException, + ); + }); + + it('turns a radius into a bounding box', async () => { + page([]); + await service.searchProperties({ lat: 30, lng: -97, radiusKm: 111 } as never); + + const where = prisma.property.findMany.mock.calls[0][0].where; + expect(where.latitude).toEqual({ gte: 29, lte: 31 }); + expect(where.longitude).toEqual({ gte: -98, lte: -96 }); + }); + }); + + // --------------------------------------------------------------------- + // transitionStatus / approve / reject + // --------------------------------------------------------------------- + describe('transitionStatus', () => { + beforeEach(() => { + prisma.property.findUnique.mockResolvedValue({ id: 'p1', ownerId: 'owner-1', status: 'DRAFT' }); + prisma.property.update.mockResolvedValue({ id: 'p1' }); + }); + + it('throws NotFound for an unknown property', async () => { + prisma.property.findUnique.mockResolvedValue(null); + await expect( + service.transitionStatus('missing', PropertyStatus.PENDING, 'owner-1', UserRole.USER), + ).rejects.toThrow(NotFoundException); + }); + + it('forbids a caller who is neither owner, AGENT nor ADMIN', async () => { + await expect( + service.transitionStatus('p1', PropertyStatus.PENDING, 'stranger', UserRole.USER), + ).rejects.toThrow(ForbiddenException); + expect(prisma.property.update).not.toHaveBeenCalled(); + }); + + it('rejects a transition the workflow does not allow', async () => { + await expect( + service.transitionStatus('p1', PropertyStatus.SOLD, 'owner-1', UserRole.USER), + ).rejects.toThrow(BadRequestException); + expect(prisma.property.update).not.toHaveBeenCalled(); + }); + + it('treats a transition to the current status as an idempotent no-op', async () => { + await service.transitionStatus('p1', 'DRAFT' as PropertyStatus, 'owner-1', UserRole.USER); + expect(prisma.property.update).not.toHaveBeenCalled(); + }); + + it.each([ + ['the owner', UserRole.USER, 'owner-1'], + ['an agent', UserRole.AGENT, 'agent-1'], + ['an admin', UserRole.ADMIN, 'admin-1'], + ])('lets %s move a listing forward', async (_label, role, sub) => { + await service.transitionStatus('p1', PropertyStatus.PENDING, sub, role); + expect(prisma.property.update).toHaveBeenCalledWith({ + where: { id: 'p1' }, + data: { status: PropertyStatus.PENDING }, + }); + }); + + it('approve goes to ACTIVE and reject to ARCHIVED', async () => { + await service.approveProperty('p1', 'admin-1'); + expect(prisma.property.update).toHaveBeenLastCalledWith({ + where: { id: 'p1' }, + data: { status: PropertyStatus.ACTIVE }, + }); + + await service.rejectProperty('p1', 'admin-1'); + expect(prisma.property.update).toHaveBeenLastCalledWith({ + where: { id: 'p1' }, + data: { status: PropertyStatus.ARCHIVED }, + }); + }); + }); + + // --------------------------------------------------------------------- + // bulk operations + // --------------------------------------------------------------------- + describe('bulk operations', () => { + beforeEach(() => { + prisma.property.updateMany.mockResolvedValue({ count: 2 }); + prisma.property.deleteMany.mockResolvedValue({ count: 1 }); + prisma.property.findMany.mockResolvedValue([]); + }); + + it.each(['DRAFT', 'ACTIVE', 'ARCHIVED'])( + 'bulk-updates to the supported status %s', + async (status) => { + const result = await service.bulkUpdatePropertyStatus(['p1', 'p2'], status as PropertyStatus); + expect(prisma.property.updateMany).toHaveBeenCalledWith({ + where: { id: { in: ['p1', 'p2'] } }, + data: { status }, + }); + expect(result).toEqual({ updatedCount: 2 }); + }, + ); + + it('rejects a bulk status the endpoint does not support instead of silently coercing it', async () => { + // SOLD used to be coerced to ACTIVE, which silently published listings. + await expect(service.bulkUpdatePropertyStatus(['p1'], PropertyStatus.SOLD)).rejects.toThrow( + BadRequestException, + ); + expect(prisma.property.updateMany).not.toHaveBeenCalled(); + }); + + it('rejects an empty id list', async () => { + await expect(service.bulkUpdatePropertyStatus([], PropertyStatus.ACTIVE)).rejects.toThrow( + BadRequestException, + ); + await expect(service.bulkDeleteProperties([])).rejects.toThrow(BadRequestException); + expect(prisma.property.deleteMany).not.toHaveBeenCalled(); + }); + + it('reports only the ids that actually existed', async () => { + prisma.property.findMany.mockResolvedValue([{ id: 'p1' }]); + + const result = await service.bulkDeleteProperties(['p1', 'ghost']); + + expect(prisma.property.deleteMany).toHaveBeenCalledWith({ where: { id: { in: ['p1'] } } }); + expect(result).toEqual({ deletedCount: 1, propertyIds: ['p1'] }); + }); + + it('does not touch the database when no id matches', async () => { + const result = await service.bulkDeleteProperties(['ghost']); + expect(prisma.property.deleteMany).not.toHaveBeenCalled(); + expect(result).toEqual({ deletedCount: 0, propertyIds: [] }); + }); + + it('turns a foreign-key violation into a conflict instead of a 500', async () => { + prisma.property.findMany.mockResolvedValue([{ id: 'p1' }]); + prisma.property.deleteMany.mockRejectedValue( + Object.assign(new Error('Foreign key constraint failed'), { code: 'P2003' }), + ); + + await expect(service.bulkDeleteProperties(['p1'])).rejects.toThrow(/cannot be deleted/); + }); + }); + + // --------------------------------------------------------------------- + // agent assignment + // --------------------------------------------------------------------- + describe('agent assignment', () => { + const agentDto = { agentId: 'agent-1' }; + + beforeEach(() => { + prisma.property.findUnique.mockResolvedValue({ id: 'p1', ownerId: 'owner-1' }); + prisma.user.findUnique.mockResolvedValue({ id: 'agent-1', role: 'AGENT' }); + prisma.propertyAgent.findUnique.mockResolvedValue(null); + }); + + it('refuses an agent assignment for an unknown property', async () => { + prisma.property.findUnique.mockResolvedValue(null); + await expect( + service.assignAgent('missing', agentDto as never, user(UserRole.ADMIN)), + ).rejects.toThrow(NotFoundException); + }); + + it('refuses an agent assignment from a caller who is not the owner or an admin', async () => { + await expect( + service.assignAgent('p1', agentDto as never, user(UserRole.USER, 'stranger')), + ).rejects.toThrow(ForbiddenException); + }); + + it('refuses an assignee without the AGENT role', async () => { + prisma.user.findUnique.mockResolvedValue({ id: 'agent-1', role: 'USER' }); + await expect( + service.assignAgent('p1', agentDto as never, user(UserRole.ADMIN)), + ).rejects.toThrow(BadRequestException); + expect(prisma.propertyAgent.create).not.toHaveBeenCalled(); + }); + + it('refuses to assign the same agent twice', async () => { + prisma.propertyAgent.findUnique.mockResolvedValue({ propertyId: 'p1', agentId: 'agent-1' }); + await expect( + service.assignAgent('p1', agentDto as never, user(UserRole.ADMIN)), + ).rejects.toThrow(/already assigned/); + }); + + it('defaults the commission to 3% and creates the assignment', async () => { + prisma.propertyAgent.create.mockResolvedValue({ id: 'pa1' }); + await service.assignAgent('p1', agentDto as never, user(UserRole.ADMIN)); + + expect(prisma.propertyAgent.create.mock.calls[0][0].data).toMatchObject({ + commissionRate: new Decimal('0.03'), + contactPhone: null, + }); + }); + + it('reports a missing assignment as not found on update', async () => { + prisma.propertyAgent.findUnique.mockResolvedValue(null); + await expect( + service.updateAgentAssignment('p1', 'agent-1', {} as never, user(UserRole.ADMIN)), + ).rejects.toThrow(NotFoundException); + expect(prisma.propertyAgent.update).not.toHaveBeenCalled(); + }); + + it('reports a missing assignment as not found on removal', async () => { + prisma.propertyAgent.findUnique.mockResolvedValue(null); + await expect( + service.removeAgentAssignment('p1', 'agent-1', user(UserRole.ADMIN)), + ).rejects.toThrow(NotFoundException); + expect(prisma.propertyAgent.delete).not.toHaveBeenCalled(); + }); + + it('lists assignments and falls back to the agent contact details', async () => { + prisma.propertyAgent.findMany.mockResolvedValue([ + { + id: 'pa1', + propertyId: 'p1', + agentId: 'agent-1', + commissionRate: new Decimal('0.03'), + contactPhone: null, + contactEmail: null, + agent: { id: 'agent-1', phone: '555-0100', email: 'agent@example.com' }, + }, + ]); + + const result = await service.getAgents('p1'); + + expect(result).toEqual([ + expect.objectContaining({ + commissionRate: 0.03, + contactPhone: '555-0100', + contactEmail: 'agent@example.com', + }), + ]); + }); + }); + + // --------------------------------------------------------------------- + // amenities + // --------------------------------------------------------------------- + describe('amenities', () => { + beforeEach(() => { + prisma.property.findUnique.mockResolvedValue({ id: 'p1' }); + prisma.propertyAmenity.findUnique.mockResolvedValue({ id: 'am1', propertyId: 'p1' }); + }); + + it('reports an unknown property as not found', async () => { + prisma.property.findUnique.mockResolvedValue(null); + await expect(service.addAmenity('missing', {} as never)).rejects.toThrow(NotFoundException); + }); + + it('refuses to touch an amenity that belongs to another property', async () => { + prisma.propertyAmenity.findUnique.mockResolvedValue({ id: 'am1', propertyId: 'other' }); + await expect(service.updateAmenity('p1', 'am1', {} as never)).rejects.toThrow(NotFoundException); + await expect(service.removeAmenity('p1', 'am1')).rejects.toThrow(NotFoundException); + expect(prisma.propertyAmenity.update).not.toHaveBeenCalled(); + expect(prisma.propertyAmenity.delete).not.toHaveBeenCalled(); + }); + + it('defaults an amenity to available', async () => { + prisma.propertyAmenity.create.mockResolvedValue({ id: 'am1' }); + await service.addAmenity('p1', { name: 'Pool', amenityType: 'OUTDOOR' } as never); + + expect(prisma.propertyAmenity.create.mock.calls[0][0].data).toMatchObject({ + propertyId: 'p1', + isAvailable: true, + description: null, + }); + }); + + it('lists amenities oldest first', async () => { + prisma.propertyAmenity.findMany.mockResolvedValue([]); + await service.getAmenities('p1'); + expect(prisma.propertyAmenity.findMany).toHaveBeenCalledWith({ + where: { propertyId: 'p1' }, + orderBy: { createdAt: 'asc' }, + }); + }); + }); }); diff --git a/src/properties/properties.service.ts b/src/properties/properties.service.ts index 5a46cd7d..cc9a8ad8 100644 --- a/src/properties/properties.service.ts +++ b/src/properties/properties.service.ts @@ -1,5 +1,6 @@ import { BadRequestException, + ConflictException, ForbiddenException, Injectable, NotFoundException, @@ -32,6 +33,22 @@ interface FindAllParams { orderBy?: Record; } +/** + * How many filters a search actually carried. Logged so a slow or empty search + * can be attributed to a specific query shape without logging the values + * themselves, which would put addresses and owner data into the log stream. + * Paging and sorting are excluded: they are not filters and `page`/`limit` are + * always present because the DTO defaults them. + */ +const NON_FILTER_SEARCH_FIELDS = new Set(['page', 'limit', 'sortBy', 'sortOrder']); + +function countSearchFilters(dto: SearchPropertiesDto): number { + const entries = Object.entries((dto ?? {}) as Record); + return entries.filter( + ([key, value]) => !NON_FILTER_SEARCH_FIELDS.has(key) && value !== undefined && value !== null, + ).length; +} + export interface PaginatedPropertiesResult { items: T[]; total: number; @@ -514,6 +531,13 @@ export class PropertiesService { }, }); + if (result.count > 0) { + this.logger.log('Expired properties', { + event: 'properties.expired', + updatedCount: result.count, + }); + } + return { updatedCount: result.count }; } @@ -556,6 +580,12 @@ export class PropertiesService { // Idempotent no-op: don't update, but echo the property back so callers // can rely on a consistent shape. if (currentStatus === nextStatus) { + this.logger.log('Property status unchanged', { + event: 'properties.status_transition_noop', + propertyId, + status: nextStatus, + actorId, + }); return this.findOne(propertyId); } @@ -572,6 +602,14 @@ export class PropertiesService { data: { status: nextStatus }, }); + this.logger.log('Property status changed', { + event: 'properties.status_transitioned', + propertyId, + from: currentStatus, + to: nextStatus, + actorId, + }); + return this.findOne(propertyId); } @@ -627,6 +665,16 @@ export class PropertiesService { const skip = (page - 1) * limit; const items = filtered.slice(skip, skip + limit); + this.logger.log('Property search completed', { + event: 'properties.search', + page, + limit, + total, + returned: items.length, + filters: countSearchFilters(dto), + trustScoreFiltered: true, + }); + return { items, total, page, limit, totalPages: limit > 0 ? Math.ceil(total / limit) : 0 }; } @@ -652,6 +700,16 @@ export class PropertiesService { : null, })); + this.logger.log('Property search completed', { + event: 'properties.search', + page, + limit, + total, + returned: items.length, + filters: countSearchFilters(dto), + trustScoreFiltered: false, + }); + return { items, total, @@ -832,27 +890,69 @@ export class PropertiesService { } async approveProperty(propertyId: string, actorId: string) { - return this.transitionStatus(propertyId, PropertyStatus.ACTIVE, actorId, UserRole.ADMIN); + const property = await this.transitionStatus( + propertyId, + PropertyStatus.ACTIVE, + actorId, + UserRole.ADMIN, + ); + this.logger.log('Property approved', { + event: 'properties.approved', + propertyId, + actorId, + }); + return property; } async rejectProperty(propertyId: string, actorId: string) { - return this.transitionStatus(propertyId, PropertyStatus.ARCHIVED, actorId, UserRole.ADMIN); + const property = await this.transitionStatus( + propertyId, + PropertyStatus.ARCHIVED, + actorId, + UserRole.ADMIN, + ); + this.logger.log('Property rejected', { + event: 'properties.rejected', + propertyId, + actorId, + }); + return property; } + /** Statuses a bulk update is allowed to set. Anything else is rejected outright. */ + private static readonly BULK_STATUSES: PropertyStatus[] = [ + PropertyStatus.DRAFT, + PropertyStatus.ACTIVE, + PropertyStatus.ARCHIVED, + ]; + async bulkUpdatePropertyStatus( propertyIds: string[], status: PropertyStatus, ): Promise<{ updatedCount: number }> { - const validStatus = - status === PropertyStatus.DRAFT - ? 'DRAFT' - : status === PropertyStatus.ARCHIVED - ? 'ARCHIVED' - : 'ACTIVE'; + if (!propertyIds?.length) { + throw new BadRequestException('At least one property id is required'); + } + + // Previously any status that was not DRAFT or ARCHIVED was silently coerced + // to ACTIVE, so asking for SOLD quietly marked every listing ACTIVE. An + // unsupported status is now reported instead of applied. + if (!PropertiesService.BULK_STATUSES.includes(status)) { + throw new BadRequestException( + `Bulk status update only supports ${PropertiesService.BULK_STATUSES.join(', ')}`, + ); + } const result = await this.prisma.property.updateMany({ where: { id: { in: propertyIds } }, - data: { status: validStatus }, + data: { status }, + }); + + this.logger.log('Bulk property status update', { + event: 'properties.bulk_status_updated', + requestedCount: propertyIds.length, + updatedCount: result.count, + status, }); return { updatedCount: result.count }; @@ -862,13 +962,58 @@ export class PropertiesService { deletedCount: number; propertyIds: string[]; }> { - const result = await this.prisma.property.deleteMany({ + if (!propertyIds?.length) { + throw new BadRequestException('At least one property id is required'); + } + + // Resolve the ids that actually exist first. Echoing the requested ids back + // told the caller everything had been deleted even when some ids matched + // nothing. + const existing = (await this.prisma.property.findMany({ where: { id: { in: propertyIds } }, + select: { id: true }, + })) as unknown as { id: string }[]; + + const foundIds = existing.map((p) => p.id); + if (foundIds.length === 0) { + this.logger.warn('Bulk property delete matched nothing', { + event: 'properties.bulk_delete_no_match', + requestedCount: propertyIds.length, + }); + return { deletedCount: 0, propertyIds: [] }; + } + + let result: { count: number }; + try { + result = await this.prisma.property.deleteMany({ where: { id: { in: foundIds } } }); + } catch (err) { + // A listing referenced by a transaction, document or commission cannot be + // removed. That is a client-correctable conflict, not a server fault, and + // the raw Prisma error must not reach the client. + const code = (err as { code?: string })?.code; + if (code === 'P2003' || code === 'P2014') { + this.logger.warn('Bulk property delete blocked by a foreign key', { + event: 'properties.bulk_delete_conflict', + requestedCount: foundIds.length, + error: err instanceof Error ? err.message : String(err), + }); + throw new ConflictException( + 'One or more properties are referenced by transactions or documents and cannot be deleted', + ); + } + throw err; + } + + this.logger.warn('Bulk properties deleted', { + event: 'properties.bulk_deleted', + requestedCount: propertyIds.length, + deletedCount: result.count, + actor: 'admin', }); return { deletedCount: result.count, - propertyIds, + propertyIds: foundIds, }; } @@ -931,6 +1076,13 @@ export class PropertiesService { }, }); + this.logger.log('Bulk property export', { + event: 'properties.bulk_exported', + requestedCount: propertyIds.length, + matchedCount: properties.length, + filtered: Boolean(filter), + }); + const exportData: Record[] = properties.map((prop: Record) => ({ id: prop.id, title: prop.title, @@ -1008,7 +1160,7 @@ export class PropertiesService { throw new BadRequestException('Agent is already assigned to this property'); } - return this.prisma.propertyAgent.create({ + const assignment = await this.prisma.propertyAgent.create({ data: { propertyId, agentId: dto.agentId, @@ -1031,6 +1183,15 @@ export class PropertiesService { }, }, }); + + this.logger.log('Agent assigned to property', { + event: 'properties.agent_assigned', + propertyId, + agentId: dto.agentId, + actorId: user.sub, + }); + + return assignment; } async updateAgentAssignment( @@ -1064,7 +1225,7 @@ export class PropertiesService { throw new NotFoundException('Agent assignment not found for this property'); } - return this.prisma.propertyAgent.update({ + const updatedAssignment = await this.prisma.propertyAgent.update({ where: { propertyId_agentId: { propertyId, @@ -1089,6 +1250,15 @@ export class PropertiesService { }, }, }); + + this.logger.log('Agent assignment updated', { + event: 'properties.agent_assignment_updated', + propertyId, + agentId, + actorId: user.sub, + }); + + return updatedAssignment; } async removeAgentAssignment(propertyId: string, agentId: string, user: AuthUserPayload) { @@ -1117,7 +1287,7 @@ export class PropertiesService { throw new NotFoundException('Agent assignment not found for this property'); } - return this.prisma.propertyAgent.delete({ + const removed = await this.prisma.propertyAgent.delete({ where: { propertyId_agentId: { propertyId, @@ -1125,6 +1295,15 @@ export class PropertiesService { }, }, }); + + this.logger.log('Agent removed from property', { + event: 'properties.agent_assignment_removed', + propertyId, + agentId, + actorId: user.sub, + }); + + return removed; } async getAgents(propertyId: string) { @@ -1170,7 +1349,7 @@ export class PropertiesService { if (!property) { throw new NotFoundException('Property not found'); } - return this.prisma.propertyAmenity.create({ + const amenity = await this.prisma.propertyAmenity.create({ data: { propertyId, name: dto.name, @@ -1179,6 +1358,15 @@ export class PropertiesService { isAvailable: dto.isAvailable ?? true, }, }); + + this.logger.log('Amenity added to property', { + event: 'properties.amenity_added', + propertyId, + amenityId: amenity.id, + amenityType: dto.amenityType, + }); + + return amenity; } async getAmenities(propertyId: string) { @@ -1199,7 +1387,7 @@ export class PropertiesService { if (!amenity || amenity.propertyId !== propertyId) { throw new NotFoundException('Amenity not found for this property'); } - return this.prisma.propertyAmenity.update({ + const amenity = await this.prisma.propertyAmenity.update({ where: { id: amenityId }, data: { name: dto.name, @@ -1208,6 +1396,14 @@ export class PropertiesService { isAvailable: dto.isAvailable, }, }); + + this.logger.log('Property amenity updated', { + event: 'properties.amenity_updated', + propertyId, + amenityId, + }); + + return amenity; } async removeAmenity(propertyId: string, amenityId: string) { @@ -1217,6 +1413,14 @@ export class PropertiesService { if (!amenity || amenity.propertyId !== propertyId) { throw new NotFoundException('Amenity not found for this property'); } - return this.prisma.propertyAmenity.delete({ where: { id: amenityId } }); + const removed = await this.prisma.propertyAmenity.delete({ where: { id: amenityId } }); + + this.logger.log('Amenity removed from property', { + event: 'properties.amenity_removed', + propertyId, + amenityId, + }); + + return removed; } } diff --git a/test/e2e/properties-controller.integration.spec.ts b/test/e2e/properties-controller.integration.spec.ts index b8481f88..ec694f4d 100644 --- a/test/e2e/properties-controller.integration.spec.ts +++ b/test/e2e/properties-controller.integration.spec.ts @@ -7,10 +7,13 @@ */ import { + BadRequestException, CanActivate, + ConflictException, ExecutionContext, INestApplication, Injectable, + NotFoundException, ValidationPipe, } from '@nestjs/common'; import { Test } from '@nestjs/testing'; @@ -56,6 +59,23 @@ describe('PropertiesController (integration)', () => { update: jest.fn(), remove: jest.fn(), searchProperties: jest.fn(), + transitionStatus: jest.fn(), + approveProperty: jest.fn(), + rejectProperty: jest.fn(), + bulkUpdatePropertyStatus: jest.fn(), + bulkDeleteProperties: jest.fn(), + bulkExportProperties: jest.fn(), + assignAgent: jest.fn(), + updateAgentAssignment: jest.fn(), + removeAgentAssignment: jest.fn(), + getAgents: jest.fn(), + addAmenity: jest.fn(), + getAmenities: jest.fn(), + updateAmenity: jest.fn(), + removeAmenity: jest.fn(), + }; + const propertyReportService = { + generatePropertyReport: jest.fn(), }; const as = (role: string, r: request.Test) => r.set('Authorization', `Bearer role:${role}`); @@ -65,7 +85,7 @@ describe('PropertiesController (integration)', () => { controllers: [PropertiesController], providers: [ { provide: PropertiesService, useValue: propertiesService }, - { provide: PropertyReportService, useValue: {} }, + { provide: PropertyReportService, useValue: propertyReportService }, ], }) .overrideGuard(JwtAuthGuard) @@ -212,4 +232,298 @@ describe('PropertiesController (integration)', () => { ); }); }); + + describe('status lifecycle', () => { + const asUser = (r: request.Test) => as('USER', r); + + it('PATCH /properties/:id/status is open to any authenticated caller and forwards the role', async () => { + propertiesService.transitionStatus.mockResolvedValue({ id: 'p1', status: 'ACTIVE' }); + + await asUser( + request(app.getHttpServer()).patch('/properties/p1/status').send({ status: 'ACTIVE' }), + ).expect(200); + + expect(propertiesService.transitionStatus).toHaveBeenCalledWith( + 'p1', + 'ACTIVE', + 'user-1', + 'USER', + ); + }); + + it('PATCH /properties/:id/status requires authentication', async () => { + await request(app.getHttpServer()) + .patch('/properties/p1/status') + .send({ status: 'ACTIVE' }) + .expect(403); + expect(propertiesService.transitionStatus).not.toHaveBeenCalled(); + }); + + it('PATCH /properties/:id/status rejects a status outside the enum with 400', async () => { + await asUser( + request(app.getHttpServer()).patch('/properties/p1/status').send({ status: 'NOPE' }), + ).expect(400); + expect(propertiesService.transitionStatus).not.toHaveBeenCalled(); + }); + + it('surfaces a rejected transition as 400', async () => { + propertiesService.transitionStatus.mockRejectedValue( + new BadRequestException('Cannot transition property from DRAFT to SOLD'), + ); + + await asUser( + request(app.getHttpServer()).patch('/properties/p1/status').send({ status: 'SOLD' }), + ).expect(400); + }); + + it('PATCH approve and reject are admin-only', async () => { + await as('AGENT', request(app.getHttpServer()).patch('/properties/p1/approve')).expect(403); + await as('AGENT', request(app.getHttpServer()).patch('/properties/p1/reject')).expect(403); + expect(propertiesService.approveProperty).not.toHaveBeenCalled(); + expect(propertiesService.rejectProperty).not.toHaveBeenCalled(); + }); + + it('PATCH approve and reject call through for an admin', async () => { + propertiesService.approveProperty.mockResolvedValue({ id: 'p1', status: 'ACTIVE' }); + propertiesService.rejectProperty.mockResolvedValue({ id: 'p1', status: 'ARCHIVED' }); + + await as('ADMIN', request(app.getHttpServer()).patch('/properties/p1/approve')).expect(200); + await as('ADMIN', request(app.getHttpServer()).patch('/properties/p1/reject')).expect(200); + + expect(propertiesService.approveProperty).toHaveBeenCalledWith('p1', 'admin-1'); + expect(propertiesService.rejectProperty).toHaveBeenCalledWith('p1', 'admin-1'); + }); + }); + + describe('bulk operations', () => { + const ids = ['11111111-1111-4111-8111-111111111111', '22222222-2222-4222-8222-222222222222']; + + it('are admin-only', async () => { + await as( + 'AGENT', + request(app.getHttpServer()).post('/properties/bulk/status').send({ propertyIds: ids, status: 'ACTIVE' }), + ).expect(403); + expect(propertiesService.bulkUpdatePropertyStatus).not.toHaveBeenCalled(); + }); + + it('POST bulk/status validates the id list and forwards it', async () => { + propertiesService.bulkUpdatePropertyStatus.mockResolvedValue({ updatedCount: 2 }); + + await as( + 'ADMIN', + request(app.getHttpServer()) + .post('/properties/bulk/status') + .send({ propertyIds: ids, status: 'ARCHIVED' }), + ).expect(201); + + expect(propertiesService.bulkUpdatePropertyStatus).toHaveBeenCalledWith(ids, 'ARCHIVED'); + }); + + it('POST bulk/status rejects an empty id list with 400', async () => { + await as( + 'ADMIN', + request(app.getHttpServer()).post('/properties/bulk/status').send({ propertyIds: [], status: 'ACTIVE' }), + ).expect(400); + expect(propertiesService.bulkUpdatePropertyStatus).not.toHaveBeenCalled(); + }); + + it('POST bulk/status rejects a malformed id with 400', async () => { + await as( + 'ADMIN', + request(app.getHttpServer()) + .post('/properties/bulk/status') + .send({ propertyIds: ['not-a-uuid'], status: 'ACTIVE' }), + ).expect(400); + }); + + it('POST bulk/delete reports the ids that were actually removed', async () => { + propertiesService.bulkDeleteProperties.mockResolvedValue({ + deletedCount: 1, + propertyIds: [ids[0]], + }); + + const res = await as( + 'ADMIN', + request(app.getHttpServer()).post('/properties/bulk/delete').send({ propertyIds: ids }), + ).expect(201); + + expect(res.body).toEqual({ deletedCount: 1, propertyIds: [ids[0]] }); + expect(propertiesService.bulkDeleteProperties).toHaveBeenCalledWith(ids); + }); + + it('POST bulk/delete surfaces a foreign-key conflict as 409', async () => { + propertiesService.bulkDeleteProperties.mockRejectedValue( + new ConflictException('One or more properties are referenced by transactions or documents'), + ); + + await as( + 'ADMIN', + request(app.getHttpServer()).post('/properties/bulk/delete').send({ propertyIds: ids }), + ).expect(409); + }); + + it('POST bulk/export forwards the optional title filter', async () => { + propertiesService.bulkExportProperties.mockResolvedValue({ total: 0, data: [] }); + + await as( + 'ADMIN', + request(app.getHttpServer()) + .post('/properties/bulk/export') + .send({ propertyIds: ids, filter: 'loft' }), + ).expect(201); + + expect(propertiesService.bulkExportProperties).toHaveBeenCalledWith(ids, 'loft'); + }); + }); + + describe('agent assignment', () => { + it('POST /properties/:id/agents requires authentication', async () => { + await request(app.getHttpServer()) + .post('/properties/p1/agents') + .send({ agentId: 'agent-1' }) + .expect(403); + }); + + it('assigns an agent and forwards the caller', async () => { + propertiesService.assignAgent.mockResolvedValue({ id: 'pa1' }); + + await as( + 'USER', + request(app.getHttpServer()).post('/properties/p1/agents').send({ agentId: 'agent-1' }), + ).expect(201); + + expect(propertiesService.assignAgent).toHaveBeenCalledWith( + 'p1', + expect.objectContaining({ agentId: 'agent-1' }), + expect.objectContaining({ sub: 'user-1' }), + ); + }); + + it('PUT /properties/:id/agents/:agentId passes the agent id through', async () => { + propertiesService.updateAgentAssignment.mockResolvedValue({ id: 'pa1' }); + + await as( + 'USER', + request(app.getHttpServer()) + .put('/properties/p1/agents/agent-1') + .send({ commissionRate: 0.05 }), + ).expect(200); + + expect(propertiesService.updateAgentAssignment).toHaveBeenCalledWith( + 'p1', + 'agent-1', + expect.objectContaining({ commissionRate: 0.05 }), + expect.objectContaining({ sub: 'user-1' }), + ); + }); + + it('DELETE /properties/:id/agents/:agentId passes the agent id through', async () => { + propertiesService.removeAgentAssignment.mockResolvedValue({ id: 'pa1' }); + + await as( + 'USER', + request(app.getHttpServer()).delete('/properties/p1/agents/agent-1'), + ).expect(200); + + expect(propertiesService.removeAgentAssignment).toHaveBeenCalledWith( + 'p1', + 'agent-1', + expect.objectContaining({ sub: 'user-1' }), + ); + }); + + it('surfaces a missing assignment as 404', async () => { + propertiesService.removeAgentAssignment.mockRejectedValue( + new NotFoundException('Agent assignment not found for this property'), + ); + + await as( + 'USER', + request(app.getHttpServer()).delete('/properties/p1/agents/agent-1'), + ).expect(404); + }); + + it('GET /properties/:id/agents is readable without authentication', async () => { + propertiesService.getAgents.mockResolvedValue([{ id: 'pa1', agentId: 'agent-1' }]); + + const res = await request(app.getHttpServer()).get('/properties/p1/agents').expect(200); + + expect(res.body).toHaveLength(1); + expect(propertiesService.getAgents).toHaveBeenCalledWith('p1'); + }); + }); + + describe('amenities', () => { + it('POST /properties/:id/amenities validates the body', async () => { + await as('USER', request(app.getHttpServer()).post('/properties/p1/amenities').send({})).expect(400); + expect(propertiesService.addAmenity).not.toHaveBeenCalled(); + }); + + it('adds an amenity', async () => { + propertiesService.addAmenity.mockResolvedValue({ id: 'am1' }); + + await as( + 'USER', + request(app.getHttpServer()) + .post('/properties/p1/amenities') + .send({ name: 'Pool', amenityType: 'OUTDOOR' }), + ).expect(201); + + expect(propertiesService.addAmenity).toHaveBeenCalledWith( + 'p1', + expect.objectContaining({ name: 'Pool', amenityType: 'OUTDOOR' }), + ); + }); + + it('GET /properties/:id/amenities is readable without authentication', async () => { + propertiesService.getAmenities.mockResolvedValue([{ id: 'am1' }]); + + const res = await request(app.getHttpServer()).get('/properties/p1/amenities').expect(200); + + expect(res.body).toHaveLength(1); + }); + + it('PATCH and DELETE amenities pass both ids through', async () => { + propertiesService.updateAmenity.mockResolvedValue({ id: 'am1' }); + propertiesService.removeAmenity.mockResolvedValue({ id: 'am1' }); + + await as( + 'USER', + request(app.getHttpServer()).patch('/properties/p1/amenities/am1').send({ isAvailable: false }), + ).expect(200); + await as('USER', request(app.getHttpServer()).delete('/properties/p1/amenities/am1')).expect(200); + + expect(propertiesService.updateAmenity).toHaveBeenCalledWith( + 'p1', + 'am1', + expect.objectContaining({ isAvailable: false }), + ); + expect(propertiesService.removeAmenity).toHaveBeenCalledWith('p1', 'am1'); + }); + }); + + describe('report download', () => { + it('streams a PDF and sets the attachment headers', async () => { + propertyReportService.generatePropertyReport.mockResolvedValue(Buffer.from('%PDF-1.4 test')); + + const res = await as('USER', request(app.getHttpServer()).get('/properties/p1/report')).expect(200); + + expect(res.headers['content-type']).toMatch(/application\/pdf/); + expect(res.headers['content-disposition']).toContain('property-report-p1.pdf'); + }); + + it('answers 404 when the listing does not exist', async () => { + propertyReportService.generatePropertyReport.mockRejectedValue(new NotFoundException('nope')); + + await as('USER', request(app.getHttpServer()).get('/properties/missing/report')).expect(404); + }); + + it('does not leak internals when the report cannot be built', async () => { + propertyReportService.generatePropertyReport.mockRejectedValue(new Error('pdfkit exploded')); + + const res = await as('USER', request(app.getHttpServer()).get('/properties/p1/report')).expect(500); + + expect(JSON.stringify(res.body)).not.toContain('pdfkit'); + }); + }); });