From f6a24ea4e1171fe3e0b34624ee379965aec7137a Mon Sep 17 00:00:00 2001 From: Michael Chihlas Date: Thu, 16 Apr 2026 21:39:21 +0000 Subject: [PATCH] fix(psa): resource assignment targets CW `owner`, status PATCH verifies apply MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previous `resources`-string PATCH was silently ignored by CW — the `resources` field is server-derived from the ticket's owner + schedule entries, not freely writable. Status PATCH could also silently no-op when a cross-board status id was sent. - add_resource: when the ticket is unassigned, set the `owner` MemberReference (the canonical writable primary-assignee field). If already owned by someone else, append the identifier to the `resources` co-assignee string best-effort. - remove_resource: clear `owner` (with remove→replace:null fallback) if the target is the current owner, otherwise strip from `resources`. - list_resources: merge owner + resources string, deduped by member id, so the UI reflects both single-owner and multi-resource assignments. - update_ticket_status: verify CW applied the status by comparing the response body's status.id — raises PSAError with a clear message when CW silently rejects the change (e.g., status invalid for ticket's board), instead of reporting spurious success. - Frontend: surface the backend error detail in the toast so users see the real reason instead of a generic "Failed to update" message. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../app/services/psa/connectwise/provider.py | 167 +++++++++++++----- .../tickets/detail/TicketDetailHeader.tsx | 8 +- .../tickets/detail/TicketResourceManager.tsx | 15 +- 3 files changed, 143 insertions(+), 47 deletions(-) diff --git a/backend/app/services/psa/connectwise/provider.py b/backend/app/services/psa/connectwise/provider.py index 7c50235d..d3f0eb5f 100644 --- a/backend/app/services/psa/connectwise/provider.py +++ b/backend/app/services/psa/connectwise/provider.py @@ -266,13 +266,30 @@ class ConnectWiseProvider(PSAProvider): async def update_ticket_status( self, ticket_id: str, status_id: int ) -> PSATicket: - """Update a CW ticket's status using JSON Patch format.""" + """Update a CW ticket's status using JSON Patch format. + + Verifies CW actually applied the change — CW silently returns 200 when + a status id is invalid for the ticket's board. We check the response + body's status.id matches what we sent, and raise PSAError if not. + """ patch_body = [ {"op": "replace", "path": "status", "value": {"id": status_id}} ] data = await self.client.patch( f"/service/tickets/{ticket_id}", json_body=patch_body ) + applied = (data.get("status") or {}) if isinstance(data, dict) else {} + applied_id = applied.get("id") + if applied_id != status_id: + logger.warning( + "CW status PATCH for ticket %s returned status id=%s instead of %s", + ticket_id, applied_id, status_id, + ) + raise PSAError( + f"ConnectWise did not apply status {status_id} " + f"(still {applied.get('name') or applied_id}). " + "The status may not be valid for this ticket's board." + ) return self._map_ticket(data) async def list_members(self) -> list[PSAMember]: @@ -631,58 +648,107 @@ class ConnectWiseProvider(PSAProvider): # ── Resource management ─────────────────────────────────────────── - async def _get_ticket_resource_identifiers(self, ticket_id: int) -> list[str]: - """Fetch the ticket's `resources` field (comma-separated identifiers).""" + async def _get_ticket_assignment(self, ticket_id: int) -> tuple[dict | None, str | None]: + """Fetch the ticket's current owner (MemberReference) and resources string. + + Returns (owner_dict, resources_string). owner_dict has keys like + {id, identifier, name} when set; None when unassigned. + """ data = await self.client.get( f"/service/tickets/{ticket_id}", - params={"fields": "id,resources"}, + params={"fields": "id,owner,resources"}, ) - raw = data.get("resources") if isinstance(data, dict) else None - if not raw: - return [] - return [p.strip() for p in str(raw).split(",") if p.strip()] + if not isinstance(data, dict): + return None, None + owner_raw = data.get("owner") + owner = owner_raw if isinstance(owner_raw, dict) and owner_raw.get("id") else None + resources = data.get("resources") + return owner, (str(resources) if resources else None) async def list_resources(self, ticket_id: int) -> list[PSAResource]: - """List members assigned to a CW ticket via the `resources` string field.""" - identifiers = await self._get_ticket_resource_identifiers(ticket_id) - if not identifiers: - return [] + """List members assigned to a CW ticket. + + CW exposes assignment in two places: the `owner` MemberReference (primary + assignee) and a derived `resources` string of identifiers. We merge both + so the UI shows everyone CW considers assigned. + """ + owner, resources_str = await self._get_ticket_assignment(ticket_id) members = await self.list_members() by_identifier = {m.identifier: m for m in members if m.identifier} + by_id = {str(m.id): m for m in members} + + seen_ids: set[str] = set() results: list[PSAResource] = [] - for ident in identifiers: - m = by_identifier.get(ident) + + if owner is not None: + owner_id = str(owner.get("id")) + m = by_id.get(owner_id) if m: results.append(PSAResource( member_id=int(m.id), member_name=m.name, member_identifier=m.identifier, )) + seen_ids.add(owner_id) else: - # Unknown identifier — surface it so the UI doesn't silently drop it results.append(PSAResource( - member_id=0, - member_name=ident, - member_identifier=ident, + member_id=int(owner.get("id") or 0), + member_name=str(owner.get("name") or ""), + member_identifier=str(owner.get("identifier") or ""), )) + seen_ids.add(owner_id) + + if resources_str: + for ident in (p.strip() for p in resources_str.split(",") if p.strip()): + m = by_identifier.get(ident) + if m and str(m.id) not in seen_ids: + results.append(PSAResource( + member_id=int(m.id), + member_name=m.name, + member_identifier=m.identifier, + )) + seen_ids.add(str(m.id)) + elif not m: + results.append(PSAResource( + member_id=0, + member_name=ident, + member_identifier=ident, + )) + return results async def add_resource(self, ticket_id: int, member_id: int) -> PSAResource: - """Assign a member to a CW ticket by updating the ticket's `resources` field.""" + """Assign a member to a CW ticket. + + CW's `resources` string is derived server-side and PATCHing it is unreliable. + The canonical writable field is `owner`. If the ticket is unassigned we set + the owner; if it already has a different owner, we append to `resources` + (best-effort — CW may not surface it in every view). + """ members = await self.list_members() target = next((m for m in members if str(m.id) == str(member_id)), None) - if target is None or not target.identifier: - raise PSAError(f"Member {member_id} not found or has no identifier") + if target is None: + raise PSAError(f"Member {member_id} not found") - current = await self._get_ticket_resource_identifiers(ticket_id) - if target.identifier not in current: - current.append(target.identifier) - new_value = ",".join(current) + current_owner, resources_str = await self._get_ticket_assignment(ticket_id) + + if current_owner is None: + # Primary assign — set owner + await self.client.patch( + f"/service/tickets/{ticket_id}", + json_body=[{"op": "replace", "path": "owner", "value": {"id": int(target.id)}}], + ) + elif str(current_owner.get("id")) != str(target.id): + # Ticket already owned by someone else — add to resources string as co-assignee. + identifiers = [p.strip() for p in (resources_str or "").split(",") if p.strip()] + if target.identifier and target.identifier not in identifiers: + identifiers.append(target.identifier) + await self.client.patch( + f"/service/tickets/{ticket_id}", + json_body=[{"op": "replace", "path": "resources", "value": ",".join(identifiers)}], + ) + # else: already the owner — idempotent no-op - await self.client.patch( - f"/service/tickets/{ticket_id}", - json_body=[{"op": "replace", "path": "resources", "value": new_value}], - ) return PSAResource( member_id=int(target.id), member_name=target.name, @@ -690,22 +756,41 @@ class ConnectWiseProvider(PSAProvider): ) async def remove_resource(self, ticket_id: int, member_id: int) -> None: - """Remove a member from a CW ticket by updating the `resources` field (idempotent).""" + """Remove a member from a CW ticket (idempotent). + + If the target is the current owner, clear the owner. Otherwise remove them + from the `resources` string. + """ members = await self.list_members() target = next((m for m in members if str(m.id) == str(member_id)), None) - if target is None or not target.identifier: - return # Unknown member — treat as idempotent + if target is None: + return - current = await self._get_ticket_resource_identifiers(ticket_id) - if target.identifier not in current: - return # Already not assigned — idempotent - new_list = [i for i in current if i != target.identifier] - new_value = ",".join(new_list) + current_owner, resources_str = await self._get_ticket_assignment(ticket_id) - await self.client.patch( - f"/service/tickets/{ticket_id}", - json_body=[{"op": "replace", "path": "resources", "value": new_value}], - ) + if current_owner is not None and str(current_owner.get("id")) == str(target.id): + # Unassign the owner. Try RFC 6902 "remove" first; fall back to + # "replace" with null if CW rejects it. + try: + await self.client.patch( + f"/service/tickets/{ticket_id}", + json_body=[{"op": "remove", "path": "owner"}], + ) + except PSAError: + await self.client.patch( + f"/service/tickets/{ticket_id}", + json_body=[{"op": "replace", "path": "owner", "value": None}], + ) + return + + if target.identifier and resources_str: + identifiers = [p.strip() for p in resources_str.split(",") if p.strip()] + if target.identifier in identifiers: + new_list = [i for i in identifiers if i != target.identifier] + await self.client.patch( + f"/service/tickets/{ticket_id}", + json_body=[{"op": "replace", "path": "resources", "value": ",".join(new_list)}], + ) # ── Ticket creation ─────────────────────────────────────────────── diff --git a/frontend/src/components/tickets/detail/TicketDetailHeader.tsx b/frontend/src/components/tickets/detail/TicketDetailHeader.tsx index 0916dff0..30f4e878 100644 --- a/frontend/src/components/tickets/detail/TicketDetailHeader.tsx +++ b/frontend/src/components/tickets/detail/TicketDetailHeader.tsx @@ -1,4 +1,5 @@ import { useState } from 'react' +import axios from 'axios' import { ticketsApi } from '@/api/tickets' import { toast } from '@/lib/toast' import type { PSATicketSearchResult, PSATicketStatusItem } from '@/types/integrations' @@ -22,8 +23,11 @@ export function TicketDetailHeader({ ticket, currentStatusId, currentStatusName, const result: PSATicketStatusUpdate = await ticketsApi.updateStatus(Number(ticket.id), statusId) onStatusUpdated(result.ticket_id, result.new_status, result.new_status_id) toast.success(`Status updated to ${result.new_status}`) - } catch { - toast.error('Failed to update status') + } catch (err) { + const detail = axios.isAxiosError(err) + ? (err.response?.data as { detail?: string })?.detail + : undefined + toast.error(detail || 'Failed to update status') } finally { setUpdating(false) } diff --git a/frontend/src/components/tickets/detail/TicketResourceManager.tsx b/frontend/src/components/tickets/detail/TicketResourceManager.tsx index b68bdde7..6ff073e2 100644 --- a/frontend/src/components/tickets/detail/TicketResourceManager.tsx +++ b/frontend/src/components/tickets/detail/TicketResourceManager.tsx @@ -1,4 +1,5 @@ import { useState } from 'react' +import axios from 'axios' import { UserPlus, X, User } from 'lucide-react' import { ticketsApi } from '@/api/tickets' import { toast } from '@/lib/toast' @@ -27,8 +28,11 @@ export function TicketResourceManager({ ticketId, resources, allMembers, onChang setAdding(false) setSelectedMemberId('') onChanged() - } catch { - toast.error('Failed to add resource') + } catch (err) { + const detail = axios.isAxiosError(err) + ? (err.response?.data as { detail?: string })?.detail + : undefined + toast.error(detail || 'Failed to add resource') } finally { setBusy(null) } @@ -40,8 +44,11 @@ export function TicketResourceManager({ ticketId, resources, allMembers, onChang await ticketsApi.removeResource(ticketId, memberId) toast.success('Resource removed') onChanged() - } catch { - toast.error('Failed to remove resource') + } catch (err) { + const detail = axios.isAxiosError(err) + ? (err.response?.data as { detail?: string })?.detail + : undefined + toast.error(detail || 'Failed to remove resource') } finally { setBusy(null) }