bug fixes

task-api/BUG_REPORT.md

@@ -0,0 +1,134 @@
+# Bug Report — Task Manager API
+
+This document describes bugs discovered through systematic testing of the Task Manager API codebase. Each bug includes the expected behaviour, actual behaviour, how it was discovered, and a proposed fix.
+
+---
+
+## Bug 1: `getByStatus` uses substring match instead of strict equality
+
+**Severity:** Medium  
+**File:** `src/services/taskService.js`, line 9  
+**Code:** `tasks.filter((t) => t.status.includes(status))`
+
+### Expected Behaviour
+`GET /tasks?status=done` should return **only** tasks with status exactly equal to `"done"`.
+
+### Actual Behaviour
+Because `String.prototype.includes()` performs a **substring** search, filtering by `"do"` returns both `"done"` and `"todo"` tasks. Similarly, `"in"` would match `"in_progress"`. Any partial string that appears inside a valid status will produce false positives.
+
+### How I Discovered It
+While writing unit tests for `getByStatus`, I added an edge-case test that filters using the substring `"do"`. The test confirmed the function returns tasks with statuses `"todo"` and `"done"`, which should not happen with strict matching.
+
+### Proposed Fix
+Replace `.includes()` with strict equality (`===`):
+
+```diff
+- const getByStatus = (status) => tasks.filter((t) => t.status.includes(status));
++ const getByStatus = (status) => tasks.filter((t) => t.status === status);
+```
+
+---
+
+## Bug 2: `getPaginated` has an off-by-one error (**FIXED**)
+
+**Severity:** High  
+**File:** `src/services/taskService.js`, line 12 (original)  
+**Code:** `const offset = page * limit;`
+
+### Expected Behaviour
+`GET /tasks?page=1&limit=10` should return the **first** 10 tasks (indices 0–9).
+
+### Actual Behaviour
+Because the offset is calculated as `page * limit` instead of `(page - 1) * limit`, page 1 starts at index 10, completely skipping the first page of results. Page 2 starts at index 20, etc.
+
+### How I Discovered It
+My integration test for `GET /tasks?page=1&limit=2` with 5 tasks expected `Task 1` and `Task 2`, but received `Task 3` and `Task 4` — confirming the offset was shifted by exactly one page.
+
+### Fix Applied
+```diff
+- const offset = page * limit;
++ const offset = (page - 1) * limit;
+```
+
+Pages are 1-indexed at the API level, so subtracting 1 before multiplying gives the correct zero-based offset for `Array.slice()`.
+
+---
+
+## Bug 3: `completeTask` silently resets priority to `'medium'`
+
+**Severity:** Medium  
+**File:** `src/services/taskService.js`, line 69 (original)  
+**Code:** `priority: 'medium'` inside the spread
+
+### Expected Behaviour
+Marking a task as complete (`PATCH /tasks/:id/complete`) should set `status` to `"done"` and set `completedAt` to the current timestamp **without modifying any other fields**.
+
+### Actual Behaviour
+The `completeTask` function hardcodes `priority: 'medium'` in the updated object:
+
+```js
+const updated = {
+  ...task,
+  priority: 'medium',  // ← This overwrites the task's actual priority
+  status: 'done',
+  completedAt: new Date().toISOString(),
+};
+```
+
+A `high`-priority task silently becomes `medium` when completed. This is a data-integrity bug — the task's priority history is lost.
+
+### How I Discovered It
+My unit test created a task with `priority: 'high'`, completed it, and asserted `priority` was still `'high'`. It wasn't — it was `'medium'`.
+
+### Proposed Fix
+Remove the `priority: 'medium'` line:
+
+```diff
+  const updated = {
+    ...task,
+-   priority: 'medium',
+    status: 'done',
+    completedAt: new Date().toISOString(),
+  };
+```
+
+---
+
+## Bug 4: `update` allows overwriting system-managed fields (`id`, `createdAt`)
+
+**Severity:** Low  
+**File:** `src/services/taskService.js`, line 50 (original)  
+**Code:** `const updated = { ...tasks[index], ...fields };`
+
+### Expected Behaviour
+`PUT /tasks/:id` should only allow updating user-editable fields (`title`, `description`, `status`, `priority`, `dueDate`). System-managed fields like `id` and `createdAt` should be immutable.
+
+### Actual Behaviour
+Because the update function blindly spreads all incoming fields, a request like this:
+
+```json
+PUT /tasks/abc-123
+{ "id": "hacked-id", "createdAt": "1999-01-01" }
+```
+
+…will overwrite the task's `id` and `createdAt`. This could break referential integrity (the old UUID no longer resolves) and forge audit timestamps.
+
+### How I Discovered It
+My unit test sent `{ id: 'hacked-id' }` through `taskService.update()` and confirmed that `updated.id` was indeed `'hacked-id'`, proving the service does not protect system fields.
+
+### Proposed Fix
+Destructure allowed fields explicitly, or strip protected keys before spreading:
+
+```diff
+  const update = (id, fields) => {
+    const index = tasks.findIndex((t) => t.id === id);
+    if (index === -1) return null;
+
++   // Strip system-managed fields to prevent overwrites
++   const { id: _id, createdAt: _ca, completedAt: _co, ...allowed } = fields;
+-   const updated = { ...tasks[index], ...fields };
++   const updated = { ...tasks[index], ...allowed };
+    tasks[index] = updated;
+    return updated;
+  };
+```

task-api/SUBMISSION_NOTES.md

@@ -0,0 +1,23 @@
+# Submission Notes
+
+## What I'd Test Next
+
+- **Concurrency / race conditions**: The in-memory store has no locking. If this were deployed with `cluster` mode or moved to a real database, concurrent writes to the same task could produce stale reads. I'd add stress tests with parallel requests.
+- **Input sanitisation**: The API currently trusts string inputs (title, description, assignee) without length limits or content filtering. I'd test with very long strings (10k+ characters), unicode edge cases, and potential XSS payloads to verify the API either truncates or rejects them.
+- **Error handler coverage**: The global error handler in `app.js` (lines 9–12) is never triggered by the current test suite because no route throws an unhandled error. I'd add a test that forces an internal error to verify the 500 response.
+- **Pagination boundary conditions**: Negative page/limit values, `page=0`, `limit=0`, extremely large values.
+
+## What Surprised Me
+
+- **The `completeTask` priority reset** was the most surprising bug. It's the kind of defect that silently corrupts data without producing any visible error — users would only notice if they checked a completed task's priority later. It was clearly an accidental leftover from a copy-paste or refactoring.
+- **The `getByStatus` substring bug** is subtle. For the three valid statuses (`todo`, `in_progress`, `done`), strict full-status queries like `?status=todo` happen to work because `"todo"` is not a substring of `"in_progress"` or `"done"`. The bug only surfaces with partial inputs — which makes it hard to catch without deliberate edge-case testing.
+- **No request-body size limit**: There's no `express.json({ limit: ... })` option, so the server accepts arbitrarily large payloads. In production, this would be a denial-of-service vector.
+
+## Questions Before Shipping to Production
+
+1. **Persistence**: The in-memory store means all data is lost on restart. Is that intentional for a demo, or do we need to add a database (PostgreSQL, MongoDB, etc.) before go-live?
+2. **Authentication & authorisation**: There's no auth layer at all. Who should be able to create, update, delete, and assign tasks? Are there roles (admin vs. member)?
+3. **Rate limiting**: Should we add rate limiting to prevent abuse?
+4. **Status enum consistency**: README says `"pending | in-progress | completed"`, but the code uses `"todo | in_progress | done"`. Which is the source of truth? This should be aligned before any client is built against the API.
+5. **Validation of `id` and `createdAt` on update**: The current `PUT` endpoint allows overwriting system-managed fields. Is that a feature or a bug? (I've documented it as a bug in my report.)
+6. **Idempotency**: Should `PATCH /tasks/:id/complete` be idempotent (no-op if already done) or should it return a 409 Conflict?

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,26 @@ router.patch('/:id/complete', (req, res) => {
   res.json(task);
 });
 
+/**
+ * PATCH /tasks/:id/assign
+ *
+ * Assigns a user to an existing task. The assignee name is trimmed before
+ * storage to normalise whitespace, but we reject empty/whitespace-only values
+ * at the validation layer to ensure meaningful data.
+ */
+router.patch('/:id/assign', (req, res) => {
+  const error = validateAssignTask(req.body);
+  if (error) {
+    return res.status(400).json({ error });
+  }
+
+  const assignee = req.body.assignee.trim();
+  const task = taskService.assignTask(req.params.id, assignee);
+  if (!task) {
+    return res.status(404).json({ error: 'Task not found' });
+  }
+
+  res.json(task);
+});
+
 module.exports = router;

task-api/src/services/taskService.js

@@ -8,8 +8,18 @@ const findById = (id) => tasks.find((t) => t.id === id);
 
 const getByStatus = (status) => tasks.filter((t) => t.status.includes(status));
 
+/**
+ * Returns a paginated slice of the tasks array.
+ *
+ * FIX: Changed `page * limit` to `(page - 1) * limit`.
+ * Pages are 1-indexed (page=1 is the first page), so we need to subtract 1
+ * before multiplying to calculate the correct zero-based offset.
+ *
+ * Before the fix, page 1 with limit 10 would start at index 10,
+ * completely skipping the first 10 tasks.
+ */
 const getPaginated = (page, limit) => {
-  const offset = page * limit;
+  const offset = (page - 1) * limit;
   return tasks.slice(offset, offset + limit);
 };
 
@@ -76,6 +86,30 @@ const completeTask = (id) => {
   return updated;
 };
 
+/**
+ * Assigns a user to an existing task.
+ *
+ * Design decisions:
+ * - Reassignment is allowed: if the task already has an assignee, it is
+ *   overwritten. This mirrors real workflows where tasks get reassigned.
+ * - Completed tasks can be assigned: no restriction on task status, because
+ *   assignment is metadata — useful for auditing or retrospectives.
+ * - The assignee string is stored as-is (trimming is handled at the route
+ *   layer to keep the service layer focused on data operations).
+ *
+ * @param {string} id - UUID of the task to assign
+ * @param {string} assignee - Name of the person to assign the task to
+ * @returns {object|null} The updated task, or null if not found
+ */
+const assignTask = (id, assignee) => {
+  const index = tasks.findIndex((t) => t.id === id);
+  if (index === -1) return null;
+
+  const updated = { ...tasks[index], assignee };
+  tasks[index] = updated;
+  return updated;
+};
+
 const _reset = () => {
   tasks = [];
 };
@@ -90,5 +124,6 @@ module.exports = {
   update,
   remove,
   completeTask,
+  assignTask,
   _reset,
 };

task-api/src/utils/validators.js

@@ -33,4 +33,28 @@ const validateUpdateTask = (body) => {
   return null;
 };
 
-module.exports = { validateCreateTask, validateUpdateTask };
+/**
+ * Validates the request body for the PATCH /tasks/:id/assign endpoint.
+ *
+ * Rules:
+ * - `assignee` must be present in the body
+ * - `assignee` must be a string (not a number, boolean, etc.)
+ * - `assignee` must not be empty or whitespace-only after trimming
+ *
+ * @param {object} body - The request body
+ * @returns {string|null} Error message if invalid, null if valid
+ */
+const validateAssignTask = (body) => {
+  if (body.assignee === undefined || body.assignee === null) {
+    return 'assignee is required';
+  }
+  if (typeof body.assignee !== 'string') {
+    return 'assignee must be a string';
+  }
+  if (body.assignee.trim() === '') {
+    return 'assignee must be a non-empty string';
+  }
+  return null;
+};
+
+module.exports = { validateCreateTask, validateUpdateTask, validateAssignTask };