From 1c346af2a6e94b7cfcfd677f6e8d4a1d7f3d4416 Mon Sep 17 00:00:00 2001 From: kdahal7 Date: Tue, 14 Apr 2026 17:40:55 +0530 Subject: [PATCH] Add full tests, fix pagination bug, implement task assign endpoint, and add deployment docs --- BUG_REPORT.md | 25 ++++ DEPLOY.md | 37 ++++++ SUBMISSION_NOTES.md | 54 +++++++++ railway.json | 12 ++ render.yaml | 10 ++ task-api/src/routes/tasks.js | 21 +++- task-api/src/services/taskService.js | 21 +++- task-api/src/utils/validators.js | 10 +- task-api/tests/taskService.test.js | 133 +++++++++++++++++++++ task-api/tests/tasks.routes.test.js | 169 +++++++++++++++++++++++++++ 10 files changed, 489 insertions(+), 3 deletions(-) create mode 100644 BUG_REPORT.md create mode 100644 DEPLOY.md create mode 100644 SUBMISSION_NOTES.md create mode 100644 railway.json create mode 100644 render.yaml create mode 100644 task-api/tests/taskService.test.js create mode 100644 task-api/tests/tasks.routes.test.js diff --git a/BUG_REPORT.md b/BUG_REPORT.md new file mode 100644 index 00000000..e1270cf2 --- /dev/null +++ b/BUG_REPORT.md @@ -0,0 +1,25 @@ +# Bug Report + +## Bug 1: Pagination starts from the wrong offset (Fixed) + +- Expected behavior: `GET /tasks?page=1&limit=2` should return the first two tasks. +- Actual behavior: Page 1 skipped the first two tasks because offset used `page * limit`. +- Discovery method: Unit test on pagination behavior and route integration test for `GET /tasks?page=1&limit=2`. +- Root cause: Offset calculation in `getPaginated` used `page * limit` instead of `(page - 1) * limit`. +- Fix implemented: Updated offset formula to `(page - 1) * limit` in `src/services/taskService.js`. + +## Bug 2: Status filtering is too permissive (Not fixed) + +- Expected behavior: `GET /tasks?status=done` should match only tasks with status exactly `done`. +- Actual behavior: Filtering uses substring matching (`includes`), so partial values can produce unexpected matches. +- Discovery method: Code review while writing service tests around status filtering. +- Root cause: `getByStatus` uses `t.status.includes(status)` instead of strict equality. +- Suggested fix: Replace with `t.status === status` and keep validator-driven status constraints. + +## Bug 3: Completing a task overrides priority (Not fixed) + +- Expected behavior: Marking a task complete should change completion state only, not silently rewrite priority. +- Actual behavior: `PATCH /tasks/:id/complete` sets `priority` to `medium` for every task. +- Discovery method: Code review and route behavior verification. +- Root cause: `completeTask` reconstructs task object with `priority: 'medium'`. +- Suggested fix: Preserve existing priority in `completeTask`. diff --git a/DEPLOY.md b/DEPLOY.md new file mode 100644 index 00000000..4bfb226e --- /dev/null +++ b/DEPLOY.md @@ -0,0 +1,37 @@ +# Quick Deploy Guide + +## Option A: Render (recommended) + +1. Push this repository to your own GitHub fork. +2. In Render, choose **New +** -> **Blueprint**. +3. Select your forked repository. +4. Render will detect `render.yaml` and create the service automatically. +5. Wait for deploy to finish and copy the generated live URL. + +Health check endpoint: +- `/tasks` + +## Option B: Railway + +1. Push this repository to your own GitHub fork. +2. In Railway, click **New Project** -> **Deploy from GitHub Repo**. +3. Select your forked repository. +4. Railway will use `railway.json` for build/start commands. +5. After deploy, open the generated public domain and verify `/tasks`. + +## Verify deployment + +Run these checks on your live URL: + +1. `GET /tasks` returns 200 +2. `GET /tasks/stats` returns 200 +3. `POST /tasks` creates a task +4. `PATCH /tasks/:id/assign` works for valid payload + +## Suggested live-link smoke test commands + +Replace `` with your deployed URL. + +- `curl /tasks` +- `curl /tasks/stats` +- `curl -X POST /tasks -H "Content-Type: application/json" -d "{\"title\":\"Live test\"}"` diff --git a/SUBMISSION_NOTES.md b/SUBMISSION_NOTES.md new file mode 100644 index 00000000..0d54bcbe --- /dev/null +++ b/SUBMISSION_NOTES.md @@ -0,0 +1,54 @@ +# Submission Notes + +## What I implemented + +1. Added comprehensive tests: +- Unit tests for task service logic (`task-api/tests/taskService.test.js`) +- Integration tests for all task routes using Supertest (`task-api/tests/tasks.routes.test.js`) + +2. Fixed one production bug: +- Corrected pagination offset logic so page 1 returns the first set of records. + +3. Added new feature endpoint: +- `PATCH /tasks/:id/assign` +- Request body: `{ "assignee": "string" }` +- Returns: + - `200` with updated task on success + - `400` for invalid/empty assignee + - `404` when task does not exist + - `409` when task is already assigned + +## Design decisions for assignment feature + +- Validation rule: `assignee` must be a non-empty string after trimming whitespace. +- Re-assignment policy: second assignment attempt returns `409 Conflict`. +- Data model change: new tasks now include `assignee: null` by default for consistent shape. + +## Coverage summary + +Coverage run (`npm run coverage`) reported: + +- Statements: 92.4% +- Branches: 82.22% +- Functions: 93.33% +- Lines: 91.66% + +This meets the 80%+ target. + +## What I would test next with more time + +1. Add explicit tests for malformed query params (for example negative/zero page and limit). +2. Add tests for unsupported status filter values at route level. +3. Add tests for race-like behavior in assignment/update flows if persistence or concurrent writes are introduced. + +## What surprised me + +1. The service logic had subtle behavior bugs (pagination and completion side effects) that are easy to miss without tests. +2. Query and transition behaviors were not uniformly validated, which can lead to inconsistent API contracts. + +## Questions I would ask before production + +1. Should assignment support re-assignment, and if yes, should audit/history be tracked? +2. Should completing a task be idempotent and preserve all existing fields except status/completedAt? +3. What is the expected behavior for invalid query params (strict validation vs. default fallback)? +4. Are status transitions restricted (for example `done` back to `todo`)? diff --git a/railway.json b/railway.json new file mode 100644 index 00000000..e34566f9 --- /dev/null +++ b/railway.json @@ -0,0 +1,12 @@ +{ + "$schema": "https://railway.app/railway.schema.json", + "build": { + "builder": "NIXPACKS", + "buildCommand": "cd task-api && npm ci" + }, + "deploy": { + "startCommand": "cd task-api && npm start", + "healthcheckPath": "/tasks", + "healthcheckTimeout": 120 + } +} diff --git a/render.yaml b/render.yaml new file mode 100644 index 00000000..ba3db2f1 --- /dev/null +++ b/render.yaml @@ -0,0 +1,10 @@ +services: + - type: web + name: task-api + runtime: node + rootDir: task-api + plan: free + buildCommand: npm ci + startCommand: npm start + healthCheckPath: /tasks + autoDeploy: true diff --git a/task-api/src/routes/tasks.js b/task-api/src/routes/tasks.js index e8c370fe..d1813180 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,23 @@ 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' }); + } + + if (task === 'already_assigned') { + return res.status(409).json({ error: 'Task is already assigned' }); + } + + res.json(task); +}); + module.exports = router; diff --git a/task-api/src/services/taskService.js b/task-api/src/services/taskService.js index f8e89189..b1dead65 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,23 @@ const completeTask = (id) => { return updated; }; +const assignTask = (id, assignee) => { + const index = tasks.findIndex((t) => t.id === id); + if (index === -1) return null; + + if (tasks[index].assignee) { + return 'already_assigned'; + } + + const updated = { + ...tasks[index], + assignee: assignee.trim(), + }; + + tasks[index] = updated; + return updated; +}; + const _reset = () => { tasks = []; }; @@ -90,5 +108,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..6e20f46e 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 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..df6e0fd2 --- /dev/null +++ b/task-api/tests/taskService.test.js @@ -0,0 +1,133 @@ +const taskService = require('../src/services/taskService'); + +describe('taskService', () => { + beforeEach(() => { + taskService._reset(); + }); + + test('create should build a task with defaults', () => { + const task = taskService.create({ title: 'Write tests' }); + + expect(task).toMatchObject({ + title: 'Write tests', + description: '', + status: 'todo', + priority: 'medium', + dueDate: null, + assignee: null, + completedAt: null, + }); + expect(typeof task.id).toBe('string'); + expect(Number.isNaN(Date.parse(task.createdAt))).toBe(false); + }); + + test('getAll should return a copy of in-memory tasks', () => { + taskService.create({ title: 'A' }); + + const tasks = taskService.getAll(); + tasks.push({ id: 'fake' }); + + expect(taskService.getAll()).toHaveLength(1); + }); + + test('findById should return the matching task', () => { + const created = taskService.create({ title: 'Find me' }); + + const found = taskService.findById(created.id); + + expect(found).toEqual(created); + }); + + test('getByStatus should return tasks matching status', () => { + taskService.create({ title: 'Todo one', status: 'todo' }); + taskService.create({ title: 'Done one', status: 'done' }); + + const doneTasks = taskService.getByStatus('done'); + + expect(doneTasks).toHaveLength(1); + expect(doneTasks[0].title).toBe('Done one'); + }); + + test('getPaginated should return tasks page-wise', () => { + taskService.create({ title: 'Task 1' }); + taskService.create({ title: 'Task 2' }); + taskService.create({ title: 'Task 3' }); + + const firstPage = taskService.getPaginated(1, 2); + const secondPage = taskService.getPaginated(2, 2); + + expect(firstPage.map((t) => t.title)).toEqual(['Task 1', 'Task 2']); + expect(secondPage.map((t) => t.title)).toEqual(['Task 3']); + }); + + test('update should update existing task and return null for missing id', () => { + const created = taskService.create({ title: 'Old title' }); + + const updated = taskService.update(created.id, { title: 'New title' }); + const missing = taskService.update('missing-id', { title: 'Nope' }); + + expect(updated.title).toBe('New title'); + expect(missing).toBeNull(); + }); + + test('remove should delete existing task and return false for missing id', () => { + const created = taskService.create({ title: 'Delete me' }); + + const removed = taskService.remove(created.id); + const removedAgain = taskService.remove(created.id); + + expect(removed).toBe(true); + expect(removedAgain).toBe(false); + }); + + test('completeTask should mark task as done and set completedAt', () => { + const created = taskService.create({ title: 'Complete me', status: 'todo' }); + + const completed = taskService.completeTask(created.id); + + expect(completed.status).toBe('done'); + expect(Number.isNaN(Date.parse(completed.completedAt))).toBe(false); + }); + + test('getStats should include status counts and overdue tasks', () => { + const pastDate = new Date(Date.now() - 86400000).toISOString(); + const futureDate = new Date(Date.now() + 86400000).toISOString(); + + taskService.create({ title: 'Overdue todo', status: 'todo', dueDate: pastDate }); + taskService.create({ title: 'Future in progress', status: 'in_progress', dueDate: futureDate }); + taskService.create({ title: 'Done in past', status: 'done', dueDate: pastDate }); + + const stats = taskService.getStats(); + + expect(stats).toEqual({ + todo: 1, + in_progress: 1, + done: 1, + overdue: 1, + }); + }); + + test('assignTask should assign and trim assignee name', () => { + const created = taskService.create({ title: 'Assign me' }); + + const updated = taskService.assignTask(created.id, ' Rohit '); + + expect(updated.assignee).toBe('Rohit'); + }); + + test('assignTask should return null when task does not exist', () => { + const result = taskService.assignTask('missing-id', 'Rohit'); + + expect(result).toBeNull(); + }); + + test('assignTask should block assignment when already assigned', () => { + const created = taskService.create({ title: 'Assigned already' }); + taskService.assignTask(created.id, 'Alice'); + + const secondAttempt = taskService.assignTask(created.id, 'Bob'); + + expect(secondAttempt).toBe('already_assigned'); + expect(taskService.findById(created.id).assignee).toBe('Alice'); + }); +}); diff --git a/task-api/tests/tasks.routes.test.js b/task-api/tests/tasks.routes.test.js new file mode 100644 index 00000000..2082af82 --- /dev/null +++ b/task-api/tests/tasks.routes.test.js @@ -0,0 +1,169 @@ +const request = require('supertest'); +const app = require('../src/app'); +const taskService = require('../src/services/taskService'); + +describe('Task API routes', () => { + beforeEach(() => { + taskService._reset(); + }); + + test('GET /tasks should return all tasks', async () => { + taskService.create({ title: 'Task A' }); + taskService.create({ title: 'Task B' }); + + const response = await request(app).get('/tasks'); + + expect(response.statusCode).toBe(200); + expect(response.body).toHaveLength(2); + }); + + test('GET /tasks?status=todo should filter by status', async () => { + taskService.create({ title: 'Todo one', status: 'todo' }); + taskService.create({ title: 'Done one', status: 'done' }); + + const response = await request(app).get('/tasks?status=todo'); + + expect(response.statusCode).toBe(200); + expect(response.body).toHaveLength(1); + expect(response.body[0].status).toBe('todo'); + }); + + test('GET /tasks with pagination should return expected page', async () => { + taskService.create({ title: 'Task 1' }); + taskService.create({ title: 'Task 2' }); + taskService.create({ title: 'Task 3' }); + + const response = await request(app).get('/tasks?page=1&limit=2'); + + expect(response.statusCode).toBe(200); + expect(response.body.map((t) => t.title)).toEqual(['Task 1', 'Task 2']); + }); + + test('POST /tasks should create a task', async () => { + const payload = { title: 'New task', priority: 'high' }; + + const response = await request(app).post('/tasks').send(payload); + + expect(response.statusCode).toBe(201); + expect(response.body.title).toBe('New task'); + expect(response.body.priority).toBe('high'); + }); + + test('POST /tasks should return 400 for invalid payload', async () => { + const response = await request(app).post('/tasks').send({ title: '' }); + + expect(response.statusCode).toBe(400); + expect(response.body.error).toBe('title is required and must be a non-empty string'); + }); + + test('PUT /tasks/:id should update task', async () => { + const created = taskService.create({ title: 'Before update' }); + + const response = await request(app) + .put(`/tasks/${created.id}`) + .send({ title: 'After update', status: 'in_progress' }); + + expect(response.statusCode).toBe(200); + expect(response.body.title).toBe('After update'); + expect(response.body.status).toBe('in_progress'); + }); + + test('PUT /tasks/:id should return 404 for missing task', async () => { + const response = await request(app).put('/tasks/missing-id').send({ title: 'No task' }); + + expect(response.statusCode).toBe(404); + expect(response.body.error).toBe('Task not found'); + }); + + test('DELETE /tasks/:id should delete existing task', async () => { + const created = taskService.create({ title: 'Delete me' }); + + const response = await request(app).delete(`/tasks/${created.id}`); + + expect(response.statusCode).toBe(204); + }); + + test('DELETE /tasks/:id should return 404 for missing task', async () => { + const response = await request(app).delete('/tasks/missing-id'); + + expect(response.statusCode).toBe(404); + expect(response.body.error).toBe('Task not found'); + }); + + test('PATCH /tasks/:id/complete should mark task done', async () => { + const created = taskService.create({ title: 'Complete me' }); + + const response = await request(app).patch(`/tasks/${created.id}/complete`); + + expect(response.statusCode).toBe(200); + expect(response.body.status).toBe('done'); + expect(response.body.completedAt).toBeTruthy(); + }); + + test('PATCH /tasks/:id/complete should return 404 for missing task', async () => { + const response = await request(app).patch('/tasks/missing-id/complete'); + + expect(response.statusCode).toBe(404); + expect(response.body.error).toBe('Task not found'); + }); + + test('GET /tasks/stats should return counts and overdue values', async () => { + const pastDate = new Date(Date.now() - 86400000).toISOString(); + + taskService.create({ title: 'Overdue', status: 'todo', dueDate: pastDate }); + taskService.create({ title: 'Done', status: 'done', dueDate: pastDate }); + + const response = await request(app).get('/tasks/stats'); + + expect(response.statusCode).toBe(200); + expect(response.body.todo).toBe(1); + expect(response.body.done).toBe(1); + expect(response.body.overdue).toBe(1); + }); + + test('PATCH /tasks/:id/assign should assign task', async () => { + const created = taskService.create({ title: 'Assign me' }); + + const response = await request(app) + .patch(`/tasks/${created.id}/assign`) + .send({ assignee: 'Rohit' }); + + expect(response.statusCode).toBe(200); + expect(response.body.assignee).toBe('Rohit'); + }); + + test('PATCH /tasks/:id/assign should return 400 for invalid assignee', async () => { + const created = taskService.create({ title: 'Assign me' }); + + const response = await request(app) + .patch(`/tasks/${created.id}/assign`) + .send({ assignee: ' ' }); + + expect(response.statusCode).toBe(400); + expect(response.body.error).toBe('assignee is required and must be a non-empty string'); + }); + + test('PATCH /tasks/:id/assign should return 404 for missing task', async () => { + const response = await request(app) + .patch('/tasks/missing-id/assign') + .send({ assignee: 'Rohit' }); + + expect(response.statusCode).toBe(404); + expect(response.body.error).toBe('Task not found'); + }); + + test('PATCH /tasks/:id/assign should return 409 if task is already assigned', async () => { + const created = taskService.create({ title: 'Assigned once' }); + + await request(app) + .patch(`/tasks/${created.id}/assign`) + .send({ assignee: 'Alice' }); + + const response = await request(app) + .patch(`/tasks/${created.id}/assign`) + .send({ assignee: 'Bob' }); + + expect(response.statusCode).toBe(409); + expect(response.body.error).toBe('Task is already assigned'); + }); +});