Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .claude/settings.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"permissions": {
"allow": [
"Bash(npm run:*)"
]
}
}
70 changes: 70 additions & 0 deletions BUG_REPORT.md
Original file line number Diff line number Diff line change
@@ -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.
73 changes: 73 additions & 0 deletions SUBMISSION.md
Original file line number Diff line number Diff line change
@@ -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
Comment on lines +15 to +17
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pasted coverage table appears internally inconsistent: it reports tasks.js (and taskService.js) at 100% lines/statements while also listing uncovered line numbers (e.g. tasks.js | ... | 21-23). If these line numbers are truly uncovered, the percentages should be <100; otherwise the uncovered-line column should be empty. Please re-run npm run coverage and update this snippet so the numbers match the actual output.

Suggested change
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
tasks.js | 100.00 | 91.66 | 100.00 | 100.00 |
src/services | 100.00 | 94.73 | 100.00 | 100.00 |
taskService.js | 100.00 | 94.73 | 100.00 | 100.00 |

Copilot uses AI. Check for mistakes.
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.
22 changes: 19 additions & 3 deletions task-api/src/routes/tasks.js
Original file line number Diff line number Diff line change
@@ -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();
Expand All @@ -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;
Comment on lines +20 to +23
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pagination defaults look inconsistent with the (now-supported) page=0 / zero-based paging model: when limit is provided without page (e.g. /tasks?limit=2), parsedPage becomes NaN and pageNum defaults to 1, which skips the first page (offset = page * limit). Consider defaulting pageNum to 0 when page is missing, or (if the API is intended to be 1-based) adjusting the service call to use (pageNum - 1) when computing the offset. Also consider rejecting negative page/limit values to avoid Array.slice's surprising behavior with negatives.

Suggested change
const parsedPage = parseInt(page);
const pageNum = isNaN(parsedPage) ? 1 : parsedPage;
const parsedLimit = parseInt(limit);
const limitNum = isNaN(parsedLimit) ? 10 : parsedLimit;
const parsedPage = page === undefined ? 0 : parseInt(page, 10);
const parsedLimit = limit === undefined ? 10 : parseInt(limit, 10);
if (Number.isNaN(parsedPage) || parsedPage < 0) {
return res.status(400).json({ error: 'page must be a non-negative integer' });
}
if (Number.isNaN(parsedLimit) || parsedLimit < 0) {
return res.status(400).json({ error: 'limit must be a non-negative integer' });
}
const pageNum = parsedPage;
const limitNum = parsedLimit;

Copilot uses AI. Check for mistakes.
const tasks = taskService.getPaginated(pageNum, limitNum);
return res.json(tasks);
}
Expand Down Expand Up @@ -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;
18 changes: 16 additions & 2 deletions task-api/src/services/taskService.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -66,7 +66,6 @@ const completeTask = (id) => {

const updated = {
...task,
priority: 'medium',
status: 'done',
completedAt: new Date().toISOString(),
};
Expand All @@ -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 = [];
};
Expand All @@ -90,5 +103,6 @@ module.exports = {
update,
remove,
completeTask,
assignTask,
_reset,
};
9 changes: 8 additions & 1 deletion task-api/src/utils/validators.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Comment on lines +36 to +40
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

validateAssignTask assumes body is a non-null object and will throw if it’s called with undefined/null (e.g., when the request has no JSON body or a non-JSON Content-Type, req.body can be undefined). Consider defensively handling a falsy body (treat as {}) and returning a 400 validation message instead of letting the request fall into the 500 error handler.

Copilot uses AI. Check for mistakes.
};

module.exports = { validateCreateTask, validateUpdateTask, validateAssignTask };
Loading
Loading