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 new file mode 100644 index 00000000..5c6ea948 --- /dev/null +++ b/BUG_REPORT.md @@ -0,0 +1,70 @@ +# Bug Report + +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 new file mode 100644 index 00000000..8475d630 --- /dev/null +++ b/SUBMISSION.md @@ -0,0 +1,73 @@ +# Submission Notes + +## Coverage Summary + +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 + +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 + +- **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. 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); + }); + }); +});