From 075e83c8e3231a916f538118916b5eccf1956af4 Mon Sep 17 00:00:00 2001 From: Gloria <55953099+gloriafolaron@users.noreply.github.com> Date: Tue, 14 Jul 2026 11:02:35 -0400 Subject: [PATCH 1/5] feat(tickets): getMyClosedTicketsForRange for period close-lists MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mobile Progress needs "Closed this week / this month" — completed arcs keyed on close-date within a period, independent of how recently the project was otherwise touched, so finished work never disappears from the reflection. The data was already one layer down: getMyClosedTicketsForDate builds on getStatusChangeEvents(ids, from, to), which already takes a date range — the service just pinned it to a single day (from == to). - Add getMyClosedTicketsForRange(userId, from, to) (@api, VIEW): same "closed" definition (currently DONE and its status changed to that DONE status within [from, to]; one row per ticket keyed to its latest completion), just over a range. Tolerates a reversed range; defaults either bound to today. - getMyClosedTicketsForDate now delegates to it with from == to — behaviour is byte-identical (getStatusChangeEvents already spans date 00:00:00..23:59:59), just DRY. One query per call, no N+1 — the mobile alternative was 7–30 per-day calls. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01NtfhjPwUEEHJb4Jf7714eR --- app/Domain/Tickets/Services/Tickets.php | 37 +++++++++++++++++++++++-- 1 file changed, 34 insertions(+), 3 deletions(-) diff --git a/app/Domain/Tickets/Services/Tickets.php b/app/Domain/Tickets/Services/Tickets.php index b8a7362620..228869fd7a 100644 --- a/app/Domain/Tickets/Services/Tickets.php +++ b/app/Domain/Tickets/Services/Tickets.php @@ -2418,12 +2418,43 @@ public function getAllDoneUserTickets(?int $userId = null, ?int $project = null) * then re-completed the same day is included once. Built on the general * getStatusChangeEvents primitive, so throughput/burndown and strategy * reporting can reuse the same query. + * + * Thin wrapper over {@see getMyClosedTicketsForRange} with from == to. */ #[RequiresPermission(TicketsPermissions::VIEW)] public function getMyClosedTicketsForDate(?int $userId = null, ?string $date = null): array { $date = $date ?: date('Y-m-d'); + return $this->getMyClosedTicketsForRange($userId, $date, $date); + } + + /** + * @api + * + * Range form of {@see getMyClosedTicketsForDate}: the user's tasks marked + * DONE anywhere within [$from, $to] (inclusive, dates 'Y-m-d'), each + * annotated with `dateClosed` (the completion timestamp). Powers the mobile + * Progress "Closed this week / this month" sections — completed arcs are + * keyed on close-date within the period, independent of how recently the + * project was otherwise touched, so finished work never disappears. + * + * Same "closed" definition as the single-date form: the ticket is currently + * DONE and its status was changed to that DONE status within the range. A + * ticket completed more than once in the range is included once, keyed to + * its latest completion (events are newest-first). Defaults to today when + * either bound is omitted. + */ + #[RequiresPermission(TicketsPermissions::VIEW)] + public function getMyClosedTicketsForRange(?int $userId = null, ?string $from = null, ?string $to = null): array + { + $from = $from ?: date('Y-m-d'); + $to = $to ?: date('Y-m-d'); + // Tolerate a reversed range rather than returning nothing. + if ($from > $to) { + [$from, $to] = [$to, $from]; + } + // Candidates: the user's currently-DONE tickets (statusType resolved). $doneTickets = $this->getAllDoneUserTickets($userId); if (empty($doneTickets)) { @@ -2435,7 +2466,7 @@ public function getMyClosedTicketsForDate(?int $userId = null, ?string $date = n $byId[$ticket['id']] = $ticket; } - $events = $this->ticketRepository->getStatusChangeEvents(array_keys($byId), $date, $date); + $events = $this->ticketRepository->getStatusChangeEvents(array_keys($byId), $from, $to); $closed = []; foreach ($events as $event) { @@ -2446,8 +2477,8 @@ public function getMyClosedTicketsForDate(?int $userId = null, ?string $date = n } // Only count changes INTO the ticket's current (DONE) status — it - // was actually marked done that day, not merely touched. Events are - // newest-first, so the first match keeps the latest completion time. + // was actually marked done in the range, not merely touched. Events + // are newest-first, so the first match keeps the latest completion. if ((string) $event['changeValue'] === (string) $ticket['status']) { $ticket['dateClosed'] = $event['dateModified']; $closed[$ticketId] = $ticket; From c480b860c7a2045f6d6d6174e78493247671b909 Mon Sep 17 00:00:00 2001 From: Gloria <55953099+gloriafolaron@users.noreply.github.com> Date: Tue, 14 Jul 2026 16:15:32 -0400 Subject: [PATCH 2/5] docs(tickets): clarify getMyClosedTicketsForRange bound normalization Copilot review: the 'defaults to today' wording didn't reflect that an omitted bound combined with a reversed range gets swapped. Document the actual behavior. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01NtfhjPwUEEHJb4Jf7714eR --- app/Domain/Tickets/Services/Tickets.php | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/app/Domain/Tickets/Services/Tickets.php b/app/Domain/Tickets/Services/Tickets.php index 228869fd7a..cdf5b9003f 100644 --- a/app/Domain/Tickets/Services/Tickets.php +++ b/app/Domain/Tickets/Services/Tickets.php @@ -2442,8 +2442,12 @@ public function getMyClosedTicketsForDate(?int $userId = null, ?string $date = n * Same "closed" definition as the single-date form: the ticket is currently * DONE and its status was changed to that DONE status within the range. A * ticket completed more than once in the range is included once, keyed to - * its latest completion (events are newest-first). Defaults to today when - * either bound is omitted. + * its latest completion (events are newest-first). + * + * Bounds: an omitted $from or $to defaults to today; the range is then + * normalized so the earlier date is the lower bound (a reversed range is + * swapped rather than returning nothing). So passing only one bound yields + * the span between that date and today, in date order. */ #[RequiresPermission(TicketsPermissions::VIEW)] public function getMyClosedTicketsForRange(?int $userId = null, ?string $from = null, ?string $to = null): array From 48b68d7ac73ea2f4263a060a907a1739cc12ea6c Mon Sep 17 00:00:00 2001 From: Gloria <55953099+gloriafolaron@users.noreply.github.com> Date: Tue, 14 Jul 2026 16:26:23 -0400 Subject: [PATCH 3/5] test(tickets): cover getMyClosedTicketsForRange range normalization + dedup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review (#3647): add unit coverage — reversed range is normalized earliest-first, only changes into the current DONE status count, and a multiply-completed ticket keeps its latest completion. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01NtfhjPwUEEHJb4Jf7714eR --- .../Tickets/Services/TicketsServiceTest.php | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/tests/Unit/app/Domain/Tickets/Services/TicketsServiceTest.php b/tests/Unit/app/Domain/Tickets/Services/TicketsServiceTest.php index 53b5e73628..1679d67275 100644 --- a/tests/Unit/app/Domain/Tickets/Services/TicketsServiceTest.php +++ b/tests/Unit/app/Domain/Tickets/Services/TicketsServiceTest.php @@ -544,4 +544,51 @@ public function test_get_all_milestones_unscoped_returns_empty_and_skips_reposit $this->assertSame([], $result); $this->assertFalse($called, 'repository should not be queried when criteria are not project-scoped'); } + + /** + * getMyClosedTicketsForRange: a reversed range is normalized (earlier date + * first), only status changes INTO the ticket's current DONE status count, + * and a ticket completed more than once keeps its latest completion. + */ + public function test_closed_tickets_range_normalizes_swapped_range_and_keeps_latest_completion(): void + { + session(['userdata' => ['id' => 1]]); + $capturedFrom = null; + $capturedTo = null; + + $ticketRepository = $this->make(TicketRepository::class, [ + 'simpleTicketQuery' => fn () => [ + ['id' => 10, 'type' => 'task', 'projectId' => 5, 'status' => 0], + ['id' => 20, 'type' => 'task', 'projectId' => 5, 'status' => 0], + ], + 'getStateLabels' => fn () => [ + 0 => ['statusType' => 'DONE', 'name' => 'Done', 'class' => ''], + 3 => ['statusType' => 'INPROGRESS', 'name' => 'In Progress', 'class' => ''], + ], + 'getStatusChangeEvents' => function ($ids, $from, $to) use (&$capturedFrom, &$capturedTo) { + $capturedFrom = $from; + $capturedTo = $to; + + return [ + ['ticketId' => 10, 'changeValue' => 0, 'dateModified' => '2026-07-10 10:00:00'], + ['ticketId' => 10, 'changeValue' => 0, 'dateModified' => '2026-07-09 09:00:00'], + ['ticketId' => 20, 'changeValue' => 3, 'dateModified' => '2026-07-10 10:00:00'], + ]; + }, + ]); + + $service = $this->buildServiceWithTicketRepository($ticketRepository); + + // Reversed range on purpose. + $result = $service->getMyClosedTicketsForRange(1, '2026-07-12', '2026-07-05'); + + $this->assertEquals('2026-07-05', $capturedFrom, 'range should be normalized earliest-first'); + $this->assertEquals('2026-07-12', $capturedTo); + + // 20's only event was a change to a non-DONE status → excluded. 10 kept + // to its latest completion (newest event wins). + $this->assertCount(1, $result); + $this->assertEquals(10, $result[0]['id']); + $this->assertEquals('2026-07-10 10:00:00', $result[0]['dateClosed']); + } } From 364068a924cea5b558e468f20a82d0406de55e98 Mon Sep 17 00:00:00 2001 From: Gloria <55953099+gloriafolaron@users.noreply.github.com> Date: Thu, 16 Jul 2026 09:00:02 -0400 Subject: [PATCH 4/5] fix(tickets): IDOR guard + single today() for closed-range (review) Copilot re-review (#3647): - IDOR: force a non-admin's userId to the session user (only admins may read another user's closures). - Resolve today's date once and reuse for both bound defaults, so a run across midnight can't make them disagree. - Test stubs take variadic args. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01NtfhjPwUEEHJb4Jf7714eR --- app/Domain/Tickets/Services/Tickets.php | 19 +++++++++++++++++-- .../Tickets/Services/TicketsServiceTest.php | 4 ++-- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/app/Domain/Tickets/Services/Tickets.php b/app/Domain/Tickets/Services/Tickets.php index cdf5b9003f..0d7712f08f 100644 --- a/app/Domain/Tickets/Services/Tickets.php +++ b/app/Domain/Tickets/Services/Tickets.php @@ -2448,12 +2448,27 @@ public function getMyClosedTicketsForDate(?int $userId = null, ?string $date = n * normalized so the earlier date is the lower bound (a reversed range is * swapped rather than returning nothing). So passing only one bound yields * the span between that date and today, in date order. + * + * A non-admin may only read their OWN closures: a caller-supplied $userId + * for someone else is forced back to the session user (IDOR guard). */ #[RequiresPermission(TicketsPermissions::VIEW)] public function getMyClosedTicketsForRange(?int $userId = null, ?string $from = null, ?string $to = null): array { - $from = $from ?: date('Y-m-d'); - $to = $to ?: date('Y-m-d'); + $sessionUser = (int) session('userdata.id'); + $userId = $userId ?: $sessionUser; + // IDOR guard: reading another user's closures requires admin. + if ($userId !== $sessionUser && ! Auth::userIsAtLeast(Roles::$admin)) { + $userId = $sessionUser; + } + if ($userId === 0) { + return []; + } + + // Resolve "today" once so a run across midnight can't disagree on bounds. + $today = date('Y-m-d'); + $from = $from ?: $today; + $to = $to ?: $today; // Tolerate a reversed range rather than returning nothing. if ($from > $to) { [$from, $to] = [$to, $from]; diff --git a/tests/Unit/app/Domain/Tickets/Services/TicketsServiceTest.php b/tests/Unit/app/Domain/Tickets/Services/TicketsServiceTest.php index 1679d67275..d900795e9a 100644 --- a/tests/Unit/app/Domain/Tickets/Services/TicketsServiceTest.php +++ b/tests/Unit/app/Domain/Tickets/Services/TicketsServiceTest.php @@ -557,11 +557,11 @@ public function test_closed_tickets_range_normalizes_swapped_range_and_keeps_lat $capturedTo = null; $ticketRepository = $this->make(TicketRepository::class, [ - 'simpleTicketQuery' => fn () => [ + 'simpleTicketQuery' => fn (...$args) => [ ['id' => 10, 'type' => 'task', 'projectId' => 5, 'status' => 0], ['id' => 20, 'type' => 'task', 'projectId' => 5, 'status' => 0], ], - 'getStateLabels' => fn () => [ + 'getStateLabels' => fn (...$args) => [ 0 => ['statusType' => 'DONE', 'name' => 'Done', 'class' => ''], 3 => ['statusType' => 'INPROGRESS', 'name' => 'In Progress', 'class' => ''], ], From 25166696f7c9a8aaf72b769971ed9bc6ffe29085 Mon Sep 17 00:00:00 2001 From: Gloria <55953099+gloriafolaron@users.noreply.github.com> Date: Sat, 18 Jul 2026 23:09:34 -0400 Subject: [PATCH 5/5] test(tickets): cover the IDOR guard in getMyClosedTicketsForRange (#3647 review) Copilot flagged that the non-admin IDOR branch (caller supplies another user's id -> forced to the session user) was untested. Add a test asserting the repository is queried with the session user, not the caller-supplied id. TicketsServiceTest green (24 tests / 53 assertions). Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01NtfhjPwUEEHJb4Jf7714eR --- .../Tickets/Services/TicketsServiceTest.php | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/tests/Unit/app/Domain/Tickets/Services/TicketsServiceTest.php b/tests/Unit/app/Domain/Tickets/Services/TicketsServiceTest.php index d900795e9a..91a49647fa 100644 --- a/tests/Unit/app/Domain/Tickets/Services/TicketsServiceTest.php +++ b/tests/Unit/app/Domain/Tickets/Services/TicketsServiceTest.php @@ -591,4 +591,28 @@ public function test_closed_tickets_range_normalizes_swapped_range_and_keeps_lat $this->assertEquals(10, $result[0]['id']); $this->assertEquals('2026-07-10 10:00:00', $result[0]['dateClosed']); } + + public function test_closed_tickets_range_forces_session_user_for_non_admin(): void + { + // Non-admin session user (no admin role granted). + session(['userdata' => ['id' => 1]]); + $capturedUserId = 'unset'; + + $ticketRepository = $this->make(TicketRepository::class, [ + 'simpleTicketQuery' => function (...$args) use (&$capturedUserId) { + $capturedUserId = $args[0] ?? null; + + return []; // no done tickets — the asserted-on value is the userId + }, + 'getStateLabels' => fn (...$args) => [], + ]); + + $service = $this->buildServiceWithTicketRepository($ticketRepository); + + // Caller supplies SOMEONE ELSE's id — the IDOR guard must force it back + // to the session user before any query runs. + $service->getMyClosedTicketsForRange(999, '2026-07-01', '2026-07-10'); + + $this->assertSame(1, $capturedUserId, 'a non-admin must not read another user\'s closures — userId is forced to the session user'); + } }