diff --git a/README.md b/README.md index 5d46160c..cdb5285c 100644 --- a/README.md +++ b/README.md @@ -1,3 +1,121 @@ +While working through this codebase, I focused on understanding behavior through testing, identifying issues systematically, and improving reliability before adding new features. + +## 🚀 My Contributions + +### 1) Fixed Pagination Offset Logic + +**File:** `task-api/src/services/taskService.js` ([line 12](./task-api/src/services/taskService.js#L12)) + +**Issue:** +Pagination skipped the first page because the offset was calculated as `page * limit`. For `page=1`, this starts at index `limit` instead of `0`. + +**Before** + +```js +const offset = page * limit; +``` + +**After** + +```js +const offset = (page - 1) * limit; +``` + +**Why this fix works:** +It aligns page numbering (1-based) with array indexing (0-based), so page 1 starts from the first record. + +### 2) Added Status Query Validation + +**File:** `task-api/src/routes/tasks.js` ([lines 14-18](./task-api/src/routes/tasks.js#L14-L18)) + +**Issue:** +`GET /tasks?status=invalid` returned `200` with an empty array. Invalid query values were not validated before calling the service layer. + +**Added Validation** + +```js +const VALID_STATUSES = ["todo", "in_progress", "done"]; + +if (status) { + if (!VALID_STATUSES.includes(status)) { + return res.status(400).json({ error: "Invalid status" }); + } + + const tasks = taskService.getByStatus(status); + return res.json(tasks); +} +``` + +**Why this fix works:** +Invalid status filters are rejected early with `400 Bad Request`, making API behavior explicit and predictable. + +### 3) Implemented `PATCH /tasks/:id/assign` + +**File:** `task-api/src/routes/tasks.js` ([lines 78-94](./task-api/src/routes/tasks.js#L78-L94)) + +**What I implemented:** + +- Added an `assignee` field update endpoint. +- Reused existing `taskService.update()` for persistence. +- Added request validation and proper error handling. + +**Implementation Snippet** + +```js +router.patch("/:id/assign", (req, res) => { + const { assignee } = req.body; + + if (!assignee || typeof assignee !== "string" || assignee.trim() === "") { + return res + .status(400) + .json({ error: "Assignee must be a non-empty string" }); + } + + const task = taskService.findById(req.params.id); + if (!task) { + return res.status(404).json({ error: "Task not found" }); + } + + const updated = taskService.update(req.params.id, { assignee }); + res.json(updated); +}); +``` + +**Validation behavior:** + +- Missing `assignee` → `400` +- Empty/blank `assignee` → `400` +- Unknown `:id` → `404` +- Valid request → `200` with updated task + +### Design Decisions + +- Kept validation in the route layer to maintain separation of concerns (request validation vs. business/data logic). +- Reused `taskService.update()` to avoid duplicating update logic and reduce maintenance overhead. +- Allowed reassignment by design (`assignee` can be overwritten) to support real-world task handoffs. +- Used REST-aligned status codes (`400`, `404`, `200`) for consistent API semantics. + +### Observations + +- Query parameter validation was missing for filters like `status`, which can lead to silent failures. +- The pagination bug was subtle but high-impact for client-side paging correctness. +- `taskService.update()` currently allows overwriting immutable fields (e.g., `id`, `createdAt`), which is a potential integrity risk and a good candidate for a hardening follow-up. + +### Test Coverage + +**File:** `task-api/tests/tasks.test.js` + +I added endpoint-focused tests to validate both expected behavior and edge cases for the changes above: + +- `GET /tasks` returns `[]` initially. +- Pagination regression test: `GET /tasks?page=1&limit=2` returns the first two created tasks (`Task 1`, `Task 2`). +- Invalid status validation: `GET /tasks?status=invalid` returns `400` with an error payload. +- Assign endpoint happy path: `PATCH /tasks/:id/assign` sets `assignee` and returns `200`. +- Assign endpoint validation: empty assignee returns `400`. +- Assign endpoint not found case: non-existent task id returns `404`. + +--- + # Take-Home Assignment — The Untested API A 2-day take-home assignment. You'll read unfamiliar code, write tests, track down bugs, and ship a small feature. @@ -11,6 +129,7 @@ Read **[ASSIGNMENT.md](./ASSIGNMENT.md)** for the full brief before you start. You're welcome to use AI tools. What we're evaluating is your ability to read and reason about unfamiliar code — so your submission should reflect your own understanding, not just generated output. Concretely: + - For each bug you report: include where in the code it lives and why it happens - For the feature you implement: briefly explain the design decisions you made - If something surprised you or you had to make a tradeoff, say so @@ -57,15 +176,15 @@ ASSIGNMENT.md # Full brief — read this first ## API Reference -| Method | Path | Description | -|----------|---------------------------|------------------------------------------| -| `GET` | `/tasks` | List all tasks. Supports `?status=`, `?page=`, `?limit=` | -| `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)_ | +| Method | Path | Description | +| -------- | --------------------- | -------------------------------------------------------- | +| `GET` | `/tasks` | List all tasks. Supports `?status=`, `?page=`, `?limit=` | +| `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)_ | ### Task shape @@ -85,6 +204,7 @@ ASSIGNMENT.md # Full brief — read this first ### Sample requests **Create a task** + ```bash curl -X POST http://localhost:3000/tasks \ -H "Content-Type: application/json" \ @@ -92,11 +212,13 @@ curl -X POST http://localhost:3000/tasks \ ``` **List tasks with filter** + ```bash curl "http://localhost:3000/tasks?status=pending&page=1&limit=10" ``` **Mark complete** + ```bash curl -X PATCH http://localhost:3000/tasks//complete ``` diff --git a/task-api/src/routes/tasks.js b/task-api/src/routes/tasks.js index e8c370fe..70a69a5a 100644 --- a/task-api/src/routes/tasks.js +++ b/task-api/src/routes/tasks.js @@ -11,11 +11,17 @@ router.get('/stats', (req, res) => { router.get('/', (req, res) => { const { status, page, limit } = req.query; - if (status) { - const tasks = taskService.getByStatus(status); - return res.json(tasks); +const VALID_STATUSES = ['todo', 'in_progress', 'done']; + +if (status) { + if (!VALID_STATUSES.includes(status)) { + return res.status(400).json({ error: 'Invalid status' }); } + const tasks = taskService.getByStatus(status); + return res.json(tasks); +} + if (page !== undefined || limit !== undefined) { const pageNum = parseInt(page) || 1; const limitNum = parseInt(limit) || 10; @@ -69,4 +75,23 @@ router.patch('/:id/complete', (req, res) => { res.json(task); }); +router.patch('/:id/assign', (req, res) => { + const { assignee } = req.body; + + // validation + if (!assignee || typeof assignee !== 'string' || assignee.trim() === '') { + return res.status(400).json({ error: 'Assignee must be a non-empty string' }); + } + + const task = taskService.findById(req.params.id); + + if (!task) { + return res.status(404).json({ error: 'Task not found' }); + } + + const updated = taskService.update(req.params.id, { assignee }); + + res.json(updated); +}); + module.exports = router; diff --git a/task-api/src/services/taskService.js b/task-api/src/services/taskService.js index f8e89189..9fc6246f 100644 --- a/task-api/src/services/taskService.js +++ b/task-api/src/services/taskService.js @@ -9,7 +9,7 @@ const findById = (id) => tasks.find((t) => t.id === id); const getByStatus = (status) => tasks.filter((t) => t.status.includes(status)); const getPaginated = (page, limit) => { - const offset = page * limit; + const offset = (page - 1) * limit; //Fixed the pagination logic to calculate the correct offset return tasks.slice(offset, offset + limit); }; diff --git a/task-api/tests/tasks.test.js b/task-api/tests/tasks.test.js new file mode 100644 index 00000000..5a83266b --- /dev/null +++ b/task-api/tests/tasks.test.js @@ -0,0 +1,79 @@ +const request = require('supertest'); +const app = require('../src/app'); + +describe('GET /tasks', () => { + test('should return empty array initially', async () => { + const res = await request(app).get('/tasks'); + + expect(res.statusCode).toBe(200); + expect(res.body).toEqual([]); + }); + + + test('pagination should return first tasks for page=1 (expected behavior)', async () => { + // create 3 tasks + await request(app).post('/tasks').send({ title: 'Task 1' }); + await request(app).post('/tasks').send({ title: 'Task 2' }); + await request(app).post('/tasks').send({ title: 'Task 3' }); + + const res = await request(app).get('/tasks?page=1&limit=2'); + + expect(res.statusCode).toBe(200); + expect(res.body.length).toBe(2); + + // expected first 2 tasks + expect(res.body[0].title).toBe('Task 1'); + expect(res.body[1].title).toBe('Task 2'); +}); + + + test('should return 400 for invalid status', async () => { + const res = await request(app).get('/tasks?status=invalid'); + + expect(res.statusCode).toBe(400); + expect(res.body).toHaveProperty('error'); +}); + + test('should assign a task to a user', async () => { + // create a task first + const createRes = await request(app) + .post('/tasks') + .send({ title: 'New Task' }); + + const taskId = createRes.body.id; + + // assign the task + const res = await request(app) + .patch(`/tasks/${taskId}/assign`) + .send({ assignee: 'Cipher' }); + + expect(res.statusCode).toBe(200); + expect(res.body.assignee).toBe('Cipher'); +}); + + test('should return 400 if assignee is empty', async () => { + const createRes = await request(app) + .post('/tasks') + .send({ title: 'Another Task' }); + + const taskId = createRes.body.id; + + const res = await request(app) + .patch(`/tasks/${taskId}/assign`) + .send({ assignee: '' }); + + expect(res.statusCode).toBe(400); +}); + +test('should return 404 if task does not exist', async () => { + const res = await request(app) + .patch('/tasks/invalid-id/assign') + .send({ assignee: 'Cipher' }); + + expect(res.statusCode).toBe(404); +}); + + + + +}); \ No newline at end of file