Take-home: add full test coverage, fix core API bugs, and implement task assignment endpoint

README.md

@@ -36,36 +36,87 @@ npm run coverage   # run with coverage report
 
 ---
 
+## Current Status
+
+- Test suites: 4 passed
+- Tests: 47 passed
+- Coverage: 100% statements, 100% branches, 100% functions, 100% lines
+- In-memory data model with deterministic test reset support
+
+---
+
 ## Project Structure
 
 ```
+.
+  ASSIGNMENT.md
+  README.md
 task-api/
+  BUG_REPORT.md
   src/
-    app.js                  # Express app setup
-    routes/tasks.js         # Route handlers
-    services/taskService.js # Business logic + in-memory data store
-    utils/validators.js     # Input validation helpers
-  tests/                    # Your tests go here
+    app.js                     # Express app setup, middleware, error handling, startup helper
+    routes/tasks.js            # All /tasks route handlers and HTTP status mapping
+    services/taskService.js    # In-memory store + business rules
+    utils/validators.js        # Pure request/query validation functions
+  tests/
+    app.bootstrap.test.js      # startServer() behavior and logging
+    tasks.routes.test.js       # Integration tests with supertest
+    taskService.test.js        # Service unit tests
+    validators.test.js         # Validator unit tests
   package.json
   jest.config.js
-ASSIGNMENT.md               # Full brief — read this first
 ```
 
 > The data store is in-memory. It resets every time the server restarts.
 
 ---
 
+## Request Flow
+
+The API request lifecycle is:
+
+1. Express app receives request in `src/app.js`
+2. `/tasks` routes dispatch in `src/routes/tasks.js`
+3. Route-level validation runs via `src/utils/validators.js`
+4. Business logic/state mutation happens in `src/services/taskService.js`
+5. Route maps service output to HTTP response codes and payloads
+
+---
+
 ## API Reference
 
 | Method   | Path                      | Description                              |
 |----------|---------------------------|------------------------------------------|
-| `GET`    | `/tasks`                  | List all tasks. Supports `?status=`, `?page=`, `?limit=` |
+| `GET`    | `/tasks`                  | List tasks. Supports `?status=`, `?assignee=`, `?page=`, `?limit=` |
+| `GET`    | `/tasks/:id`              | Get a single task by ID                  |
 | `POST`   | `/tasks`                  | Create a new task                        |
 | `PUT`    | `/tasks/:id`              | Full update of a task                    |
 | `DELETE` | `/tasks/:id`              | Delete a task (returns 204)              |
 | `PATCH`  | `/tasks/:id/complete`     | Mark a task as complete                  |
 | `GET`    | `/tasks/stats`            | Counts by status + overdue count         |
-| `PATCH`  | `/tasks/:id/assign`       | **Assign a task to a user** _(to implement)_ |
+| `PATCH`  | `/tasks/:id/assign`       | Assign a task to a user                  |
+
+### Endpoint Behavior Notes
+
+- `GET /tasks`
+  - Applies validation before processing query values
+  - Applies filtering first (`status`, then `assignee`), then pagination (`page`, `limit`)
+  - Assignee filtering is case-insensitive to tolerate client-side casing differences
+- `PUT /tasks/:id`
+  - Validates request body before existence lookup
+  - Ignores immutable fields (like `id` and `createdAt`) at the service layer
+- `PATCH /tasks/:id/complete`
+  - Sets `status` to `done`
+  - Sets `completedAt` when needed
+  - Preserves unrelated fields like `priority`
+- `PATCH /tasks/:id/assign`
+  - `400` for invalid payload
+  - `404` when task does not exist
+  - `409` when already assigned to a different user (state conflict; prevents silent ownership overwrite)
+  - `200` idempotent success when assigning to the same user (repeat call does not change resource state)
+- `GET /tasks/stats`
+  - Returns `todo`, `in_progress`, `done`, and `overdue`
+  - Overdue count excludes tasks already marked `done`
 
 ### Task shape
 
@@ -74,14 +125,21 @@ ASSIGNMENT.md               # Full brief — read this first
   "id": "uuid",
   "title": "string",
   "description": "string",
-  "status": "pending | in-progress | completed",
+  "status": "todo | in_progress | done",
   "priority": "low | medium | high",
   "dueDate": "ISO 8601 or null",
+  "assignee": "string | null",
   "completedAt": "ISO 8601 or null",
   "createdAt": "ISO 8601"
 }
 ```
 
+Additional model notes:
+
+- `id` and `createdAt` are treated as immutable once created
+- `dueDate` can be `null` to represent no deadline
+- `assignee` can be `null` in general update flows (unassignment)
+
 ### Sample requests
 
 **Create a task**
@@ -93,14 +151,56 @@ curl -X POST http://localhost:3000/tasks \
 
 **List tasks with filter**
 ```bash
-curl "http://localhost:3000/tasks?status=pending&page=1&limit=10"
+curl "http://localhost:3000/tasks?status=todo&page=1&limit=10"
+```
+
+**List tasks by assignee**
+```bash
+curl "http://localhost:3000/tasks?assignee=alice"
 ```
 
 **Mark complete**
 ```bash
 curl -X PATCH http://localhost:3000/tasks/<id>/complete
 ```
 
+**Assign task**
+```bash
+curl -X PATCH http://localhost:3000/tasks/<id>/assign \
+  -H "Content-Type: application/json" \
+  -d '{"assignee":"Alice"}'
+```
+
+If a task is already assigned to a different person, `/tasks/:id/assign` returns `409 Conflict` to prevent accidental reassignment.
+
+---
+
+## Testing
+
+The suite is split by responsibility:
+
+- `tests/validators.test.js` validates pure request/query validation rules
+- `tests/taskService.test.js` validates in-memory business logic and edge cases
+- `tests/tasks.routes.test.js` validates HTTP contract and route-service integration
+- `tests/app.bootstrap.test.js` validates startup helper behavior
+
+Run commands:
+
+```bash
+cd task-api
+npm test
+npm run coverage
+```
+
+---
+
+## Bug Report
+
+This repository now includes:
+
+- `task-api/BUG_REPORT.md` with expected vs actual behavior for discovered defects
+- Detailed root-cause analysis, impact, and fix rationale for each resolved issue
+
 ---
 
 ## What to Submit
@@ -111,3 +211,26 @@ See [ASSIGNMENT.md](./ASSIGNMENT.md) for full submission requirements. At minimu
 - **Bug report** — what you found, where in the code, and why it's a bug (not just symptoms)
 - **At least one fix** — with a note on your approach
 - **`PATCH /tasks/:id/assign` implementation** — plus a short explanation of any design decisions (validation, edge cases, etc.)
+
+
+## Feature
+
+## The primary new feature added is the task assignment endpoint:
+
+- PATCH /tasks/:id/assign
+- Accepts body with assignee as a non-empty string
+- Stores assignee on the task and returns the updated task
+- Returns 404 if the task does not exist
+- Returns 409 if the task is already assigned to a different user
+- Is idempotent for the same assignee (repeating the same assign request keeps returning success)
+
+**Design decisions for assignment behavior:**
+
+- `409 Conflict` is used for reassignment attempts because the payload is valid, but the request conflicts with current resource ownership state.
+- Repeating assignment to the same assignee returns `200 OK` to keep the endpoint idempotent and safe for retries.
+- Assignee filtering for `GET /tasks?assignee=` is case-insensitive so clients with mixed casing still get consistent results.
+
+**Alongside that, we also added related enhancements:**
+
+- GET /tasks/:id for single-task fetch
+- Optional assignee filtering on GET /tasks via query params

task-api/BUG_REPORT.md

@@ -0,0 +1,134 @@
+# Bug Report
+
+This report documents each bug found in the Task Manager API, why it happened, the impact, and exactly what code was changed.
+
+## 1) Pagination Off-By-One
+
+- Location: src/services/taskService.js (getPaginated)
+- Expected behavior:
+	Page 1 should start from index 0, so page=1 and limit=10 returns the first 10 tasks.
+- Actual behavior:
+	The service calculated offset as page * limit, so page=1 started at index 10 and skipped the first page entirely.
+- Why this happened:
+	The implementation mixed one-based page semantics (API contract) with zero-based offset math (array slicing).
+- Risk/impact:
+	Clients received incomplete or empty lists for first-page requests, causing incorrect UI rendering and misleading pagination.
+- How it was discovered:
+	Integration test for GET /tasks?page=1&limit=2 returned the wrong rows.
+- What changed:
+	Offset was corrected to (page - 1) * limit.
+	Additional safety was added by clamping page and limit to minimum 1 in the service.
+- Validation added:
+	Unit tests now assert one-based page behavior and clamp behavior for invalid numeric inputs.
+
+## 2) Status Filter Matched Partial Strings
+
+- Location: src/services/taskService.js (getByStatus)
+- Expected behavior:
+	Status filtering should only match exact status values: todo, in_progress, done.
+- Actual behavior:
+	Filtering used string includes, so partial fragments (for example, "pro") could match in_progress.
+- Why this happened:
+	Substring matching was used for a field that is categorical/enumerated, not free text.
+- Risk/impact:
+	Incorrect task subsets were returned, which can inflate or hide work items in dashboards.
+- How it was discovered:
+	Unit test intentionally passed a status fragment and observed false-positive matches.
+- What changed:
+	Matching was switched from includes to strict equality.
+- Validation added:
+	Service test now verifies partial status values produce zero matches.
+
+## 3) Completing a Task Overwrote Priority
+
+- Location: src/services/taskService.js (completeTask)
+- Expected behavior:
+	Completing a task should set status to done and set completedAt, without mutating unrelated fields.
+- Actual behavior:
+	The function forcibly reset priority to medium.
+- Why this happened:
+	Completion logic included an unrelated field mutation, likely from an earlier assumption that completed tasks should be normalized.
+- Risk/impact:
+	Business-critical prioritization data was silently lost when users completed tasks.
+- How it was discovered:
+	Route-level test for PATCH /tasks/:id/complete asserted original priority should remain unchanged and failed.
+- What changed:
+	Priority overwrite was removed from completion flow.
+	Completion timestamp logic now sets completedAt only when needed and preserves existing completion timestamps.
+- Validation added:
+	Unit and integration tests verify completion does not change priority.
+
+## 4) Unsafe Update Allowed Immutable Field Overrides
+
+- Location: src/services/taskService.js (update)
+- Expected behavior:
+	Immutable fields (id, createdAt) must never be client-overwritable.
+- Actual behavior:
+	Update merged arbitrary request payload keys into persisted task objects.
+- Why this happened:
+	Broad object spread was used without a field allow-list.
+- Risk/impact:
+	Data integrity and traceability could break (identity spoofing, createdAt tampering).
+- How it was discovered:
+	Unit test passed id and createdAt in update payload and observed persisted values change.
+- What changed:
+	Update now uses a strict mutable-field allow-list:
+	title, description, status, priority, dueDate, assignee.
+	Unknown or immutable keys are ignored.
+	Mutable string fields are normalized (trimmed) before persistence.
+- Validation added:
+	Unit tests verify immutable fields remain unchanged and normalization is applied.
+
+## 5) Missing List Query Validation
+
+- Location: src/routes/tasks.js and src/utils/validators.js
+- Expected behavior:
+	Invalid list query params should fail fast with clear 400 responses.
+- Actual behavior:
+	Invalid status/page/limit values were accepted, causing inconsistent behavior and ambiguous API responses.
+- Why this happened:
+	Route accepted raw query params and only loosely parsed page/limit defaults.
+- Risk/impact:
+	Clients could send logically invalid requests and receive inconsistent data, making troubleshooting difficult.
+- How it was discovered:
+	Integration tests for status=invalid, page=0, and limit=999 failed expected validation behavior.
+- What changed:
+	Added validateListQuery with strict checks:
+	status must be one of todo/in_progress/done,
+	page and limit must be positive integers,
+	limit has an upper bound of 100.
+	Route now validates before data access.
+- Validation added:
+	Route and validator tests now cover valid and invalid query combinations.
+
+## 6) Assignment Endpoint Missing
+
+- Location: src/routes/tasks.js and src/services/taskService.js
+- Expected behavior:
+	PATCH /tasks/:id/assign should exist and support safe assignment behavior.
+- Actual behavior:
+	Endpoint did not exist in initial codebase.
+- Why this happened:
+	Feature was defined in assignment scope but never implemented in source.
+- Risk/impact:
+	API contract was incomplete and downstream clients could not assign ownership.
+- How it was discovered:
+	Contract review against assignment requirements during integration test planning.
+- What changed:
+	Added PATCH /tasks/:id/assign route and service implementation with:
+	payload validation (non-empty string assignee),
+	404 on missing task,
+	409 on reassignment to a different user,
+	idempotent success when assigning the same user again.
+	Design decision rationale:
+	409 Conflict is used for reassignment because the request conflicts with current task ownership state (rather than being a malformed payload), and repeated assignment to the same user returns success to preserve idempotency.
+- Validation added:
+	Integration tests cover happy path, empty assignee, missing task, idempotent repeat, and conflict behavior.
+
+## 7) Additional Quality Fixes (Non-functional Defects)
+
+- Added GET /tasks/:id for direct single-task retrieval.
+- Added case-insensitive assignee filtering for GET /tasks to avoid client-casing mismatches.
+- Added consistent JSON 404 response for unknown routes.
+- Added defensive copy behavior from service methods to prevent accidental external mutation of in-memory state.
+- Added explicit comments in updated source sections for maintainability and reviewer clarity.