chore: Created AdminHistory functionalities#90
Merged
Conversation
Contributor
Reviewer's GuideAdds an admin-focused ticket history feature that exposes a new backend endpoint returning enriched history items (including performer metadata) and a corresponding frontend admin history timeline view wired to a new admin history API, while extending the shared history schema with performed-by information. Sequence diagram for the new admin ticket history retrieval flowsequenceDiagram
actor Admin
participant AdminHistory as AdminHistorySvelte
participant TicketHistoryView as TicketHistoryViewSvelte
participant AdminHistoryTimeline as AdminHistoryTimelineSvelte
participant HistoryApi as historyApiModule
participant Backend as TicketsRouter
participant View as admin_ticket_history
participant DB as Database
Admin->>AdminHistory: Open admin history page
AdminHistory->>TicketHistoryView: Render with fetchItems
TicketHistoryView->>HistoryApi: fetchAdminTicketHistory()
HistoryApi->>Backend: GET /tickets/admin/history (with cookies)
Backend->>View: Dispatch admin_ticket_history(request)
View->>View: Check request.user.is_staff
View-->>Backend: 403 if not staff
Backend-->>HistoryApi: 403 Forbidden (if not staff)
HistoryApi-->>TicketHistoryView: Error "You do not have permission..."
TicketHistoryView-->>Admin: Show error state
rect rgb(230,230,255)
View->>DB: Query TicketStatusHistory with select_related(changed_by)
View->>DB: Query TicketComment with select_related(user)
View->>DB: Query Ticket with category, priority, student and prefetch_related
DB-->>View: Tickets with status_history and comments
View->>View: Build events list
View->>View: Add created event (performed_by None)
View->>View: Add status events with performed_by changed_by.full_name
View->>View: Add comment events with performed_by user.full_name
View->>View: Sort events by at desc
View->>View: Map to TicketHistoryItemSchema
View-->>Backend: 200, list[TicketHistoryItemSchema]
end
Backend-->>HistoryApi: 200 OK, JSON history items
HistoryApi-->>TicketHistoryView: HistoryItem[]
TicketHistoryView->>TicketHistoryView: Apply filters and sorting
TicketHistoryView->>AdminHistoryTimeline: Render items with performedBy
AdminHistoryTimeline-->>Admin: Display admin history timeline
Admin->>AdminHistoryTimeline: Click history card
AdminHistoryTimeline->>AdminHistoryTimeline: navigate(ticketId)
AdminHistoryTimeline->>AdminHistory: goto("tickets/" + ticketId)
AdminHistory-->>Admin: Show ticket detail page
Updated class diagram for admin ticket history models and componentsclassDiagram
class Ticket {
+int id
+string ticket_number
+string title
+string status
+datetime created_at
+Category category
+Priority priority
+Student student
}
class TicketStatusHistory {
+int id
+string old_status
+string new_status
+datetime changed_at
+User changed_by
}
class TicketComment {
+int id
+string message
+datetime created_at
+User user
}
class TicketHistoryItemSchema {
+string id
+int ticketPk
+string ticketId
+string title
+string action
+string description
+string timestamp
+string date
+string status
+string priority
+string category
+string performedBy
}
class HistoryItem {
+string id
+string ticketId
+string title
+string action
+string description
+string timestamp
+string date
+string status
+string priority
+string category
+string performedBy
}
class historyApiModule {
+string BASE
+fetchTicketHistory()
+fetchAdminTicketHistory()
}
class TicketHistoryViewSvelte {
+HistoryItem[] historyItems
+HistoryFilterType activeFilter
+HistorySortType sortBy
+string searchQuery
+function clearFilters()
}
class AdminHistoryTimelineSvelte {
+HistoryItem[] items
+HistoryFilterType activeFilter
+HistorySortType sortBy
+string searchQuery
+function onclearfilters()
+function navigate(ticketId)
}
class AdminHistorySvelte {
}
class admin_ticket_history {
+admin_ticket_history(request)
}
Ticket "1" --> "*" TicketStatusHistory : has
Ticket "1" --> "*" TicketComment : has
admin_ticket_history ..> Ticket : reads
admin_ticket_history ..> TicketStatusHistory : reads
admin_ticket_history ..> TicketComment : reads
admin_ticket_history ..> TicketHistoryItemSchema : returns
TicketHistoryItemSchema <.. HistoryItem : mirrored_on_frontend
AdminHistorySvelte ..> TicketHistoryViewSvelte : embeds
TicketHistoryViewSvelte ..> historyApiModule : uses_fetchAdminTicketHistory
TicketHistoryViewSvelte ..> AdminHistoryTimelineSvelte : renders
AdminHistoryTimelineSvelte ..> HistoryItem : displays
historyApiModule ..> admin_ticket_history : calls_endpoint
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
📝 WalkthroughWalkthroughAdds an admin-only /admin/history endpoint and admin UI to aggregate and view ticket history; extends backend and frontend history schemas with an optional performedBy / performed_by field and adds frontend API and components to fetch and display admin history (filters, navigation, keyboard support). Changes
Sequence Diagram(s)sequenceDiagram
participant Admin as Admin (Browser)
participant FE as Frontend (Svelte)
participant BE as Backend (API)
participant DB as Database
Admin->>FE: Open Admin History page
FE->>BE: GET /admin/history (with credentials)
BE->>DB: Query tickets, prefetch status history and comments
DB-->>BE: Tickets + status history + comments
BE->>BE: Aggregate events, compute event_status and performed_by
BE-->>FE: 200 [ list of TicketHistoryItemSchema ]
FE->>Admin: Render AdminHistoryTimeline (filter/search/sort)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new
admin_ticket_historyview duplicates most of the logic fromticket_history; consider extracting shared helpers for building the events list and mapping toTicketHistoryItemSchemato avoid divergence and make future changes easier. - In
AdminHistoryTimeline.svelte,goto(tickets/${ticketId})generates a relative URL, so from/admin/historyit would navigate to/admin/history/tickets/...; if you intend to go to the global ticket detail route, add a leading slash (e.g.goto(/tickets/${ticketId})).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `admin_ticket_history` view duplicates most of the logic from `ticket_history`; consider extracting shared helpers for building the events list and mapping to `TicketHistoryItemSchema` to avoid divergence and make future changes easier.
- In `AdminHistoryTimeline.svelte`, `goto(`tickets/${ticketId}`)` generates a relative URL, so from `/admin/history` it would navigate to `/admin/history/tickets/...`; if you intend to go to the global ticket detail route, add a leading slash (e.g. `goto(`/tickets/${ticketId}`)`).
## Individual Comments
### Comment 1
<location path="frontend/src/components/ui/history/AdminHistoryTimeline.svelte" line_range="17-18" />
<code_context>
+ export let sortBy: HistorySortType = "newest";
+ export let onclearfilters: () => void = () => {};
+
+ function navigate(ticketId: string) {
+ goto(`tickets/${ticketId}`);
+ }
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Use an absolute path in `goto` to avoid routing to a wrong relative URL.
Because this path is relative, from routes like `/admin/history` it would resolve to `/admin/tickets/...` instead of the intended `/tickets/...`. Use an absolute path, e.g. `goto(`/tickets/${ticketId}`)`, or the appropriate absolute base for ticket details so navigation works from any route.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+17
to
+18
| function navigate(ticketId: string) { | ||
| goto(`tickets/${ticketId}`); |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Use an absolute path in goto to avoid routing to a wrong relative URL.
Because this path is relative, from routes like /admin/history it would resolve to /admin/tickets/... instead of the intended /tickets/.... Use an absolute path, e.g. goto(/tickets/${ticketId}), or the appropriate absolute base for ticket details so navigation works from any route.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
frontend/src/lib/api/history.ts (1)
19-33: Consider extracting shared fetch logic to reduce duplication.Both
fetchTicketHistoryandfetchAdminTicketHistoryshare nearly identical structure. Consider a shared helper to reduce duplication:♻️ Suggested refactor
+async function fetchHistory(endpoint: string, permissionErrorMsg: string): Promise<HistoryItem[]> { + const res = await fetch(`${BASE}${endpoint}`, { + method: "GET", + credentials: "include", + headers: { Accept: "application/json" }, + }); + if (!res.ok) { + if (res.status === 403) { + throw new Error(permissionErrorMsg); + } + throw new Error(res.status === 401 ? "Unauthorized" : "Failed to load history"); + } + const data = await res.json(); + return (data ?? []) as HistoryItem[]; +} + export async function fetchTicketHistory(): Promise<HistoryItem[]> { - const res = await fetch(`${BASE}/history`, { - method: "GET", - credentials: "include", - headers: { Accept: "application/json" }, - }); - if (!res.ok) { - throw new Error(res.status === 401 ? "Unauthorized" : "Failed to load history"); - } - const data = await res.json(); - return (data ?? []) as HistoryItem[]; + return fetchHistory("/history", "Permission denied"); } export async function fetchAdminTicketHistory(): Promise<HistoryItem[]> { - const res = await fetch(`${BASE}/admin/history`, { - method: "GET", - credentials: "include", - headers: { Accept: "application/json" }, - }); - if (!res.ok) { - if (res.status === 403) { - throw new Error("You do not have permission to view admin history"); - } - throw new Error(res.status === 401 ? "Unauthorized" : "Failed to load admin history"); - } - const data = await res.json(); - return (data ?? []) as HistoryItem[]; + return fetchHistory("/admin/history", "You do not have permission to view admin history"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/api/history.ts` around lines 19 - 33, Both fetchTicketHistory and fetchAdminTicketHistory duplicate the same fetch/error/json logic; extract a small helper (e.g., fetchJson or fetchWithAuth) that accepts the endpoint path and optional permission-specific error messages, uses BASE, method "GET", credentials: "include", and Accept: "application/json", checks res.ok and maps 401/403 to the same specific Error messages, calls res.json() and returns (data ?? []) as HistoryItem[]; then replace both fetchTicketHistory and fetchAdminTicketHistory to call this helper with "/history" and "/admin/history" (preserving the exact error texts currently used).frontend/src/components/ui/history/AdminHistoryTimeline.svelte (1)
56-56: Unusedindexvariable.The
indexvariable is declared but never used in the loop body. Remove it for cleaner code.♻️ Proposed fix
- {`#each` items as item, index (item.id)} + {`#each` items as item (item.id)}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/ui/history/AdminHistoryTimeline.svelte` at line 56, The each block declares an unused loop index; in the Svelte template where the loop is written as iterating over items with "item" and "index" (using the key item.id), remove the unused "index" identifier so the loop only declares "item" with the same key (keep items, item and item.id intact) to clean up the code and eliminate the unused variable.backend/apps/tickets/views.py (2)
168-171: Consider adding pagination for scalability.This endpoint returns all tickets' history events without pagination. For systems with many tickets, this could cause performance issues and large response payloads.
Consider adding pagination parameters (e.g.,
limit,offsetor cursor-based) to handle large datasets gracefully. This becomes important as the ticket volume grows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/tickets/views.py` around lines 168 - 171, The current query that builds `tickets` using Ticket.objects.select_related(...).prefetch_related(...) returns all tickets and should be paginated to avoid huge payloads; update the view handling this logic (the function or class-based view in backend/apps/tickets/views.py that defines `tickets`) to accept pagination parameters (e.g., `limit` and `offset` or a cursor) from the request, validate defaults/max limits, apply slicing to the queryset (e.g., `.order_by(...)` then `[offset:offset+limit]` or implement Django Paginator/cursor logic), and return paginated metadata (total/count, next/prev offsets or cursor) along with the page of `tickets` while keeping the existing `select_related`/`prefetch_related` (refer to the `tickets` variable, `status_history_qs`, and `comments_qs`) to ensure related objects are still fetched efficiently.
160-240: Consider extracting shared event-building logic to reduce duplication.The
admin_ticket_historyfunction (lines 160-240) duplicates nearly all logic fromticket_history(lines 78-158). The only meaningful difference is the authorization filter. Note thatticket_historyalready returns all tickets for staff users (line 86-87), so the admin endpoint may be redundant unless there's a specific need to separate the API paths.♻️ Suggested refactor: Extract shared helper
def _build_ticket_history_events(tickets): """Build history events from tickets queryset.""" events = [] for ticket in tickets: events.append({ "at": ticket.created_at, "id": f"created-{ticket.id}-{ticket.created_at.isoformat()}", "ticket": ticket, "action": "created", "description": "Ticket created", "event_status": "pending", "performed_by": None, }) for h in ticket.status_history.all(): action = history_action_for_status_change(h.old_status, h.new_status) events.append({ "at": h.changed_at, "id": f"status-{h.id}", "ticket": ticket, "action": action, "new_status": h.new_status, "description": ( f"Status changed from {map_status_for_history(h.old_status)} " f"to {map_status_for_history(h.new_status)}" ), "event_status": h.new_status, "performed_by": h.changed_by.full_name if h.changed_by else None, }) for c in ticket.comments.all(): events.append({ "at": c.created_at, "id": f"comment-{c.id}", "ticket": ticket, "action": "commented", "description": f"Comment: {c.message[:100]}{'…' if len(c.message) > 100 else ''}", "performed_by": c.user.full_name if c.user else None, }) return events def _events_to_schema(events): """Convert events to TicketHistoryItemSchema list.""" events.sort(key=lambda e: e["at"], reverse=True) status_change_actions = ("updated", "resolved", "closed", "reopened") out = [] for e in events: t = e["ticket"] # ... (rest of mapping logic) out.append(TicketHistoryItemSchema(...)) return outThen both endpoints become thin wrappers:
`@router.get`("/history", response=list[TicketHistoryItemSchema]) def ticket_history(request): tickets = _get_tickets_with_history(request) events = _build_ticket_history_events(tickets) return _events_to_schema(events) `@router.get`("/admin/history", response={200: list[TicketHistoryItemSchema], 403: dict}) def admin_ticket_history(request): if not request.user.is_staff: return 403, {"detail": "Not authorized."} tickets = Ticket.objects.select_related(...).prefetch_related(...).all() events = _build_ticket_history_events(tickets) return 200, _events_to_schema(events)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/tickets/views.py` around lines 160 - 240, The admin_ticket_history function duplicates the event-building and mapping logic from ticket_history; extract that shared logic into two helpers (e.g., _build_ticket_history_events(tickets) to collect created/status/comment events and _events_to_schema(events) to sort and map to TicketHistoryItemSchema) and have both endpoints call these helpers; keep admin_ticket_history's staff check (return 403 if not staff) and only use a different ticket queryset there if intentional, otherwise simply call the same ticket retrieval used by ticket_history to avoid redundancy.frontend/src/components/ui/history/TicketHistoryView.svelte (1)
111-165: Significant duplication between student and admin views.The admin block (lines 111-165) largely mirrors the student block (lines 56-110), differing only in header text and the timeline component. While functional, consider extracting shared structure to reduce maintenance burden.
♻️ Possible consolidation approach
You could extract the common layout into a shared structure with slots or props for the differences:
<!-- Conceptual approach --> {`#snippet` historyLayout(title, description, TimelineComponent)} <div class="flex flex-col h-[calc(100vh-8rem)]"> <div class="shrink-0 mb-6"> <h1 class="text-2xl font-black text-base-content mb-1">{title}</h1> <p class="text-sm text-base-content/60">{description}</p> </div> <!-- shared filters, loading, error, and timeline rendering --> </div> {/snippet}This would reduce ~50 lines of duplication, but the current approach is also acceptable if you prefer explicit separation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/ui/history/TicketHistoryView.svelte` around lines 111 - 165, The admin view duplicates most of the student view in TicketHistoryView.svelte; extract the shared layout (filters, loading/error/summary, scroll container) into a new reusable component (e.g., HistoryLayout or HistoryContainer) that accepts props/slots for title, description and the timeline component, and forward/bind the reactive state (activeFilter, sortBy, searchQuery, totalCount/historyItems, loading, error, filteredItems) and the clearFilters handler; then replace the duplicated admin block with the new component passing title="Admin History", the admin description, and AdminHistoryTimeline (while student continues to pass StudentHistoryTimeline) so AdminHistoryTimeline and StudentHistoryTimeline remain the only differing pieces.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/apps/tickets/views.py`:
- Around line 160-164: The admin_ticket_history endpoint currently returns a
dict for unauthorized access but must return a (status_code, body) tuple to
match the declared response map; update the unauthorized branch in
admin_ticket_history to return a tuple with the status first and the dict second
(e.g., (403, {"detail": "Not authorized."})) so it matches the response
signature {200: list[TicketHistoryItemSchema], 403: dict}.
In `@frontend/src/components/ui/history/AdminHistoryTimeline.svelte`:
- Around line 17-19: The navigate function uses a relative path which can lead
to incorrect routing; update the navigate(ticketId: string) function to call
goto with an absolute path (prepend a leading slash) so it always navigates to
`/tickets/${ticketId}`; modify the goto invocation in the navigate function to
use the absolute path string.
---
Nitpick comments:
In `@backend/apps/tickets/views.py`:
- Around line 168-171: The current query that builds `tickets` using
Ticket.objects.select_related(...).prefetch_related(...) returns all tickets and
should be paginated to avoid huge payloads; update the view handling this logic
(the function or class-based view in backend/apps/tickets/views.py that defines
`tickets`) to accept pagination parameters (e.g., `limit` and `offset` or a
cursor) from the request, validate defaults/max limits, apply slicing to the
queryset (e.g., `.order_by(...)` then `[offset:offset+limit]` or implement
Django Paginator/cursor logic), and return paginated metadata (total/count,
next/prev offsets or cursor) along with the page of `tickets` while keeping the
existing `select_related`/`prefetch_related` (refer to the `tickets` variable,
`status_history_qs`, and `comments_qs`) to ensure related objects are still
fetched efficiently.
- Around line 160-240: The admin_ticket_history function duplicates the
event-building and mapping logic from ticket_history; extract that shared logic
into two helpers (e.g., _build_ticket_history_events(tickets) to collect
created/status/comment events and _events_to_schema(events) to sort and map to
TicketHistoryItemSchema) and have both endpoints call these helpers; keep
admin_ticket_history's staff check (return 403 if not staff) and only use a
different ticket queryset there if intentional, otherwise simply call the same
ticket retrieval used by ticket_history to avoid redundancy.
In `@frontend/src/components/ui/history/AdminHistoryTimeline.svelte`:
- Line 56: The each block declares an unused loop index; in the Svelte template
where the loop is written as iterating over items with "item" and "index" (using
the key item.id), remove the unused "index" identifier so the loop only declares
"item" with the same key (keep items, item and item.id intact) to clean up the
code and eliminate the unused variable.
In `@frontend/src/components/ui/history/TicketHistoryView.svelte`:
- Around line 111-165: The admin view duplicates most of the student view in
TicketHistoryView.svelte; extract the shared layout (filters,
loading/error/summary, scroll container) into a new reusable component (e.g.,
HistoryLayout or HistoryContainer) that accepts props/slots for title,
description and the timeline component, and forward/bind the reactive state
(activeFilter, sortBy, searchQuery, totalCount/historyItems, loading, error,
filteredItems) and the clearFilters handler; then replace the duplicated admin
block with the new component passing title="Admin History", the admin
description, and AdminHistoryTimeline (while student continues to pass
StudentHistoryTimeline) so AdminHistoryTimeline and StudentHistoryTimeline
remain the only differing pieces.
In `@frontend/src/lib/api/history.ts`:
- Around line 19-33: Both fetchTicketHistory and fetchAdminTicketHistory
duplicate the same fetch/error/json logic; extract a small helper (e.g.,
fetchJson or fetchWithAuth) that accepts the endpoint path and optional
permission-specific error messages, uses BASE, method "GET", credentials:
"include", and Accept: "application/json", checks res.ok and maps 401/403 to the
same specific Error messages, calls res.json() and returns (data ?? []) as
HistoryItem[]; then replace both fetchTicketHistory and fetchAdminTicketHistory
to call this helper with "/history" and "/admin/history" (preserving the exact
error texts currently used).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/apps/tickets/schemas.pybackend/apps/tickets/views.pyfrontend/src/components/ui/admin/AdminHistory.sveltefrontend/src/components/ui/history/AdminHistoryTimeline.sveltefrontend/src/components/ui/history/TicketHistoryView.sveltefrontend/src/lib/api/history.tsfrontend/src/types/history.ts
Comment on lines
+160
to
+164
| @router.get("/admin/history", response={200: list[TicketHistoryItemSchema], 403: dict}) | ||
| def admin_ticket_history(request): | ||
| """Admin-only endpoint to view all ticket history for accountability tracking.""" | ||
| if not request.user.is_staff: | ||
| return {"detail": "Not authorized."}, 403 |
Contributor
There was a problem hiding this comment.
Response format issue for 403 error.
The 403 response should return a tuple (403, {"detail": "..."}) to match the declared response signature {200: list[...], 403: dict} in django-ninja.
🐛 Proposed fix
def admin_ticket_history(request):
"""Admin-only endpoint to view all ticket history for accountability tracking."""
if not request.user.is_staff:
- return {"detail": "Not authorized."}, 403
+ return 403, {"detail": "Not authorized."}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @router.get("/admin/history", response={200: list[TicketHistoryItemSchema], 403: dict}) | |
| def admin_ticket_history(request): | |
| """Admin-only endpoint to view all ticket history for accountability tracking.""" | |
| if not request.user.is_staff: | |
| return {"detail": "Not authorized."}, 403 | |
| `@router.get`("/admin/history", response={200: list[TicketHistoryItemSchema], 403: dict}) | |
| def admin_ticket_history(request): | |
| """Admin-only endpoint to view all ticket history for accountability tracking.""" | |
| if not request.user.is_staff: | |
| return 403, {"detail": "Not authorized."} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/apps/tickets/views.py` around lines 160 - 164, The
admin_ticket_history endpoint currently returns a dict for unauthorized access
but must return a (status_code, body) tuple to match the declared response map;
update the unauthorized branch in admin_ticket_history to return a tuple with
the status first and the dict second (e.g., (403, {"detail": "Not
authorized."})) so it matches the response signature {200:
list[TicketHistoryItemSchema], 403: dict}.
Comment on lines
+17
to
+19
| function navigate(ticketId: string) { | ||
| goto(`tickets/${ticketId}`); | ||
| } |
Contributor
There was a problem hiding this comment.
Navigation path should be absolute.
The path tickets/${ticketId} is relative, which may cause incorrect navigation depending on the current route context. Use an absolute path for consistent behavior.
🐛 Proposed fix
function navigate(ticketId: string) {
- goto(`tickets/${ticketId}`);
+ goto(`/tickets/${ticketId}`);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function navigate(ticketId: string) { | |
| goto(`tickets/${ticketId}`); | |
| } | |
| function navigate(ticketId: string) { | |
| goto(`/tickets/${ticketId}`); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/ui/history/AdminHistoryTimeline.svelte` around lines
17 - 19, The navigate function uses a relative path which can lead to incorrect
routing; update the navigate(ticketId: string) function to call goto with an
absolute path (prepend a leading slash) so it always navigates to
`/tickets/${ticketId}`; modify the goto invocation in the navigate function to
use the absolute path string.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
backend/apps/tickets/views.py (1)
167-168:⚠️ Potential issue | 🔴 CriticalResponse tuple order is incorrect.
The 403 response returns
(body, status)but django-ninja expects(status, body)format. This will cause incorrect HTTP responses.🐛 Proposed fix
if not request.user.is_staff: - return {"detail": "Not authorized."}, 403 + return 403, {"detail": "Not authorized."}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/tickets/views.py` around lines 167 - 168, The authorization early-return in backend.apps.tickets.views (the block that checks request.user.is_staff) returns (body, status) which is backwards for django-ninja; change the return to the expected (status, body) order—e.g. return 403, {"detail": "Not authorized."}—so the HTTP status and body are sent correctly from that staff-check branch.
🧹 Nitpick comments (2)
backend/apps/tickets/views.py (2)
98-106: Consider including the ticket creator for "created" events.For complete accountability tracking, the "created" event could include the student who submitted the ticket rather than
None. Theticket.studentis already available.♻️ Suggested enhancement
events.append({ "at": ticket.created_at, "id": f"created-{ticket.id}-{ticket.created_at.isoformat()}", "ticket": ticket, "action": "created", "description": "Ticket created", "event_status": "pending", - "performed_by": None, + "performed_by": ticket.student.full_name if ticket.student else None, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/tickets/views.py` around lines 98 - 106, The "created" event currently sets "performed_by" to None; change it to use the ticket submitter by setting "performed_by" to ticket.student in the events.append dict for the created event so the events list records who created the ticket (locate the events.append block that builds the "created" event and update the "performed_by" field).
164-244: Significant code duplication withticket_history.Lines 170-244 duplicate ~75 lines from
ticket_history(lines 84-162). Only the authorization check and ticket queryset differ. This violates DRY and increases maintenance burden.Consider extracting the event collection and schema mapping into a shared helper.
♻️ Suggested refactor
def _build_history_events(tickets): """Build history events list from tickets queryset.""" events = [] for ticket in tickets: events.append({ "at": ticket.created_at, "id": f"created-{ticket.id}-{ticket.created_at.isoformat()}", "ticket": ticket, "action": "created", "description": "Ticket created", "event_status": "pending", "performed_by": ticket.student.full_name if ticket.student else None, }) for h in ticket.status_history.all(): action = history_action_for_status_change(h.old_status, h.new_status) events.append({ "at": h.changed_at, "id": f"status-{h.id}", "ticket": ticket, "action": action, "new_status": h.new_status, "description": ( f"Status changed from {map_status_for_history(h.old_status)} " f"to {map_status_for_history(h.new_status)}" ), "event_status": h.new_status, "performed_by": h.changed_by.full_name if h.changed_by else None, }) for c in ticket.comments.all(): events.append({ "at": c.created_at, "id": f"comment-{c.id}", "ticket": ticket, "action": "commented", "description": f"Comment: {c.message[:100]}{'…' if len(c.message) > 100 else ''}", "performed_by": c.user.full_name if c.user else None, }) return events def _events_to_schema(events): """Convert events to TicketHistoryItemSchema list.""" events.sort(key=lambda e: e["at"], reverse=True) status_change_actions = ("updated", "resolved", "closed", "reopened") out = [] for e in events: t = e["ticket"] priority_name = t.priority.name if t.priority else "" category_name = t.category.name if t.category else None event_status = e.get("event_status", t.status) if e["action"] in status_change_actions: event_status = map_status_for_history(e["new_status"]) elif e["action"] == "created": event_status = map_status_for_history("pending") else: event_status = map_status_for_history(t.status) out.append(TicketHistoryItemSchema( id=e["id"], ticketPk=t.id, ticketId=t.ticket_number, title=t.title, action=e["action"], description=e["description"], timestamp=format_timestamp(e["at"]), date=format_date(e["at"]), status=map_status_for_history(event_status), priority=map_priority_for_history(priority_name), category=category_name, performedBy=e.get("performed_by"), )) return outThen both endpoints become:
`@router.get`("/history", response=list[TicketHistoryItemSchema]) def ticket_history(request): # ... prefetch setup ... tickets = base.all() if request.user.is_staff else base.filter(student=request.user) events = _build_history_events(tickets) return _events_to_schema(events) `@router.get`("/admin/history", response={200: list[TicketHistoryItemSchema], 403: dict}) def admin_ticket_history(request): if not request.user.is_staff: return 403, {"detail": "Not authorized."} # ... prefetch setup ... events = _build_history_events(tickets) return 200, _events_to_schema(events)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/tickets/views.py` around lines 164 - 244, The admin_ticket_history function duplicates the event collection and schema-mapping logic from ticket_history; extract that logic into two helpers (e.g., _build_history_events(tickets) to collect created/status/comment events and _events_to_schema(events) to sort and convert to TicketHistoryItemSchema), update admin_ticket_history to call these helpers, and ensure you preserve existing details (use ticket.student.full_name for created performed_by when present, keep history_action_for_status_change/map_status_for_history/map_priority_for_history/format_timestamp/format_date usage and the same event id formats), and return the same response shape (403 error must remain returned when not request.user.is_staff).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@backend/apps/tickets/views.py`:
- Around line 167-168: The authorization early-return in
backend.apps.tickets.views (the block that checks request.user.is_staff) returns
(body, status) which is backwards for django-ninja; change the return to the
expected (status, body) order—e.g. return 403, {"detail": "Not authorized."}—so
the HTTP status and body are sent correctly from that staff-check branch.
---
Nitpick comments:
In `@backend/apps/tickets/views.py`:
- Around line 98-106: The "created" event currently sets "performed_by" to None;
change it to use the ticket submitter by setting "performed_by" to
ticket.student in the events.append dict for the created event so the events
list records who created the ticket (locate the events.append block that builds
the "created" event and update the "performed_by" field).
- Around line 164-244: The admin_ticket_history function duplicates the event
collection and schema-mapping logic from ticket_history; extract that logic into
two helpers (e.g., _build_history_events(tickets) to collect
created/status/comment events and _events_to_schema(events) to sort and convert
to TicketHistoryItemSchema), update admin_ticket_history to call these helpers,
and ensure you preserve existing details (use ticket.student.full_name for
created performed_by when present, keep
history_action_for_status_change/map_status_for_history/map_priority_for_history/format_timestamp/format_date
usage and the same event id formats), and return the same response shape (403
error must remain returned when not request.user.is_staff).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/apps/tickets/schemas.pybackend/apps/tickets/views.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/apps/tickets/schemas.py
Polqt
approved these changes
Feb 28, 2026
| status: Literal["pending", "in-progress", "resolved", "closed"] | ||
| priority: Literal["low", "medium", "high"] | ||
| category: str | None = None | ||
| performedBy: str | None = None |
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
2 participants
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.
, created AdminHistoryTimeline
Summary by Sourcery
Add an admin-focused ticket history view that surfaces who performed each action for accountability tracking.
New Features:
Enhancements:
Summary by CodeRabbit