Vihaan Aswal - feat: add task assignment support and harden task completion flow

BUG_REPORT.md

@@ -0,0 +1,97 @@
+# Bug Report
+
+This report reflects the current codebase after implementation work. Bugs #4, #5, and #9 from the original audit have been fixed.
+
+## Resolved During Implementation
+
+- Bug #4: Completing a task no longer resets priority to `medium`.
+- Bug #5: Re-completing a task no longer rewrites the original `completedAt` timestamp.
+- Bug #9: `PATCH /tasks/:id/assign` now exists and stores `assignee` on the task.
+
+## Open Bugs
+
+**Bug #1: Status filter does substring matching**
+
+- **File:** src/services/taskService.js
+- **Line:** ~9
+- **Expected behavior:** `GET /tasks?status=...` should return only tasks whose status exactly matches the requested enum value.
+- **Actual behavior:** `getByStatus()` uses `t.status.includes(status)`, so partial values match too. For example, `status=do` returns both `todo` and `done`.
+- **How discovered:** Test `getByStatus() -> does not match partial status fragments` and integration check for `GET /tasks?status=`.
+- **Proposed fix:** Replace substring matching with strict equality, and validate the query status before filtering.
+
+---
+
+**Bug #2: Pagination skips the first page**
+
+- **File:** src/services/taskService.js
+- **Line:** ~12
+- **Expected behavior:** Page 1 with limit N should return the first N tasks.
+- **Actual behavior:** `getPaginated()` computes `offset = page * limit`, so page 1 starts at index `limit` and skips the first page entirely.
+- **How discovered:** Test `getPaginated() -> returns the first page of tasks when page=1 and limit=2` and integration check for `GET /tasks` with pagination.
+- **Proposed fix:** Use `offset = (page - 1) * limit` for 1-based pagination, and reject invalid page/limit values.
+
+---
+
+**Bug #3: PUT allows immutable fields like createdAt to be overwritten**
+
+- **File:** src/services/taskService.js
+- **Line:** ~50
+- **Expected behavior:** Client updates should only be able to change mutable task fields.
+- **Actual behavior:** `update()` merges the entire request body into the existing task object with `{ ...tasks[index], ...fields }`, so a request body containing `createdAt` or `id` overwrites those system fields.
+- **How discovered:** Test `update() -> does not allow createdAt to be overwritten by client updates`.
+- **Proposed fix:** Whitelist allowed update fields before merging and ignore or reject immutable properties.
+
+---
+
+**Bug #4: Invalid status query values are accepted instead of rejected**
+
+- **File:** src/routes/tasks.js
+- **Line:** ~14-16
+- **Expected behavior:** `GET /tasks?status=...` should validate the requested status and return 400 for unsupported values.
+- **Actual behavior:** The route forwards whatever string is in `req.query.status` to `getByStatus()`. Because filtering is substring-based and there is no validation step, unsupported values like `pending` are not rejected consistently.
+- **How discovered:** Integration test `GET /tasks -> returns 400 for invalid status filter value`.
+- **Proposed fix:** Validate the query parameter against the allowed enum before calling the service and return 400 on invalid input.
+
+---
+
+**Bug #5: Invalid page and limit values are silently coerced**
+
+- **File:** src/routes/tasks.js
+- **Line:** ~18-21
+- **Expected behavior:** Non-numeric, zero, or negative pagination values should be rejected with 400.
+- **Actual behavior:** `parseInt(page) || 1` and `parseInt(limit) || 10` coerce invalid values into defaults. That means `page=abc` becomes page 1 and `limit=0` becomes 10 instead of returning a validation error.
+- **How discovered:** Integration tests `GET /tasks -> returns 400 for invalid page query value` and `GET /tasks -> returns 400 for invalid limit query value`.
+- **Proposed fix:** Parse and validate query parameters explicitly, require positive integers, and return 400 when parsing fails or values are out of range.
+
+---
+
+**Bug #6: PUT accepts an empty payload instead of rejecting it**
+
+- **File:** src/routes/tasks.js
+- **Line:** ~37-46
+- **Expected behavior:** Updating a task should require at least one valid field, or the API should reject empty bodies with 400.
+- **Actual behavior:** An empty object passes validation because `validateUpdateTask({})` returns null, and `taskService.update()` simply merges nothing and returns the existing task.
+- **How discovered:** Integration test `PUT /tasks/:id -> returns 400 when required update fields are missing`.
+- **Proposed fix:** Add a validation rule that rejects empty update payloads and return 400 before calling the service.
+
+---
+
+**Bug #7: The error middleware converts all errors into a 500 response**
+
+- **File:** src/app.js
+- **Line:** ~9-11
+- **Expected behavior:** Client-side validation and JSON parsing errors should return 400-class responses where appropriate.
+- **Actual behavior:** The global error handler always returns `500 Internal server error`, regardless of whether the error originated from bad input or a server fault.
+- **How discovered:** Manual inspection of the Express error middleware and the null/invalid-body edge cases in the tests.
+- **Proposed fix:** Detect known client errors and map them to 400 responses; reserve 500 for unexpected server failures.
+
+---
+
+**Bug #8: Documentation status enum disagrees with the implementation**
+
+- **File:** README.md
+- **Line:** ~77
+- **Expected behavior:** The API documentation should describe the same task status values that the code accepts and returns.
+- **Actual behavior:** README uses `pending | in-progress | completed`, while the code and assignment use `todo | in_progress | done`.
+- **How discovered:** Manual inspection of README and assignment docs.
+- **Proposed fix:** Update README examples and schema to match the actual API contract, or refactor the code to match the documented values.

SUBMISSION.md

@@ -0,0 +1,19 @@
+# Submission Note
+
+This submission includes the implementation and cleanup work for the Task API take-home assignment.
+
+## Completed
+
+- Added the task assignment flow end to end.
+- Fixed task completion so it preserves priority and is idempotent.
+- Expanded Jest and Supertest coverage to exercise the missing branches and edge cases.
+- Updated the bug report to reflect the current open issues.
+- Removed the long-form audit notes file from the repository.
+
+## Verification
+
+- Verified the API package from `task-api` with the test and coverage workflow used during implementation.
+
+## Remaining Known Issues
+
+- The open issues listed in `BUG_REPORT.md` are still reproducible and are intentionally left as follow-up items.

task-api/src/app.js

@@ -1,22 +1,27 @@
-const express = require('express');
-const taskRoutes = require('./routes/tasks');
+const express = require("express");
+const taskRoutes = require("./routes/tasks");
 
 const app = express();
 
 app.use(express.json());
-app.use('/tasks', taskRoutes);
+app.use("/tasks", taskRoutes);
 
 app.use((err, req, res, next) => {
   console.error(err.stack);
-  res.status(500).json({ error: 'Internal server error' });
+  res.status(500).json({ error: "Internal server error" });
 });
 
 const PORT = process.env.PORT || 3000;
 
-if (require.main === module) {
-  app.listen(PORT, () => {
-    console.log(`Task API running on port ${PORT}`);
+const startServer = (port = PORT) =>
+  app.listen(port, () => {
+    console.log(`Task API running on port ${port}`);
   });
+
+if (require.main === module) {
+  startServer(PORT);
 }
 
+app.startServer = startServer;
+
 module.exports = app;

task-api/src/routes/tasks.js

@@ -1,14 +1,18 @@
-const express = require('express');
+const express = require("express");
 const router = express.Router();
-const taskService = require('../services/taskService');
-const { validateCreateTask, validateUpdateTask } = require('../utils/validators');
+const taskService = require("../services/taskService");
+const {
+  validateCreateTask,
+  validateUpdateTask,
+  validateAssignTask,
+} = require("../utils/validators");
 
-router.get('/stats', (req, res) => {
+router.get("/stats", (req, res) => {
   const stats = taskService.getStats();
   res.json(stats);
 });
 
-router.get('/', (req, res) => {
+router.get("/", (req, res) => {
   const { status, page, limit } = req.query;
 
   if (status) {
@@ -27,7 +31,7 @@ router.get('/', (req, res) => {
   res.json(tasks);
 });
 
-router.post('/', (req, res) => {
+router.post("/", (req, res) => {
   const error = validateCreateTask(req.body);
   if (error) {
     return res.status(400).json({ error });
@@ -37,36 +41,59 @@ router.post('/', (req, res) => {
   res.status(201).json(task);
 });
 
-router.put('/:id', (req, res) => {
+router.put("/:id", (req, res) => {
   const error = validateUpdateTask(req.body);
   if (error) {
     return res.status(400).json({ error });
   }
 
   const task = taskService.update(req.params.id, req.body);
   if (!task) {
-    return res.status(404).json({ error: 'Task not found' });
+    return res.status(404).json({ error: "Task not found" });
   }
 
   res.json(task);
 });
 
-router.delete('/:id', (req, res) => {
+router.delete("/:id", (req, res) => {
   const deleted = taskService.remove(req.params.id);
   if (!deleted) {
-    return res.status(404).json({ error: 'Task not found' });
+    return res.status(404).json({ error: "Task not found" });
   }
 
   res.status(204).send();
 });
 
-router.patch('/:id/complete', (req, res) => {
+router.patch("/:id/complete", (req, res) => {
   const task = taskService.completeTask(req.params.id);
   if (!task) {
-    return res.status(404).json({ error: 'Task not found' });
+    return res.status(404).json({ error: "Task not found" });
   }
 
   res.json(task);
 });
 
+/*
+ * PATCH /tasks/:id/assign
+ * Assigns a task to a named person.
+ * Design decisions:
+ *   - Reassignment is allowed (overwrites existing assignee).
+ *   - Assignee is trimmed server-side to avoid whitespace inconsistencies.
+ *   - No user list validation — assignee is a free-form string by design.
+ *     (In production, this would validate against a users collection.)
+ */
+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.status(200).json(task);
+});
+
 module.exports = router;

task-api/src/services/taskService.js

@@ -1,4 +1,4 @@
-const { v4: uuidv4 } = require('uuid');
+const { v4: uuidv4 } = require("uuid");
 
 let tasks = [];
 
@@ -20,22 +20,30 @@ const getStats = () => {
 
   tasks.forEach((t) => {
     if (counts[t.status] !== undefined) counts[t.status]++;
-    if (t.dueDate && t.status !== 'done' && new Date(t.dueDate) < now) {
+    if (t.dueDate && t.status !== "done" && new Date(t.dueDate) < now) {
       overdue++;
     }
   });
 
   return { ...counts, overdue };
 };
 
-const create = ({ title, description = '', status = 'todo', priority = 'medium', dueDate = null }) => {
+const create = ({
+  title,
+  description = "",
+  status = "todo",
+  priority = "medium",
+  dueDate = null,
+}) => {
   const task = {
     id: uuidv4(),
     title,
     description,
     status,
     priority,
     dueDate,
+    // assignee: null means unassigned
+    assignee: null,
     completedAt: null,
     createdAt: new Date().toISOString(),
   };
@@ -64,10 +72,15 @@ const completeTask = (id) => {
   const task = findById(id);
   if (!task) return null;
 
+  // GUARD: Already-completed tasks are returned unchanged (idempotent).
+  if (task.status === "done") return task;
+
+  // FIX: Removed hardcoded priority reset — completing a task should
+  // preserve its existing priority, not silently downgrade it.
+
   const updated = {
     ...task,
-    priority: 'medium',
-    status: 'done',
+    status: "done",
     completedAt: new Date().toISOString(),
   };
 
@@ -76,6 +89,23 @@ const completeTask = (id) => {
   return updated;
 };
 
+// assignTask: stores an assignee name on a task.
+// Overwrites existing assignee — reassignment is intentionally allowed.
+// Trimming handles accidental whitespace from API consumers.
+const assignTask = (id, assignee) => {
+  const task = findById(id);
+  if (!task) return null;
+
+  const updated = {
+    ...task,
+    assignee: assignee.trim(),
+  };
+
+  const index = tasks.findIndex((t) => t.id === id);
+  tasks[index] = updated;
+  return updated;
+};
+
 const _reset = () => {
   tasks = [];
 };
@@ -90,5 +120,6 @@ module.exports = {
   update,
   remove,
   completeTask,
+  assignTask,
   _reset,
 };