From b22d2555d8c2510704170d08ca34d1574ee9b90a Mon Sep 17 00:00:00 2001 From: Hayat Azeem Date: Thu, 16 Apr 2026 23:10:47 +0530 Subject: [PATCH] feat: completed take-home assignment --- bug_report.md | 30 +++++ task-api/package-lock.json | 18 +-- task-api/src/routes/tasks.js | 16 ++- task-api/src/services/taskService.js | 13 +- task-api/src/utils/validators.js | 9 +- task-api/tests/taskService.test.js | 160 ++++++++++++++++++++++++ task-api/tests/tasks.test.js | 180 +++++++++++++++++++++++++++ 7 files changed, 414 insertions(+), 12 deletions(-) create mode 100644 bug_report.md create mode 100644 task-api/tests/taskService.test.js create mode 100644 task-api/tests/tasks.test.js diff --git a/bug_report.md b/bug_report.md new file mode 100644 index 00000000..60209c45 --- /dev/null +++ b/bug_report.md @@ -0,0 +1,30 @@ +# Bug Report + +During the testing phase, I identified three bugs in the API logic. I fixed the first one (Pagination Bug) as part of this assignment. + +--- + +## 1. Pagination Offset Calculation Bug (Fixed) + +- **Expected Behavior**: When requesting `GET /tasks?page=1&limit=10`, the API should return the first 10 items (items 0 to 9). +- **Actual Behavior**: The API skips the first 10 items and returns items 10 to 19. +- **How it was discovered**: I noticed the formula `const offset = page * limit` in `src/services/taskService.js`. Since `page` safely defaults to `1` in the route handler, `offset` becomes `10` instead of `0`. +- **Fix Applied**: Changed the offset calculation in `getPaginated` to: `const offset = (page - 1) * limit;` + +--- + +## 2. Invalid Status Matching in getByStatus + +- **Expected Behavior**: Filtering by status like `GET /tasks?status=todo` should return exactly tasks that have the status `todo`. +- **Actual Behavior**: If a user submits `?status=o`, it matches both `todo` and `in_progress` because the code uses `.includes(status)`. +- **How it was discovered**: Reviewing the `taskService.getByStatus` method, it uses `tasks.filter((t) => t.status.includes(status))`. +- **Proposed Fix**: Use exact equality matching in `getByStatus`: `t.status === status`. + +--- + +## 3. completeTask Overwrites Priority + +- **Expected Behavior**: Marking a task as complete (`PATCH /tasks/:id/complete`) should update the status to `done` and set `completedAt` without modifying other unconnected properties like `priority`. +- **Actual Behavior**: The method hardcodes `priority: 'medium'`, which overwrites any `high` or `low` priority that the task originally had. +- **How it was discovered**: Reviewing `taskService.completeTask()`. +- **Proposed Fix**: Remove the `priority: 'medium'` line from the updated object payload in `completeTask`. diff --git a/task-api/package-lock.json b/task-api/package-lock.json index 901be207..45bb9467 100644 --- a/task-api/package-lock.json +++ b/task-api/package-lock.json @@ -1357,9 +1357,9 @@ } }, "node_modules/brace-expansion": { - "version": "1.1.12", - "resolved": "https://registry.npmjs.org/brace-expansion/-/brace-expansion-1.1.12.tgz", - "integrity": "sha512-9T9UjW3r0UW5c1Q7GTwllptXwhvYmEzFhzMfZ9H7FQWt+uZePjZPjBP/W1ZEyZ1twGWom5/56TF4lPcqjnDHcg==", + "version": "1.1.14", + "resolved": "https://registry.npmjs.org/brace-expansion/-/brace-expansion-1.1.14.tgz", + "integrity": "sha512-MWPGfDxnyzKU7rNOW9SP/c50vi3xrmrua/+6hfPbCS2ABNWfx24vPidzvC7krjU/RTo235sV776ymlsMtGKj8g==", "dev": true, "license": "MIT", "dependencies": { @@ -3773,9 +3773,9 @@ "license": "MIT" }, "node_modules/path-to-regexp": { - "version": "0.1.12", - "resolved": "https://registry.npmjs.org/path-to-regexp/-/path-to-regexp-0.1.12.tgz", - "integrity": "sha512-RA1GjUVMnvYFxuqovrEqZoxxW5NUZqbwKtYz/Tt7nXerk0LbLblQmrsgdeOxV5SFHf0UDggjS/bSeOZwt1pmEQ==", + "version": "0.1.13", + "resolved": "https://registry.npmjs.org/path-to-regexp/-/path-to-regexp-0.1.13.tgz", + "integrity": "sha512-A/AGNMFN3c8bOlvV9RreMdrv7jsmF9XIfDeCd87+I8RNg6s78BhJxMu69NEMHBSJFxKidViTEdruRwEk/WIKqA==", "license": "MIT" }, "node_modules/picocolors": { @@ -3786,9 +3786,9 @@ "license": "ISC" }, "node_modules/picomatch": { - "version": "2.3.1", - "resolved": "https://registry.npmjs.org/picomatch/-/picomatch-2.3.1.tgz", - "integrity": "sha512-JU3teHTNjmE2VCGFzuY8EXzCDVwEqB2a8fsIvwaStHhAWJEeVd1o1QD80CU6+ZdEXXSLbSsuLwJjkCBWqRQUVA==", + "version": "2.3.2", + "resolved": "https://registry.npmjs.org/picomatch/-/picomatch-2.3.2.tgz", + "integrity": "sha512-V7+vQEJ06Z+c5tSye8S+nHUfI51xoXIXjHQ99cQtKUkQqqO1kO/KCJUfZXuB47h/YBlDhah2H3hdUGXn8ie0oA==", "dev": true, "license": "MIT", "engines": { diff --git a/task-api/src/routes/tasks.js b/task-api/src/routes/tasks.js index e8c370fe..89a14581 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(); @@ -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); + 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..f12431bc 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; return tasks.slice(offset, offset + limit); }; @@ -36,6 +36,7 @@ const create = ({ title, description = '', status = 'todo', priority = 'medium', status, priority, dueDate, + assignee: null, completedAt: null, createdAt: new Date().toISOString(), }; @@ -76,6 +77,15 @@ const completeTask = (id) => { return updated; }; +const assignTask = (id, assignee) => { + const index = tasks.findIndex((t) => t.id === id); + if (index === -1) return null; + + const updated = { ...tasks[index], assignee }; + tasks[index] = updated; + return updated; +}; + const _reset = () => { tasks = []; }; @@ -90,5 +100,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..07afe5f3 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 (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..c44ce9a2 --- /dev/null +++ b/task-api/tests/taskService.test.js @@ -0,0 +1,160 @@ +const taskService = require('../src/services/taskService'); + +describe('Task Service', () => { + beforeEach(() => { + taskService._reset(); + }); + + describe('create', () => { + it('should create a new 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).toHaveProperty('createdAt'); + }); + + it('should create a new task with provided values', () => { + const task = taskService.create({ + title: 'Another task', + description: 'Details', + status: 'in_progress', + priority: 'high', + dueDate: '2026-12-31T23:59:59.000Z' + }); + expect(task.status).toBe('in_progress'); + expect(task.priority).toBe('high'); + expect(task.dueDate).toBe('2026-12-31T23:59:59.000Z'); + }); + }); + + describe('getAll', () => { + it('should return all tasks', () => { + taskService.create({ title: 'Task 1' }); + taskService.create({ title: 'Task 2' }); + const tasks = taskService.getAll(); + expect(tasks).toHaveLength(2); + }); + }); + + describe('findById', () => { + it('should return the correct task by id', () => { + const created = taskService.create({ title: 'Find me' }); + const found = taskService.findById(created.id); + expect(found).toEqual(created); + }); + + it('should return undefined if id not found', () => { + const found = taskService.findById('invalid-id'); + expect(found).toBeUndefined(); + }); + }); + + describe('getByStatus', () => { + it('should return tasks matching the status', () => { + taskService.create({ title: 'Task 1', status: 'todo' }); + taskService.create({ title: 'Task 2', status: 'done' }); + const tasks = taskService.getByStatus('todo'); + expect(tasks).toHaveLength(1); + expect(tasks[0].title).toBe('Task 1'); + }); + + // Note: there is a bug with getByStatus matching using .includes + // Let's document its actual existing behavior so the test passes, + // or test the strict behavior since we'll fix it anyway? + // The instructions say "Write tests... At minimum: happy path...". We'll just test the happy path here. + }); + + describe('getPaginated', () => { + it('should return a paginated slice of tasks based on page and limit', () => { + for (let i = 0; i < 5; i++) { + taskService.create({ title: `Task ${i}` }); + } + // After our bug fix, page 1 limit 2 should return first 2 + const tasks1 = taskService.getPaginated(1, 2); + expect(tasks1).toHaveLength(2); + expect(tasks1[0].title).toBe('Task 0'); + expect(tasks1[1].title).toBe('Task 1'); + + const tasks2 = taskService.getPaginated(2, 2); + expect(tasks2).toHaveLength(2); + expect(tasks2[0].title).toBe('Task 2'); + expect(tasks2[1].title).toBe('Task 3'); + }); + }); + + describe('getStats', () => { + it('should calculate task stats correctly', () => { + taskService.create({ title: 'T1', status: 'todo' }); + taskService.create({ title: 'T2', status: 'in_progress' }); + taskService.create({ title: 'T3', status: 'done' }); + taskService.create({ title: 'T4', status: 'todo', dueDate: '2000-01-01T00:00:00Z' }); // 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 created = taskService.create({ title: 'Old title' }); + const updated = taskService.update(created.id, { title: 'New title', status: 'in_progress' }); + expect(updated.title).toBe('New title'); + expect(updated.status).toBe('in_progress'); + expect(updated.id).toBe(created.id); + }); + + it('should return null when updating a non-existent task', () => { + const result = taskService.update('invalid', { title: 'Test' }); + expect(result).toBeNull(); + }); + }); + + describe('remove', () => { + it('should delete an existing task', () => { + const created = taskService.create({ title: 'Delete me' }); + const success = taskService.remove(created.id); + expect(success).toBe(true); + expect(taskService.getAll()).toHaveLength(0); + }); + + it('should return false for non-existent task', () => { + const result = taskService.remove('invalid'); + expect(result).toBe(false); + }); + }); + + describe('completeTask', () => { + it('should mark task as complete and set completedAt', () => { + const created = taskService.create({ title: 'Complete me' }); + const completed = taskService.completeTask(created.id); + expect(completed.status).toBe('done'); + expect(completed.completedAt).not.toBeNull(); + }); + + it('should return null for non-existent task', () => { + const result = taskService.completeTask('invalid'); + expect(result).toBeNull(); + }); + }); + + describe('assignTask', () => { + it('should assign an assignee to the task', () => { + const created = taskService.create({ title: 'To assign' }); + const assigned = taskService.assignTask(created.id, 'Alice'); + expect(assigned.assignee).toBe('Alice'); + }); + + it('should return null for non-existent task', () => { + const result = taskService.assignTask('invalid', 'Bob'); + expect(result).toBeNull(); + }); + }); +}); diff --git a/task-api/tests/tasks.test.js b/task-api/tests/tasks.test.js new file mode 100644 index 00000000..ec3deed0 --- /dev/null +++ b/task-api/tests/tasks.test.js @@ -0,0 +1,180 @@ +const request = require('supertest'); +const app = require('../src/app'); +const taskService = require('../src/services/taskService'); + +describe('Tasks API', () => { + beforeEach(() => { + taskService._reset(); + }); + + describe('POST /tasks', () => { + it('should create a new task', async () => { + const response = await request(app) + .post('/tasks') + .send({ title: 'New Task' }); + + expect(response.status).toBe(201); + expect(response.body).toHaveProperty('id'); + expect(response.body.title).toBe('New Task'); + }); + + it('should return 400 if title is missing', async () => { + const response = await request(app) + .post('/tasks') + .send({}); + + expect(response.status).toBe(400); + expect(response.body.error).toBeDefined(); + }); + + it('should return 400 if status is invalid', async () => { + const response = await request(app) + .post('/tasks') + .send({ title: 'Task', status: 'invalid_status' }); + + expect(response.status).toBe(400); + expect(response.body.error).toBeDefined(); + }); + }); + + describe('GET /tasks', () => { + beforeEach(() => { + taskService.create({ title: 'Task 1', status: 'todo' }); + taskService.create({ title: 'Task 2', status: 'done' }); + taskService.create({ title: 'Task 3', status: 'in_progress' }); + }); + + it('should return all tasks', async () => { + const response = await request(app).get('/tasks'); + expect(response.status).toBe(200); + expect(response.body).toHaveLength(3); + }); + + it('should filter tasks by status', async () => { + const response = await request(app).get('/tasks?status=todo'); + expect(response.status).toBe(200); + expect(response.body).toHaveLength(1); + expect(response.body[0].title).toBe('Task 1'); + }); + + it('should paginate tasks (with bug in mind)', async () => { + // After fixing bug, page 1 limit 2 should return first 2 items + const response = await request(app).get('/tasks?page=1&limit=2'); + expect(response.status).toBe(200); + expect(response.body).toHaveLength(2); + expect(response.body[0].title).toBe('Task 1'); + expect(response.body[1].title).toBe('Task 2'); + }); + }); + + describe('GET /tasks/stats', () => { + it('should return task statistics', async () => { + taskService.create({ title: 'T1', status: 'todo' }); + taskService.create({ title: 'T2', status: 'done' }); + + const response = await request(app).get('/tasks/stats'); + expect(response.status).toBe(200); + expect(response.body.todo).toBe(1); + expect(response.body.done).toBe(1); + expect(response.body.in_progress).toBe(0); + }); + }); + + describe('PUT /tasks/:id', () => { + let task; + beforeEach(() => { + task = taskService.create({ title: 'Old Title' }); + }); + + it('should update task successfully', async () => { + const response = await request(app) + .put(`/tasks/${task.id}`) + .send({ title: 'New Title' }); + + expect(response.status).toBe(200); + expect(response.body.title).toBe('New Title'); + }); + + it('should return 404 for non-existent task', async () => { + const response = await request(app) + .put('/tasks/invalid-id') + .send({ title: 'New Title' }); + + expect(response.status).toBe(404); + }); + + it('should return 400 for invalid data', async () => { + const response = await request(app) + .put(`/tasks/${task.id}`) + .send({ status: 'invalid_status' }); + + expect(response.status).toBe(400); + }); + }); + + describe('DELETE /tasks/:id', () => { + let task; + beforeEach(() => { + task = taskService.create({ title: 'To Delete' }); + }); + + it('should delete a task', async () => { + const response = await request(app).delete(`/tasks/${task.id}`); + expect(response.status).toBe(204); + expect(taskService.getAll()).toHaveLength(0); + }); + + it('should return 404 for non-existent task', async () => { + const response = await request(app).delete('/tasks/invalid-id'); + expect(response.status).toBe(404); + }); + }); + + describe('PATCH /tasks/:id/complete', () => { + let task; + beforeEach(() => { + task = taskService.create({ title: 'To Complete', status: 'todo' }); + }); + + it('should mark task as complete', async () => { + const response = await request(app).patch(`/tasks/${task.id}/complete`); + expect(response.status).toBe(200); + expect(response.body.status).toBe('done'); + expect(response.body.completedAt).not.toBeNull(); + }); + + it('should return 404 for non-existent task', async () => { + const response = await request(app).patch('/tasks/invalid-id/complete'); + expect(response.status).toBe(404); + }); + }); + + describe('PATCH /tasks/:id/assign', () => { + let task; + beforeEach(() => { + task = taskService.create({ title: 'To Assign' }); + }); + + it('should assign a task', async () => { + const response = await request(app) + .patch(`/tasks/${task.id}/assign`) + .send({ assignee: 'Alice' }); + expect(response.status).toBe(200); + expect(response.body.assignee).toBe('Alice'); + }); + + it('should return 400 for empty assignee', async () => { + const response = await request(app) + .patch(`/tasks/${task.id}/assign`) + .send({ assignee: '' }); + expect(response.status).toBe(400); + }); + + it('should return 404 for non-existent task', async () => { + const response = await request(app) + .patch('/tasks/invalid-id/assign') + .send({ assignee: 'Bob' }); + expect(response.status).toBe(404); + }); + }); +});