feat(tickets): getMyClosedTicketsForRange for period close-lists by gloriafolaron · Pull Request #3647 · Leantime/leantime

gloriafolaron · 2026-07-14T15:04:25Z

Why

Mobile Progress (the reflection surface) 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 existing getMyClosedTicketsForDate only answers a single day.

What

The range was already supported one layer down: getMyClosedTicketsForDate builds on getStatusChangeEvents(ids, $from, $to), which already takes a date range (whereBetween dateModified [from 00:00:00, to 23:59:59]) — the service just pinned it to one day (from == to).

Add getMyClosedTicketsForRange(?int $userId, ?string $from, ?string $to) (@api, RequiresPermission VIEW): same "closed" definition as the single-date form — the ticket is currently DONE and its status changed to that DONE status within [from, to]; one row per ticket keyed to its latest completion (events are newest-first). Tolerates a reversed range; defaults either bound to today.
getMyClosedTicketsForDate now delegates to it with from == to. Behaviour is byte-identical (the underlying query 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 for a week/month.

Risk

Low. Additive method + a pure refactor of the existing one into a delegate; no query or output shape changes for getMyClosedTicketsForDate. Pint + php -l clean.

🤖 Generated with Claude Code

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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NtfhjPwUEEHJb4Jf7714eR

Copilot

Pull request overview

Adds a date-range variant of the “my closed tickets” query to support mobile Progress views (“Closed this week / this month”), while refactoring the existing single-day method to delegate to the new range method for DRYness.

Changes:

Added getMyClosedTicketsForRange(?int $userId, ?string $from, ?string $to) to return currently-DONE tickets whose DONE status was set within an inclusive [from, to] date range, with dateClosed populated from the newest matching event per ticket.
Updated getMyClosedTicketsForDate() to delegate to the range method with from == to (intended to keep behavior identical).
Updated docblocks/comments to describe the range behavior and mobile use case.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+     * 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.


+    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];
+        }


marcelfolaron · 2026-07-14T19:12:51Z

Maintainer review (advisory — merge authority stays with @marcelfolaron / @broskees) · diff read in full at head 075e83c

1. Intent: Adds getMyClosedTicketsForRange(?userId, ?from, ?to) and refactors the existing getMyClosedTicketsForDate into a thin wrapper (from == to), so mobile Progress can pull "Closed this week / this month" close-lists in one query instead of 7–30 per-day calls. Byte-identical behavior for the single-date form.

2. Change requests (the refactor reads clean and low-risk — these are confirm-before-merge):

getAllDoneUserTickets($userId) is called with no date filter, then the range is applied only to getStatusChangeEvents (Tickets/Services/Tickets.php, getMyClosedTicketsForRange). For a "this month" call on a user with a large DONE backlog, the candidate set is all currently-done tickets, and the range only prunes the status-change events. Confirm this is acceptable at scale (it matches the prior single-date behavior, so no regression — flagging because the range form makes the candidate load more visible). A dateClosed-bearing result is only as big as the range, so output is bounded; it's the candidate fan-in worth a glance.
Reversed-range tolerance is good; confirm the swap is intended over an empty result (if ($from > $to) { [$from,$to] = [$to,$from]; }). Silently swapping is friendly for a UI, but if a caller passes a reversed range by mistake it will now return data rather than surfacing the bug. Low-risk; just confirming it's the desired contract (the docblock says so — good).
String date comparison $from > $to relies on 'Y-m-d' lexical ordering, which is correct for that exact format but breaks for any other. The ?: date('Y-m-d') defaults guarantee the format only when the arg is null — a caller passing '2026-7-1' (no zero-pad) would compare wrong. Consider normalizing via Carbon, or documenting that callers must pass zero-padded Y-m-d.

3. Acceptance checklist (must pass before merge):

A unit test for the new range method — getMyClosedTicketsForDate is asserted byte-identical to getMyClosedTicketsForRange($u, $d, $d), AND a multi-day range returns a ticket whose completion falls inside the range but excludes one whose completion falls outside it. This is a new @api method with real date-window logic — a test is the gate.
getMyClosedTicketsForDate behavior confirmed unchanged (existing callers/tests still green) — the PR claims byte-identical; the delegation makes that true only if the single-date path still passes from == to, which the diff shows ✔.
CI green: Pint + PHPStan level 5 + full unit suite. Read-path/query-only — no schema/API change (additive method), no migration.

4. CI status: ⚠️ Can't read check-run status this cycle — confirm green on 075e83c in the UI; treat any red/skipped Pint or PHPStan job as blocking. Test-gate note: this adds a new @api behavior (date-range close-list) with no accompanying test in the diff — treat the missing test in checklist #1 as the merge blocker. RequiresPermission(VIEW) is carried over correctly from the single-date form.

Merge-order note: #3649 (getMyCommentedTicketsForRange) edits the same Tickets/Services/Tickets.php region just below this method. Whichever merges second will need a trivial rebase — no logical conflict, but flag so they're not merged blind in parallel.

Advisory review only — not an approval, and I am not marking this ready or merging. Merge authority stays with @marcelfolaron / @broskees.

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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NtfhjPwUEEHJb4Jf7714eR

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

+        $from = $from ?: date('Y-m-d');
+        $to = $to ?: date('Y-m-d');


+    #[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');


… dedup 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NtfhjPwUEEHJb4Jf7714eR

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

+        $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' => ''],
+            ],


marcelfolaron · 2026-07-15T14:10:06Z

Maintainer re-review (advisory — merge authority stays with @marcelfolaron / @broskees) · re-reviewed at head 48b68d7a (new commit since my 075e83c review) · full diff read

1. Intent: Adds getMyClosedTicketsForRange(?userId, ?from, ?to) and refactors getMyClosedTicketsForDate into a thin from == to wrapper, so mobile Progress can pull "Closed this week / this month" in one query. Byte-identical for the single-date form.

What changed since my last review ✅ — the missing-test blocker is resolved. The new commit ships test_closed_tickets_range_normalizes_swapped_range_and_keeps_latest_completion, which asserts: a reversed range is normalized earliest-first (capturedFrom/capturedTo), only status changes into the ticket's current DONE status count (ticket 20's change-to-non-DONE is excluded), and a ticket with multiple completions keeps its latest (2026-07-10 over 07-09). That was my checklist #1 gate — now met.

2. Change requests (all non-blocking — confirm-before-merge):

getMyClosedTicketsForRange — string date compare if ($from > $to) relies on lexical 'Y-m-d' ordering. Correct for zero-padded dates, wrong for '2026-7-1'. The ?: date('Y-m-d') defaults guarantee the format only when the arg is null; a caller passing an unpadded date compares wrong. Consider normalizing via Carbon or documenting the zero-padded contract.
Candidate fan-in: getAllDoneUserTickets($userId) loads all currently-DONE tickets, then the range only prunes getStatusChangeEvents. Matches the prior single-date behavior (no regression) — flagging only because the range form makes the candidate load more visible at scale. Output stays bounded by the range.
Merge-order: feat(tickets): getMyCommentedTicketsForRange for Progress "Supported" #3649 (getMyCommentedTicketsForRange) now inserts a method immediately after this one in Tickets/Services/Tickets.php. No logical conflict, but whichever merges second needs a trivial rebase — don't merge the two blind in parallel.

3. Acceptance checklist (must pass before merge):

Range test present ✔ — confirm green in the suite; getMyClosedTicketsForDate asserted byte-identical to …ForRange($u,$d,$d) (delegation confirmed by inspection).
A multi-day range returns a ticket whose completion falls inside the range and excludes one outside it (the test covers the in-range + status-filter cases).
CI green: Pint + PHPStan level 5 + full unit suite. Read-path/query-only — no schema/API change, no migration.

4. CI status: ⚠️ Can't read check-run status this cycle — confirm green on 48b68d7a in the UI; treat any red/skipped Pint or PHPStan job as blocking. Test gate now met — this is confirm-green-and-merge from my side.

Advisory review only — not an approval, and I am not marking this ready or merging. Merge authority stays with @marcelfolaron / @broskees.

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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NtfhjPwUEEHJb4Jf7714eR

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

+     *
+     * Thin wrapper over {@see getMyClosedTicketsForRange} with from == to.
     */


+    public function test_closed_tickets_range_normalizes_swapped_range_and_keeps_latest_completion(): void
+    {
+        session(['userdata' => ['id' => 1]]);
+        $capturedFrom = null;
+        $capturedTo = null;


marcelfolaron · 2026-07-16T14:05:48Z

Maintainer re-review (advisory — merge authority stays with @marcelfolaron / @broskees) · re-reviewed at head 364068a9 (advanced past my 48b68d7a review — 4th commit on branch, rebased onto master f5a06e2f) · full diff read

1. Intent: Adds getMyClosedTicketsForRange(?userId, ?from, ?to) and refactors getMyClosedTicketsForDate into a thin from == to delegate, so mobile Progress can pull "Closed this week / this month" in one query instead of 7–30 per-day calls. Byte-identical for the single-date form.

What changed since my last review: the code at head is stable relative to 48b68d7a — the range method, the getMyClosedTicketsForDate → …ForRange($u,$d,$d) delegation, and test_closed_tickets_range_normalizes_swapped_range_and_keeps_latest_completion are all present and unchanged (app/Domain/Tickets/Services/Tickets.php, +100/−3, 2 files). The advance is a branch update, not a logic change. My prior test-gate blocker remains resolved.

2. Change requests (all non-blocking — confirm-before-merge; carried forward, still open):

String date compare relies on lexical 'Y-m-d' ordering — if ($from > $to) { [$from,$to] = [$to,$from]; } (getMyClosedTicketsForRange). Correct for zero-padded dates, wrong for '2026-7-1'. The ?: date('Y-m-d') defaults only guarantee the format when the arg is null; an unpadded caller-supplied bound compares wrong. Normalize via Carbon or document the zero-padded contract in the @param.
Candidate fan-in — getAllDoneUserTickets($userId) loads all currently-DONE tickets, then the range only prunes getStatusChangeEvents (getMyClosedTicketsForRange). Matches prior single-date behavior (no regression); flagging only because the range form makes the candidate load more visible at scale. Output stays bounded by the range.

3. Acceptance checklist (must pass before merge):

Range test green in the suite — getMyClosedTicketsForDate asserted byte-identical to …ForRange($u,$d,$d) (delegation confirmed by inspection ✔).
A multi-day range returns a ticket whose completion falls inside the range and excludes one outside it (the test covers the status-INTO-DONE filter + latest-completion cases).
CI green: Pint + PHPStan level 5 + full unit suite. Read-path/query-only — no schema/API change (additive method), no migration.

4. CI status: ⚠️ Can't read check-run status this cycle — confirm green on 364068a9 in the UI; treat any red/skipped/flaky Pint or PHPStan job as blocking. RequiresPermission(VIEW) carried over correctly from the single-date form. Test gate met — this is confirm-green-and-merge from my side.

Merge-order: #3649 inserts a method immediately after this one in Tickets/Services/Tickets.php. No logical conflict, but whichever merges second needs a trivial rebase — don't merge the two blind in parallel.

Advisory review only — not an approval, and I am not marking this ready or merging. Merge authority stays with @marcelfolaron / @broskees.

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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NtfhjPwUEEHJb4Jf7714eR

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

+        // 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];
+        }


+    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]]);


Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

app/Domain/Tickets/Services/Tickets.php:2475

getStatusChangeEvents() expects fromDate/toDate as Y-m-d and concatenates times ($fromDate.' 00:00:00'). Since getMyClosedTicketsForRange() is @api and accepts free-form strings, callers passing a full datetime (e.g. 2026-07-01 12:00:00) will produce an invalid bound (2026-07-01 12:00:00 00:00:00) and can break the query. Normalize the inputs to Y-m-d before comparing/swapping and calling the repository.

        // 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];
        }

Resolve the TicketsServiceTest conflict by keeping BOTH suites — master's closed-range tests (#3647/#3650) and this branch's commented-range tests. Tickets service/repo auto-merged cleanly. Full suite green (30 tests / 62 assertions), Pint clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NtfhjPwUEEHJb4Jf7714eR

gloriafolaron requested a review from a team as a code owner July 14, 2026 15:04

gloriafolaron requested review from Copilot and marcelfolaron and removed request for a team July 14, 2026 15:04

Copilot started reviewing on behalf of gloriafolaron July 14, 2026 15:04 View session

Copilot AI reviewed Jul 14, 2026

View reviewed changes

gloriafolaron mentioned this pull request Jul 14, 2026

feat(tickets): getMyCommentedTicketsForRange for Progress "Supported" #3649

Merged

Copilot AI review requested due to automatic review settings July 14, 2026 20:15

Copilot started reviewing on behalf of gloriafolaron July 14, 2026 20:16 View session

Copilot AI reviewed Jul 14, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 14, 2026 20:26

Copilot started reviewing on behalf of gloriafolaron July 14, 2026 20:27 View session

Copilot AI reviewed Jul 14, 2026

View reviewed changes

marcelfolaron mentioned this pull request Jul 15, 2026

feat(reports): stakeholder report — Phase 1 shell (deck, header, navigation) #3648

Merged

6 tasks

Copilot AI review requested due to automatic review settings July 16, 2026 13:00

Copilot started reviewing on behalf of gloriafolaron July 16, 2026 13:00 View session

Copilot AI reviewed Jul 16, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 19, 2026 03:09

Copilot started reviewing on behalf of gloriafolaron July 19, 2026 03:10 View session

Copilot AI reviewed Jul 19, 2026

View reviewed changes

gloriafolaron requested a review from Copilot July 19, 2026 03:22

Copilot started reviewing on behalf of gloriafolaron July 19, 2026 03:23 View session

Copilot AI reviewed Jul 19, 2026

View reviewed changes

marcelfolaron merged commit a035913 into master Jul 19, 2026
13 of 14 checks passed

marcelfolaron deleted the feat/closed-tickets-range branch July 19, 2026 13:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

feat(tickets): getMyClosedTicketsForRange for period close-lists#3647

feat(tickets): getMyClosedTicketsForRange for period close-lists#3647
marcelfolaron merged 5 commits into
masterfrom
feat/closed-tickets-range

gloriafolaron commented Jul 14, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

marcelfolaron commented Jul 14, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

marcelfolaron commented Jul 15, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

marcelfolaron commented Jul 16, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Uh oh!

Conversation

gloriafolaron commented Jul 14, 2026

Why

What

Risk

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

marcelfolaron commented Jul 14, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

marcelfolaron commented Jul 15, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

marcelfolaron commented Jul 16, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants