From e0720e59d26416e6ff335408c80abbced89a3817 Mon Sep 17 00:00:00 2001 From: Kumar-Saurabh-Tiwari Date: Tue, 14 Apr 2026 15:21:06 +0530 Subject: [PATCH] Complete assignment: tests, bug fix, assign endpoint, docs, and submission notes --- task-api/BUG_REPORT.md | 26 ++++ task-api/SUBMISSION_NOTES.md | 37 ++++++ task-api/src/routes/tasks.js | 20 ++- task-api/src/services/taskService.js | 23 +++- task-api/src/utils/validators.js | 10 +- task-api/tests/taskService.test.js | 129 ++++++++++++++++++++ task-api/tests/tasks.routes.test.js | 175 +++++++++++++++++++++++++++ 7 files changed, 417 insertions(+), 3 deletions(-) create mode 100644 task-api/BUG_REPORT.md create mode 100644 task-api/SUBMISSION_NOTES.md create mode 100644 task-api/tests/taskService.test.js create mode 100644 task-api/tests/tasks.routes.test.js diff --git a/task-api/BUG_REPORT.md b/task-api/BUG_REPORT.md new file mode 100644 index 00000000..381e04a8 --- /dev/null +++ b/task-api/BUG_REPORT.md @@ -0,0 +1,26 @@ +# Bug Report + +## 1. Pagination skips the first page (fixed) + +- Location: `src/services/taskService.js` in `getPaginated` +- Expected behavior: `page=1&limit=2` should return the first two tasks. +- Actual behavior: it returned tasks starting from index 2 (effectively page 2 behavior). +- How discovered: failing unit test in `tests/taskService.test.js` and failing integration test in `tests/tasks.routes.test.js`. +- Root cause: offset was calculated as `page * limit` instead of `(page - 1) * limit`. +- Fix applied: changed offset formula to `(page - 1) * limit`. + +## 2. Status filtering is too permissive (not fixed) + +- Location: `src/services/taskService.js` in `getByStatus` +- Expected behavior: filtering by status should match exact status values only. +- Actual behavior: it uses substring matching (`includes`), so invalid partial values can match unexpectedly. +- How discovered: code review while writing service tests. +- Suggested fix: replace `t.status.includes(status)` with `t.status === status` and consider validating `status` query values in the route. + +## 3. Completing a task overwrites priority (not fixed) + +- Location: `src/services/taskService.js` in `completeTask` +- Expected behavior: marking completion should update completion fields, not silently change unrelated task properties. +- Actual behavior: completion always sets `priority` to `medium`. +- How discovered: code review while writing tests for completion behavior. +- Suggested fix: remove forced `priority: 'medium'` from the completion update payload. diff --git a/task-api/SUBMISSION_NOTES.md b/task-api/SUBMISSION_NOTES.md new file mode 100644 index 00000000..c0af9eac --- /dev/null +++ b/task-api/SUBMISSION_NOTES.md @@ -0,0 +1,37 @@ +# Submission Notes + +## Coverage Summary + +- Command run: `npm run coverage -- --runInBand` +- Result: + - Statements: 93.67% + - Branches: 86.66% + - Functions: 93.33% + - Lines: 93.10% + +## Feature Design Decisions: PATCH /tasks/:id/assign + +- Validation: `assignee` must be a non-empty string after trim. +- Missing task: returns `404` with `Task not found`. +- Already assigned task: returns `409` with `Task is already assigned`. +- Data model: task now stores `assignee` and defaults to `null` on creation. + +## What I’d Test Next + +- Validation and behavior for pagination edge cases such as negative page/limit or very large values. +- Behavior when `status` query is invalid (currently no explicit query validation in route). +- Additional API contract tests for date parsing edge cases and timezone-related overdue calculations. +- More branch-focused tests around global error handler behavior. + +## What Surprised Me + +- A core pagination bug was present in the service offset calculation and was caught quickly by tests. +- Completion logic changes `priority` to `medium`, which is unexpected for a completion action. +- Status filtering uses substring matching, which allows accidental matches rather than strict status checks. + +## Questions Before Production + +- Should task assignment be reassignable (replace existing assignee), or should unassign/reassign endpoints be added explicitly? +- Should status query values be strictly validated and return `400` for unsupported values? +- Should completion preserve all existing fields except completion metadata (status and completedAt)? +- Should idempotency rules be defined for assignment and completion endpoints? diff --git a/task-api/src/routes/tasks.js b/task-api/src/routes/tasks.js index e8c370fe..85f39458 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,22 @@ 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 result = taskService.assignTask(req.params.id, req.body.assignee.trim()); + if (result.error === 'not_found') { + return res.status(404).json({ error: 'Task not found' }); + } + + if (result.error === 'already_assigned') { + return res.status(409).json({ error: 'Task is already assigned' }); + } + + res.json(result.task); +}); + module.exports = router; diff --git a/task-api/src/services/taskService.js b/task-api/src/services/taskService.js index f8e89189..21f49513 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,25 @@ const completeTask = (id) => { return updated; }; +const assignTask = (id, assignee) => { + const index = tasks.findIndex((t) => t.id === id); + if (index === -1) { + return { error: 'not_found' }; + } + + if (tasks[index].assignee) { + return { error: 'already_assigned' }; + } + + const updated = { + ...tasks[index], + assignee, + }; + + tasks[index] = updated; + return { task: updated }; +}; + const _reset = () => { tasks = []; }; @@ -90,5 +110,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..0821f5ed 100644 --- a/task-api/src/utils/validators.js +++ b/task-api/src/utils/validators.js @@ -33,4 +33,12 @@ const validateUpdateTask = (body) => { return null; }; -module.exports = { validateCreateTask, validateUpdateTask }; +const validateAssignTask = (body) => { + if (!body || typeof body.assignee !== 'string' || body.assignee.trim() === '') { + return 'assignee 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..fc6518e4 --- /dev/null +++ b/task-api/tests/taskService.test.js @@ -0,0 +1,129 @@ +const taskService = require('../src/services/taskService'); + +describe('taskService', () => { + beforeEach(() => { + taskService._reset(); + }); + + describe('create and read operations', () => { + test('create should apply defaults and return new task', () => { + const task = taskService.create({ title: 'Write tests' }); + + expect(task.id).toBeDefined(); + expect(task.title).toBe('Write tests'); + 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(); + }); + + test('getAll should return a copy, not the internal array', () => { + taskService.create({ title: 'Task 1' }); + const all = taskService.getAll(); + + all.push({ id: 'x', title: 'Injected' }); + + expect(taskService.getAll()).toHaveLength(1); + }); + + test('findById returns task when it exists', () => { + const created = taskService.create({ title: 'Find me' }); + + const found = taskService.findById(created.id); + + expect(found).toBeDefined(); + expect(found.id).toBe(created.id); + }); + }); + + describe('filtering and pagination', () => { + test('getByStatus should return only exact status matches', () => { + taskService.create({ title: 'Todo task', status: 'todo' }); + taskService.create({ title: 'Done task', status: 'done' }); + + const tasks = taskService.getByStatus('done'); + + expect(tasks).toHaveLength(1); + expect(tasks[0].status).toBe('done'); + }); + + test('getPaginated should return first page when page=1', () => { + taskService.create({ title: 'Task 1' }); + taskService.create({ title: 'Task 2' }); + taskService.create({ title: 'Task 3' }); + + const page1 = taskService.getPaginated(1, 2); + + expect(page1).toHaveLength(2); + expect(page1[0].title).toBe('Task 1'); + expect(page1[1].title).toBe('Task 2'); + }); + }); + + describe('update and delete operations', () => { + test('update should merge fields and return updated task', () => { + const created = taskService.create({ title: 'Old', priority: 'low' }); + + const updated = taskService.update(created.id, { title: 'New', status: 'in_progress' }); + + expect(updated).toBeDefined(); + expect(updated.title).toBe('New'); + expect(updated.status).toBe('in_progress'); + expect(updated.priority).toBe('low'); + }); + + test('update should return null for missing task', () => { + const updated = taskService.update('missing-id', { title: 'Nope' }); + expect(updated).toBeNull(); + }); + + test('remove should delete existing task and return true', () => { + const created = taskService.create({ title: 'Delete me' }); + + const deleted = taskService.remove(created.id); + + expect(deleted).toBe(true); + expect(taskService.findById(created.id)).toBeUndefined(); + }); + + test('remove should return false for missing task', () => { + const deleted = taskService.remove('missing-id'); + expect(deleted).toBe(false); + }); + }); + + describe('completion and stats', () => { + test('completeTask should mark task as done and set completedAt', () => { + const created = taskService.create({ title: 'Complete me', priority: 'high' }); + + const completed = taskService.completeTask(created.id); + + expect(completed).toBeDefined(); + expect(completed.status).toBe('done'); + expect(completed.completedAt).toBeDefined(); + }); + + test('completeTask should return null for missing task', () => { + const completed = taskService.completeTask('missing-id'); + expect(completed).toBeNull(); + }); + + test('getStats should return status counts and overdue count', () => { + const yesterday = new Date(Date.now() - 24 * 60 * 60 * 1000).toISOString(); + const tomorrow = new Date(Date.now() + 24 * 60 * 60 * 1000).toISOString(); + + taskService.create({ title: 'Todo overdue', status: 'todo', dueDate: yesterday }); + taskService.create({ title: 'In progress due later', status: 'in_progress', dueDate: tomorrow }); + taskService.create({ title: 'Done overdue but completed', status: 'done', dueDate: yesterday }); + + const stats = taskService.getStats(); + + expect(stats.todo).toBe(1); + expect(stats.in_progress).toBe(1); + expect(stats.done).toBe(1); + expect(stats.overdue).toBe(1); + }); + }); +}); diff --git a/task-api/tests/tasks.routes.test.js b/task-api/tests/tasks.routes.test.js new file mode 100644 index 00000000..f3a4153a --- /dev/null +++ b/task-api/tests/tasks.routes.test.js @@ -0,0 +1,175 @@ +const request = require('supertest'); +const app = require('../src/app'); +const taskService = require('../src/services/taskService'); + +describe('tasks routes', () => { + beforeEach(() => { + taskService._reset(); + }); + + test('GET /tasks returns all tasks', async () => { + const response = await request(app).get('/tasks'); + + expect(response.status).toBe(200); + expect(response.body).toEqual([]); + }); + + test('POST /tasks creates a task', async () => { + const response = await request(app).post('/tasks').send({ title: 'API task', priority: 'high' }); + + expect(response.status).toBe(201); + expect(response.body.title).toBe('API task'); + expect(response.body.priority).toBe('high'); + expect(response.body.status).toBe('todo'); + }); + + test('POST /tasks returns 400 for invalid payload', async () => { + const response = await request(app).post('/tasks').send({ title: ' ' }); + + expect(response.status).toBe(400); + expect(response.body.error).toMatch(/title is required/i); + }); + + test('GET /tasks?status=todo filters tasks by status', async () => { + await request(app).post('/tasks').send({ title: 'Todo 1', status: 'todo' }); + await request(app).post('/tasks').send({ title: 'Done 1', status: 'done' }); + + const response = await request(app).get('/tasks?status=todo'); + + expect(response.status).toBe(200); + expect(response.body).toHaveLength(1); + expect(response.body[0].status).toBe('todo'); + }); + + test('GET /tasks pagination returns first page with expected items', async () => { + 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 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'); + }); + + test('PUT /tasks/:id updates an existing task', async () => { + const created = await request(app).post('/tasks').send({ title: 'Original', status: 'todo' }); + + const response = await request(app) + .put(`/tasks/${created.body.id}`) + .send({ title: 'Updated title', status: 'in_progress' }); + + expect(response.status).toBe(200); + expect(response.body.title).toBe('Updated title'); + expect(response.body.status).toBe('in_progress'); + }); + + test('PUT /tasks/:id returns 400 for invalid update payload', async () => { + const created = await request(app).post('/tasks').send({ title: 'Original' }); + + const response = await request(app).put(`/tasks/${created.body.id}`).send({ title: '' }); + + expect(response.status).toBe(400); + expect(response.body.error).toMatch(/title must be a non-empty string/i); + }); + + test('PUT /tasks/:id returns 404 when task does not exist', async () => { + const response = await request(app).put('/tasks/missing-id').send({ title: 'Updated' }); + + expect(response.status).toBe(404); + expect(response.body.error).toBe('Task not found'); + }); + + test('DELETE /tasks/:id deletes task and returns 204', async () => { + const created = await request(app).post('/tasks').send({ title: 'Delete me' }); + + const delResponse = await request(app).delete(`/tasks/${created.body.id}`); + const listResponse = await request(app).get('/tasks'); + + expect(delResponse.status).toBe(204); + expect(listResponse.body).toHaveLength(0); + }); + + test('DELETE /tasks/:id returns 404 for missing task', async () => { + const response = await request(app).delete('/tasks/missing-id'); + + expect(response.status).toBe(404); + expect(response.body.error).toBe('Task not found'); + }); + + test('PATCH /tasks/:id/complete marks task as done', async () => { + const created = await request(app).post('/tasks').send({ title: 'Complete me' }); + + const response = await request(app).patch(`/tasks/${created.body.id}/complete`); + + expect(response.status).toBe(200); + expect(response.body.status).toBe('done'); + expect(response.body.completedAt).toBeDefined(); + }); + + test('PATCH /tasks/:id/complete returns 404 for missing task', async () => { + const response = await request(app).patch('/tasks/missing-id/complete'); + + expect(response.status).toBe(404); + expect(response.body.error).toBe('Task not found'); + }); + + test('GET /tasks/stats returns counts and overdue value', async () => { + const yesterday = new Date(Date.now() - 24 * 60 * 60 * 1000).toISOString(); + + await request(app).post('/tasks').send({ title: 'Todo overdue', status: 'todo', dueDate: yesterday }); + await request(app).post('/tasks').send({ title: 'In progress', status: 'in_progress' }); + await request(app).post('/tasks').send({ title: 'Done task', status: 'done', dueDate: yesterday }); + + const response = await request(app).get('/tasks/stats'); + + expect(response.status).toBe(200); + expect(response.body.todo).toBe(1); + expect(response.body.in_progress).toBe(1); + expect(response.body.done).toBe(1); + expect(response.body.overdue).toBe(1); + }); + + test('PATCH /tasks/:id/assign assigns a user to task', async () => { + const created = await request(app).post('/tasks').send({ title: 'Assign me' }); + + const response = await request(app) + .patch(`/tasks/${created.body.id}/assign`) + .send({ assignee: 'Alice' }); + + expect(response.status).toBe(200); + expect(response.body.assignee).toBe('Alice'); + }); + + test('PATCH /tasks/:id/assign returns 400 for empty assignee', async () => { + const created = await request(app).post('/tasks').send({ title: 'Assign me' }); + + const response = await request(app) + .patch(`/tasks/${created.body.id}/assign`) + .send({ assignee: ' ' }); + + expect(response.status).toBe(400); + expect(response.body.error).toMatch(/assignee must be a non-empty string/i); + }); + + test('PATCH /tasks/:id/assign returns 404 for missing task', async () => { + const response = await request(app).patch('/tasks/missing-id/assign').send({ assignee: 'Alice' }); + + expect(response.status).toBe(404); + expect(response.body.error).toBe('Task not found'); + }); + + test('PATCH /tasks/:id/assign returns 409 when task is already assigned', async () => { + const created = await request(app).post('/tasks').send({ title: 'Assign me' }); + await request(app).patch(`/tasks/${created.body.id}/assign`).send({ assignee: 'Alice' }); + + const response = await request(app) + .patch(`/tasks/${created.body.id}/assign`) + .send({ assignee: 'Bob' }); + + expect(response.status).toBe(409); + expect(response.body.error).toMatch(/already assigned/i); + }); +});