From cddb3fde624c94f7c1b2d01fa3f71a47bd1c72d0 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Fri, 30 Jan 2026 12:59:56 -0500 Subject: [PATCH 1/5] CS-105 Allow to remove the duplicated device on Employee Device Tab (#2079) * feat(app): add 'Remove Device' menu on Employ Devices tab * feat(api): create delete endpoint to remove single host from fleet * feat(app): integrate delete endpoint to remove the host * fix(api): missing host ownership validation allows cross-organization deletion * fix(app): redundant null check after early return guard on handleRemoveDeviceClick * fix(api): duplicated host deletion logic not consolidated on fleet.service * fix(app): remove unnecessary accessorKey on actions column * fix(app): remove duplicated database query for current user member --------- Co-authored-by: chasprowebdev --- apps/api/src/lib/fleet.service.ts | 22 +++- apps/api/src/people/people.controller.ts | 38 ++++++ apps/api/src/people/people.service.ts | 57 +++++++++ .../src/people/schemas/people-operations.ts | 5 + apps/api/src/people/schemas/people-params.ts | 5 + .../people/schemas/remove-host.responses.ts | 74 +++++++++++ .../people/all/components/MemberRow.tsx | 8 +- .../all/components/RemoveDeviceAlert.tsx | 12 +- .../people/all/components/TeamMembers.tsx | 24 ++-- .../devices/components/DeviceDropdownMenu.tsx | 80 ++++++++++++ .../components/EmployeeDevicesColumns.tsx | 12 +- .../components/EmployeeDevicesList.tsx | 9 +- .../app/src/app/(app)/[orgId]/people/page.tsx | 23 +++- apps/app/src/hooks/use-people-api.ts | 14 ++ packages/docs/openapi.json | 121 ++++++++++++++++++ 15 files changed, 473 insertions(+), 31 deletions(-) create mode 100644 apps/api/src/people/schemas/remove-host.responses.ts create mode 100644 apps/app/src/app/(app)/[orgId]/people/devices/components/DeviceDropdownMenu.tsx diff --git a/apps/api/src/lib/fleet.service.ts b/apps/api/src/lib/fleet.service.ts index f0888f78e..f23c8245e 100644 --- a/apps/api/src/lib/fleet.service.ts +++ b/apps/api/src/lib/fleet.service.ts @@ -67,6 +67,20 @@ export class FleetService { } } + /** + * Remove a single host from FleetDM by ID + * @param hostId - The FleetDM host ID + */ + async removeHostById(hostId: number): Promise { + try { + await this.fleetInstance.delete(`/hosts/${hostId}`); + this.logger.debug(`Deleted host ${hostId} from FleetDM`); + } catch (error) { + this.logger.error(`Failed to delete host ${hostId}:`, error); + throw new Error(`Failed to remove host ${hostId}`); + } + } + async getMultipleHosts(hostIds: number[]) { try { const requests = hostIds.map((id) => this.getHostById(id)); @@ -104,14 +118,12 @@ export class FleetService { // Extract host IDs const hostIds = labelHosts.hosts.map((host: { id: number }) => host.id); - // Delete each host + // Delete each host using removeHostById for consistent behavior const deletePromises = hostIds.map(async (hostId: number) => { try { - await this.fleetInstance.delete(`/hosts/${hostId}`); - this.logger.debug(`Deleted host ${hostId} from FleetDM`); + await this.removeHostById(hostId); return { success: true, hostId }; - } catch (error) { - this.logger.error(`Failed to delete host ${hostId}:`, error); + } catch { return { success: false, hostId }; } }); diff --git a/apps/api/src/people/people.controller.ts b/apps/api/src/people/people.controller.ts index 89c9bc3e7..8be17cb61 100644 --- a/apps/api/src/people/people.controller.ts +++ b/apps/api/src/people/people.controller.ts @@ -6,6 +6,7 @@ import { Delete, Body, Param, + ParseIntPipe, UseGuards, HttpCode, HttpStatus, @@ -22,6 +23,7 @@ import { } from '@nestjs/swagger'; import { AuthContext, OrganizationId } from '../auth/auth-context.decorator'; import { HybridAuthGuard } from '../auth/hybrid-auth.guard'; +import { RequireRoles } from '../auth/role-validator.guard'; import type { AuthContext as AuthContextType } from '../auth/types'; import { CreatePeopleDto } from './dto/create-people.dto'; import { UpdatePeopleDto } from './dto/update-people.dto'; @@ -34,6 +36,7 @@ import { BULK_CREATE_MEMBERS_RESPONSES } from './schemas/bulk-create-members.res import { GET_PERSON_BY_ID_RESPONSES } from './schemas/get-person-by-id.responses'; import { UPDATE_MEMBER_RESPONSES } from './schemas/update-member.responses'; import { DELETE_MEMBER_RESPONSES } from './schemas/delete-member.responses'; +import { REMOVE_HOST_RESPONSES } from './schemas/remove-host.responses'; import { PEOPLE_OPERATIONS } from './schemas/people-operations'; import { PEOPLE_PARAMS } from './schemas/people-params'; import { PEOPLE_BODIES } from './schemas/people-bodies'; @@ -199,6 +202,41 @@ export class PeopleController { }; } + @Delete(':id/host/:hostId') + @HttpCode(HttpStatus.OK) + @UseGuards(RequireRoles('owner')) + @ApiOperation(PEOPLE_OPERATIONS.removeHost) + @ApiParam(PEOPLE_PARAMS.memberId) + @ApiParam(PEOPLE_PARAMS.hostId) + @ApiResponse(REMOVE_HOST_RESPONSES[200]) + @ApiResponse(REMOVE_HOST_RESPONSES[401]) + @ApiResponse(REMOVE_HOST_RESPONSES[404]) + @ApiResponse(REMOVE_HOST_RESPONSES[500]) + async removeHost( + @Param('id') memberId: string, + @Param('hostId', ParseIntPipe) hostId: number, + @OrganizationId() organizationId: string, + @AuthContext() authContext: AuthContextType, + ) { + const result = await this.peopleService.removeHostById( + memberId, + organizationId, + hostId, + ); + + return { + ...result, + authType: authContext.authType, + ...(authContext.userId && + authContext.userEmail && { + authenticatedUser: { + id: authContext.userId, + email: authContext.userEmail, + }, + }), + }; + } + @Delete(':id') @ApiOperation(PEOPLE_OPERATIONS.deleteMember) @ApiParam(PEOPLE_PARAMS.memberId) diff --git a/apps/api/src/people/people.service.ts b/apps/api/src/people/people.service.ts index 11646475a..19728361e 100644 --- a/apps/api/src/people/people.service.ts +++ b/apps/api/src/people/people.service.ts @@ -338,4 +338,61 @@ export class PeopleService { throw new Error(`Failed to unlink device: ${error.message}`); } } + + async removeHostById( + memberId: string, + organizationId: string, + hostId: number, + ): Promise<{ success: true }> { + try { + await MemberValidator.validateOrganization(organizationId); + const member = await MemberQueries.findByIdInOrganization( + memberId, + organizationId, + ); + + if (!member) { + throw new NotFoundException( + `Member with ID ${memberId} not found in organization ${organizationId}`, + ); + } + + if (!member.fleetDmLabelId) { + throw new BadRequestException( + `Member ${memberId} has no Fleet label; cannot remove host`, + ); + } + + const labelHosts = await this.fleetService.getHostsByLabel( + member.fleetDmLabelId, + ); + const hostIds = (labelHosts?.hosts ?? []).map( + (host: { id: number }) => host.id, + ); + if (!hostIds.includes(hostId)) { + throw new NotFoundException( + `Host ${hostId} not found for member ${memberId} in organization ${organizationId}`, + ); + } + + await this.fleetService.removeHostById(hostId); + + this.logger.log( + `Removed host ${hostId} from FleetDM for member ${memberId} in organization ${organizationId}`, + ); + return { success: true }; + } catch (error) { + if ( + error instanceof NotFoundException || + error instanceof BadRequestException + ) { + throw error; + } + this.logger.error( + `Failed to remove host ${hostId} for member ${memberId} in organization ${organizationId}:`, + error, + ); + throw new Error(`Failed to remove host: ${error.message}`); + } + } } diff --git a/apps/api/src/people/schemas/people-operations.ts b/apps/api/src/people/schemas/people-operations.ts index 5116ba927..f1d700472 100644 --- a/apps/api/src/people/schemas/people-operations.ts +++ b/apps/api/src/people/schemas/people-operations.ts @@ -36,4 +36,9 @@ export const PEOPLE_OPERATIONS: Record = { description: 'Resets the fleetDmLabelId for a member, effectively unlinking their device from FleetDM. This will disconnect the device from the organization. Supports both API key authentication (X-API-Key header) and session authentication (cookies + X-Organization-Id header).', }, + removeHost: { + summary: 'Remove host (device) from Fleet', + description: + 'Removes a single host (device) from FleetDM by host ID. Only organization owners can perform this action. Validates that the organization exists and the member exists within the organization. Supports both API key authentication (X-API-Key header) and session authentication (cookies + X-Organization-Id header).', + }, }; diff --git a/apps/api/src/people/schemas/people-params.ts b/apps/api/src/people/schemas/people-params.ts index 5c8124d46..387f27fc7 100644 --- a/apps/api/src/people/schemas/people-params.ts +++ b/apps/api/src/people/schemas/people-params.ts @@ -6,4 +6,9 @@ export const PEOPLE_PARAMS: Record = { description: 'Member ID', example: 'mem_abc123def456', }, + hostId: { + name: 'hostId', + description: 'FleetDM host ID', + example: 1, + }, }; diff --git a/apps/api/src/people/schemas/remove-host.responses.ts b/apps/api/src/people/schemas/remove-host.responses.ts new file mode 100644 index 000000000..18af92943 --- /dev/null +++ b/apps/api/src/people/schemas/remove-host.responses.ts @@ -0,0 +1,74 @@ +import { ApiResponseOptions } from '@nestjs/swagger'; + +export const REMOVE_HOST_RESPONSES: Record = { + 200: { + status: 200, + description: 'Host removed from Fleet successfully', + content: { + 'application/json': { + schema: { + type: 'object', + properties: { + success: { + type: 'boolean', + description: 'Indicates successful removal', + example: true, + }, + authType: { + type: 'string', + enum: ['api-key', 'session'], + description: 'How the request was authenticated', + }, + }, + }, + }, + }, + }, + 401: { + status: 401, + description: + 'Unauthorized - Invalid authentication, insufficient permissions, or not organization owner', + content: { + 'application/json': { + schema: { + type: 'object', + properties: { + message: { type: 'string', example: 'Unauthorized' }, + }, + }, + }, + }, + }, + 404: { + status: 404, + description: 'Organization or member not found', + content: { + 'application/json': { + schema: { + type: 'object', + properties: { + message: { + type: 'string', + example: + 'Member with ID mem_abc123def456 not found in organization org_abc123def456', + }, + }, + }, + }, + }, + }, + 500: { + status: 500, + description: 'Internal server error', + content: { + 'application/json': { + schema: { + type: 'object', + properties: { + message: { type: 'string', example: 'Failed to remove host' }, + }, + }, + }, + }, + }, +}; diff --git a/apps/app/src/app/(app)/[orgId]/people/all/components/MemberRow.tsx b/apps/app/src/app/(app)/[orgId]/people/all/components/MemberRow.tsx index a4643cd52..3bc4ab5df 100644 --- a/apps/app/src/app/(app)/[orgId]/people/all/components/MemberRow.tsx +++ b/apps/app/src/app/(app)/[orgId]/people/all/components/MemberRow.tsx @@ -269,8 +269,14 @@ export function MemberRow({ /> + {'Are you sure you want to remove all devices for this user '} {memberName}?{' '} + {'This will disconnect all devices from the organization.'} + + )} onOpenChange={setIsRemoveDeviceAlertOpen} - memberName={memberName} onRemove={handleRemoveDeviceClick} isRemoving={isRemovingDevice} /> diff --git a/apps/app/src/app/(app)/[orgId]/people/all/components/RemoveDeviceAlert.tsx b/apps/app/src/app/(app)/[orgId]/people/all/components/RemoveDeviceAlert.tsx index f02f22524..df9850607 100644 --- a/apps/app/src/app/(app)/[orgId]/people/all/components/RemoveDeviceAlert.tsx +++ b/apps/app/src/app/(app)/[orgId]/people/all/components/RemoveDeviceAlert.tsx @@ -10,19 +10,22 @@ import { AlertDialogTitle, } from '@comp/ui/alert-dialog'; import { Button } from '@comp/ui/button'; +import { ReactNode } from 'react'; interface RemoveDeviceAlertProps { open: boolean; + title: string; + description: ReactNode; onOpenChange: (open: boolean) => void; - memberName: string; onRemove: () => void; isRemoving: boolean; } export function RemoveDeviceAlert({ open, + title, + description, onOpenChange, - memberName, onRemove, isRemoving, }: RemoveDeviceAlertProps) { @@ -30,10 +33,9 @@ export function RemoveDeviceAlert({ - {'Remove Device'} + {title} - {'Are you sure you want to remove the device for this user '} {memberName}?{' '} - {'This will disconnect the device from the organization.'} + {description} diff --git a/apps/app/src/app/(app)/[orgId]/people/all/components/TeamMembers.tsx b/apps/app/src/app/(app)/[orgId]/people/all/components/TeamMembers.tsx index 533644ea3..c04899ab9 100644 --- a/apps/app/src/app/(app)/[orgId]/people/all/components/TeamMembers.tsx +++ b/apps/app/src/app/(app)/[orgId]/people/all/components/TeamMembers.tsx @@ -18,7 +18,15 @@ export interface TeamMembersData { pendingInvitations: Invitation[]; } -export async function TeamMembers() { +export interface TeamMembersProps { + canManageMembers: boolean; + canInviteUsers: boolean; + isAuditor: boolean; + isCurrentUserOwner: boolean; +} + +export async function TeamMembers(props: TeamMembersProps) { + const { canManageMembers, canInviteUsers, isAuditor, isCurrentUserOwner } = props; const session = await auth.api.getSession({ headers: await headers(), }); @@ -28,20 +36,6 @@ export async function TeamMembers() { return null; } - const currentUserMember = await db.member.findFirst({ - where: { - organizationId: organizationId, - userId: session?.user.id, - }, - }); - - // Parse roles from comma-separated string and check if user has admin or owner role - const currentUserRoles = currentUserMember?.role?.split(',').map((r) => r.trim()) ?? []; - const canManageMembers = currentUserRoles.some((role) => ['owner', 'admin'].includes(role)); - const isAuditor = currentUserRoles.includes('auditor'); - const canInviteUsers = canManageMembers || isAuditor; - const isCurrentUserOwner = currentUserRoles.includes('owner'); - let members: MemberWithUser[] = []; let pendingInvitations: Invitation[] = []; diff --git a/apps/app/src/app/(app)/[orgId]/people/devices/components/DeviceDropdownMenu.tsx b/apps/app/src/app/(app)/[orgId]/people/devices/components/DeviceDropdownMenu.tsx new file mode 100644 index 000000000..748a0c573 --- /dev/null +++ b/apps/app/src/app/(app)/[orgId]/people/devices/components/DeviceDropdownMenu.tsx @@ -0,0 +1,80 @@ +'use client'; + +import { Button } from '@comp/ui'; +import { + DropdownMenu, + DropdownMenuContent, + DropdownMenuItem, + DropdownMenuTrigger, +} from '@comp/ui/dropdown-menu'; +import { Laptop, MoreHorizontal } from 'lucide-react'; +import { Host } from '../types'; +import { RemoveDeviceAlert } from '../../all/components/RemoveDeviceAlert'; +import { useState } from 'react'; +import { toast } from 'sonner'; +import { usePeopleActions } from '@/hooks/use-people-api'; +import { useRouter } from 'next/navigation'; + +interface DeviceDropdownMenuProps { + host: Host; + isCurrentUserOwner: boolean; +} + +export const DeviceDropdownMenu = ({ host, isCurrentUserOwner }: DeviceDropdownMenuProps) => { + const router = useRouter(); + const [isRemoveDeviceAlertOpen, setIsRemoveDeviceAlertOpen] = useState(false); + const [isRemovingDevice, setIsRemovingDevice] = useState(false); + + const { removeHostFromFleet } = usePeopleActions(); + + if (!isCurrentUserOwner || !host.member_id) { + return null; + } + + const memberId = host.member_id; + + const handleRemoveDeviceClick = async () => { + try { + setIsRemovingDevice(true); + await removeHostFromFleet(memberId, host.id); + setIsRemoveDeviceAlertOpen(false); + toast.success('Device removed successfully'); + router.refresh(); // Revalidate data to update UI + } catch (error) { + toast.error(error instanceof Error ? error.message : 'Failed to remove device'); + } finally { + setIsRemovingDevice(false); + } + }; + + return ( +
e.stopPropagation()}> + + + + + + setIsRemoveDeviceAlertOpen(true)}> + + {'Remove Device'} + + + + + {'Are you sure you want to remove this device '} {host.computer_name}?{' '} + {'This will disconnect the device from the user.'} + + )} + onOpenChange={setIsRemoveDeviceAlertOpen} + onRemove={handleRemoveDeviceClick} + isRemoving={isRemovingDevice} + /> +
+ ); +}; \ No newline at end of file diff --git a/apps/app/src/app/(app)/[orgId]/people/devices/components/EmployeeDevicesColumns.tsx b/apps/app/src/app/(app)/[orgId]/people/devices/components/EmployeeDevicesColumns.tsx index 3388c5a4c..11ee1b9b0 100644 --- a/apps/app/src/app/(app)/[orgId]/people/devices/components/EmployeeDevicesColumns.tsx +++ b/apps/app/src/app/(app)/[orgId]/people/devices/components/EmployeeDevicesColumns.tsx @@ -6,6 +6,7 @@ import { CheckCircle2, XCircle } from 'lucide-react'; import Link from 'next/link'; import { useParams } from 'next/navigation'; import type { FleetPolicy, Host } from '../types'; +import { DeviceDropdownMenu } from './DeviceDropdownMenu'; function UserNameCell({ userName, memberId }: { userName: string | null | undefined; memberId: string | undefined }) { const params = useParams(); @@ -26,7 +27,7 @@ function UserNameCell({ userName, memberId }: { userName: string | null | undefi ); } -export function getEmployeeDevicesColumns(): ColumnDef[] { +export function getEmployeeDevicesColumns(isCurrentUserOwner: boolean): ColumnDef[] { return [ { id: 'computer_name', @@ -72,5 +73,14 @@ export function getEmployeeDevicesColumns(): ColumnDef[] { ); }, }, + { + id: 'actions', + header: ({ column }) => , + enableColumnFilter: false, + enableSorting: false, + cell: ({ row }) => ( + + ), + } ]; } diff --git a/apps/app/src/app/(app)/[orgId]/people/devices/components/EmployeeDevicesList.tsx b/apps/app/src/app/(app)/[orgId]/people/devices/components/EmployeeDevicesList.tsx index ad806fe08..6d70b7258 100644 --- a/apps/app/src/app/(app)/[orgId]/people/devices/components/EmployeeDevicesList.tsx +++ b/apps/app/src/app/(app)/[orgId]/people/devices/components/EmployeeDevicesList.tsx @@ -8,9 +8,14 @@ import type { Host } from '../types/index'; import { getEmployeeDevicesColumns } from './EmployeeDevicesColumns'; import { HostDetails } from './HostDetails'; -export const EmployeeDevicesList = ({ devices }: { devices: Host[] }) => { +export interface EmployeeDevicesListProps { + devices: Host[]; + isCurrentUserOwner: boolean; +} + +export const EmployeeDevicesList = ({ devices, isCurrentUserOwner }: EmployeeDevicesListProps) => { const [selectedRow, setSelectedRow] = useState(null); - const columns = useMemo(() => getEmployeeDevicesColumns(), []); + const columns = useMemo(() => getEmployeeDevicesColumns(isCurrentUserOwner), [isCurrentUserOwner]); const { table } = useDataTable({ data: devices, diff --git a/apps/app/src/app/(app)/[orgId]/people/page.tsx b/apps/app/src/app/(app)/[orgId]/people/page.tsx index b15f9784e..105ce94d6 100644 --- a/apps/app/src/app/(app)/[orgId]/people/page.tsx +++ b/apps/app/src/app/(app)/[orgId]/people/page.tsx @@ -22,6 +22,18 @@ export default async function PeoplePage({ params }: { params: Promise<{ orgId: return redirect('/'); } + const currentUserMember = await db.member.findFirst({ + where: { + organizationId: orgId, + userId: session.user.id, + }, + }); + const currentUserRoles = currentUserMember?.role?.split(',').map((r) => r.trim()) ?? []; + const canManageMembers = currentUserRoles.some((role) => ['owner', 'admin'].includes(role)); + const isAuditor = currentUserRoles.includes('auditor'); + const canInviteUsers = canManageMembers || isAuditor; + const isCurrentUserOwner = currentUserRoles.includes('owner'); + // Check if there are employees to show the Employee Tasks tab const allMembers = await db.member.findMany({ where: { @@ -49,12 +61,19 @@ export default async function PeoplePage({ params }: { params: Promise<{ orgId: return ( } + peopleContent={ + + } employeeTasksContent={showEmployeeTasks ? : null} devicesContent={ <> - + } showEmployeeTasks={showEmployeeTasks} diff --git a/apps/app/src/hooks/use-people-api.ts b/apps/app/src/hooks/use-people-api.ts index 374d168e2..1eb141737 100644 --- a/apps/app/src/hooks/use-people-api.ts +++ b/apps/app/src/hooks/use-people-api.ts @@ -43,7 +43,21 @@ export function usePeopleActions() { [api], ); + const removeHostFromFleet = useCallback( + async (memberId: string, hostId: number) => { + const response = await api.delete<{ success: boolean }>( + `/v1/people/${memberId}/host/${hostId}`, + ); + if (response.error) { + throw new Error(response.error); + } + return response.data!; + }, + [api], + ); + return { unlinkDevice, + removeHostFromFleet, }; } diff --git a/packages/docs/openapi.json b/packages/docs/openapi.json index ed754fa9c..48aa00b2b 100644 --- a/packages/docs/openapi.json +++ b/packages/docs/openapi.json @@ -1577,6 +1577,127 @@ ] } }, + "/v1/people/{id}/host/{hostId}": { + "delete": { + "description": "Removes a single host (device) from FleetDM by host ID. Only organization owners can perform this action. Validates that the organization exists and the member exists within the organization. Supports both API key authentication (X-API-Key header) and session authentication (cookies + X-Organization-Id header).", + "operationId": "PeopleController_removeHost_v1", + "parameters": [ + { + "name": "X-Organization-Id", + "in": "header", + "description": "Organization ID (required for session auth, optional for API key auth)", + "required": false, + "schema": { + "type": "string" + } + }, + { + "name": "id", + "required": true, + "in": "path", + "description": "Member ID", + "schema": { + "example": "mem_abc123def456", + "type": "string" + } + }, + { + "name": "hostId", + "required": true, + "in": "path", + "description": "FleetDM host ID", + "schema": { + "example": 1, + "type": "number" + } + } + ], + "responses": { + "200": { + "description": "Host removed from Fleet successfully", + "content": { + "application/json": { + "schema": { + "type": "object", + "properties": { + "success": { + "type": "boolean", + "description": "Indicates successful removal", + "example": true + }, + "authType": { + "type": "string", + "enum": [ + "api-key", + "session" + ], + "description": "How the request was authenticated" + } + } + } + } + } + }, + "401": { + "description": "Unauthorized - Invalid authentication, insufficient permissions, or not organization owner", + "content": { + "application/json": { + "schema": { + "type": "object", + "properties": { + "message": { + "type": "string", + "example": "Unauthorized" + } + } + } + } + } + }, + "404": { + "description": "Organization or member not found", + "content": { + "application/json": { + "schema": { + "type": "object", + "properties": { + "message": { + "type": "string", + "example": "Member with ID mem_abc123def456 not found in organization org_abc123def456" + } + } + } + } + } + }, + "500": { + "description": "Internal server error", + "content": { + "application/json": { + "schema": { + "type": "object", + "properties": { + "message": { + "type": "string", + "example": "Failed to remove host" + } + } + } + } + } + } + }, + "security": [ + { + "apikey": [] + } + ], + "summary": "Remove host (device) from Fleet", + "tags": [ + "People" + ] + } + }, "/v1/people/{id}/unlink-device": { "patch": { "description": "Resets the fleetDmLabelId for a member, effectively unlinking their device from FleetDM. This will disconnect the device from the organization. Supports both API key authentication (X-API-Key header) and session authentication (cookies + X-Organization-Id header).", From 8d29d8869be33f3069ebb9cf24b5f1ac859a1574 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Fri, 30 Jan 2026 21:27:00 -0500 Subject: [PATCH 2/5] [dev] [tofikwest] tofik/finding-improvements (#2084) * feat(api): add revision note handling for findings update * feat(app): update onStatusChange to support async handling in FindingItem --------- Co-authored-by: Tofik Hasanov --- .../src/findings/dto/update-finding.dto.ts | 12 + apps/api/src/findings/findings.service.ts | 54 ++++- .../components/FindingsOverview.tsx | 4 +- .../components/findings/FindingItem.tsx | 218 +++++++++++++++--- .../components/findings/FindingsList.tsx | 50 +++- apps/app/src/hooks/use-findings-api.ts | 2 + .../migration.sql | 2 + packages/db/prisma/schema/finding.prisma | 9 +- packages/docs/openapi.json | 7 + 9 files changed, 301 insertions(+), 57 deletions(-) create mode 100644 packages/db/prisma/migrations/20260131010939_add_revision_note_to_finding/migration.sql diff --git a/apps/api/src/findings/dto/update-finding.dto.ts b/apps/api/src/findings/dto/update-finding.dto.ts index 7c0ade1b7..16fc7ed2c 100644 --- a/apps/api/src/findings/dto/update-finding.dto.ts +++ b/apps/api/src/findings/dto/update-finding.dto.ts @@ -39,4 +39,16 @@ export class UpdateFindingDto { @IsNotEmpty({ message: 'Content cannot be empty if provided' }) @MaxLength(5000) content?: string; + + @ApiProperty({ + description: 'Auditor note when requesting revision (only for needs_revision status)', + example: 'Please provide clearer screenshots showing the timestamp.', + maxLength: 2000, + required: false, + nullable: true, + }) + @IsString() + @IsOptional() + @MaxLength(2000) + revisionNote?: string | null; } diff --git a/apps/api/src/findings/findings.service.ts b/apps/api/src/findings/findings.service.ts index 9b7d18e66..0ec3955fe 100644 --- a/apps/api/src/findings/findings.service.ts +++ b/apps/api/src/findings/findings.service.ts @@ -71,9 +71,7 @@ export class FindingsService { ], }); - this.logger.log( - `Retrieved ${findings.length} findings for task ${taskId}`, - ); + this.logger.log(`Retrieved ${findings.length} findings for task ${taskId}`); return findings; } @@ -311,19 +309,38 @@ export class FindingsService { } // ready_for_review can only be set by non-auditor admins/owners (client signals to auditor) - if (updateDto.status === FindingStatus.ready_for_review && isAuditor && !isPlatformAdmin) { + if ( + updateDto.status === FindingStatus.ready_for_review && + isAuditor && + !isPlatformAdmin + ) { throw new ForbiddenException( `Auditors cannot set status to 'ready_for_review'. This status is for clients to signal readiness.`, ); } } + // Handle revisionNote logic: + // - Set revisionNote when status is needs_revision and a note is provided + // - Clear revisionNote when status changes to anything other than needs_revision + let revisionNoteUpdate: { revisionNote?: string | null } = {}; + if (updateDto.status === FindingStatus.needs_revision) { + // Set revision note if provided (can be null to clear) + if (updateDto.revisionNote !== undefined) { + revisionNoteUpdate = { revisionNote: updateDto.revisionNote || null }; + } + } else if (updateDto.status !== undefined) { + // Clear revision note when moving to a different status + revisionNoteUpdate = { revisionNote: null }; + } + const updatedFinding = await db.finding.update({ where: { id: findingId }, data: { ...(updateDto.status !== undefined && { status: updateDto.status }), ...(updateDto.type !== undefined && { type: updateDto.type }), ...(updateDto.content !== undefined && { content: updateDto.content }), + ...revisionNoteUpdate, }, include: { createdBy: { @@ -411,22 +428,34 @@ export class FindingsService { switch (updateDto.status) { case FindingStatus.ready_for_review: - this.logger.log(`Triggering 'ready_for_review' notification for finding ${findingId}`); + this.logger.log( + `Triggering 'ready_for_review' notification for finding ${findingId}`, + ); void this.findingNotifierService.notifyReadyForReview({ ...notificationParams, findingCreatorMemberId: finding.createdById, }); break; case FindingStatus.needs_revision: - this.logger.log(`Triggering 'needs_revision' notification for finding ${findingId}`); - void this.findingNotifierService.notifyNeedsRevision(notificationParams); + this.logger.log( + `Triggering 'needs_revision' notification for finding ${findingId}`, + ); + void this.findingNotifierService.notifyNeedsRevision( + notificationParams, + ); break; case FindingStatus.closed: - this.logger.log(`Triggering 'closed' notification for finding ${findingId}`); - void this.findingNotifierService.notifyFindingClosed(notificationParams); + this.logger.log( + `Triggering 'closed' notification for finding ${findingId}`, + ); + void this.findingNotifierService.notifyFindingClosed( + notificationParams, + ); break; case FindingStatus.open: - this.logger.log(`Status changed to 'open' for finding ${findingId}. No notification sent.`); + this.logger.log( + `Status changed to 'open' for finding ${findingId}. No notification sent.`, + ); break; default: this.logger.warn( @@ -489,6 +518,9 @@ export class FindingsService { // Verify finding exists await this.findById(organizationId, findingId); - return this.findingAuditService.getFindingActivity(findingId, organizationId); + return this.findingAuditService.getFindingActivity( + findingId, + organizationId, + ); } } diff --git a/apps/app/src/app/(app)/[orgId]/frameworks/components/FindingsOverview.tsx b/apps/app/src/app/(app)/[orgId]/frameworks/components/FindingsOverview.tsx index d35037136..da7363f98 100644 --- a/apps/app/src/app/(app)/[orgId]/frameworks/components/FindingsOverview.tsx +++ b/apps/app/src/app/(app)/[orgId]/frameworks/components/FindingsOverview.tsx @@ -18,7 +18,7 @@ const STATUS_COLORS: Record = { const STATUS_LABELS: Record = { open: 'Open', - ready_for_review: 'Review', + ready_for_review: 'Auditor Review', needs_revision: 'Revision', closed: 'Closed', }; @@ -215,7 +215,7 @@ export function FindingsOverview({ diff --git a/apps/app/src/app/(app)/[orgId]/tasks/[taskId]/components/findings/FindingItem.tsx b/apps/app/src/app/(app)/[orgId]/tasks/[taskId]/components/findings/FindingItem.tsx index 21003576d..de37dcac7 100644 --- a/apps/app/src/app/(app)/[orgId]/tasks/[taskId]/components/findings/FindingItem.tsx +++ b/apps/app/src/app/(app)/[orgId]/tasks/[taskId]/components/findings/FindingItem.tsx @@ -6,6 +6,14 @@ import { cn } from '@/lib/utils'; import { Avatar, AvatarFallback, AvatarImage } from '@comp/ui/avatar'; import { Badge } from '@comp/ui/badge'; import { Button } from '@comp/ui/button'; +import { + Dialog, + DialogContent, + DialogDescription, + DialogFooter, + DialogHeader, + DialogTitle, +} from '@comp/ui/dialog'; import { DropdownMenu, DropdownMenuContent, @@ -13,13 +21,17 @@ import { DropdownMenuSeparator, DropdownMenuTrigger, } from '@comp/ui/dropdown-menu'; +import { Textarea } from '@comp/ui/textarea'; import { FindingStatus, FindingType } from '@db'; import { formatDistanceToNow } from 'date-fns'; -import { ChevronDown, MoreVertical, Trash2 } from 'lucide-react'; -import { useState } from 'react'; +import { ChevronDown, ChevronUp, MoreVertical, Trash2 } from 'lucide-react'; +import { useEffect, useMemo, useRef, useState } from 'react'; import { FindingHistorySheet } from './FindingHistorySheet'; import { FindingStatusBadge } from './FindingStatusBadge'; +// Character threshold for showing "Read more" - approximate 2 lines +const CONTENT_TRUNCATE_LENGTH = 150; + interface FindingItemProps { finding: Finding; isExpanded: boolean; @@ -27,12 +39,16 @@ interface FindingItemProps { canSetRestrictedStatus: boolean; isAuditor: boolean; isPlatformAdmin: boolean; + isTarget?: boolean; // Whether this finding is the navigation target onToggleExpand: () => void; - onStatusChange: (status: FindingStatus) => void; + onStatusChange: (status: FindingStatus, revisionNote?: string) => Promise | void; onDelete: () => void; onViewHistory?: () => void; } +// How long to show the highlight (in ms) +const HIGHLIGHT_DURATION = 2000; + export function FindingItem({ finding, isExpanded, @@ -40,6 +56,7 @@ export function FindingItem({ canSetRestrictedStatus, isAuditor, isPlatformAdmin, + isTarget = false, onToggleExpand, onStatusChange, onDelete, @@ -47,6 +64,37 @@ export function FindingItem({ }: FindingItemProps) { // Only use local state for sheet if onViewHistory is not provided const [historyOpen, setHistoryOpen] = useState(false); + const [showHighlight, setShowHighlight] = useState(false); + const [revisionDialogOpen, setRevisionDialogOpen] = useState(false); + const [revisionNote, setRevisionNote] = useState(''); + const [isSubmitting, setIsSubmitting] = useState(false); + const itemRef = useRef(null); + + // Scroll to center and show highlight when this is the target finding + useEffect(() => { + if (isTarget && itemRef.current) { + // Show highlight immediately + setShowHighlight(true); + + // Scroll to center after a small delay + const scrollTimer = setTimeout(() => { + itemRef.current?.scrollIntoView({ behavior: 'smooth', block: 'center' }); + }, 150); + + // Fade out highlight after duration + const highlightTimer = setTimeout(() => { + setShowHighlight(false); + }, HIGHLIGHT_DURATION); + + return () => { + clearTimeout(scrollTimer); + clearTimeout(highlightTimer); + }; + } + }, [isTarget]); + + // Check if content is long enough to need truncation + const needsTruncation = finding.content.length > CONTENT_TRUNCATE_LENGTH; const handleViewHistory = () => { if (onViewHistory) { @@ -56,6 +104,38 @@ export function FindingItem({ } }; + // Handle status selection - show dialog for "Needs Revision" + const handleStatusSelect = (status: FindingStatus) => { + if (status === FindingStatus.needs_revision) { + setRevisionDialogOpen(true); + } else { + onStatusChange(status); + } + }; + + // Skip adding a note - just change status + const handleRevisionSkip = async () => { + setIsSubmitting(true); + try { + await onStatusChange(FindingStatus.needs_revision); + setRevisionDialogOpen(false); + } finally { + setIsSubmitting(false); + } + }; + + // Submit revision status with note + const handleRevisionSubmit = async () => { + setIsSubmitting(true); + try { + await onStatusChange(FindingStatus.needs_revision, revisionNote.trim() || undefined); + setRevisionDialogOpen(false); + setRevisionNote(''); + } finally { + setIsSubmitting(false); + } + }; + const author = finding.createdBy?.user; const initials = author?.name @@ -69,18 +149,16 @@ export function FindingItem({ // - Platform admins can set any status (full control) // - Auditors can set: open, needs_revision, closed // - Non-auditor admins/owners can set: open, ready_for_review - const getStatusOptions = () => { - // Platform admins bypass all restrictions + const statusOptions = useMemo(() => { const canSetReadyForReview = isPlatformAdmin || !isAuditor; - const showReadyForReviewHint = !canSetReadyForReview; - const options: { value: FindingStatus; label: string; disabled: boolean; hint?: string }[] = [ + return [ { value: FindingStatus.open, label: 'Open', disabled: false }, { value: FindingStatus.ready_for_review, label: 'Ready for Review', disabled: !canSetReadyForReview, - hint: showReadyForReviewHint ? 'Client only' : undefined, + hint: !canSetReadyForReview ? 'Client only' : undefined, }, { value: FindingStatus.needs_revision, @@ -95,16 +173,19 @@ export function FindingItem({ hint: !canSetRestrictedStatus ? 'Auditor only' : undefined, }, ]; - return options; - }; + }, [isPlatformAdmin, isAuditor, canSetRestrictedStatus]); return (
@@ -129,7 +210,46 @@ export function FindingItem({ )}

-

{finding.content}

+ + {/* Content - show full or truncated based on expansion state */} +
+ {isExpanded || !needsTruncation ? ( +

{finding.content}

+ ) : ( +

+ {finding.content.slice(0, CONTENT_TRUNCATE_LENGTH).trim()}... +

+ )} + + {/* Show expand/collapse toggle only when content is long */} + {needsTruncation && ( + + )} + + {/* Auditor Note - displayed when status is needs_revision and note exists */} + {finding.status === FindingStatus.needs_revision && finding.revisionNote && ( +
+

Auditor Note

+

{finding.revisionNote}

+
+ )} +
@@ -142,11 +262,11 @@ export function FindingItem({ - {getStatusOptions().map((option) => ( + {statusOptions.map((option) => ( onStatusChange(option.value)} + onClick={() => handleStatusSelect(option.value)} className={cn( finding.status === option.value && 'bg-accent', option.disabled && 'opacity-50 cursor-not-allowed', @@ -169,12 +289,7 @@ export function FindingItem({ - - {isExpanded ? 'Collapse' : 'Expand'} - - - History - + History {canSetRestrictedStatus && ( <> @@ -201,19 +316,54 @@ export function FindingItem({ /> )} - {/* Expanded content */} -
{ + if (isSubmitting) return; // Prevent closing while submitting + setRevisionDialogOpen(open); + if (!open) setRevisionNote(''); + }} > -
-
-

{finding.content}

+ + + Request Revision + + Add a note explaining what needs to be revised, or skip to change the status without a + note. + + +
+