Add tests, fix bugs, and implement assign endpoint

task-api/BUG_REPORT.md

@@ -0,0 +1,122 @@
+# Bug Report — Task Manager API
+
+---
+
+## Bug 1: `getByStatus` uses partial string matching instead of exact match
+
+**File:** `src/services/taskService.js` — line 9  
+**Severity:** High
+
+### Expected behavior
+Filtering by `status=todo` should return only tasks with status `"todo"`.
+
+### What actually happens
+`getByStatus` uses `t.status.includes(status)` which performs a **substring match**. This means:
+- `getByStatus('do')` matches both `"todo"` and `"done"`
+- `getByStatus('in')` matches `"in_progress"`
+
+Any partial status string returns incorrect results.
+
+### How I discovered it
+Wrote a unit test that creates tasks with different statuses and filtered with a partial string `"do"` — the test revealed it matched `"todo"` and `"done"`.
+
+### Fix
+Replace `t.status.includes(status)` with `t.status === status` for strict equality.
+
+```diff
+- const getByStatus = (status) => tasks.filter((t) => t.status.includes(status));
++ const getByStatus = (status) => tasks.filter((t) => t.status === status);
+```
+
+**Status:** ✅ Fixed
+
+---
+
+## Bug 2: `getPaginated` has off-by-one error — page 1 skips results
+
+**File:** `src/services/taskService.js` — line 12  
+**Severity:** High
+
+### Expected behavior
+Page 1 should return the first `limit` items (offset 0).
+
+### What actually happens
+The offset is calculated as `page * limit` instead of `(page - 1) * limit`. So page 1 with limit 2 starts at index 2 (skipping the first 2 items), and page 0 would be needed to get the first page — which is not the expected API convention.
+
+### How I discovered it
+Integration test for `GET /tasks?page=1&limit=2` returned `Task 3` instead of `Task 1` as the first result.
+
+### Fix
+```diff
+- const offset = page * limit;
++ const offset = (page - 1) * limit;
+```
+
+**Status:** ✅ Fixed
+
+---
+
+## Bug 3: `completeTask` resets priority to `"medium"`
+
+**File:** `src/services/taskService.js` — line 69  
+**Severity:** Medium
+
+### Expected behavior
+Marking a task as complete should only change `status` to `"done"` and set `completedAt`. All other fields (including `priority`) should remain unchanged.
+
+### What actually happens
+The `completeTask` function includes `priority: 'medium'` in the updated object, overwriting whatever priority the task originally had.
+
+### How I discovered it
+Created a task with `priority: 'high'`, then completed it via `PATCH /tasks/:id/complete`. The response showed `priority: 'medium'` instead of `'high'`.
+
+### Fix
+Remove the `priority: 'medium'` line from the `completeTask` function.
+
+```diff
+  const updated = {
+    ...task,
+-   priority: 'medium',
+    status: 'done',
+    completedAt: new Date().toISOString(),
+  };
+```
+
+**Status:** ✅ Fixed
+
+---
+
+## Bug 4: `update` allows overwriting internal fields (`id`, `createdAt`)
+
+**File:** `src/services/taskService.js` — line 50  
+**Severity:** Low
+
+### Expected behavior
+Internal/system-managed fields like `id` and `createdAt` should not be overwritable by a client update request.
+
+### What actually happens
+The update function uses a plain spread: `{ ...tasks[index], ...fields }`. If a client sends `{ id: 'hacked' }` in the PUT body, the task's ID gets overwritten, making it unreachable by the original ID.
+
+### How I discovered it
+Wrote a unit test that sends `{ id: 'hacked' }` as an update — it succeeded.
+
+### Suggested fix
+Strip protected fields before applying the spread:
+
+```js
+const { id, createdAt, ...safeFields } = fields;
+const updated = { ...tasks[index], ...safeFields };
+```
+
+**Status:** ⚠️ Not fixed (documented only — low severity, not required by assignment)
+
+---
+
+## Summary
+
+| # | Bug | Severity | Fixed? |
+|---|-----|----------|--------|
+| 1 | `getByStatus` partial matching | High | ✅ |
+| 2 | `getPaginated` off-by-one | High | ✅ |
+| 3 | `completeTask` resets priority | Medium | ✅ |
+| 4 | `update` allows internal field overwrite | Low | ⚠️ Documented |

task-api/SUBMISSION_NOTES.md

@@ -0,0 +1,43 @@
+# Submission Notes
+
+## Coverage Summary
+
+```
+-|---------|----------|---------|---------|-------------------
+ | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
+-|---------|----------|---------|---------|-------------------
+ |   97.41 |    97.67 |   93.33 |   97.16 |
+-|---------|----------|---------|---------|-------------------
+
+Test Suites: 3 passed, 3 total
+Tests:       85 passed, 85 total
+```
+
+## What I'd test next if I had more time
+
+- **Concurrency / race conditions:** The in-memory store uses direct array mutation. Concurrent requests could cause data corruption (e.g., two updates to the same task at once).
+- **Request body edge cases:** Sending deeply nested objects, extremely long strings, or unexpected content types (XML, form-data).
+- **Error handler middleware:** The global error handler in `app.js` is currently uncovered. I'd test it by forcing an unhandled error in a route.
+- **Performance/load testing:** How does the in-memory store handle thousands of tasks? Pagination would become critical.
+
+## What surprised me in the codebase
+
+- The `completeTask` function silently resetting `priority` to `"medium"` — this is the kind of bug that would be extremely hard to catch visually in production, since tasks still get marked as "done" correctly.
+- The `getByStatus` using `.includes()` for string matching — it works for exact status names by coincidence (e.g., "todo" doesn't contain the substring "done"), but breaks for partial matches like "do".
+- The `update` function allows overwriting `id` and `createdAt` via the spread, which could cause data integrity issues.
+
+## Questions I'd ask before shipping to production
+
+1. **Authentication/Authorization:** Who can create/assign/complete tasks? Should there be user accounts?
+2. **Persistence:** The in-memory store means all data is lost on restart. What database should back this?
+3. **Rate limiting:** Should we add rate limiting to prevent abuse?
+4. **Input sanitization:** Should we sanitize title/description for XSS if they're ever rendered in a frontend?
+5. **Assignee validation:** Should `assignee` be validated against a list of known users, or is it freeform text?
+6. **Idempotency:** What should happen if you `PATCH /complete` on a task that's already done? Currently it overwrites `completedAt`.
+
+## Design decisions for `PATCH /tasks/:id/assign`
+
+- **Validation:** Requires `assignee` to be a non-empty string (whitespace-only is rejected). Non-string values (like numbers) are also rejected.
+- **Re-assignment:** Allowed. If a task is already assigned, sending a new assignee simply overwrites the previous one. This keeps the API simple and follows the principle of least surprise.
+- **Trimming:** The assignee name is trimmed before storage to normalize whitespace.
+- **No unassign:** To unassign, we'd need a separate mechanism (or allow `null`). I chose not to allow empty strings as an unassign action since the assignment explicitly asks about empty string validation.

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;

task-api/src/services/taskService.js

@@ -6,10 +6,10 @@ const getAll = () => [...tasks];
 
 const findById = (id) => tasks.find((t) => t.id === id);
 
-const getByStatus = (status) => tasks.filter((t) => t.status.includes(status));
+const getByStatus = (status) => tasks.filter((t) => t.status === status);
 
 const getPaginated = (page, limit) => {
-  const offset = page * limit;
+  const offset = (page - 1) * limit;
   return tasks.slice(offset, offset + limit);
 };
 
@@ -66,7 +66,6 @@ const completeTask = (id) => {
 
   const updated = {
     ...task,
-    priority: 'medium',
     status: 'done',
     completedAt: new Date().toISOString(),
   };
@@ -76,6 +75,16 @@ const completeTask = (id) => {
   return updated;
 };
 
+const assignTask = (id, assignee) => {
+  const task = findById(id);
+  if (!task) return null;
+
+  const updated = { ...task, assignee };
+  const index = tasks.findIndex((t) => t.id === id);
+  tasks[index] = updated;
+  return updated;
+};
+
 const _reset = () => {
   tasks = [];
 };
@@ -90,5 +99,6 @@ module.exports = {
   update,
   remove,
   completeTask,
+  assignTask,
   _reset,
 };

task-api/src/utils/validators.js

@@ -33,4 +33,11 @@ const validateUpdateTask = (body) => {
   return null;
 };
 
-module.exports = { validateCreateTask, validateUpdateTask };
+const validateAssignTask = (body) => {
+  if (!body.assignee || typeof body.assignee !== 'string' || body.assignee.trim() === '') {
+    return 'assignee is required and must be a non-empty string';
+  }
+  return null;
+};
+
+module.exports = { validateCreateTask, validateUpdateTask, validateAssignTask };