From c529eb016f9570a934b710408e2c75fd8d6e58b3 Mon Sep 17 00:00:00 2001 From: Parthmudgal15105 Date: Wed, 15 Apr 2026 01:29:33 +0530 Subject: [PATCH 1/2] solution: test suite, bugfixes, and assign endpoint --- BUG_REPORT.md | 19 +++ SUBMISSION.md | 18 +++ task-api/src/routes/tasks.js | 22 +++- task-api/src/services/taskService.js | 18 ++- task-api/src/utils/validators.js | 9 +- task-api/tests/taskService.test.js | 169 +++++++++++++++++++++++++ task-api/tests/tasks.api.test.js | 178 +++++++++++++++++++++++++++ 7 files changed, 427 insertions(+), 6 deletions(-) create mode 100644 BUG_REPORT.md create mode 100644 SUBMISSION.md create mode 100644 task-api/tests/taskService.test.js create mode 100644 task-api/tests/tasks.api.test.js diff --git a/BUG_REPORT.md b/BUG_REPORT.md new file mode 100644 index 00000000..00ba7e20 --- /dev/null +++ b/BUG_REPORT.md @@ -0,0 +1,19 @@ +# Bug Report + +## Bug 1: Status Filter Inexact Matching +- **Expected Behavior:** Searching by status (e.g. `?status=todo`) should strictly return tasks with exactly that status. +- **Actual Behavior:** Searching for `status="do"` returns both `"todo"` and `"done"` tasks. +- **How I discovered it:** Writing unit tests for `getByStatus` in `taskService.js`; observing that `.includes(status)` was used instead of strict `===` equality. +- **Proposed Fix:** Change `tasks.filter((t) => t.status.includes(status))` to `tasks.filter((t) => t.status === status)`. + +## Bug 2: Task Completion Overwrites Priority +- **Expected Behavior:** Marking a task as complete should set its status to `"done"` and update `completedAt`, but leave its `priority` untouched. +- **Actual Behavior:** The `completeTask` method explicitly sets `priority: 'medium'`, resetting high or low priority entries to medium. +- **How I discovered it:** Writing the unit test for `completeTask()` in `taskService.js` and asserting the `priority` remains the same. +- **Proposed Fix:** Remove `priority: 'medium'` from the updated object in the `completeTask` function. + +## Bug 3: Pagination Defaults to Page 1 on Page 0 Request +- **Expected Behavior:** Passing `?page=0&limit=10` should evaluate to `page=0` and return the first 10 items. +- **Actual Behavior:** The server processes `parseInt('0') || 1`, determining `0` is falsy and forcing `page = 1`. This leads to skipping the first page in 0-indexed systems. +- **How I discovered it:** Writing the integration test (`GET /tasks?page=0&limit=2`) and seeing that instead of 2 items returned, only 1 was returned (offset was shifted). +- **Proposed Fix:** Use stricter falsy checks `isNaN(parseInt(page)) ? 1 : parseInt(page)`. diff --git a/SUBMISSION.md b/SUBMISSION.md new file mode 100644 index 00000000..fa2583db --- /dev/null +++ b/SUBMISSION.md @@ -0,0 +1,18 @@ +# Submission Notes + +Here are my notes for the conclusion of this take-home assignment: + +## Next Steps with More Time +- **Pagination Boundary Assertions:** Add tests to catch extreme boundaries on limit (e.g. `limit=100000`) and ensure negative pages fail gracefully. +- **Data Store Modularity:** I would mock an external data provider or database to test `taskService.js` under simulated delay/rejection conditions instead of tight coupling. +- **Validation Combinations:** Test deeper combinations like completing tasks without updating `completedAt`, or verifying `assignee` doesn't break standard `createdAt` tracking. + +## Surprises in the Codebase +- **Priority Override:** Noticing that the `completeTask` function forcefully reset priority to `"medium"` was unexpected — and arguably quite jarring to user experience! +- **Idempotency Holes:** `update()` could overwrite internal structure components since it carelessly merged `...fields` over top of the task rather than isolating permitted keys. +- **Node Start Strategy:** Seeing `require.main === module` inside `app.js` was a neat addition not generally seen in bare-bone setups; great for isolating testing safely! + +## Questions Before Shipping to Production +- **Database & Migration:** Which database are we targeting (e.g., MongoDB, Postgres)? This entirely changes the indexing approach for the `assignee` and `status` fields. +- **Authentication/Ownership:** Should endpoints enforce authentication boundaries? Should `assignee` map against a table of known users? +- **Rate-Limiting:** Do we expect burst submissions? We might need rate limiters (`express-rate-limit`) to prevent server crashes since there's no capping. diff --git a/task-api/src/routes/tasks.js b/task-api/src/routes/tasks.js index e8c370fe..a25cc424 100644 --- a/task-api/src/routes/tasks.js +++ b/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(); @@ -17,8 +17,10 @@ router.get('/', (req, res) => { } if (page !== undefined || limit !== undefined) { - const pageNum = parseInt(page) || 1; - const limitNum = parseInt(limit) || 10; + const parsedPage = parseInt(page); + const pageNum = isNaN(parsedPage) ? 1 : parsedPage; + const parsedLimit = parseInt(limit); + const limitNum = isNaN(parsedLimit) ? 10 : parsedLimit; const tasks = taskService.getPaginated(pageNum, limitNum); return res.json(tasks); } @@ -69,4 +71,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); + if (!task) { + return res.status(404).json({ error: 'Task not found' }); + } + + res.json(task); +}); + module.exports = router; diff --git a/task-api/src/services/taskService.js b/task-api/src/services/taskService.js index f8e89189..34bad326 100644 --- a/task-api/src/services/taskService.js +++ b/task-api/src/services/taskService.js @@ -6,7 +6,7 @@ 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; @@ -66,7 +66,6 @@ const completeTask = (id) => { const updated = { ...task, - priority: 'medium', status: 'done', completedAt: new Date().toISOString(), }; @@ -76,6 +75,20 @@ 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 +103,6 @@ module.exports = { update, remove, completeTask, + assignTask, _reset, }; diff --git a/task-api/src/utils/validators.js b/task-api/src/utils/validators.js index 1e908ff5..a87e14fa 100644 --- a/task-api/src/utils/validators.js +++ b/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 }; diff --git a/task-api/tests/taskService.test.js b/task-api/tests/taskService.test.js new file mode 100644 index 00000000..3c18f906 --- /dev/null +++ b/task-api/tests/taskService.test.js @@ -0,0 +1,169 @@ +const taskService = require('../src/services/taskService'); + +describe('taskService', () => { + beforeEach(() => { + taskService._reset(); + }); + + describe('create', () => { + it('should create a task with default values', () => { + const task = taskService.create({ title: 'Test Task' }); + expect(task).toHaveProperty('id'); + expect(task.title).toBe('Test Task'); + expect(task.description).toBe(''); + expect(task.status).toBe('todo'); + expect(task.priority).toBe('medium'); + expect(task.dueDate).toBeNull(); + expect(task.completedAt).toBeNull(); + expect(task.createdAt).toBeDefined(); + }); + + it('should create a task with provided values', () => { + const task = taskService.create({ + title: 'Another Task', + description: 'Desc', + status: 'in_progress', + priority: 'high', + dueDate: '2025-01-01T00:00:00.000Z' + }); + expect(task.title).toBe('Another Task'); + expect(task.description).toBe('Desc'); + expect(task.status).toBe('in_progress'); + expect(task.priority).toBe('high'); + expect(task.dueDate).toBe('2025-01-01T00:00:00.000Z'); + }); + }); + + describe('getAll', () => { + it('should return all tasks', () => { + expect(taskService.getAll()).toHaveLength(0); + taskService.create({ title: 'Task 1' }); + taskService.create({ title: 'Task 2' }); + expect(taskService.getAll()).toHaveLength(2); + }); + }); + + describe('findById', () => { + it('should find a task by id', () => { + const task = taskService.create({ title: 'Find Me' }); + const found = taskService.findById(task.id); + expect(found).toEqual(task); + }); + + it('should return undefined for non-existent id', () => { + expect(taskService.findById('not-a-real-id')).toBeUndefined(); + }); + }); + + describe('getByStatus', () => { + it('should filter tasks by status', () => { + taskService.create({ title: 'Todo 1', status: 'todo' }); + taskService.create({ title: 'Todo 2', status: 'todo' }); + taskService.create({ title: 'Done 1', status: 'done' }); + + const todos = taskService.getByStatus('todo'); + expect(todos).toHaveLength(2); + expect(todos.every(t => t.status === 'todo')).toBe(true); + + const doneTasks = taskService.getByStatus('done'); + expect(doneTasks).toHaveLength(1); + expect(doneTasks[0].status).toBe('done'); + }); + + // Expose the bug! + it('should match status exactly (not partially)', () => { + taskService.create({ title: 'Todo Task', status: 'todo' }); + taskService.create({ title: 'Done Task', status: 'done' }); + + const doTasks = taskService.getByStatus('do'); + // If we search for 'do' exactly, it should not match 'todo' or 'done' + expect(doTasks).toHaveLength(0); + }); + }); + + describe('getPaginated', () => { + it('should return paginated tasks', () => { + for (let i = 0; i < 5; i++) { + taskService.create({ title: `Task ${i}` }); + } + const page0 = taskService.getPaginated(0, 2); + expect(page0).toHaveLength(2); + expect(page0[0].title).toBe('Task 0'); + + const page1 = taskService.getPaginated(1, 2); + expect(page1).toHaveLength(2); + expect(page1[0].title).toBe('Task 2'); + + const page2 = taskService.getPaginated(2, 2); + expect(page2).toHaveLength(1); + expect(page2[0].title).toBe('Task 4'); + }); + }); + + describe('getStats', () => { + it('should return correct counts and overdue tasks', () => { + const now = new Date(); + const past = new Date(now.getTime() - 100000).toISOString(); + const future = new Date(now.getTime() + 100000).toISOString(); + + taskService.create({ title: 'T1', status: 'todo' }); + taskService.create({ title: 'T2', status: 'todo', dueDate: past }); // overdue + taskService.create({ title: 'T3', status: 'in_progress', dueDate: future }); + taskService.create({ title: 'T4', status: 'done', dueDate: past }); // done, not overdue + + const stats = taskService.getStats(); + expect(stats.todo).toBe(2); + expect(stats.in_progress).toBe(1); + expect(stats.done).toBe(1); + expect(stats.overdue).toBe(1); + }); + }); + + describe('update', () => { + it('should update an existing task', () => { + const task = taskService.create({ title: 'Old Title' }); + const updated = taskService.update(task.id, { title: 'New Title', status: 'in_progress' }); + expect(updated.title).toBe('New Title'); + expect(updated.status).toBe('in_progress'); + + const fetched = taskService.findById(task.id); + expect(fetched.title).toBe('New Title'); + }); + + it('should return null if task not found', () => { + const updated = taskService.update('invalid-id', { title: 'New' }); + expect(updated).toBeNull(); + }); + }); + + describe('remove', () => { + it('should remove an existing task', () => { + const task = taskService.create({ title: 'Delete me' }); + const success = taskService.remove(task.id); + expect(success).toBe(true); + expect(taskService.findById(task.id)).toBeUndefined(); + }); + + it('should return false if task not found', () => { + const success = taskService.remove('invalid-id'); + expect(success).toBe(false); + }); + }); + + describe('completeTask', () => { + it('should mark a task as done', () => { + const task = taskService.create({ title: 'Complete me', priority: 'high', status: 'todo' }); + const completed = taskService.completeTask(task.id); + expect(completed.status).toBe('done'); + expect(completed.completedAt).not.toBeNull(); + + // Expose the bug! Complete shouldn't alter priority + expect(completed.priority).toBe('high'); + }); + + it('should return null if task not found', () => { + const completed = taskService.completeTask('invalid-id'); + expect(completed).toBeNull(); + }); + }); +}); diff --git a/task-api/tests/tasks.api.test.js b/task-api/tests/tasks.api.test.js new file mode 100644 index 00000000..595f9f88 --- /dev/null +++ b/task-api/tests/tasks.api.test.js @@ -0,0 +1,178 @@ +const request = require('supertest'); +const app = require('../src/app'); +const taskService = require('../src/services/taskService'); + +describe('Tasks API', () => { + beforeEach(() => { + taskService._reset(); + }); + + describe('GET /tasks/stats', () => { + it('should return 200 and task stats', async () => { + taskService.create({ title: 'Task 1', status: 'todo' }); + taskService.create({ title: 'Task 2', status: 'done' }); + + const res = await request(app).get('/tasks/stats'); + expect(res.statusCode).toBe(200); + expect(res.body.todo).toBe(1); + expect(res.body.done).toBe(1); + }); + }); + + describe('GET /tasks', () => { + beforeEach(() => { + taskService.create({ title: 'Task 1', status: 'todo' }); + taskService.create({ title: 'Task 2', status: 'in_progress' }); + taskService.create({ title: 'Task 3', status: 'done' }); + }); + + it('should return all tasks', async () => { + const res = await request(app).get('/tasks'); + expect(res.statusCode).toBe(200); + expect(res.body.length).toBe(3); + }); + + it('should filter tasks by status', async () => { + const res = await request(app).get('/tasks?status=todo'); + expect(res.statusCode).toBe(200); + expect(res.body.length).toBe(1); + expect(res.body[0].status).toBe('todo'); + }); + + it('should return paginated tasks', async () => { + const res = await request(app).get('/tasks?page=0&limit=2'); + expect(res.statusCode).toBe(200); + expect(res.body.length).toBe(2); + expect(res.body[0].title).toBe('Task 1'); + expect(res.body[1].title).toBe('Task 2'); + }); + }); + + describe('POST /tasks', () => { + it('should return 201 and create a task', async () => { + const res = await request(app) + .post('/tasks') + .send({ title: 'New Task', priority: 'high' }); + expect(res.statusCode).toBe(201); + expect(res.body.title).toBe('New Task'); + expect(res.body.priority).toBe('high'); + expect(taskService.getAll().length).toBe(1); + }); + + it('should return 400 if validation fails', async () => { + const res = await request(app) + .post('/tasks') + .send({ priority: 'high' }); // missing title + expect(res.statusCode).toBe(400); + expect(res.body.error).toBeDefined(); + }); + + it('should return 400 if priority is invalid', async () => { + const res = await request(app) + .post('/tasks') + .send({ title: 'T1', priority: 'ultra' }); + expect(res.statusCode).toBe(400); + expect(res.body.error).toMatch(/priority/); + }); + }); + + describe('PUT /tasks/:id', () => { + let taskId; + beforeEach(() => { + const task = taskService.create({ title: 'Original' }); + taskId = task.id; + }); + + it('should return 200 and update the task', async () => { + const res = await request(app) + .put(`/tasks/${taskId}`) + .send({ title: 'Updated Task', status: 'in_progress' }); + expect(res.statusCode).toBe(200); + expect(res.body.title).toBe('Updated Task'); + expect(res.body.status).toBe('in_progress'); + }); + + it('should return 404 if task not found', async () => { + const res = await request(app) + .put('/tasks/invalid-id') + .send({ title: 'Updated' }); + expect(res.statusCode).toBe(404); + }); + + it('should return 400 if validation fails', async () => { + const res = await request(app) + .put(`/tasks/${taskId}`) + .send({ title: '' }); // invalid empty title + expect(res.statusCode).toBe(400); + }); + }); + + describe('DELETE /tasks/:id', () => { + let taskId; + beforeEach(() => { + const task = taskService.create({ title: 'To Delete' }); + taskId = task.id; + }); + + it('should return 204 and delete the task', async () => { + const res = await request(app).delete(`/tasks/${taskId}`); + expect(res.statusCode).toBe(204); + expect(taskService.findById(taskId)).toBeUndefined(); + }); + + it('should return 404 if task not found', async () => { + const res = await request(app).delete('/tasks/invalid-id'); + expect(res.statusCode).toBe(404); + }); + }); + + describe('PATCH /tasks/:id/complete', () => { + let taskId; + beforeEach(() => { + const task = taskService.create({ title: 'To Complete', status: 'todo' }); + taskId = task.id; + }); + + it('should return 200 and mark task complete', async () => { + const res = await request(app).patch(`/tasks/${taskId}/complete`); + expect(res.statusCode).toBe(200); + expect(res.body.status).toBe('done'); + expect(res.body.completedAt).not.toBeNull(); + }); + + it('should return 404 if task not found', async () => { + const res = await request(app).patch('/tasks/invalid-id/complete'); + expect(res.statusCode).toBe(404); + }); + }); + + describe('PATCH /tasks/:id/assign', () => { + let taskId; + beforeEach(() => { + const task = taskService.create({ title: 'To Assign' }); + taskId = task.id; + }); + + it('should return 200 and assign task', async () => { + const res = await request(app) + .patch(`/tasks/${taskId}/assign`) + .send({ assignee: 'Parth' }); + expect(res.statusCode).toBe(200); + expect(res.body.assignee).toBe('Parth'); + }); + + it('should return 400 for invalid assignee', async () => { + const res = await request(app) + .patch(`/tasks/${taskId}/assign`) + .send({ assignee: '' }); + expect(res.statusCode).toBe(400); + }); + + it('should return 404 if task not found', async () => { + const res = await request(app) + .patch('/tasks/invalid-id/assign') + .send({ assignee: 'Parth' }); + expect(res.statusCode).toBe(404); + }); + }); +}); From 68e5c997113296b896a17096520818ec9e31e744 Mon Sep 17 00:00:00 2001 From: Parthmudgal15105 Date: Wed, 15 Apr 2026 01:57:08 +0530 Subject: [PATCH 2/2] changes --- .claude/settings.json | 7 ++++ BUG_REPORT.md | 85 ++++++++++++++++++++++++++++++++++--------- SUBMISSION.md | 77 +++++++++++++++++++++++++++++++++------ 3 files changed, 141 insertions(+), 28 deletions(-) create mode 100644 .claude/settings.json diff --git a/.claude/settings.json b/.claude/settings.json new file mode 100644 index 00000000..47b3190c --- /dev/null +++ b/.claude/settings.json @@ -0,0 +1,7 @@ +{ + "permissions": { + "allow": [ + "Bash(npm run:*)" + ] + } +} diff --git a/BUG_REPORT.md b/BUG_REPORT.md index 00ba7e20..5c6ea948 100644 --- a/BUG_REPORT.md +++ b/BUG_REPORT.md @@ -1,19 +1,70 @@ # Bug Report -## Bug 1: Status Filter Inexact Matching -- **Expected Behavior:** Searching by status (e.g. `?status=todo`) should strictly return tasks with exactly that status. -- **Actual Behavior:** Searching for `status="do"` returns both `"todo"` and `"done"` tasks. -- **How I discovered it:** Writing unit tests for `getByStatus` in `taskService.js`; observing that `.includes(status)` was used instead of strict `===` equality. -- **Proposed Fix:** Change `tasks.filter((t) => t.status.includes(status))` to `tasks.filter((t) => t.status === status)`. - -## Bug 2: Task Completion Overwrites Priority -- **Expected Behavior:** Marking a task as complete should set its status to `"done"` and update `completedAt`, but leave its `priority` untouched. -- **Actual Behavior:** The `completeTask` method explicitly sets `priority: 'medium'`, resetting high or low priority entries to medium. -- **How I discovered it:** Writing the unit test for `completeTask()` in `taskService.js` and asserting the `priority` remains the same. -- **Proposed Fix:** Remove `priority: 'medium'` from the updated object in the `completeTask` function. - -## Bug 3: Pagination Defaults to Page 1 on Page 0 Request -- **Expected Behavior:** Passing `?page=0&limit=10` should evaluate to `page=0` and return the first 10 items. -- **Actual Behavior:** The server processes `parseInt('0') || 1`, determining `0` is falsy and forcing `page = 1`. This leads to skipping the first page in 0-indexed systems. -- **How I discovered it:** Writing the integration test (`GET /tasks?page=0&limit=2`) and seeing that instead of 2 items returned, only 1 was returned (offset was shifted). -- **Proposed Fix:** Use stricter falsy checks `isNaN(parseInt(page)) ? 1 : parseInt(page)`. +Three bugs were found while writing the test suite. All three have been fixed. + +--- + +## Bug 1: Status filter does partial string matching + +**Expected behavior:** +`GET /tasks?status=todo` should return only tasks with status exactly equal to `"todo"`. + +**Actual behavior:** +The original `getByStatus` implementation used `.includes(status)`, which matches substrings. A query like `?status=do` would incorrectly return both `"todo"` and `"done"` tasks. + +**How I found it:** +While writing a unit test for `getByStatus` in `taskService.js`, I added a case that searched for the partial string `"do"` and expected zero results. The test failed — both `todo` and `done` tasks came back. + +**Fix applied** (`src/services/taskService.js` line 9): +```js +// Before +const getByStatus = (status) => tasks.filter((t) => t.status.includes(status)); + +// After +const getByStatus = (status) => tasks.filter((t) => t.status === status); +``` + +--- + +## Bug 2: Completing a task silently resets its priority + +**Expected behavior:** +Marking a task complete should update `status` to `"done"` and set `completedAt`. All other fields — including `priority` — should stay exactly as they were. + +**Actual behavior:** +The `completeTask` function was spreading the task and then hardcoding `priority: 'medium'`. So a `"high"` priority task would silently become `"medium"` when completed. No error, no warning — just quiet data corruption. + +**How I found it:** +I wrote a test that created a task with `priority: 'high'`, called `completeTask`, and asserted the priority hadn't changed. The assertion failed. I traced it back to a stray field in the return object inside `completeTask`. + +**Fix applied** (`src/services/taskService.js` lines 63–76): +```js +// Removed the line: +priority: 'medium', +// from the updated task spread inside completeTask() +``` + +--- + +## Bug 3: Page 0 is treated as page 1 + +**Expected behavior:** +The API uses zero-based pagination internally (offset = page × limit). A request with `?page=0&limit=10` should return results starting at index 0. + +**Actual behavior:** +The original route handler used `parseInt(page) || 1`. Since `0` is falsy in JavaScript, `parseInt('0') || 1` evaluates to `1`, silently skipping to the second page. So page 0 and page 1 would return the same results. + +**How I found it:** +I wrote an integration test for `GET /tasks?page=0&limit=2` that expected the first two tasks. The test returned the wrong set — offset was one page ahead. + +**Fix applied** (`src/routes/tasks.js` lines 20–22): +```js +// Before +const pageNum = parseInt(page) || 1; + +// After +const parsedPage = parseInt(page); +const pageNum = isNaN(parsedPage) ? 1 : parsedPage; +``` + +This correctly defaults to `1` when the query param is missing or non-numeric, while allowing `0` through as a valid page number. diff --git a/SUBMISSION.md b/SUBMISSION.md index fa2583db..8475d630 100644 --- a/SUBMISSION.md +++ b/SUBMISSION.md @@ -1,18 +1,73 @@ # Submission Notes -Here are my notes for the conclusion of this take-home assignment: +## Coverage Summary -## Next Steps with More Time -- **Pagination Boundary Assertions:** Add tests to catch extreme boundaries on limit (e.g. `limit=100000`) and ensure negative pages fail gracefully. -- **Data Store Modularity:** I would mock an external data provider or database to test `taskService.js` under simulated delay/rejection conditions instead of tight coupling. -- **Validation Combinations:** Test deeper combinations like completing tasks without updating `completedAt`, or verifying `assignee` doesn't break standard `createdAt` tracking. +All 32 tests pass. Here's the final coverage output: + +``` +-----------------|---------|----------|---------|---------|------------------- +File | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s +-----------------|---------|----------|---------|---------|------------------- +All files | 94.26 | 84.88 | 93.33 | 93.70 | + src | 69.23 | 75.00 | 0.00 | 69.23 | 10-11,17-18 + app.js | 69.23 | 75.00 | 0.00 | 69.23 | 10-11,17-18 + src/routes | 100.00 | 91.66 | 100.00 | 100.00 | + tasks.js | 100.00 | 91.66 | 100.00 | 100.00 | 21-23 + src/services | 100.00 | 94.73 | 100.00 | 100.00 | + taskService.js | 100.00 | 94.73 | 100.00 | 100.00 | 22 + src/utils | 81.48 | 76.92 | 100.00 | 81.48 | 9,15,25,28,31 + validators.js | 81.48 | 76.92 | 100.00 | 81.48 | 9,15,25,28,31 +-----------------|---------|----------|---------|---------|------------------- +``` + +The lower coverage in `app.js` is expected — those uncovered lines are the `app.listen` block that only runs when the file is invoked directly, not during testing. The gap in `validators.js` is a few defensive branches (e.g. `dueDate` validation) that aren't exercised by the current test suite. + +--- + +## Bugs Found and Fixed + +All three bugs from the bug report were fixed. A quick summary: + +- **Partial status matching** — `getByStatus` used `.includes()` instead of `===`, so searching for `"do"` would match both `"todo"` and `"done"`. Fixed with an exact equality check. +- **Priority reset on complete** — `completeTask` was hardcoding `priority: 'medium'` in the returned object, silently overwriting whatever priority the task had. Removed that line. +- **Page 0 treated as page 1** — The route used `parseInt(page) || 1`, and since `0` is falsy, page 0 defaulted to page 1. Fixed by checking `isNaN` explicitly instead of relying on truthiness. + +See [BUG_REPORT.md](./BUG_REPORT.md) for the full breakdown including where each bug lives in the code and what the fix looks like. + +--- + +## The Assign Endpoint + +`PATCH /tasks/:id/assign` accepts `{ "assignee": "string" }` and stores it directly on the task object. + +A few decisions worth noting: + +**Validation:** I required `assignee` to be a non-empty string. An empty string or a missing field returns a 400. I didn't validate against a list of known users — since there's no user table, that would require assumptions about architecture that aren't in scope here. + +**Overwriting:** Re-assigning a task to someone new just overwrites the current `assignee`. There's no history or ownership check. That felt right for a simple task manager — if you want to reassign it, you reassign it. + +**Field placement:** The `assignee` field is stored alongside the other task fields and returned in the response. It wasn't in the original task shape, so it won't appear on tasks that haven't been assigned yet (it'll just be absent rather than `null`). A production version might want to initialize `assignee: null` in `create()` for consistency. + +--- + +## What I'd Test Next With More Time + +- **Boundary conditions on pagination** — what happens with `limit=0`, `limit=999999`, or `page=-1`? The service would behave oddly and there are no guards for it. +- **Validator edge cases** — the `dueDate` validation branches in `validators.js` aren't fully covered. I'd add tests for things like `dueDate: "not-a-date"` or `dueDate: 12345` (a number, not a string). +- **Concurrency / ordering guarantees** — the in-memory store is a plain array. In a real environment you'd want tests that verify behavior under concurrent writes, but that's more relevant once there's a real database behind it. + +--- ## Surprises in the Codebase -- **Priority Override:** Noticing that the `completeTask` function forcefully reset priority to `"medium"` was unexpected — and arguably quite jarring to user experience! -- **Idempotency Holes:** `update()` could overwrite internal structure components since it carelessly merged `...fields` over top of the task rather than isolating permitted keys. -- **Node Start Strategy:** Seeing `require.main === module` inside `app.js` was a neat addition not generally seen in bare-bone setups; great for isolating testing safely! + +The priority override in `completeTask` was the most surprising thing. It wasn't obviously intentional — there's no comment explaining why completing a task would reset priority to medium, and it's the kind of silent data mutation that's easy to miss in manual testing. It would only surface once you actually checked the priority field after completion, which you'd only do if you thought to test it. That's exactly the kind of bug that a test suite catches. + +I also appreciated the `require.main === module` guard in `app.js`. It's a small thing, but it makes the app cleanly importable by tests without accidentally starting a server on port 3000 every time a test file loads. + +--- ## Questions Before Shipping to Production -- **Database & Migration:** Which database are we targeting (e.g., MongoDB, Postgres)? This entirely changes the indexing approach for the `assignee` and `status` fields. -- **Authentication/Ownership:** Should endpoints enforce authentication boundaries? Should `assignee` map against a table of known users? -- **Rate-Limiting:** Do we expect burst submissions? We might need rate limiters (`express-rate-limit`) to prevent server crashes since there's no capping. + +- **What database are we targeting?** The in-memory store means all data disappears on restart. The schema (UUIDs, ISO dates, status enum) would map to Postgres reasonably well, but `assignee` and `status` would want indexes if we're filtering on them at scale. +- **Should `assignee` map to real users?** Right now it's a free-text string. If this connects to an auth system, you'd want to validate against actual user IDs and handle cases like "assigned user's account is deleted." +- **Is there any rate limiting?** The API currently has no throttling. A client could hammer `POST /tasks` indefinitely. Worth adding `express-rate-limit` before putting this in front of real traffic.