-
Notifications
You must be signed in to change notification settings - Fork 118
changes #31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
changes #31
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| { | ||
| "permissions": { | ||
| "allow": [ | ||
| "Bash(npm run:*)" | ||
| ] | ||
| } | ||
| } |
| 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. |
| 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 | ||
| 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. | ||
| 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(); | ||||||||||||||||||||||||||||||||||||
|
|
@@ -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
|
||||||||||||||||||||||||||||||||||||
| 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; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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
|
||
| }; | ||
|
|
||
| module.exports = { validateCreateTask, validateUpdateTask, validateAssignTask }; | ||
There was a problem hiding this comment.
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(andtaskService.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-runnpm run coverageand update this snippet so the numbers match the actual output.