feat(tickets): getMyCommentedTicketsForRange for Progress "Supported"#3649
Merged
Conversation
Mobile Progress "Supported" honors invisible/presence labor — work you did on
someone else's arc. Commenting on others' tickets is one of its clearest
signals, and there was no way to query it (comments are only fetchable per
ticket). There is no zp_watchers table, so "watching" isn't available; comments
are.
- Repo: getTicketIdsCommentedByUser(userId, from, to) — distinct ticket ids the
user commented on in the range (zp_comment, module=ticket). Access-agnostic on
its own; the caller MUST intersect with an access-scoped set.
- Service: getMyCommentedTicketsForRange(userId, from, to) (@api, VIEW) —
intersects those ids with simpleTicketQuery's access-scoped universe (project
access + collaborator clause), then drops tickets the user is the editor of
("your work", not support). So a historical comment can never surface a ticket
the user can no longer see (no IDOR), and only genuinely-others' work returns.
Returns the same ticket-row shape as the other user-ticket queries. Pint +
php -l clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NtfhjPwUEEHJb4Jf7714eR
gloriafolaron
requested review from
marcelfolaron
and removed request for
a team
July 14, 2026 16:00
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new Tickets service/repository pathway to support Mobile Progress → “Supported” by returning tickets a user commented on within a date range, while attempting to keep results access-safe by intersecting with an access-scoped ticket set and excluding tickets the user “owns”.
Changes:
- Add repository query to fetch distinct ticket IDs the user commented on within an inclusive
[from, to]date range. - Add service API
getMyCommentedTicketsForRangeto intersect commented ticket IDs with an access-scoped ticket universe and exclude editor-owned tickets.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| app/Domain/Tickets/Services/Tickets.php | Adds getMyCommentedTicketsForRange API to compute “Supported” tickets from comments + access-scoped intersection + ownership exclusion. |
| app/Domain/Tickets/Repositories/Tickets.php | Adds getTicketIdsCommentedByUser primitive to retrieve distinct commented ticket IDs for a user within a date range. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+2496
to
+2501
| // Access-scoped universe — never returns a ticket outside the user's | ||
| // project access, so intersecting against it makes the comment lookup | ||
| // safe regardless of what was commented on historically. | ||
| $accessible = $this->ticketRepository->simpleTicketQuery($userId, null); | ||
| if (! is_array($accessible) || empty($accessible)) { | ||
| return []; |
Comment on lines
+2475
to
+2476
| * @return array<int, array<string, mixed>> ticket rows (id, headline, | ||
| * projectName, …), same shape as the other user-ticket queries. |
Comment on lines
+1896
to
+1904
| $rows = $this->connection->table('zp_comment') | ||
| ->select('moduleId') | ||
| ->distinct() | ||
| ->where('module', 'ticket') | ||
| ->where('userId', $userId) | ||
| ->whereBetween('date', [$fromDate.' 00:00:00', $toDate.' 23:59:59']) | ||
| ->get(); | ||
|
|
||
| return array_values(array_unique(array_map(fn ($row) => (int) $row->moduleId, $rows->all()))); |
Comment on lines
+2478
to
+2489
| #[RequiresPermission(TicketsPermissions::VIEW)] | ||
| public function getMyCommentedTicketsForRange(?int $userId = null, ?string $from = null, ?string $to = null): array | ||
| { | ||
| $userId = $userId ?? (int) session('userdata.id'); | ||
| if ($userId === 0) { | ||
| return []; | ||
| } | ||
| $from = $from ?: date('Y-m-d'); | ||
| $to = $to ?: date('Y-m-d'); | ||
| if ($from > $to) { | ||
| [$from, $to] = [$to, $from]; | ||
| } |
Collaborator
|
Maintainer review (advisory — merge authority stays with @marcelfolaron / @broskees) · diff read in full at head 1. Intent: Adds 2. Change requests (the access-scoping design is deliberately careful — these are confirm-before-merge; access-control code raises the bar):
3. Acceptance checklist (must pass before merge):
4. CI status: Merge-order note: edits the same Advisory review only — not an approval, and I am not marking this ready or merging. Merge authority stays with @marcelfolaron / @broskees. |
…ator Copilot review (#3649): intersecting commented ids against simpleTicketQuery constrained results to tickets the user EDITS or COLLABORATES on — but you comment on others' work without being either, so most supported tickets were silently dropped (could return empty even with comments). Now scope commented ids to the user's accessible PROJECTS (getProjectsUserHasAccessTo) — the correct, still-safe boundary — then exclude editor-owned tickets. Also: docblock no longer claims identical shape to getAll*UserTickets (raw rows, no statusLabel), and getTicketIdsCommentedByUser uses pluck() (drops redundant array_unique). New repo method getTicketsByIdsWithinProjects fetches the rows by id within the access-scoped projects. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NtfhjPwUEEHJb4Jf7714eR
Comment on lines
+2486
to
+2489
| $userId = $userId ?? (int) session('userdata.id'); | ||
| if ($userId === 0) { | ||
| return []; | ||
| } |
Comment on lines
+2468
to
+2473
| * Access safety: commented ticket ids are constrained to the PROJECTS the | ||
| * user can access (getProjectsUserHasAccessTo) — NOT to tickets they edit | ||
| * or collaborate on. Commenting on a ticket rarely makes you its editor or | ||
| * a collaborator, so scoping by those (as an earlier version did via | ||
| * simpleTicketQuery) silently dropped most supported work — it collapsed to | ||
| * "commented AND collaborator" and could return empty even when comments |
Comment on lines
+1919
to
+1933
| $rows = $this->connection->table('zp_tickets') | ||
| ->leftJoin('zp_projects', 'zp_tickets.projectId', '=', 'zp_projects.id') | ||
| ->select( | ||
| 'zp_tickets.id', | ||
| 'zp_tickets.headline', | ||
| 'zp_tickets.projectId', | ||
| 'zp_tickets.editorId', | ||
| 'zp_tickets.userId', | ||
| 'zp_tickets.status', | ||
| 'zp_tickets.dateToFinish', | ||
| 'zp_projects.name as projectName', | ||
| ) | ||
| ->whereIn('zp_tickets.id', $ticketIds) | ||
| ->whereIn('zp_tickets.projectId', $projectIds) | ||
| ->get(); |
Comment on lines
+2483
to
+2485
| #[RequiresPermission(TicketsPermissions::VIEW)] | ||
| public function getMyCommentedTicketsForRange(?int $userId = null, ?string $from = null, ?string $to = null): array | ||
| { |
… scope Copilot review (#3649): add unit coverage — editor-owned tickets are excluded, tickets outside the accessible-project fetch don't appear, and no project access short-circuits to empty. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NtfhjPwUEEHJb4Jf7714eR
Comment on lines
+2463
to
+2465
| * Tickets the user COMMENTED on within [$from, $to] that they do NOT own | ||
| * (they're not the assignee/editor) — i.e. work they supported by weighing | ||
| * in on someone else's arc. Powers the mobile Progress "Supported" section |
Collaborator
|
Maintainer re-review (advisory — merge authority stays with @marcelfolaron / @broskees) · re-reviewed at head 1. Intent: Adds What changed since my last review — the access model was rewritten, and tests were added. Two substantive changes:
2. Change requests (this is access-control code, so these are confirm-before-merge):
3. Acceptance checklist (must pass before merge):
4. CI status: Advisory review only — not an approval, and I am not marking this ready or merging. Merge authority stays with @marcelfolaron / @broskees. |
…eview) Copilot re-review (#3649): - IDOR: a non-admin passing someone else's userId could read that user's comment activity (scoped to the caller's projects). Force userId to the session user unless admin — mirrors Projects::getProjectsUserHasAccessTo(). - Resolve today's date once so a run across midnight can't disagree on bounds. - getTicketsByIdsWithinProjects now has an explicit ORDER BY (DB default order is unspecified/nondeterministic). - Docstring: 'editor', not 'assignee/editor' (the filter is editorId only). - Test stubs take variadic args. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NtfhjPwUEEHJb4Jf7714eR
Comment on lines
+1933
to
+1936
| // Stable, deterministic order (DB default order is unspecified). | ||
| ->orderBy('zp_tickets.dateToFinish') | ||
| ->orderBy('zp_tickets.id') | ||
| ->get(); |
Comment on lines
+610
to
+621
| $ticketRepository = $this->make(TicketRepository::class, [ | ||
| 'getTicketIdsCommentedByUser' => fn (...$args) => [10], | ||
| 'getTicketsByIdsWithinProjects' => fn (...$args) => [['id' => 10, 'editorId' => '99']], | ||
| ]); | ||
| $projectService = $this->make(ProjectService::class, [ | ||
| 'getProjectsUserHasAccessTo' => fn (...$args) => false, | ||
| ]); | ||
|
|
||
| $service = $this->buildServiceWithTicketRepoAndProjectService($ticketRepository, $projectService); | ||
|
|
||
| $this->assertSame([], $service->getMyCommentedTicketsForRange(1, '2026-07-01', '2026-07-07')); | ||
| } |
Collaborator
|
Maintainer re-review (advisory — merge authority stays with @marcelfolaron / @broskees) · re-reviewed at head 1. Intent: Adds What changed since my last review: the code at head is stable relative to 2. Change requests (this is access-control code — these are confirm-before-merge; #2 is the one I'd treat as the gate):
3. Acceptance checklist (must pass before merge):
4. CI status: Merge-order: edits the same Advisory review only — not an approval, and I am not marking this ready or merging. Merge authority stays with @marcelfolaron / @broskees. |
…mentedTicketsForRange (#3649 review) Copilot asked for coverage of the access-sensitive branches. Add: - IDOR guard: a non-admin caller supplying another user's id is forced to the session user before the query runs. - Reversed range is normalized earliest-first. - An empty commented set short-circuits and never calls getTicketsByIdsWithinProjects (asserted explicitly, so the short-circuit can't silently regress). TicketsServiceTest green (27 tests / 55 assertions). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NtfhjPwUEEHJb4Jf7714eR
Comment on lines
+2499
to
+2502
| // Resolve "today" once so a run across midnight can't disagree on bounds. | ||
| $today = date('Y-m-d'); | ||
| $from = $from ?: $today; | ||
| $to = $to ?: $today; |
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
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
tests/Unit/app/Domain/Tickets/Services/TicketsServiceTest.php:706
- This test’s docblock says it should return empty without ever fetching tickets, but it currently doesn’t assert that
getTicketsByIdsWithinProjects()was not called. Add a flag/assertion so the test will fail if the implementation regresses and performs the ticket fetch anyway.
public function test_commented_tickets_range_empty_without_project_access(): void
{
session(['userdata' => ['id' => 1]]);
$ticketRepository = $this->make(TicketRepository::class, [
'getTicketIdsCommentedByUser' => fn (...$args) => [10],
'getTicketsByIdsWithinProjects' => fn (...$args) => [['id' => 10, 'editorId' => '99']],
]);
Comment on lines
+1914
to
+1921
| return $this->connection->table('zp_comment') | ||
| ->where('module', 'ticket') | ||
| ->where('userId', $userId) | ||
| ->whereBetween('date', [$fromDate.' 00:00:00', $toDate.' 23:59:59']) | ||
| ->distinct() | ||
| ->pluck('moduleId') | ||
| ->map(fn ($id) => (int) $id) | ||
| ->all(); |
…review) Copilot: zp_comment.moduleId is nullable, so (int) NULL cast a bogus id 0 into the commented-ids list, which then reached the downstream whereIn(). Filter NULLs at the query level (whereNotNull) plus a defensive ->filter() to drop any 0 after the cast, so only real ticket ids flow to getTicketsByIdsWithinProjects. TicketsServiceTest 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
Comment on lines
+1954
to
+1956
| ->whereIn('zp_tickets.id', $ticketIds) | ||
| ->whereIn('zp_tickets.projectId', $projectIds) | ||
| // Stable, deterministic order (DB default order is unspecified). |
…3649 review) Copilot: a comment on a milestone would surface a milestone row in the 'Supported' list, inconsistent with the other mobile user-ticket queries which exclude milestones. Add the same 'type <> milestone' guard so Supported stays a task surface. TicketsServiceTest 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
Comment on lines
+2536
to
+2538
| * @return array<int, array<string, mixed>> raw ticket rows (id, headline, | ||
| * projectName, editorId, status, …). NOTE: these do NOT carry the resolved | ||
| * statusLabel/statusClass/statusType that getAll*UserTickets attach. |
…ts own lines (#3649 review) Copilot readability nit — the NOTE was crammed onto the @return type line. Split the type/description from the status-fields note. Comment-only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NtfhjPwUEEHJb4Jf7714eR
Comment on lines
+2567
to
+2583
| // Scope to the projects the user can see — the correct access boundary | ||
| // for "tickets I commented on" (you comment on others' work without | ||
| // being its editor/collaborator, so filtering by those would drop it). | ||
| $projects = $this->projectService->getProjectsUserHasAccessTo($userId); | ||
| if (! is_array($projects) || empty($projects)) { | ||
| return []; | ||
| } | ||
| $projectIds = array_values(array_filter(array_map( | ||
| fn ($p) => (int) ($p['id'] ?? 0), | ||
| $projects | ||
| ))); | ||
| if (empty($projectIds)) { | ||
| return []; | ||
| } | ||
|
|
||
| $tickets = $this->ticketRepository->getTicketsByIdsWithinProjects($commentedIds, $projectIds); | ||
|
|
Comment on lines
+645
to
+663
| private function buildServiceWithTicketRepoAndProjectService( | ||
| TicketRepository $ticketRepository, | ||
| ProjectService $projectService | ||
| ): TicketsService { | ||
| return new TicketsService( | ||
| language: $this->make(LanguageCore::class), | ||
| ticketRepository: $ticketRepository, | ||
| timesheetsRepo: $this->make(TimesheetRepository::class), | ||
| settingsRepo: $this->make(SettingRepository::class), | ||
| projectService: $projectService, | ||
| timesheetService: $this->make(TimesheetService::class), | ||
| sprintService: $this->make(SprintService::class), | ||
| ticketHistoryRepo: $this->make(TicketHistory::class), | ||
| goalcanvasService: $this->make(Goalcanvas::class), | ||
| dateTimeHelper: $this->make(DateTimeHelper::class), | ||
| commentService: $this->make(CommentService::class), | ||
| clientService: $this->make(ClientService::class) | ||
| ); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a new Tickets service/repository pathway to support Mobile Progress → "Supported" — tickets a user commented on within a date range that they don't own.
What
getTicketIdsCommentedByUser(userId, from, to)— distinct ticket ids the user commented on in the range (zp_comment,module=ticket), viapluck.getTicketsByIdsWithinProjects(ids, projectIds)— ticket rows for those ids, constrained to the given projects, with an explicit deterministic ORDER BY.getMyCommentedTicketsForRange(userId, from, to)(@api,VIEW) — resolves the commented ids, scopes them to the projects the user can access (getProjectsUserHasAccessTo), then drops tickets the user is the editor of ("your work", not support).Access / security
userIdis forced back to the session user (mirrorsProjects::getProjectsUserHasAccessTo()).Notes
statusLabel/statusClass/statusTypethatgetAll*UserTicketsattach; the docblock says so.🤖 Generated with Claude Code