Tests, bug fix, and PATCH /tasks/:id/assign endpoint

BUGS.md

@@ -0,0 +1,151 @@
+# Bug Report: Task Manager API
+
+This document details the bugs identified in the Task Manager API through comprehensive unit and integration testing. Entries are ordered by severity, with the most impactful bugs listed first.
+
+---
+
+## Bug 1: Pagination offset calculation skips page 1 entirely
+
+**Location:** `src/services/taskService.js`, function `getPaginated` (line 12)
+
+**Expected behavior:** In a standard 1-indexed pagination API (where page 1 is default, as handled in routes via `parseInt(page) || 1`), querying `page=1` with `limit=10` must compute an offset of `0` and return the first 10 items (index 0 to 9, e.g. `Task 1` through `Task 10`).
+
+**Actual behavior:** `offset` is computed as `const offset = page * limit;`. For `page=1` and `limit=10`, `offset` evaluates to `10`. As a result, requesting `page=1` skips the first 10 tasks and returns items 11 through 20. Requesting `page=2` returns items 21 through 30. The first `limit` tasks in the store can never be retrieved by standard 1-indexed pagination.
+
+**How discovered:** 
+- Unit test: `tests/taskService.test.js` -> `getPaginated() -> returns the first page (items 1 to 10) for page=1 and limit=10`
+- Integration test: `tests/tasks.routes.test.js` -> `GET /tasks?page=&limit= -> returns page 1 containing the first 10 items (Task 1 to Task 10)`
+
+**Root cause:** The calculation treats the 1-based `page` parameter as a 0-based page index. It fails to convert the 1-indexed page number into a zero-based slice offset.
+
+**Suggested fix:**
+```javascript
+const getPaginated = (page, limit) => {
+  const pageNum = Math.max(1, page);
+  const offset = (pageNum - 1) * limit;
+  return tasks.slice(offset, offset + limit);
+};
+```
+
+**Severity/impact:** **Critical / High**. In production, all API clients requesting the first page of tasks will silently miss the most recent / initial `limit` tasks. If a user has fewer tasks than `limit` (e.g., 5 tasks total), `GET /tasks?page=1&limit=10` returns an empty array `[]`, making it appear that no tasks exist.
+
+**Fix Notes:**
+Fixed in `src/services/taskService.js` by converting the 1-based page number to a 0-based offset with `const pageNum = Math.max(1, page); const offset = (pageNum - 1) * limit;`. Using `Math.max(1, page)` defensively guards against non-positive integers (e.g. `0` or negative numbers), ensuring deterministic pagination that always maps to valid slice offsets. All unit and route integration pagination tests now pass cleanly with 100% boundary accuracy without impacting any other endpoints.
+
+---
+
+## Bug 2: `completeTask` silently mutates and resets task `priority` to `'medium'`
+
+**Location:** `src/services/taskService.js`, function `completeTask` (line 69)
+
+**Expected behavior:** Marking a task as complete via `completeTask(id)` (and `PATCH /tasks/:id/complete`) should only change `status` to `'done'` and assign `completedAt`. All other task properties, including `priority`, must remain unchanged.
+
+**Actual behavior:** The function constructs the updated object with a hardcoded `priority: 'medium'`, overwriting existing `'high'` or `'low'` priorities upon task completion.
+
+**How discovered:**
+- Unit test: `tests/taskService.test.js` -> `completeTask() -> preserves the original priority when marking a task as complete`
+- Integration test: `tests/tasks.routes.test.js` -> `PATCH /tasks/:id/complete -> preserves task priority when marking as complete`
+
+**Root cause:** The updated object literal in `completeTask` contains `priority: 'medium'`, likely a copy-paste artifact from the default task template in `create()`.
+
+**Suggested fix:**
+```javascript
+const completeTask = (id) => {
+  const task = findById(id);
+  if (!task) return null;
+
+  const updated = {
+    ...task,
+    status: 'done',
+    completedAt: new Date().toISOString(),
+  };
+
+  const index = tasks.findIndex((t) => t.id === id);
+  tasks[index] = updated;
+  return updated;
+};
+```
+
+**Severity/impact:** **High**. Data corruption of task metadata upon state transition. Critical audit records and priority-based filtering on completed tasks become permanently inaccurate.
+
+---
+
+## Bug 3: `update` allows client to overwrite immutable server-managed fields (`id` and `createdAt`)
+
+**Location:** `src/services/taskService.js`, function `update` (line 50)
+
+**Expected behavior:** Updating a task via `PUT /tasks/:id` should only update mutable fields (`title`, `description`, `status`, `priority`, `dueDate`). Server-generated identity and timestamp properties (`id`, `createdAt`) must be protected from client modification.
+
+**Actual behavior:** `update` performs an unchecked shallow merge `const updated = { ...tasks[index], ...fields };`. If a client passes `id` or `createdAt` in the request body, those fields overwrite the internal record values.
+
+**How discovered:**
+- Unit test: `tests/taskService.test.js` -> `update() -> does not allow overwriting server-managed immutable fields (id and createdAt)`
+- Integration test: `tests/tasks.routes.test.js` -> `PUT /tasks/:id -> does not allow overriding server-controlled fields (id, createdAt) via PUT body`
+
+**Root cause:** Absence of field whitelisting or sanitization before merging the incoming `fields` object onto the existing task record.
+
+**Suggested fix:**
+```javascript
+const update = (id, fields) => {
+  const index = tasks.findIndex((t) => t.id === id);
+  if (index === -1) return null;
+
+  const { id: _id, createdAt: _createdAt, ...allowedFields } = fields;
+  const updated = { ...tasks[index], ...allowedFields };
+  tasks[index] = updated;
+  return updated;
+};
+```
+
+**Severity/impact:** **High (Security & Data Integrity)**. A client can maliciously or accidentally alter a task's UUID or backdate its `createdAt` timestamp, corrupting data integrity and breaking client lookup references.
+
+---
+
+## Bug 4: Status filtering uses substring `.includes()` matching instead of exact equality
+
+**Location:** `src/services/taskService.js`, function `getByStatus` (line 9)
+
+**Expected behavior:** `getByStatus(status)` (and `GET /tasks?status=...`) should only return tasks whose `status` exactly matches the requested string (e.g. `'todo'` only returns tasks with status `'todo'`). A search for a substring like `'do'` should return no tasks since `'do'` is not a valid task status.
+
+**Actual behavior:** `getByStatus` uses `tasks.filter((t) => t.status.includes(status))`. Because `'do'` is a substring of both `'todo'` and `'done'`, querying `status=do` returns tasks with both `todo` and `done` statuses.
+
+**How discovered:**
+- Unit test: `tests/taskService.test.js` -> `getByStatus() -> does not return tasks whose status merely contains the search term as a substring`
+- Integration test: `tests/tasks.routes.test.js` -> `GET /tasks?status= -> does not return tasks on partial substring matches for status query`
+
+**Root cause:** The filter callback uses `String.prototype.includes` instead of strict equality `===`.
+
+**Suggested fix:**
+```javascript
+const getByStatus = (status) => tasks.filter((t) => t.status === status);
+```
+
+**Severity/impact:** **Medium**. Causes unintended task matching and breaks UI views that rely on precise category filtering.
+
+---
+
+## Bug 5: Global error handler converts malformed JSON syntax errors into 500 Internal Server Errors
+
+**Location:** `src/app.js`, error middleware (lines 9–12)
+
+**Expected behavior:** When a client sends malformed JSON with `Content-Type: application/json`, Express's `body-parser` raises a 400 Bad Request error. The API should respond with an HTTP 400 status code indicating a client error.
+
+**Actual behavior:** The error middleware catches the `SyntaxError` and indiscriminately responds with `500 { error: 'Internal server error' }`.
+
+**How discovered:**
+- Integration test: `tests/tasks.routes.test.js` -> `Malformed JSON handling -> responds with 400 Client Error rather than 500 Internal Server Error when receiving malformed JSON`
+
+**Root cause:** The error handler does not check `err.status` or `err instanceof SyntaxError` before defaulting to status 500.
+
+**Suggested fix:**
+```javascript
+app.use((err, req, res, next) => {
+  if (err instanceof SyntaxError && err.status === 400 && 'body' in err) {
+    return res.status(400).json({ error: 'Invalid JSON payload' });
+  }
+  console.error(err.stack);
+  res.status(500).json({ error: 'Internal server error' });
+});
+```
+
+**Severity/impact:** **Medium / Low**. Misleads clients and monitoring tools into treating client payload syntax errors as server crashes.

README.md

@@ -65,7 +65,7 @@ ASSIGNMENT.md               # Full brief — read this first
 | `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                  |
 
 ### Task shape
 
@@ -74,11 +74,12 @@ 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",
   "completedAt": "ISO 8601 or null",
-  "createdAt": "ISO 8601"
+  "createdAt": "ISO 8601",
+  "assignee": "string or null"
 }
 ```
 
@@ -93,14 +94,21 @@ 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"
 ```
 
 **Mark complete**
 ```bash
 curl -X PATCH http://localhost:3000/tasks/<id>/complete
 ```
 
+**Assign a task**
+```bash
+curl -X PATCH http://localhost:3000/tasks/<id>/assign \
+  -H "Content-Type: application/json" \
+  -d '{"assignee": "Soumya"}'
+```
+
 ---
 
 ## What to Submit

SUBMISSION.md

@@ -0,0 +1,132 @@
+# Take-Home Assignment Submission: Task Manager API
+
+**Candidate:** Senior Full-Stack Node.js / SDET  
+**Date:** August 2026  
+**Repository:** [Task Manager API](.)  
+
+---
+
+## 1. Test Coverage Summary
+
+Full test suite execution with Jest and Supertest across all unit and route integration test files (`tests/taskService.test.js`, `tests/tasks.routes.test.js`, `tests/tasks.assign.test.js`):
+
+```
+-----------------|---------|----------|---------|---------|-------------------
+File             | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
+-----------------|---------|----------|---------|---------|-------------------
+All files        |   98.73 |    98.87 |   96.66 |   98.61 |                   
+ src             |   84.61 |       75 |      50 |   84.61 |                   
+  app.js         |   84.61 |       75 |      50 |   84.61 | 17-18             
+ src/routes      |     100 |      100 |     100 |     100 |                   
+  tasks.js       |     100 |      100 |     100 |     100 |                   
+ src/services    |     100 |      100 |     100 |     100 |                   
+  taskService.js |     100 |      100 |     100 |     100 |                   
+ src/utils       |     100 |      100 |     100 |     100 |                   
+  validators.js  |     100 |      100 |     100 |     100 |                   
+-----------------|---------|----------|---------|---------|-------------------
+
+Test Suites: 3 passed, 3 total
+Tests:       80 passed, 80 total
+Snapshots:   0 total
+Time:        5.119 s
+```
+
+> **Note on uncovered lines in `src/app.js` (lines 17–18):**  
+> Lines 17–18 contain `if (require.main === module) { app.listen(PORT, ...); }`. In the automated test environment, `app` is imported directly into Supertest without binding a live TCP socket port, which is the intended design for clean test isolation.
+
+---
+
+## 2. Discovered Bugs & Fix
+
+A comprehensive bug report detailing all 5 discovered bugs is documented in **[`BUGS.md`](./BUGS.md)**.
+
+### Summary of Bugs:
+1. **Bug 1 (Critical / High — FIXED):** `taskService.getPaginated` calculated `offset = page * limit`, causing `page=1` to skip the first 10 items.
+2. **Bug 2 (High):** `taskService.completeTask` silently reset task `priority` to `'medium'`.
+3. **Bug 3 (High - Data Integrity):** `taskService.update` allowed client payloads to overwrite internal `id` and `createdAt` timestamps.
+4. **Bug 4 (Medium):** `taskService.getByStatus` used `.includes()` substring matching instead of exact equality.
+5. **Bug 5 (Medium):** Express global error handler returned HTTP 500 on client malformed JSON syntax errors.
+
+### Implemented Fix: Bug 1 (Pagination Offset Math)
+- **Location:** `src/services/taskService.js` (`getPaginated`)
+- **Fix:** Replaced `page * limit` with `const pageNum = Math.max(1, page); const offset = (pageNum - 1) * limit;`.
+- **Rationale:** Highest impact and lowest blast radius. It ensures `page=1` correctly maps to offset 0 while defensively guarding against 0 or negative page numbers.
+
+---
+
+## 3. New Feature: `PATCH /tasks/:id/assign`
+
+### Endpoint Specification
+- **Method & Route:** `PATCH /tasks/:id/assign`
+- **Request Body:** `{ "assignee": "string" }`
+- **Response Codes:**
+  - `200 OK`: Returns the updated task object with `assignee` populated.
+  - `400 Bad Request`: Validation failure (missing `assignee`, non-string, empty or whitespace-only, or exceeding 100 characters).
+  - `404 Not Found`: Task with the given `:id` does not exist.
+
+### Design Decisions
+
+1. **Reassignment Policy (Idempotent PATCH Semantics):**  
+   We decided to **allow free reassignment** of already-assigned tasks rather than returning a 409 Conflict. In task management domains (e.g. Jira, Linear, Asana), tickets are frequently handed off between team members. Modeling reassignment as an idempotent `PATCH` mutation adheres to standard REST semantics and avoids requiring clients to perform an artificial unassign-then-assign round trip.
+
+2. **Validation & Guards:**  
+   - The validator `validateAssignTask` requires `assignee` to be present and of `string` type.
+   - It strips leading/trailing whitespace (`.trim()`) and rejects empty or whitespace-only strings.
+   - A maximum length guard of **100 characters** is enforced to comfortably accommodate personal names, full names with titles, or email identifiers (`user@company.com`), while guarding the in-memory store against memory bloat or DoS attacks.
+   - The stored value is the sanitized (trimmed) string.
+
+3. **Task Shape & `assignee: null` Default:**  
+   We updated `taskService.create()` to initialize `assignee: null` by default on new tasks. Maintaining a predictable, uniform object schema prevents client-side `undefined` property errors and ensures consistent serialization across API responses.
+
+---
+
+## 4. Documentation Inconsistencies Identified
+
+1. **Status Enum Inconsistency (`README.md` vs `ASSIGNMENT.md` / Code):**  
+   - `README.md` lists status values as `"pending | in-progress | completed"`.
+   - `ASSIGNMENT.md` and the actual runtime validators (`src/utils/validators.js`) enforce `"todo | in_progress | done"`.  
+   - *Resolution:* All tests and features conform to `"todo | in_progress | done"`, which is what the server actively enforces.
+
+2. **`PUT` Endpoint Semantics (`README.md` vs Implementation):**  
+   - `README.md` describes `PUT /tasks/:id` as a *"Full update of a task"*.
+   - However, `src/routes/tasks.js` and `src/utils/validators.js` allow partial updates (all fields in `PUT` are optional), and `taskService.update` performs a shallow merge (`{ ...tasks[index], ...fields }`).  
+   - *Recommendation:* In a future iteration, either rename `PUT` to `PATCH /tasks/:id` for partial updates or make all mandatory fields required for `PUT` to enforce strict REST full replacement semantics.
+
+---
+
+## 5. What I'd Test Next (Given More Time)
+
+1. **In-Memory Store Concurrency & Race Conditions:**  
+   Simulate high-concurrency scenarios (e.g. 50 parallel requests creating, updating, and deleting tasks) to evaluate race conditions on the mutable module-level `tasks` array.
+2. **Missing Single-Resource Endpoint (`GET /tasks/:id`):**  
+   Currently, there is no route to retrieve a single task by its ID (`GET /tasks/:id`), forcing clients to fetch all tasks and filter client-side. Adding and testing this endpoint is a high-priority gap.
+3. **Request Payload Size & Rate Limiting:**  
+   Test payload size limits (`express.json({ limit: '100kb' })`) and add rate-limiting integration tests (`express-rate-limit`) to protect public endpoints.
+4. **Timezone & Date Parsing Edge Cases:**  
+   Deep testing of edge cases around ISO 8601 dates: UTC offsets, leap years, daylight saving transitions, and invalid calendar dates like `2026-02-30`.
+5. **Combined Query Filters (`GET /tasks?status=todo&page=1&limit=10`):**  
+   Currently, the route handler evaluates `if (status)` before `if (page || limit)`, meaning status filtering completely ignores pagination query parameters. Testing and supporting combined filtering + pagination is essential.
+
+---
+
+## 6. What Surprised Me in the Codebase
+
+- **Substring Matching for Status Filtering:** `taskService.getByStatus` used `t.status.includes(status)` rather than `===`. This caused queries like `status=do` to match both `todo` and `done` tasks simultaneously.
+- **Unintended Side Effect in `completeTask`:** `completeTask` explicitly hardcoded `priority: 'medium'`, silently wiping out `'high'` or `'low'` priority settings whenever a task was completed.
+- **Direct Multiplier for Pagination:** `getPaginated` computed `offset = page * limit` without converting 1-based page numbers to 0-based index offsets, meaning page 1 immediately started at index 10 and skipped the first 10 items.
+- **Global Error Handler Masking 400s:** The error middleware caught Express `body-parser` JSON syntax errors and unconditionally returned HTTP 500 instead of HTTP 400.
+
+---
+
+## 7. Questions I'd Ask Before Shipping to Production
+
+1. **Storage & Durability Strategy:**  
+   *What database will replace the in-memory array (e.g. PostgreSQL, DynamoDB, MongoDB), and what transaction isolation level is needed for atomic task state transitions?*
+2. **User Identity & Assignee Relation:**  
+   *Should `assignee` remain a free-form string, or should it validate against a real User Service / Auth provider with UUID foreign keys?*
+3. **Pagination Semantics & Contract:**  
+   *Should we standardize on 1-indexed offset pagination with total count metadata in the response headers/envelope (`{ data: [...], total, page, totalPages }`), or migrate to cursor-based pagination for large datasets?*
+4. **API Versioning & Breaking Changes:**  
+   *Before fixing `PUT` full update semantics and status enum documentation mismatches, are there existing mobile or web clients in production that rely on the partial merge behavior or existing enum strings?*
+5. **Authentication & Authorization:**  
+   *What authentication mechanism (e.g. JWT, OAuth2) and permission model (e.g. can any user assign/complete any task, or only task creators and assignees?) should be integrated before public deployment?*

task-api/src/routes/tasks.js

@@ -1,7 +1,7 @@
 const express = require('express');
 const router = express.Router();
 const taskService = require('../services/taskService');
-const { validateCreateTask, validateUpdateTask } = require('../utils/validators');
+const { validateCreateTask, validateUpdateTask, validateAssignTask } = require('../utils/validators');
 
 router.get('/stats', (req, res) => {
   const stats = taskService.getStats();
@@ -69,4 +69,18 @@ router.patch('/:id/complete', (req, res) => {
   res.json(task);
 });
 
+router.patch('/:id/assign', (req, res) => {
+  const error = validateAssignTask(req.body);
+  if (error) {
+    return res.status(400).json({ error });
+  }
+
+  const task = taskService.assignTask(req.params.id, req.body.assignee.trim());
+  if (!task) {
+    return res.status(404).json({ error: 'Task not found' });
+  }
+
+  res.json(task);
+});
+
 module.exports = router;