filter archived tasks by user uuid#3070
Conversation
salihudickson
force-pushed
the
fix/filter-archives
branch
from
8e3c8a9 to
ae2ee34
Compare
|
Reviews (1): Last reviewed commit: "add tests" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return archivedTasks.reduce<ArchivedTaskWithDetails[]>((acc, archived) => { | ||
| const task = taskMap.get(archived.taskId) ?? null; | ||
| if (task?.created_by?.uuid === userId) { | ||
| acc.push({ archived, task }); | ||
| } | ||
| return acc; | ||
| }, []); |
There was a problem hiding this comment.
userId is undefined (the optional parameter is omitted, or the user query hasn't resolved yet), the condition task?.created_by?.uuid === userId evaluates as task?.created_by?.uuid === undefined, which is true for any archived task whose corresponding task is missing from the map (null?.created_by?.uuid is undefined) or has no created_by — returning a wrong, non-empty subset instead of either all items or an empty list. This breaks the pre-existing "no filter" contract for callers that don't pass a userId, and causes the UI to briefly flash incorrect results while the user query is still loading.
| return archivedTasks.reduce<ArchivedTaskWithDetails[]>((acc, archived) => { | |
| const task = taskMap.get(archived.taskId) ?? null; | |
| if (task?.created_by?.uuid === userId) { | |
| acc.push({ archived, task }); | |
| } | |
| return acc; | |
| }, []); | |
| return archivedTasks.reduce<ArchivedTaskWithDetails[]>((acc, archived) => { | |
| const task = taskMap.get(archived.taskId) ?? null; | |
| if (userId === undefined || task?.created_by?.uuid === userId) { | |
| acc.push({ archived, task }); | |
| } | |
| return acc; | |
| }, []); |
| describe("mergeArchivedWithTasks", () => { | ||
| it("returns only archived tasks owned by the requested user id", () => { | ||
| const archived = [ | ||
| makeArchived("a", "2024-01-02T00:00:00.000Z"), | ||
| makeArchived("b", "2024-01-03T00:00:00.000Z"), | ||
| makeArchived("c", "2024-01-04T00:00:00.000Z"), | ||
| ]; | ||
|
|
||
| const tasks = [ | ||
| makeTask("a", { created_by: { uuid: "user-1" } as Task["created_by"] }), | ||
| makeTask("b", { created_by: { uuid: "user-2" } as Task["created_by"] }), | ||
| makeTask("c", { created_by: { uuid: "user-1" } as Task["created_by"] }), | ||
| ]; | ||
|
|
||
| const result = mergeArchivedWithTasks(archived, tasks, "user-1"); | ||
| expect(result.map((item) => item.archived.taskId)).toEqual(["a", "c"]); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
userId = undefined
The team rule prefers parameterised tests. The existing single case tests the happy path, but the userId = undefined scenario (user query still loading, or userId omitted by a non-UI caller) is entirely untested. Adding cases for: undefined userId returning all items, a userId with zero matches, and an archived task whose id is absent from the task map would both document the intended contract and catch the present regression.
Context Used: Do not attempt to comment on incorrect alphabetica... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
Problem
#2745
Changes
How did you test this?
Automatic notifications