ENG-2647: Privacy request detail — Jira ticket section#7742
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
There was a problem hiding this comment.
Overall this is clean, well-structured frontend work. The component decomposition (RequestJiraTickets → JiraTicketRow + LinkJiraTicketModal), per-ticket loading state tracking via originalArgs, and the RTK Query slice pattern all look solid. A few things to address:
Suggestions (worth fixing before merge):
ticket_url: stringintypes.tsshould bestring | null— it's used with|| undefinedin the component, indicating the API can return an empty/falsy value.- The
getButtonVisibilityparameterized test inhelpers.test.tsdoesn't include a case for the newly addedPENDING_EXTERNALstatus. It should. - The permanently disabled "Force close" button with a "not yet available" tooltip should be removed until the feature is ready — it's confusing as dead UI.
Nice to have:
- A light format validation in
LinkJiraTicketModal(e.g./^[A-Z]+-\d+$/) would improve UX since the submit button currently only checks for non-empty input. - The
instance_id/instance_statusgap noted in the PR description warrants a guard: ifinstance_idis null, the refresh URL would be.../jira-tickets/null/refresh. A null check before calling either mutation would prevent a confusing API error. - No component-level tests for
RequestJiraTicketsorLinkJiraTicketModalare included. Given that tests exist for other parts of this feature area, adding at minimum a smoke test for the happy path rendering and the link modal submit flow would be consistent with the codebase.
| instance_id: string; | ||
| instance_status: string | null; | ||
| ticket_key: string; | ||
| ticket_url: string; |
There was a problem hiding this comment.
ticket_url is typed as string (non-nullable), but in RequestJiraTickets.tsx it's used as ticket.ticket_url || undefined, which implies it can be falsy (empty string or null from the API). This type should be string | null to match the actual backend response and the defensive usage in the component — otherwise TypeScript won't catch cases where ticket_url is unexpectedly empty.
There was a problem hiding this comment.
Done — updated ticket_url to string | null to match the defensive usage in the component.
| </Button> | ||
| <Tooltip title="Force close not yet available"> | ||
| <Button danger disabled data-testid="force-close-btn"> | ||
| Force close |
There was a problem hiding this comment.
The "Force close" button is permanently disabled with a placeholder tooltip. Shipping disabled UI with "not yet available" messaging creates a confusing experience — users will see a dead button with no way to understand when or if it will ever work. Consider removing this button entirely and adding it back when the feature is ready, rather than keeping it as a non-functional placeholder.
| icon={<Icons.Undo />} | ||
| onClick={onRetry} | ||
| loading={isRetrying} | ||
| disabled={ticket.instance_status !== "failed" || isRefreshing} |
There was a problem hiding this comment.
The retry button is disabled when instance_status !== "failed". The PR description notes that instance_id is not yet populated by the backend, which means instance_status will also be null for all existing tickets — so retry will always be disabled in practice. This is fine as a temporary guard, but worth making explicit with a comment so the next developer knows why the button appears inert. Also consider: if instance_id is missing/null, calling retry/refresh will still hit the API with a null instance_id in the URL (e.g. .../jira-tickets/null/retry). It may be worth guarding against a null instance_id more explicitly.
There was a problem hiding this comment.
Done — typed instance_id as string | null and added a null guard in the retry/refresh handlers.
| error as RTKErrorResult["error"], | ||
| "Failed to link Jira ticket.", | ||
| ), | ||
| ); |
There was a problem hiding this comment.
The submit button is only gated on !ticketKey.trim() (non-empty), but there's no format validation for the ticket key. A user could submit arbitrary strings like "hello" or "123" which would hit the backend and produce an error. Even a simple regex check (e.g. /^[A-Z]+-\d+$/) would give instant feedback and avoid a round-trip. This is a nice-to-have if validation also happens server-side, but worth considering for UX.
There was a problem hiding this comment.
Skipping for now — the backend will error if the key doesn't exist in Jira, so the user gets feedback either way. Can add client-side format validation in a follow-up if needed.
Greptile SummaryThis PR adds a Jira ticket section to the privacy request detail view, including a new Key findings:
Confidence Score: 4/5
Important Files Changed
Reviews (1): Last reviewed commit: "format" | Re-trigger Greptile |
clients/admin-ui/src/features/privacy-requests/jira-tickets/LinkJiraTicketModal.tsx
Show resolved
Hide resolved
clients/admin-ui/src/features/privacy-requests/jira-tickets/RequestJiraTickets.tsx
Outdated
Show resolved
Hide resolved
clients/admin-ui/src/features/privacy-requests/jira-tickets/RequestJiraTickets.tsx
Show resolved
Hide resolved
eastandwestwind
left a comment
There was a problem hiding this comment.
Small UX-facing things, nice work so far!
| data-testid="force-close-modal" | ||
| > | ||
| <div className="mb-2 text-sm text-gray-500"> | ||
| Force closing will complete all pending Jira gates for this request, |
There was a problem hiding this comment.
Nit on customer-facing messages like this one - I don't think it's obvious what is a Jira "gate" / "DSR runner"
There was a problem hiding this comment.
does this actually complete the JIRA tickets? e.g. move them to done?
There was a problem hiding this comment.
Good call-- yep, I've updated to use clearer language and specify that tickets won't be closed (which is the actual behavior).
| privacy_request_id: privacyRequestId, | ||
| reason: reason.trim() || null, | ||
| }).unwrap(); | ||
| message.success("Privacy request Jira gates force-closed successfully."); |
| message.error( | ||
| getErrorMessage( | ||
| error as RTKErrorResult["error"], | ||
| "Failed to force close Jira gates.", |
| return ( | ||
| <div className="mt-6"> | ||
| <div className="mb-4"> | ||
| <Title level={3}>Jira tickets</Title> |
There was a problem hiding this comment.
Will customers who have never set up the "Jira for Ticketing" integration see these jira-related components on every privacy request?
There was a problem hiding this comment.
Yeah, that's probably wrong. Adding a check for the integration itself is a bit awkward so for now I've just stuck this behind the alphaJiraIntegration flag.
Ticket ENG-2647
Description Of Changes
Adds a Jira ticket section to the privacy request detail view. Operators can see all linked Jira tickets, click through to the ticket in Jira, retry failed ticket creation, refresh ticket status, and link an existing ticket by key.
Code Changes
RequestJiraTicketscomponent displaying linked tickets with status badges and retry/refresh actionsLinkJiraTicketModalfor linking an existing Jira ticket by key (e.g.PRIV-123)Steps to Confirm
failedstate, click the retry icon — expect a success toast or an appropriate error messagePre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works