From d641c7b575ef2c90efab94f05fd542abe23c65a0 Mon Sep 17 00:00:00 2001 From: Rahul Raj Jaiswal Date: Wed, 15 Apr 2026 16:18:27 +0530 Subject: [PATCH] feat: implement task assignment, fix core API bugs, and add tests --- ASSIGNMENT.md | 1 + task-api/BUG_REPORT.md | 27 ++++ task-api/FINAL_NOTES.md | 29 +++++ task-api/README.md | 35 ++++++ task-api/src/routes/tasks.js | 36 +++++- task-api/src/services/taskService.js | 16 ++- task-api/src/utils/validators.js | 15 ++- task-api/tests/integration/tasks.test.js | 154 +++++++++++++++++++++++ task-api/tests/unit/taskService.test.js | 151 ++++++++++++++++++++++ 9 files changed, 456 insertions(+), 8 deletions(-) create mode 100644 task-api/BUG_REPORT.md create mode 100644 task-api/FINAL_NOTES.md create mode 100644 task-api/README.md create mode 100644 task-api/tests/integration/tasks.test.js create mode 100644 task-api/tests/unit/taskService.test.js diff --git a/ASSIGNMENT.md b/ASSIGNMENT.md index f1d0d824..adcfddc2 100644 --- a/ASSIGNMENT.md +++ b/ASSIGNMENT.md @@ -1,3 +1,4 @@ + # Take-Home Assignment: The Untested API **Estimated time:** 2 days diff --git a/task-api/BUG_REPORT.md b/task-api/BUG_REPORT.md new file mode 100644 index 00000000..2eae058e --- /dev/null +++ b/task-api/BUG_REPORT.md @@ -0,0 +1,27 @@ +# Bug Report + +While building out the test suite, I identified several logic errors that would have caused issues in a production environment. Here’s what I found and how I handled it: + +### 1. Pagination Offset Logic +* **Location:** `src/services/taskService.js` -> `getPaginated()` +* **The Issue:** The offset was calculated as `page * limit`. +* **Why it happened:** This missed the fact that array indexing starts at 0. If a user requested Page 1 with a limit of 10, the code set the offset to 10, effectively hiding the first 10 tasks. +* **The Fix:** I updated the math to `(page - 1) * limit`. + +### 2. Loose Status Filtering +* **Location:** `src/services/taskService.js` -> `getByStatus()` +* **The Issue:** The filter used `.includes()`. +* **Why it happened:** This allowed for partial matches. For example, a request for tasks with a status of "do" would return both "todo" and "done" tasks. This makes the filter unreliable for categorical data. +* **The Fix:** I switched this to a strict equality check (`===`). + +### 3. Priority Reset on Completion +* **Location:** `src/services/taskService.js` -> `completeTask()` +* **The Issue:** Completing a task forced its priority to "medium". +* **Why it happened:** The original code had the priority hardcoded in the update object. This meant a "high" priority task would lose its importance level as soon as it was marked finished. +* **The Fix:** I removed the hardcoded field so the task's original priority is preserved. + +### 4. Silent Failures on Bad Query Params +* **Location:** `src/routes/tasks.js` -> `GET /` +* **The Issue:** Invalid query parameters (like a negative page number or an unknown status) were processed without warning. +* **Why it happened:** There was no validation layer for query strings before they hit the service layer. +* **The Fix:** I added a validation check at the route level to return a `400` error if the user provides invalid filter or pagination values. diff --git a/task-api/FINAL_NOTES.md b/task-api/FINAL_NOTES.md new file mode 100644 index 00000000..2b404516 --- /dev/null +++ b/task-api/FINAL_NOTES.md @@ -0,0 +1,29 @@ +# Final Submission Notes + +This document provides additional context regarding design tradeoffs, production readiness, and future testing strategies. + +## Surprises in the Codebase + +- **Initial State:** The core logic was largely functional but lacked automated tests, which allowed critical bugs (like the pagination offset) to persist. +- **Service Layer Design:** The `taskService` was well-structured, which made implementing the `assignTask` feature straightforward without needing to refactor the entire system. + +## Technical Tradeoffs + +- **In-Memory Store:** For a take-home assignment, an in-memory array is sufficient for demonstrating logic. However, it lacks persistence and would be replaced by a database (e.g., MongoDB/PostgreSQL) in a real-world scenario. +- **Synchronous Logic:** Most operations are currently synchronous due to the in-memory store. In production, these would be `async` to handle database I/O without blocking the event loop. +- **Validation Library:** I used custom validation functions to keep dependencies minimal. For a larger project, I would typically use a library like `Joi` or `Zod` for more declarative and complex schemas. + +## Production Considerations + +Before deploying to production, the following questions and improvements should be addressed: + +1. **Concurrency:** How should the system handle simultaneous updates to the same task? (e.g., using optimistic locking or database transactions). +2. **Scalability:** The current in-memory store is not suitable for horizontal scaling. A shared database is required. +3. **Security:** The API lacks authentication. Who is allowed to assign tasks? Who can delete them? +4. **Logging & Monitoring:** Implement structured logging to track errors and performance metrics. + +## Future Testing Strategies + +- **Property-Based Testing:** Use a library like `fast-check` to generate random inputs for the `create` and `update` methods to discover edge cases in validation. +- **Load Testing:** Benchmark the `getPaginated` and `getStats` endpoints with thousands of tasks to ensure performance remains within acceptable limits. +- **E2E Testing:** Implement a small frontend or use a tool like Cypress to test the entire user flow from task creation to completion. diff --git a/task-api/README.md b/task-api/README.md new file mode 100644 index 00000000..d0c15d1c --- /dev/null +++ b/task-api/README.md @@ -0,0 +1,35 @@ +# Task Manager API - Submission + +I’ve updated this API to be more robust and production-ready. My focus was on implementing a solid testing suite, identifying a few critical logic bugs, and adding a new task assignment feature. I also cleaned up the validation logic to ensure the data stays consistent as the project grows. + +## What's been done +* **Comprehensive Testing:** I wrote 32 tests (Unit + Integration) covering every endpoint. I focused on "happy paths" as well as edge cases like missing fields and invalid IDs. +* **Bug Squashing:** I fixed three core logic issues involving pagination, status filtering, and task priority. +* **New Feature:** Implemented `PATCH /tasks/:id/assign` to allow tasks to be assigned to specific users. +* **Validation:** Added strict checks for both request bodies and query parameters to prevent invalid data from entering the system. + +## Getting Started + +1. **Install dependencies:** + ```bash + npm install + ``` +2. **Run the server:** + ```bash + npm start + ``` + The API will be live at `http://localhost:3000`. + +3. **Run tests:** + ```bash + npm test + ``` + All **32 tests** are passing. + +## Feature Spotlight: Task Assignment +I added the `PATCH /tasks/:id/assign` endpoint. I chose `PATCH` because we’re only updating a specific field (the assignee) rather than replacing the whole task resource. +* **Validation:** I ensured the `assignee` must be a non-empty string. If you send an empty string or just whitespace, the API will return a `400 Bad Request`. +* **Error Handling:** If the task ID doesn't exist, you'll get a clear `404 Task not found` response. + +## Key Bug Fix: The "Invisible Page 1" +The most significant bug I found was in the pagination logic. The original code used `page * limit` to calculate the data offset. This meant if you asked for Page 1, it actually skipped the first 10 items and started on Page 2. I corrected this to `(page - 1) * limit` so that the first page of results is actually accessible. diff --git a/task-api/src/routes/tasks.js b/task-api/src/routes/tasks.js index e8c370fe..da1955d0 100644 --- a/task-api/src/routes/tasks.js +++ b/task-api/src/routes/tasks.js @@ -1,7 +1,12 @@ const express = require('express'); const router = express.Router(); const taskService = require('../services/taskService'); -const { validateCreateTask, validateUpdateTask } = require('../utils/validators'); +const { + validateCreateTask, + validateUpdateTask, + validateAssignTask, + VALID_STATUSES +} = require('../utils/validators'); router.get('/stats', (req, res) => { const stats = taskService.getStats(); @@ -12,14 +17,23 @@ router.get('/', (req, res) => { const { status, page, limit } = req.query; if (status) { + if (!VALID_STATUSES.includes(status)) { + return res.status(400).json({ error: `Invalid status. Must be one of: ${VALID_STATUSES.join(', ')}` }); + } const tasks = taskService.getByStatus(status); return res.json(tasks); } if (page !== undefined || limit !== undefined) { - const pageNum = parseInt(page) || 1; - const limitNum = parseInt(limit) || 10; - const tasks = taskService.getPaginated(pageNum, limitNum); + const pageNum = parseInt(page); + const limitNum = parseInt(limit); + + if ((page !== undefined && (isNaN(pageNum) || pageNum <= 0)) || + (limit !== undefined && (isNaN(limitNum) || limitNum <= 0))) { + return res.status(400).json({ error: 'Page and limit must be positive integers' }); + } + + const tasks = taskService.getPaginated(pageNum || 1, limitNum || 10); return res.json(tasks); } @@ -69,4 +83,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..25bc42ac 100644 --- a/task-api/src/services/taskService.js +++ b/task-api/src/services/taskService.js @@ -6,10 +6,10 @@ 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; + const offset = (page - 1) * limit; return tasks.slice(offset, offset + limit); }; @@ -66,7 +66,6 @@ const completeTask = (id) => { const updated = { ...task, - priority: 'medium', status: 'done', completedAt: new Date().toISOString(), }; @@ -76,6 +75,16 @@ 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 +99,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..3f784e47 100644 --- a/task-api/src/utils/validators.js +++ b/task-api/src/utils/validators.js @@ -33,4 +33,17 @@ 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, + VALID_STATUSES, + VALID_PRIORITIES +}; diff --git a/task-api/tests/integration/tasks.test.js b/task-api/tests/integration/tasks.test.js new file mode 100644 index 00000000..98477478 --- /dev/null +++ b/task-api/tests/integration/tasks.test.js @@ -0,0 +1,154 @@ +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 task', async () => { + const res = await request(app) + .post('/tasks') + .send({ title: 'Integration Task' }); + + expect(res.status).toBe(201); + expect(res.body.title).toBe('Integration Task'); + }); + + it('should return 400 for missing title', async () => { + const res = await request(app) + .post('/tasks') + .send({}); + + expect(res.status).toBe(400); + expect(res.body.error).toContain('title is required'); + }); + + it('should return 400 for invalid status', async () => { + const res = await request(app) + .post('/tasks') + .send({ title: 'Task', status: 'invalid' }); + + expect(res.status).toBe(400); + }); + }); + + describe('GET /tasks', () => { + it('should get all tasks', async () => { + taskService.create({ title: 'Task 1' }); + taskService.create({ title: 'Task 2' }); + + const res = await request(app).get('/tasks'); + expect(res.status).toBe(200); + expect(res.body.length).toBe(2); + }); + + it('should filter by status', async () => { + taskService.create({ title: 'T1', status: 'todo' }); + taskService.create({ title: 'T2', status: 'done' }); + + const res = await request(app).get('/tasks?status=done'); + expect(res.status).toBe(200); + expect(res.body.length).toBe(1); + expect(res.body[0].status).toBe('done'); + }); + }); + + describe('PUT /tasks/:id', () => { + it('should update a task', async () => { + const task = taskService.create({ title: 'Old' }); + const res = await request(app) + .put(`/tasks/${task.id}`) + .send({ title: 'New' }); + + expect(res.status).toBe(200); + expect(res.body.title).toBe('New'); + }); + + it('should return 404 for unknown id', async () => { + const res = await request(app) + .put('/tasks/unknown') + .send({ title: 'New' }); + + expect(res.status).toBe(404); + }); + }); + + describe('DELETE /tasks/:id', () => { + it('should delete a task', async () => { + const task = taskService.create({ title: 'To Delete' }); + const res = await request(app).delete(`/tasks/${task.id}`); + expect(res.status).toBe(204); + expect(taskService.getAll().length).toBe(0); + }); + }); + + describe('PATCH /tasks/:id/complete', () => { + it('should mark task as complete', async () => { + const task = taskService.create({ title: 'Incomplete' }); + const res = await request(app).patch(`/tasks/${task.id}/complete`); + expect(res.status).toBe(200); + expect(res.body.status).toBe('done'); + }); + }); + + describe('PATCH /tasks/:id/assign', () => { + it('should assign a user to a task', async () => { + const task = taskService.create({ title: 'Unassigned' }); + const res = await request(app) + .patch(`/tasks/${task.id}/assign`) + .send({ assignee: 'Jane Doe' }); + + expect(res.status).toBe(200); + expect(res.body.assignee).toBe('Jane Doe'); + }); + + it('should return 400 for empty assignee', async () => { + const task = taskService.create({ title: 'Task' }); + const res = await request(app) + .patch(`/tasks/${task.id}/assign`) + .send({ assignee: '' }); + + expect(res.status).toBe(400); + }); + + it('should return 404 for unknown task', async () => { + const res = await request(app) + .patch('/tasks/unknown/assign') + .send({ assignee: 'Jane Doe' }); + + expect(res.status).toBe(404); + }); + }); + + describe('GET /tasks (with query parameters)', () => { + it('should return 400 for invalid status', async () => { + const res = await request(app).get('/tasks?status=invalid'); + expect(res.status).toBe(400); + expect(res.body.error).toContain('Invalid status'); + }); + + it('should return 400 for negative page', async () => { + const res = await request(app).get('/tasks?page=-1'); + expect(res.status).toBe(400); + expect(res.body.error).toBe('Page and limit must be positive integers'); + }); + + it('should return 400 for non-numeric limit', async () => { + const res = await request(app).get('/tasks?limit=abc'); + expect(res.status).toBe(400); + }); + }); + + describe('GET /tasks/stats', () => { + it('should return stats', async () => { + taskService.create({ title: 'T1', status: 'todo' }); + const res = await request(app).get('/tasks/stats'); + expect(res.status).toBe(200); + expect(res.body).toHaveProperty('todo'); + expect(res.body).toHaveProperty('overdue'); + }); + }); +}); diff --git a/task-api/tests/unit/taskService.test.js b/task-api/tests/unit/taskService.test.js new file mode 100644 index 00000000..9f707bcd --- /dev/null +++ b/task-api/tests/unit/taskService.test.js @@ -0,0 +1,151 @@ +const taskService = require('../../src/services/taskService'); + +describe('taskService', () => { + 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.status).toBe('todo'); + expect(task.priority).toBe('medium'); + expect(task.description).toBe(''); + expect(task.dueDate).toBeNull(); + expect(task.createdAt).toBeDefined(); + }); + + it('should create a new task with provided values', () => { + const data = { + title: 'Task 2', + description: 'Details', + status: 'in_progress', + priority: 'high', + dueDate: '2026-12-31T00:00:00.000Z', + }; + const task = taskService.create(data); + expect(task.title).toBe(data.title); + expect(task.description).toBe(data.description); + expect(task.status).toBe(data.status); + expect(task.priority).toBe(data.priority); + expect(task.dueDate).toBe(data.dueDate); + }); + }); + + describe('getAll', () => { + it('should return an empty array initially', () => { + expect(taskService.getAll()).toEqual([]); + }); + + it('should return all created tasks', () => { + taskService.create({ title: 'Task 1' }); + taskService.create({ title: 'Task 2' }); + expect(taskService.getAll().length).toBe(2); + }); + }); + + describe('findById', () => { + it('should find a task by id', () => { + const task = taskService.create({ title: 'Task' }); + const found = taskService.findById(task.id); + expect(found).toEqual(task); + }); + + it('should return undefined if task not found', () => { + expect(taskService.findById('non-existent')).toBeUndefined(); + }); + }); + + describe('getByStatus', () => { + it('should filter tasks by exact status', () => { + taskService.create({ title: 'T1', status: 'todo' }); + taskService.create({ title: 'T2', status: 'done' }); + + // This is expected to FAIL because the current implementation uses .includes() + const results = taskService.getByStatus('do'); + expect(results.length).toBe(0); + }); + }); + + describe('getPaginated', () => { + it('should return the first page of results (page 1)', () => { + for (let i = 0; i < 10; i++) { + taskService.create({ title: `Task ${i}` }); + } + + // This is expected to FAIL because current offset is page * limit (1 * 5 = 5) + const page1 = taskService.getPaginated(1, 5); + expect(page1.length).toBe(5); + expect(page1[0].title).toBe('Task 0'); + }); + }); + + describe('update', () => { + it('should update a task', () => { + const task = taskService.create({ title: 'Old Title' }); + const updated = taskService.update(task.id, { title: 'New Title' }); + expect(updated.title).toBe('New Title'); + expect(taskService.findById(task.id).title).toBe('New Title'); + }); + + it('should return null if task to update not found', () => { + expect(taskService.update('none', { title: 'X' })).toBeNull(); + }); + }); + + describe('remove', () => { + it('should remove a task', () => { + const task = taskService.create({ title: 'Task' }); + const result = taskService.remove(task.id); + expect(result).toBe(true); + expect(taskService.getAll().length).toBe(0); + }); + + it('should return false if task to remove not found', () => { + expect(taskService.remove('none')).toBe(false); + }); + }); + + describe('completeTask', () => { + it('should mark a task as done and maintain original priority', () => { + const task = taskService.create({ title: 'High Task', priority: 'high' }); + + // This is expected to FAIL because current implementation resets priority to medium + const completed = taskService.completeTask(task.id); + expect(completed.status).toBe('done'); + expect(completed.priority).toBe('high'); + expect(completed.completedAt).toBeDefined(); + }); + }); + + describe('assignTask', () => { + it('should assign a task to a user', () => { + const task = taskService.create({ title: 'Task' }); + const assigned = taskService.assignTask(task.id, 'John Doe'); + expect(assigned.assignee).toBe('John Doe'); + expect(taskService.findById(task.id).assignee).toBe('John Doe'); + }); + + it('should return null if task to assign not found', () => { + expect(taskService.assignTask('none', 'John Doe')).toBeNull(); + }); + }); + + describe('getStats', () => { + it('should return correct counts and overdue stats', () => { + const pastDate = new Date(); + pastDate.setDate(pastDate.getDate() - 1); + + taskService.create({ title: 'Done', status: 'done' }); + taskService.create({ title: 'Todo', status: 'todo' }); + taskService.create({ title: 'Overdue', status: 'todo', dueDate: pastDate.toISOString() }); + + const stats = taskService.getStats(); + expect(stats.todo).toBe(2); + expect(stats.done).toBe(1); + expect(stats.overdue).toBe(1); + }); + }); +});