From db775397d91232b87a92f7609bc51aa86c3cef2c Mon Sep 17 00:00:00 2001 From: dal Date: Fri, 22 Aug 2025 11:46:30 -0600 Subject: [PATCH] Enhance report sharing functionality with response type integration - Updated DELETE, GET, and POST handlers to return structured response types (ShareDeleteResponse, ShareGetResponse, SharePostResponse) for better consistency and clarity. - Refactored permission checks and report existence validation to streamline logic. - Improved optimistic UI updates in the frontend for sharing reports, ensuring a smoother user experience. - Added response schemas for sharing operations to the shared reports module. --- .../src/api/v2/reports/[id]/sharing/DELETE.ts | 15 +++--- .../src/api/v2/reports/[id]/sharing/GET.ts | 26 ++-------- .../src/api/v2/reports/[id]/sharing/POST.ts | 24 ++++++--- .../src/api/v2/reports/[id]/sharing/PUT.ts | 21 +++++--- .../api/buster_rest/reports/queryRequests.ts | 51 +++++++++++++++++++ .../src/api/buster_rest/reports/requests.ts | 10 +++- .../ShareMenu/ShareMenuContentEmbed.tsx | 2 +- .../ReportThreeDotMenu.test.tsx | 2 - .../server-shared/src/reports/responses.ts | 46 ++++++++++++++++- 9 files changed, 146 insertions(+), 51 deletions(-) diff --git a/apps/server/src/api/v2/reports/[id]/sharing/DELETE.ts b/apps/server/src/api/v2/reports/[id]/sharing/DELETE.ts index 53bcd3945..0292cc7c1 100644 --- a/apps/server/src/api/v2/reports/[id]/sharing/DELETE.ts +++ b/apps/server/src/api/v2/reports/[id]/sharing/DELETE.ts @@ -5,6 +5,7 @@ import { removeAssetPermission, } from '@buster/database'; import type { User } from '@buster/database'; +import type { ShareDeleteResponse } from '@buster/server-shared/reports'; import type { ShareDeleteRequest } from '@buster/server-shared/share'; import { ShareDeleteRequestSchema } from '@buster/server-shared/share'; import { zValidator } from '@hono/zod-validator'; @@ -15,13 +16,7 @@ export async function deleteReportSharingHandler( reportId: string, emails: ShareDeleteRequest, user: User -): Promise<{ success: boolean; removed: string[]; notFound: string[] }> { - // Check if report exists - const report = await getReport({ reportId, userId: user.id }); - if (!report) { - throw new HTTPException(404, { message: 'Report not found' }); - } - +): Promise { // Check if user has permission to modify sharing for the report const permissionCheck = await checkAssetPermission({ assetId: reportId, @@ -39,6 +34,12 @@ export async function deleteReportSharingHandler( }); } + // Get the report to verify it exists and get owner info + const report = await getReport({ reportId, userId: user.id }); + if (!report) { + throw new HTTPException(404, { message: 'Report not found' }); + } + // Find users by emails const userMap = await findUsersByEmails(emails); diff --git a/apps/server/src/api/v2/reports/[id]/sharing/GET.ts b/apps/server/src/api/v2/reports/[id]/sharing/GET.ts index 521be1aa0..ddada2ced 100644 --- a/apps/server/src/api/v2/reports/[id]/sharing/GET.ts +++ b/apps/server/src/api/v2/reports/[id]/sharing/GET.ts @@ -1,22 +1,13 @@ import { checkAssetPermission, getReport, listAssetPermissions } from '@buster/database'; import type { User } from '@buster/database'; +import type { ShareGetResponse } from '@buster/server-shared/reports'; import { Hono } from 'hono'; import { HTTPException } from 'hono/http-exception'; -interface SharePermission { - userId: string; - email: string; - name: string | null; - avatarUrl: string | null; - role: string; - createdAt: string; - updatedAt: string; -} - export async function getReportSharingHandler( reportId: string, user: User -): Promise<{ permissions: SharePermission[] }> { +): Promise { // Check if report exists const report = await getReport({ reportId, userId: user.id }); if (!report) { @@ -42,19 +33,8 @@ export async function getReportSharingHandler( assetType: 'report_file', }); - // Format the permissions for the response - const formattedPermissions: SharePermission[] = permissions.map((perm) => ({ - userId: perm.user?.id || '', - email: perm.user?.email || '', - name: perm.user?.name || null, - avatarUrl: perm.user?.avatarUrl || null, - role: perm.permission.role, - createdAt: perm.permission.createdAt, - updatedAt: perm.permission.updatedAt, - })); - return { - permissions: formattedPermissions, + permissions, }; } diff --git a/apps/server/src/api/v2/reports/[id]/sharing/POST.ts b/apps/server/src/api/v2/reports/[id]/sharing/POST.ts index f1f5f4bd1..b789750c2 100644 --- a/apps/server/src/api/v2/reports/[id]/sharing/POST.ts +++ b/apps/server/src/api/v2/reports/[id]/sharing/POST.ts @@ -5,6 +5,7 @@ import { getReport, } from '@buster/database'; import type { User } from '@buster/database'; +import type { SharePostResponse } from '@buster/server-shared/reports'; import type { SharePostRequest } from '@buster/server-shared/share'; import { SharePostRequestSchema } from '@buster/server-shared/share'; import { zValidator } from '@hono/zod-validator'; @@ -15,13 +16,7 @@ export async function createReportSharingHandler( reportId: string, shareRequests: SharePostRequest, user: User -): Promise<{ success: boolean; shared: string[]; notFound: string[] }> { - // Check if report exists - const report = await getReport({ reportId, userId: user.id }); - if (!report) { - throw new HTTPException(404, { message: 'Report not found' }); - } - +): Promise { // Check if user has permission to share the report const permissionCheck = await checkAssetPermission({ assetId: reportId, @@ -39,6 +34,12 @@ export async function createReportSharingHandler( }); } + // Get the report to verify it exists + const report = await getReport({ reportId, userId: user.id }); + if (!report) { + throw new HTTPException(404, { message: 'Report not found' }); + } + // Extract emails from the share requests const emails = shareRequests.map((req) => req.email); @@ -69,12 +70,19 @@ export async function createReportSharingHandler( viewer: 'can_view', // Map viewer to can_view } as const; + const mappedRole = roleMapping[shareRequest.role]; + if (!mappedRole) { + throw new HTTPException(400, { + message: `Invalid role: ${shareRequest.role} for user ${shareRequest.email}`, + }); + } + permissions.push({ identityId: targetUser.id, identityType: 'user' as const, assetId: reportId, assetType: 'report_file' as const, - role: roleMapping[shareRequest.role] || 'can_view', + role: mappedRole, createdBy: user.id, }); } diff --git a/apps/server/src/api/v2/reports/[id]/sharing/PUT.ts b/apps/server/src/api/v2/reports/[id]/sharing/PUT.ts index 1dde93b1a..b4120c4bb 100644 --- a/apps/server/src/api/v2/reports/[id]/sharing/PUT.ts +++ b/apps/server/src/api/v2/reports/[id]/sharing/PUT.ts @@ -19,12 +19,6 @@ export async function updateReportShareHandler( request: ShareUpdateRequest, user: User & { organizationId: string } ) { - // Check if report exists - const report = await getReport({ reportId, userId: user.id }); - if (!report) { - throw new HTTPException(404, { message: 'Report not found' }); - } - // Check if user has permission to edit asset permissions const permissionCheck = await checkAssetPermission({ assetId: reportId, @@ -42,6 +36,12 @@ export async function updateReportShareHandler( }); } + // Check if report exists + const report = await getReport({ reportId, userId: user.id }); + if (!report) { + throw new HTTPException(404, { message: 'Report not found' }); + } + const { publicly_accessible, public_expiry_date, public_password, workspace_sharing, users } = request; @@ -73,12 +73,19 @@ export async function updateReportShareHandler( viewer: 'can_view', // Map viewer to can_view } as const; + const mappedRole = roleMapping[userPermission.role]; + if (!mappedRole) { + throw new HTTPException(400, { + message: `Invalid role: ${userPermission.role} for user ${userPermission.email}`, + }); + } + permissions.push({ identityId: targetUser.id, identityType: 'user' as const, assetId: reportId, assetType: 'report_file' as const, - role: roleMapping[userPermission.role] || 'can_view', + role: mappedRole, createdBy: user.id, }); } diff --git a/apps/web/src/api/buster_rest/reports/queryRequests.ts b/apps/web/src/api/buster_rest/reports/queryRequests.ts index 14869beba..fa50a4dc5 100644 --- a/apps/web/src/api/buster_rest/reports/queryRequests.ts +++ b/apps/web/src/api/buster_rest/reports/queryRequests.ts @@ -274,6 +274,57 @@ export const useShareReport = () => { return useMutation({ mutationFn: shareReport, + onMutate: async (variables) => { + // Cancel any outgoing refetches + await queryClient.cancelQueries({ + queryKey: reportsQueryKeys.reportsGetReport(variables.id).queryKey + }); + + // Snapshot the previous value + const previousReport = queryClient.getQueryData( + reportsQueryKeys.reportsGetReport(variables.id).queryKey + ); + + // Optimistically update the report with new permissions + queryClient.setQueryData( + reportsQueryKeys.reportsGetReport(variables.id).queryKey, + (old: GetReportResponse | undefined) => { + if (!old) return old; + return create(old, (draft) => { + // Add new permissions optimistically + variables.params.forEach((shareRequest) => { + const exists = draft.individual_permissions?.some( + (p) => p.email === shareRequest.email + ); + if (!exists) { + draft.individual_permissions = [ + ...(draft.individual_permissions || []), + { + id: `temp-${shareRequest.email}`, + email: shareRequest.email, + name: shareRequest.name || shareRequest.email, + role: shareRequest.role, + avatar_url: shareRequest.avatar_url || null + } + ]; + } + }); + }); + } + ); + + // Return a context object with the snapshotted value + return { previousReport }; + }, + onError: (err, variables, context) => { + // If the mutation fails, use the context returned from onMutate to roll back + if (context?.previousReport) { + queryClient.setQueryData( + reportsQueryKeys.reportsGetReport(variables.id).queryKey, + context.previousReport + ); + } + }, onSuccess: (_, { id }) => { // Invalidate the report cache to get updated sharing info queryClient.invalidateQueries({ diff --git a/apps/web/src/api/buster_rest/reports/requests.ts b/apps/web/src/api/buster_rest/reports/requests.ts index 935e2d981..25a9ff6f8 100644 --- a/apps/web/src/api/buster_rest/reports/requests.ts +++ b/apps/web/src/api/buster_rest/reports/requests.ts @@ -5,6 +5,8 @@ import type { GetReportsListRequest, GetReportsListResponse, GetReportResponse, + ShareDeleteResponse, + SharePostResponse, UpdateReportRequest, UpdateReportResponse } from '@buster/server-shared/reports'; @@ -66,14 +68,18 @@ export const updateReport = async ({ * Share a report with users */ export const shareReport = async ({ id, params }: { id: string; params: SharePostRequest }) => { - return mainApiV2.post(`/reports/${id}/sharing`, params).then((res) => res.data); + return mainApiV2 + .post(`/reports/${id}/sharing`, params) + .then((res) => res.data); }; /** * Remove sharing permissions from a report */ export const unshareReport = async ({ id, data }: { id: string; data: ShareDeleteRequest }) => { - return mainApiV2.delete(`/reports/${id}/sharing`, { data }).then((res) => res.data); + return mainApiV2 + .delete(`/reports/${id}/sharing`, { data }) + .then((res) => res.data); }; /** diff --git a/apps/web/src/components/features/ShareMenu/ShareMenuContentEmbed.tsx b/apps/web/src/components/features/ShareMenu/ShareMenuContentEmbed.tsx index 399480664..c6569820a 100644 --- a/apps/web/src/components/features/ShareMenu/ShareMenuContentEmbed.tsx +++ b/apps/web/src/components/features/ShareMenu/ShareMenuContentEmbed.tsx @@ -98,7 +98,7 @@ export const ShareMenuContentEmbedFooter = ({ } else if (assetType === 'report') { await onShareReport(payload); } - openSuccessMessage('Succuessfully published'); + openSuccessMessage('Successfully published'); }); return ( diff --git a/apps/web/src/layouts/ChatLayout/FileContainer/FileContainerHeader/ReportContainerHeaderButtons/ReportThreeDotMenu.test.tsx b/apps/web/src/layouts/ChatLayout/FileContainer/FileContainerHeader/ReportContainerHeaderButtons/ReportThreeDotMenu.test.tsx index b390dad3e..2ece0f133 100644 --- a/apps/web/src/layouts/ChatLayout/FileContainer/FileContainerHeader/ReportContainerHeaderButtons/ReportThreeDotMenu.test.tsx +++ b/apps/web/src/layouts/ChatLayout/FileContainer/FileContainerHeader/ReportContainerHeaderButtons/ReportThreeDotMenu.test.tsx @@ -382,9 +382,7 @@ describe('ReportThreeDotMenu', () => { 'Add to collection', 'Add to favorites', // Or 'Remove from favorites' based on state 'Version history', - 'Request verification', 'Refresh report', - 'Duplicate', 'Download as PDF' ]; diff --git a/packages/server-shared/src/reports/responses.ts b/packages/server-shared/src/reports/responses.ts index c49413570..188d2c2da 100644 --- a/packages/server-shared/src/reports/responses.ts +++ b/packages/server-shared/src/reports/responses.ts @@ -1,4 +1,4 @@ -import type { z } from 'zod'; +import { z } from 'zod'; import { PaginatedResponseSchema } from '../type-utilities/pagination'; import { ReportIndividualResponseSchema, ReportListItemSchema } from './reports.types'; @@ -6,7 +6,51 @@ export const GetReportsListResponseSchema = PaginatedResponseSchema(ReportListIt export const UpdateReportResponseSchema = ReportIndividualResponseSchema; export const ShareUpdateResponseSchema = ReportIndividualResponseSchema; +// Sharing operation response schemas +export const SharePostResponseSchema = z.object({ + success: z.boolean(), + shared: z.array(z.string()), + notFound: z.array(z.string()), +}); + +export const ShareDeleteResponseSchema = z.object({ + success: z.boolean(), + removed: z.array(z.string()), + notFound: z.array(z.string()), +}); + +// For GET sharing endpoint - matches AssetPermissionWithUser from database +export const ShareGetResponseSchema = z.object({ + permissions: z.array( + z.object({ + permission: z.object({ + identityId: z.string(), + identityType: z.string(), + assetId: z.string(), + assetType: z.string(), + role: z.string(), + createdAt: z.string(), + updatedAt: z.string(), + createdBy: z.string(), + updatedBy: z.string(), + deletedAt: z.string().nullable(), + }), + user: z + .object({ + id: z.string(), + email: z.string(), + name: z.string().nullable(), + avatarUrl: z.string().nullable(), + }) + .nullable(), + }) + ), +}); + export type GetReportsListResponse = z.infer; export type UpdateReportResponse = z.infer; export type GetReportResponse = z.infer; export type ShareUpdateResponse = z.infer; +export type SharePostResponse = z.infer; +export type ShareDeleteResponse = z.infer; +export type ShareGetResponse = z.infer;