From 0a83b0797e7e4e669175e8ad17fce7ca6dcf6dfb Mon Sep 17 00:00:00 2001 From: Dirk Date: Mon, 31 Aug 2026 11:05:17 +0200 Subject: [PATCH 1/4] fix(EditContent): handle image duplication warnings when source has no storage bucket --- cms/src/components/content/EditContent.vue | 27 ++-- .../content/EditContentDuplication.spec.ts | 118 +++++++++++++++++- .../content/util/buildContentDuplicate.ts | 30 ++++- 3 files changed, 161 insertions(+), 14 deletions(-) diff --git a/cms/src/components/content/EditContent.vue b/cms/src/components/content/EditContent.vue index d1c03c68a6..ac518c71fb 100644 --- a/cms/src/components/content/EditContent.vue +++ b/cms/src/components/content/EditContent.vue @@ -106,8 +106,11 @@ const acceptServerVersion = () => window.location.reload(); const icon = props.docType === DocType.Tag ? TagIcon : DocumentIcon; -const notify = (state: "success" | "error" | "info", title: string, description: string) => - addNotification({ title, description, state }); +const notify = ( + state: "success" | "error" | "info" | "warning", + title: string, + description: string, +) => addNotification({ title, description, state }); // Which translation is being edited (driven by the route), + the language lists. const { @@ -302,11 +305,13 @@ watch(showDuplicateModal, (open) => { const duplicate = async () => { showDuplicateModal.value = false; if (!editableParent.value) return; - const { parent: clonedParent, content: clonedContent } = buildContentDuplicate( - editableParent.value, - editableContent.value, - { duplicateImage: duplicateImageOnCopy.value }, - ); + const { + parent: clonedParent, + content: clonedContent, + imageOutcome, + } = buildContentDuplicate(editableParent.value, editableContent.value, { + duplicateImage: duplicateImageOnCopy.value, + }); source.installClones(clonedParent, clonedContent); if (import.meta.env.MODE !== "test") { await router.replace({ @@ -325,6 +330,14 @@ const duplicate = async () => { "Successfully duplicated", `This ${props.tagOrPostType} has successfully been duplicated`, ); + // The image is only copyable when the original records the bucket it is stored in. + if (imageOutcome === "noSourceBucket") { + notify( + "warning", + "Image not copied", + `The original ${props.tagOrPostType} has no storage bucket saved on it, so its image could not be copied. Add an image to the duplicate before saving.`, + ); + } }; const showLanguageSelector = ref(false); diff --git a/cms/src/components/content/EditContentDuplication.spec.ts b/cms/src/components/content/EditContentDuplication.spec.ts index 89b166ee98..a6e7ae000e 100644 --- a/cms/src/components/content/EditContentDuplication.spec.ts +++ b/cms/src/components/content/EditContentDuplication.spec.ts @@ -1,7 +1,15 @@ import { describe, it, afterEach, beforeEach, expect, vi } from "vitest"; import { mount } from "@vue/test-utils"; import { createTestingPinia } from "@pinia/testing"; -import { db, DocType, accessMap, PostType, TagType, type TagDto, PublishStatus } from "luminary-shared"; +import { + db, + DocType, + accessMap, + PostType, + TagType, + type TagDto, + PublishStatus, +} from "luminary-shared"; import * as mockData from "@/tests/mockdata"; import { setActivePinia } from "pinia"; import EditContent from "./EditContent.vue"; @@ -513,6 +521,114 @@ describe("EditContent.vue - Duplication", () => { expect(vm.editableParent.imageData.duplicate).toBe(true); }, 15000); + it("warns when the source has image files but no storage bucket to copy them from", async () => { + const mockNotification = vi.fn(); + const notificationStore = useNotificationStore(); + notificationStore.addNotification = mockNotification; + + // mockPostDto carries fileCollections but no imageBucketId — the legacy shape. + expect(mockData.mockPostDto.imageData?.fileCollections.length).toBeGreaterThan(0); + expect((mockData.mockPostDto as any).imageBucketId).toBeUndefined(); + + const wrapper = mount(EditContent, { + props: { + docType: DocType.Post, + id: mockData.mockPostDto._id, + languageCode: "eng", + tagOrPostType: PostType.Blog, + }, + }); + + await waitForExpect(() => { + expect(wrapper.text()).toContain("English"); + }); + + const dropdownTrigger = wrapper.find('[data-test="dropdown-trigger"]'); + await dropdownTrigger.trigger("click"); + await nextTick(); + + let duplicateBtn; + await waitForExpect(() => { + duplicateBtn = wrapper.find("[data-test='duplicate-button']"); + expect(duplicateBtn.exists()).toBe(true); + }); + + let confirmBtn; + await waitForExpect(async () => { + duplicateBtn!.trigger("click"); + confirmBtn = wrapper.find('[data-test="modal-primary-button"]'); + expect(confirmBtn.exists()).toBe(true); + }); + await confirmBtn!.trigger("click"); + + await waitForExpect(() => { + expect(mockNotification).toHaveBeenCalledWith( + expect.objectContaining({ title: "Image not copied", state: "warning" }), + ); + }); + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const vm: any = wrapper.vm; + expect(vm.editableParent.imageData.fileCollections).toEqual([]); + }, 15000); + + it("does not warn about the image when the source has a storage bucket", async () => { + await db.docs.put({ + ...mockData.mockPostDto, + imageBucketId: "storage-image-bucket", + } as any); + + const mockNotification = vi.fn(); + const notificationStore = useNotificationStore(); + notificationStore.addNotification = mockNotification; + + const wrapper = mount(EditContent, { + props: { + docType: DocType.Post, + id: mockData.mockPostDto._id, + languageCode: "eng", + tagOrPostType: PostType.Blog, + }, + }); + + await waitForExpect(() => { + expect(wrapper.text()).toContain("English"); + }); + + await waitForExpect(() => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const vm: any = wrapper.vm; + expect(vm.editableParent.imageBucketId).toBe("storage-image-bucket"); + }); + + const dropdownTrigger = wrapper.find('[data-test="dropdown-trigger"]'); + await dropdownTrigger.trigger("click"); + await nextTick(); + + let duplicateBtn; + await waitForExpect(() => { + duplicateBtn = wrapper.find("[data-test='duplicate-button']"); + expect(duplicateBtn.exists()).toBe(true); + }); + + let confirmBtn; + await waitForExpect(async () => { + duplicateBtn!.trigger("click"); + confirmBtn = wrapper.find('[data-test="modal-primary-button"]'); + expect(confirmBtn.exists()).toBe(true); + }); + await confirmBtn!.trigger("click"); + + await waitForExpect(() => { + expect(mockNotification).toHaveBeenCalledWith( + expect.objectContaining({ title: "Successfully duplicated" }), + ); + }); + expect(mockNotification).not.toHaveBeenCalledWith( + expect.objectContaining({ title: "Image not copied" }), + ); + }, 15000); + it("clears image fileCollections when duplicate image is unchecked", async () => { const wrapper = mount(EditContent, { props: { diff --git a/cms/src/components/content/util/buildContentDuplicate.ts b/cms/src/components/content/util/buildContentDuplicate.ts index 9ea383dbcc..093980d43b 100644 --- a/cms/src/components/content/util/buildContentDuplicate.ts +++ b/cms/src/components/content/util/buildContentDuplicate.ts @@ -8,33 +8,51 @@ import { } from "luminary-shared"; import * as _ from "lodash"; +/** + * What happened to the source image when building the duplicate. + * `noSourceBucket` is a failure the caller must surface — the source has image files but no + * bucket to copy them from, so the duplicate is created without an image. + */ +export type DuplicateImageOutcome = "copied" | "skipped" | "none" | "noSourceBucket"; + /** * Build unsaved duplicate clones of a content parent and its translations: fresh ids, * stripped `_rev`, drafted + "(Copy)"/"-copy"-suffixed children. Returns new objects; * the inputs are not mutated. * * `duplicateImage` carries over a copyable image collection — only possible when the - * source actually has an image bucket; otherwise the collection is cleared. + * source actually has an image bucket; otherwise the collection is cleared and the + * returned `imageOutcome` says why. */ export function buildContentDuplicate( parent: ContentParentDto, content: ContentDto[], options: { duplicateImage: boolean }, -): { parent: ContentParentDto; content: ContentDto[] } { +): { parent: ContentParentDto; content: ContentDto[]; imageOutcome: DuplicateImageOutcome } { const clonedParent = _.cloneDeep(parent); clonedParent._id = db.uuid(); delete (clonedParent as any)._rev; if (clonedParent.type === DocType.Tag) (clonedParent as TagDto).taggedDocs = []; + let imageOutcome: DuplicateImageOutcome = "none"; + if (clonedParent.imageData) { const imageData = clonedParent.imageData as typeof clonedParent.imageData & { duplicate?: boolean; }; delete clonedParent.imageData.uploadData; delete imageData.duplicate; - if (options.duplicateImage && imageData.fileCollections?.length > 0) { - if (parent.imageBucketId) imageData.duplicate = true; - else imageData.fileCollections = []; + if (imageData.fileCollections?.length > 0) { + if (!options.duplicateImage) { + imageData.fileCollections = []; + imageOutcome = "skipped"; + } else if (parent.imageBucketId) { + imageData.duplicate = true; + imageOutcome = "copied"; + } else { + imageData.fileCollections = []; + imageOutcome = "noSourceBucket"; + } } else if (imageData.fileCollections) { imageData.fileCollections = []; } @@ -55,5 +73,5 @@ export function buildContentDuplicate( return newContent; }); - return { parent: clonedParent, content: clonedContent }; + return { parent: clonedParent, content: clonedContent, imageOutcome }; } From 1a221f1037bc3a7c738f96f6c7bdc2c9853648f4 Mon Sep 17 00:00:00 2001 From: Dirk Date: Wed, 2 Sep 2026 16:36:04 +0200 Subject: [PATCH 2/4] temp: stop the CMS discarding imageBucketId, pending the real duplication fix Provisional and expected to be superseded. ImageEditor cleared a parent's imageBucketId whenever the bucket was missing from the user's synced Storage list, which strips the reference the duplication path reads. The clearing is removed here and the condition only reported. The duplication path still trusts client-supplied bucket state, so this does not yet make the copy reliable. The server-side duplicateFrom resolution replaces the remaining warning path. Co-Authored-By: Claude Opus 5 --- cms/src/components/content/EditContent.vue | 25 ++++++- .../content/EditContentDuplication.spec.ts | 4 + cms/src/components/images/ImageEditor.spec.ts | 73 +++++++++++++++++-- cms/src/components/images/ImageEditor.vue | 51 ++++++------- 4 files changed, 116 insertions(+), 37 deletions(-) diff --git a/cms/src/components/content/EditContent.vue b/cms/src/components/content/EditContent.vue index ac518c71fb..33dec8cb12 100644 --- a/cms/src/components/content/EditContent.vue +++ b/cms/src/components/content/EditContent.vue @@ -302,6 +302,13 @@ watch(showDuplicateModal, (open) => { if (open) duplicateImageOnCopy.value = true; }); +// The copy is made from the bucket the original records, so an image without one cannot travel. +const imageCannotBeCopied = computed( + () => + !!editableParent.value?.imageData?.fileCollections?.length && + !editableParent.value?.imageBucketId, +); + const duplicate = async () => { showDuplicateModal.value = false; if (!editableParent.value) return; @@ -330,12 +337,11 @@ const duplicate = async () => { "Successfully duplicated", `This ${props.tagOrPostType} has successfully been duplicated`, ); - // The image is only copyable when the original records the bucket it is stored in. if (imageOutcome === "noSourceBucket") { notify( "warning", "Image not copied", - `The original ${props.tagOrPostType} has no storage bucket saved on it, so its image could not be copied. Add an image to the duplicate before saving.`, + `No storage bucket is set on the original ${props.tagOrPostType}, so its image could not be copied. Set a storage bucket on the original, then duplicate it again.`, ); } }; @@ -726,8 +732,21 @@ const actionsWrapperProps = computed(() => ({ class="mt-3 flex cursor-pointer select-none items-start gap-2 text-sm text-zinc-700" data-test="duplicate-image-toggle" > - + Duplicate image +

+ No storage bucket is set on this {{ props.tagOrPostType }}, so its image cannot be + copied. Set a storage bucket first if you want the duplicate to keep the image. +

diff --git a/cms/src/components/content/EditContentDuplication.spec.ts b/cms/src/components/content/EditContentDuplication.spec.ts index a6e7ae000e..8e7dc6efb6 100644 --- a/cms/src/components/content/EditContentDuplication.spec.ts +++ b/cms/src/components/content/EditContentDuplication.spec.ts @@ -559,6 +559,10 @@ describe("EditContent.vue - Duplication", () => { confirmBtn = wrapper.find('[data-test="modal-primary-button"]'); expect(confirmBtn.exists()).toBe(true); }); + + // The modal says the image cannot travel before the duplicate is committed. + expect(wrapper.find("[data-test='duplicate-image-unavailable']").exists()).toBe(true); + await confirmBtn!.trigger("click"); await waitForExpect(() => { diff --git a/cms/src/components/images/ImageEditor.spec.ts b/cms/src/components/images/ImageEditor.spec.ts index caf7b1bce5..37b8cf3fd4 100644 --- a/cms/src/components/images/ImageEditor.spec.ts +++ b/cms/src/components/images/ImageEditor.spec.ts @@ -181,7 +181,13 @@ describe("ImageEditor", () => { const origBuckets = [...mockImageBuckets.value]; mockImageBuckets.value = [ ...origBuckets, - { _id: "bucket-2", name: "Second", publicUrl: "http://test2.com", storageType: "image", mimeTypes: ["image/*"] }, + { + _id: "bucket-2", + name: "Second", + publicUrl: "http://test2.com", + storageType: "image", + mimeTypes: ["image/*"], + }, ]; const parent: ContentParentDto = { @@ -195,7 +201,11 @@ describe("ImageEditor", () => { const component = wrapper.vm as any; const mockFile = new File(["img"], "test.jpg", { type: "image/jpeg" }); - const fileList = { 0: mockFile, length: 1, item: (i: number) => (i === 0 ? mockFile : null) }; + const fileList = { + 0: mockFile, + length: 1, + item: (i: number) => (i === 0 ? mockFile : null), + }; component.handleFiles(fileList); await wrapper.vm.$nextTick(); @@ -227,6 +237,47 @@ describe("ImageEditor", () => { mockImageBuckets.value = origBuckets; }); + it("keeps an imageBucketId the user cannot resolve, and reports it", async () => { + const parent: ContentParentDto = { + ...JSON.parse(JSON.stringify(mockPostDto)), + imageBucketId: "bucket-in-a-group-this-user-cannot-view", + }; + + const wrapper = mount(ImageEditor, { + props: { parent, disabled: false }, + }); + await wrapper.vm.$nextTick(); + + expect(parent.imageBucketId).toBe("bucket-in-a-group-this-user-cannot-view"); + expect(parent.imageData!.fileCollections.length).toBeGreaterThan(0); + expect(wrapper.text()).toContain("bucket you don't have access to"); + }); + + it("refuses an upload to an unresolved bucket without discarding the reference", async () => { + const parent: ContentParentDto = { + ...JSON.parse(JSON.stringify(mockPostDto)), + imageBucketId: "bucket-in-a-group-this-user-cannot-view", + }; + + const wrapper = mount(ImageEditor, { + props: { parent, disabled: false }, + }); + const component = wrapper.vm as any; + const mockFile = new File(["img"], "test.jpg", { type: "image/jpeg" }); + const fileList = { + 0: mockFile, + length: 1, + item: (i: number) => (i === 0 ? mockFile : null), + }; + + component.handleFiles(fileList); + await wrapper.vm.$nextTick(); + + expect(parent.imageBucketId).toBe("bucket-in-a-group-this-user-cannot-view"); + expect(parent.imageData!.uploadData).toBeUndefined(); + expect(wrapper.text()).toContain("bucket you don't have access to"); + }); + it("processFiles adds upload data to parent", async () => { const parent: ContentParentDto = { ...mockPostDto, @@ -241,7 +292,11 @@ describe("ImageEditor", () => { const mockFile = new File(["image-data"], "test.jpg", { type: "image/jpeg" }); Object.defineProperty(mockFile, "size", { value: 1024 }); // Small file - const fileList = { 0: mockFile, length: 1, item: (i: number) => (i === 0 ? mockFile : null) }; + const fileList = { + 0: mockFile, + length: 1, + item: (i: number) => (i === 0 ? mockFile : null), + }; const component = wrapper.vm as any; component.handleFiles(fileList); @@ -268,7 +323,11 @@ describe("ImageEditor", () => { const mockFile = new File(["x"], "huge.jpg", { type: "image/jpeg" }); Object.defineProperty(mockFile, "size", { value: largeSize }); - const fileList = { 0: mockFile, length: 1, item: (i: number) => (i === 0 ? mockFile : null) }; + const fileList = { + 0: mockFile, + length: 1, + item: (i: number) => (i === 0 ? mockFile : null), + }; const component = wrapper.vm as any; component.handleFiles(fileList); @@ -294,7 +353,11 @@ describe("ImageEditor", () => { // An actual upload is a real user edit and should persist the effective bucket. const mockFile = new File(["x"], "img.jpg", { type: "image/jpeg" }); Object.defineProperty(mockFile, "size", { value: 1024 }); - const fileList = { 0: mockFile, length: 1, item: (i: number) => (i === 0 ? mockFile : null) }; + const fileList = { + 0: mockFile, + length: 1, + item: (i: number) => (i === 0 ? mockFile : null), + }; (wrapper.vm as any).handleFiles(fileList); await wrapper.vm.$nextTick(); diff --git a/cms/src/components/images/ImageEditor.vue b/cms/src/components/images/ImageEditor.vue index 24bac8d320..1db600633f 100644 --- a/cms/src/components/images/ImageEditor.vue +++ b/cms/src/components/images/ImageEditor.vue @@ -26,6 +26,9 @@ const emit = defineEmits<{ const parent = defineModel("parent"); const maxUploadFileSizeMb = computed(() => maxUploadFileSize.value / 1000000); +const UNRESOLVED_BUCKET_MESSAGE = + "This image is stored in a bucket you don't have access to. The image is unaffected, but you cannot upload a new one until a bucket you can access is selected."; + // Bucket selection (simplified approach using existing database data) const bucketSelection = storageSelection(); @@ -112,31 +115,26 @@ const dragCounter = ref(0); const showFailureMessage = ref(false); const failureMessage = ref(undefined); -// Validate that selected bucket still exists and auto-select if only one available -watchEffect(() => { - // Check if the currently selected bucket still exists in the database - if (parent.value?.imageBucketId) { - // Only validate if buckets have loaded (array is not empty) - // This prevents clearing imageBucketId while buckets are still loading from IndexedDB - if (bucketSelection.imageBuckets.value.length > 0) { - const currentBucketExists = bucketSelection.imageBuckets.value.some( - (b) => b._id === parent.value?.imageBucketId, - ); - - // If the bucket no longer exists, clear it - if (!currentBucketExists) { - parent.value.imageBucketId = undefined; - } - } - } +// A bucket absent from the list is either deleted or in a group this user cannot view — +// the two are indistinguishable here, so the reference is reported and never discarded. +// Dropping it would strip the image from the document on the next save. +const bucketIsUnresolved = computed( + () => + !!parent.value?.imageBucketId && + bucketSelection.imageBuckets.value.length > 0 && + !bucketSelection.imageBuckets.value.some((b) => b._id === parent.value?.imageBucketId), +); - // Proactively show error messages for bucket configuration issues - // This ensures users see the error immediately, not just when they try to upload +// Surface bucket configuration issues on load rather than only on upload. +watchEffect(() => { if (!bucketSelection.hasImageBuckets.value) { // No buckets configured at all failureMessage.value = "No storage buckets configured. Please configure at least one S3 bucket in the Storage settings before uploading images."; showFailureMessage.value = true; + } else if (bucketIsUnresolved.value) { + failureMessage.value = UNRESOLVED_BUCKET_MESSAGE; + showFailureMessage.value = true; } else if (!effectiveImageBucketId.value && bucketSelection.imageBuckets.value.length > 1) { // Multiple buckets available but none selected failureMessage.value = "Please select a storage bucket before uploading images."; @@ -147,7 +145,7 @@ watchEffect(() => { const bucketRelatedErrors = [ "No storage buckets configured. Please configure at least one S3 bucket in the Storage settings before uploading images.", "Please select a storage bucket before uploading images.", - "The selected storage bucket no longer exists. Please select another bucket.", + UNRESOLVED_BUCKET_MESSAGE, ]; if (failureMessage.value && bucketRelatedErrors.includes(failureMessage.value)) { failureMessage.value = undefined; @@ -174,15 +172,10 @@ const handleFiles = (files: FileList | null) => { emit("bucketSelected", effectiveImageBucketId.value); } - // Check if the currently selected bucket still exists in the database - const currentBucketExists = bucketSelection.imageBuckets.value.some( - (b) => b._id === parent.value?.imageBucketId, - ); - - if (!currentBucketExists) { - if (parent.value) parent.value.imageBucketId = undefined; - failureMessage.value = - "The selected storage bucket no longer exists. Please select another bucket."; + // An upload needs a bucket this user can write to, so it is refused — but the existing + // reference is left alone; selecting another bucket is what replaces it. + if (bucketIsUnresolved.value) { + failureMessage.value = UNRESOLVED_BUCKET_MESSAGE; showFailureMessage.value = true; return; } From f434c2c790164b63719bd5e38d7fe0d4549a5a49 Mon Sep 17 00:00:00 2001 From: Dirk Date: Wed, 2 Sep 2026 16:43:55 +0200 Subject: [PATCH 3/4] fix(api,cms): resolve a duplicated image's source server-side MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Duplication asked the client to supply the bucket its image files live in, via the clone's own imageBucketId. A CMS that had lost or never synced that reference produced a duplicate with no image, and the API had no way to tell. ImageDto now carries duplicateFrom — the id of the parent being copied. The API reads that document's bucket and file collections itself and copies out of the source bucket into the duplicate's, so the result no longer depends on client state. duplicateFrom requires View access to the named document, since the server reads it on the caller's behalf. The duplicate boolean is still honoured for deployed clients that send it (ADR 0005), falling back to the same-bucket copy. Co-Authored-By: Claude Opus 5 --- .../documentProcessing/processImageDto.ts | 12 ++- .../processPostTagDto.spec.ts | 85 ++++++++++++++++++- .../documentProcessing/processPostTagDto.ts | 21 ++++- .../validateChangeRequestAccess.spec.ts | 54 ++++++++++++ .../validateChangeRequestAccess.ts | 26 ++++++ api/src/dto/ImageDto.ts | 13 ++- cms/src/components/content/EditContent.vue | 41 ++------- .../content/EditContentDuplication.spec.ts | 23 ++--- .../content/util/buildContentDuplicate.ts | 38 +++------ shared/src/types/dto.ts | 6 ++ 10 files changed, 245 insertions(+), 74 deletions(-) diff --git a/api/src/changeRequests/documentProcessing/processImageDto.ts b/api/src/changeRequests/documentProcessing/processImageDto.ts index 9dddc765c3..90175deb43 100644 --- a/api/src/changeRequests/documentProcessing/processImageDto.ts +++ b/api/src/changeRequests/documentProcessing/processImageDto.ts @@ -187,6 +187,8 @@ async function migrateImagesBetweenBuckets( * Processes an embedded image upload by resizing the image and uploading to S3 * Requires bucket-specific credentials configured at the post/tag level * Bucket ID is passed from the parent post/tag document for consistency + * `sourceBucketId` is where a duplicated image's files are read from, resolved from the source + * document rather than the incoming one. * Returns object with migration failure status and warnings */ export async function processImage( @@ -195,13 +197,14 @@ export async function processImage( db: DbService, parentBucketId?: string, prevParentBucketId?: string, + sourceBucketId?: string, ): Promise<{ migrationFailed: boolean; warnings: string[] }> { const warnings: string[] = []; let migrationFailed = false; let duplicatedNow = false; try { - if (image.duplicate && image.fileCollections.length > 0) { + if ((image.duplicateFrom || image.duplicate) && image.fileCollections.length > 0) { const prevHasImageFiles = !!prevImage?.fileCollections?.some( (collection) => collection.imageFiles?.length > 0, ); @@ -209,6 +212,9 @@ export async function processImage( if (prevHasImageFiles) { image.fileCollections = prevImage.fileCollections; } else { + // Copy out of the bucket the source files actually live in. It falls back to the + // target bucket for older clients, which only send `duplicate` and no source id. + const copyFromBucketId = sourceBucketId || parentBucketId; if (!parentBucketId) { warnings.push("Parent bucket ID is required for duplicated image copy."); return { migrationFailed, warnings }; @@ -217,7 +223,7 @@ export async function processImage( const duplicateResult = await duplicateImageFilesWithoutReencoding( image, db, - parentBucketId, + copyFromBucketId, parentBucketId, ); warnings.push(...duplicateResult.warnings); @@ -226,6 +232,7 @@ export async function processImage( // Avoid persisting stale source filenames when copy fails. image.fileCollections = []; delete image.duplicate; + delete image.duplicateFrom; return { migrationFailed, warnings }; } duplicatedNow = true; @@ -323,6 +330,7 @@ export async function processImage( } delete image.duplicate; + delete image.duplicateFrom; } catch (error) { warnings.push(`Image processing failed: ${error.message}`); } diff --git a/api/src/changeRequests/documentProcessing/processPostTagDto.spec.ts b/api/src/changeRequests/documentProcessing/processPostTagDto.spec.ts index 8c43e0cb12..242439332e 100644 --- a/api/src/changeRequests/documentProcessing/processPostTagDto.spec.ts +++ b/api/src/changeRequests/documentProcessing/processPostTagDto.spec.ts @@ -95,7 +95,6 @@ describe("processPostTagDto", () => { await processChangeRequest("test-user", postCr, ["group-super-admins"], db); await processChangeRequest("test-user", contentCr, ["group-super-admins"], db); - let contentRes = await db.getDoc(contentCr.doc._id); expect(contentRes.docs[0].parentAlwaysOffline).toBeUndefined(); @@ -638,4 +637,88 @@ describe("processPostTagDto", () => { (changeRequest.doc as PostDto).mediaBucketId, ); }); + it("resolves the source bucket and files from the document named by duplicateFrom", async () => { + const sourceRequest = changeRequest_post(); + sourceRequest.doc._id = "test-duplicate-source-post"; + (sourceRequest.doc as any).imageBucketId = "storage-bucket-1"; + (sourceRequest.doc as any).imageData = { + fileCollections: [ + { + aspectRatio: 1.5, + imageFiles: [{ width: 100, height: 66, filename: "src.webp" }], + }, + ], + }; + await db.upsertDoc(sourceRequest.doc); + + // The duplicate carries neither a bucket nor the source files — the API supplies both. + const duplicateRequest = changeRequest_post(); + duplicateRequest.doc._id = "test-duplicate-target-post"; + delete (duplicateRequest.doc as any).imageBucketId; + (duplicateRequest.doc as any).imageData = { + fileCollections: [], + duplicateFrom: "test-duplicate-source-post", + }; + + await processChangeRequest( + "test-user", + duplicateRequest as ChangeReqDto, + ["group-super-admins"], + db, + ); + + const stored = (await db.getDoc("test-duplicate-target-post")).docs[0] as PostDto; + expect(stored.imageBucketId).toBe("storage-bucket-1"); + + expect(processImage).toHaveBeenCalledWith( + expect.objectContaining({ + fileCollections: (sourceRequest.doc as any).imageData.fileCollections, + }), + undefined, + db, + "storage-bucket-1", + undefined, + "storage-bucket-1", + ); + }); + + it("keeps the duplicate's own bucket as the copy target when it has one", async () => { + const sourceRequest = changeRequest_post(); + sourceRequest.doc._id = "test-duplicate-source-post-2"; + (sourceRequest.doc as any).imageBucketId = "storage-bucket-1"; + (sourceRequest.doc as any).imageData = { + fileCollections: [ + { + aspectRatio: 1.5, + imageFiles: [{ width: 100, height: 66, filename: "src.webp" }], + }, + ], + }; + await db.upsertDoc(sourceRequest.doc); + + const duplicateRequest = changeRequest_post(); + duplicateRequest.doc._id = "test-duplicate-target-post-2"; + (duplicateRequest.doc as any).imageBucketId = "storage-bucket-2"; + (duplicateRequest.doc as any).imageData = { + fileCollections: [], + duplicateFrom: "test-duplicate-source-post-2", + }; + + await processChangeRequest( + "test-user", + duplicateRequest as ChangeReqDto, + ["group-super-admins"], + db, + ); + + // Source bucket is read from, the duplicate's own bucket is written to. + expect(processImage).toHaveBeenCalledWith( + expect.anything(), + undefined, + db, + "storage-bucket-2", + undefined, + "storage-bucket-1", + ); + }); }); diff --git a/api/src/changeRequests/documentProcessing/processPostTagDto.ts b/api/src/changeRequests/documentProcessing/processPostTagDto.ts index dfbd06fd45..4f9ec2d5b6 100644 --- a/api/src/changeRequests/documentProcessing/processPostTagDto.ts +++ b/api/src/changeRequests/documentProcessing/processPostTagDto.ts @@ -55,19 +55,37 @@ export default async function processPostTagDto( if (doc.imageData) { let imageWarnings: string[] = []; + // The source document is authoritative for where a duplicated image's files live and what + // they are, so a client that has lost or never synced its own bucket reference still + // produces a correct copy. + let sourceBucketId: Uuid | undefined; + if (doc.imageData.duplicateFrom) { + const sourceDoc = (await db.getDoc(doc.imageData.duplicateFrom)).docs[0] as + | PostDto + | TagDto + | undefined; + sourceBucketId = sourceDoc?.imageBucketId; + if (sourceDoc?.imageData?.fileCollections?.length) { + doc.imageData.fileCollections = sourceDoc.imageData.fileCollections; + } + // The copy stays in the source bucket unless the duplicate names its own. + if (!doc.imageBucketId) doc.imageBucketId = sourceBucketId; + } + if (!doc.imageBucketId) { imageWarnings.push("Bucket is not specified for image processing."); } // prevDoc is undefined on first upsert. A duplication request must include // existing file references and a source bucket on the parent document. - if (!prevDoc && doc.imageData.duplicate) { + if (!prevDoc && (doc.imageData.duplicateFrom || doc.imageData.duplicate)) { const hasSourceFiles = doc.imageData.fileCollections?.some( (collection) => collection.imageFiles?.length > 0, ); if (!doc.imageBucketId || !hasSourceFiles) { imageWarnings.push("Image duplication request is invalid."); delete doc.imageData.duplicate; + delete doc.imageData.duplicateFrom; doc.imageData.fileCollections = []; } } @@ -80,6 +98,7 @@ export default async function processPostTagDto( db, doc.imageBucketId, prevDoc?.imageBucketId, // Pass previous bucket ID for migration + sourceBucketId, ); imageWarnings = result.warnings; diff --git a/api/src/changeRequests/validateChangeRequestAccess.spec.ts b/api/src/changeRequests/validateChangeRequestAccess.spec.ts index 83948e1d9f..4de27d5835 100644 --- a/api/src/changeRequests/validateChangeRequestAccess.spec.ts +++ b/api/src/changeRequests/validateChangeRequestAccess.spec.ts @@ -809,4 +809,58 @@ describe("validateChangeRequestAccess", () => { expect(res.validated).toBe(true); }); }); + describe("image duplication source access", () => { + it("rejects a duplicate naming a source the user cannot view", async () => { + const changeReq = plainToClass(ChangeReqDto, { + doc: { + _id: "post-duplicate-of-private", + type: "post", + memberOf: ["group-public-content"], + image: "", + tags: [], + imageData: { fileCollections: [], duplicateFrom: "post-blog2" }, + }, + }); + + const res = await validateChangeRequestAccess(changeReq, ["group-public-editors"], db); + + expect(res.validated).toBe(false); + expect(res.error).toBe("No 'View' access to the document the image is duplicated from"); + }); + + it("rejects a duplicate naming a source that does not exist", async () => { + const changeReq = plainToClass(ChangeReqDto, { + doc: { + _id: "post-duplicate-of-missing", + type: "post", + memberOf: ["group-public-content"], + image: "", + tags: [], + imageData: { fileCollections: [], duplicateFrom: "post-does-not-exist" }, + }, + }); + + const res = await validateChangeRequestAccess(changeReq, ["group-public-editors"], db); + + expect(res.validated).toBe(false); + expect(res.error).toBe("No 'View' access to the document the image is duplicated from"); + }); + + it("accepts a duplicate naming a source the user can view", async () => { + const changeReq = plainToClass(ChangeReqDto, { + doc: { + _id: "post-duplicate-of-blog1", + type: "post", + memberOf: ["group-public-content"], + image: "", + tags: [], + imageData: { fileCollections: [], duplicateFrom: "post-blog1" }, + }, + }); + + const res = await validateChangeRequestAccess(changeReq, ["group-public-editors"], db); + + expect(res.validated).toBe(true); + }); + }); }); diff --git a/api/src/changeRequests/validateChangeRequestAccess.ts b/api/src/changeRequests/validateChangeRequestAccess.ts index b6e73b0491..19da8c785d 100644 --- a/api/src/changeRequests/validateChangeRequestAccess.ts +++ b/api/src/changeRequests/validateChangeRequestAccess.ts @@ -114,6 +114,32 @@ export async function validateChangeRequestAccess( }; } + // A duplication names the document its image is copied from, and the server reads that + // document's files and bucket. The id is client-supplied, so it needs the same view access + // as reading the source document directly would. + if (doc.type === DocType.Post || doc.type === DocType.Tag) { + const duplicateFrom = (doc as _contentParentDto).imageData?.duplicateFrom; + if (duplicateFrom) { + const sourceDoc = (await dbService.getDoc(duplicateFrom)).docs[0]; + if ( + !sourceDoc || + sourceDoc.type !== doc.type || + !PermissionSystem.verifyAccess( + (sourceDoc as _contentBaseDto).memberOf, + sourceDoc.type, + AclPermission.View, + groupMembership, + "all", + ) + ) { + return { + validated: false, + error: "No 'View' access to the document the image is duplicated from", + }; + } + } + } + // Validate edit, translate and group ACL assign access // ==================================================== diff --git a/api/src/dto/ImageDto.ts b/api/src/dto/ImageDto.ts index ce9e8473a3..672c62aa5f 100644 --- a/api/src/dto/ImageDto.ts +++ b/api/src/dto/ImageDto.ts @@ -1,8 +1,9 @@ import "reflect-metadata"; // https://stackoverflow.com/questions/72009995/typeerror-reflect-getmetadata-is-not-a-function -import { IsArray, IsBoolean, IsOptional, ValidateNested } from "class-validator"; +import { IsArray, IsBoolean, IsOptional, IsString, ValidateNested } from "class-validator"; import { Expose, Type } from "class-transformer"; import { ImageUploadDto } from "./ImageUploadDto"; import { ImageFileCollectionDto } from "./ImageFileCollectionDto"; +import { Uuid } from "../enums"; /** * Database structured Image object @@ -20,6 +21,16 @@ export class ImageDto { @Expose() uploadData?: ImageUploadDto[]; + /** + * Id of the parent this image is being copied from. The source bucket and files are resolved + * from that document server-side, so the copy does not depend on the client's own state. + */ + @IsOptional() + @IsString() + @Expose() + duplicateFrom?: Uuid; + + /** @deprecated Superseded by `duplicateFrom`; still accepted from older clients. */ @IsOptional() @IsBoolean() @Expose() diff --git a/cms/src/components/content/EditContent.vue b/cms/src/components/content/EditContent.vue index 33dec8cb12..4ac2d212dc 100644 --- a/cms/src/components/content/EditContent.vue +++ b/cms/src/components/content/EditContent.vue @@ -302,23 +302,14 @@ watch(showDuplicateModal, (open) => { if (open) duplicateImageOnCopy.value = true; }); -// The copy is made from the bucket the original records, so an image without one cannot travel. -const imageCannotBeCopied = computed( - () => - !!editableParent.value?.imageData?.fileCollections?.length && - !editableParent.value?.imageBucketId, -); - const duplicate = async () => { showDuplicateModal.value = false; if (!editableParent.value) return; - const { - parent: clonedParent, - content: clonedContent, - imageOutcome, - } = buildContentDuplicate(editableParent.value, editableContent.value, { - duplicateImage: duplicateImageOnCopy.value, - }); + const { parent: clonedParent, content: clonedContent } = buildContentDuplicate( + editableParent.value, + editableContent.value, + { duplicateImage: duplicateImageOnCopy.value }, + ); source.installClones(clonedParent, clonedContent); if (import.meta.env.MODE !== "test") { await router.replace({ @@ -337,13 +328,6 @@ const duplicate = async () => { "Successfully duplicated", `This ${props.tagOrPostType} has successfully been duplicated`, ); - if (imageOutcome === "noSourceBucket") { - notify( - "warning", - "Image not copied", - `No storage bucket is set on the original ${props.tagOrPostType}, so its image could not be copied. Set a storage bucket on the original, then duplicate it again.`, - ); - } }; const showLanguageSelector = ref(false); @@ -732,21 +716,8 @@ const actionsWrapperProps = computed(() => ({ class="mt-3 flex cursor-pointer select-none items-start gap-2 text-sm text-zinc-700" data-test="duplicate-image-toggle" > - + Duplicate image -

- No storage bucket is set on this {{ props.tagOrPostType }}, so its image cannot be - copied. Set a storage bucket first if you want the duplicate to keep the image. -

diff --git a/cms/src/components/content/EditContentDuplication.spec.ts b/cms/src/components/content/EditContentDuplication.spec.ts index 8e7dc6efb6..d49d5aee56 100644 --- a/cms/src/components/content/EditContentDuplication.spec.ts +++ b/cms/src/components/content/EditContentDuplication.spec.ts @@ -516,17 +516,20 @@ describe("EditContent.vue - Duplication", () => { // eslint-disable-next-line @typescript-eslint/no-explicit-any const vm: any = wrapper.vm; - // Image fileCollections should be preserved by default and duplicate intent should be set + // Image fileCollections should be preserved by default, and the clone should name the + // document the API copies the image from. expect(vm.editableParent.imageData.fileCollections.length).toBeGreaterThan(0); - expect(vm.editableParent.imageData.duplicate).toBe(true); + expect(vm.editableParent.imageData.duplicateFrom).toBe(mockData.mockPostDto._id); + expect(vm.editableParent.imageData.duplicate).toBeUndefined(); }, 15000); - it("warns when the source has image files but no storage bucket to copy them from", async () => { + it("carries the image across when the source has no local storage bucket", async () => { const mockNotification = vi.fn(); const notificationStore = useNotificationStore(); notificationStore.addNotification = mockNotification; - // mockPostDto carries fileCollections but no imageBucketId — the legacy shape. + // mockPostDto carries fileCollections but no imageBucketId — the legacy shape. The API + // resolves the bucket from the source document, so the copy no longer depends on it. expect(mockData.mockPostDto.imageData?.fileCollections.length).toBeGreaterThan(0); expect((mockData.mockPostDto as any).imageBucketId).toBeUndefined(); @@ -559,21 +562,21 @@ describe("EditContent.vue - Duplication", () => { confirmBtn = wrapper.find('[data-test="modal-primary-button"]'); expect(confirmBtn.exists()).toBe(true); }); - - // The modal says the image cannot travel before the duplicate is committed. - expect(wrapper.find("[data-test='duplicate-image-unavailable']").exists()).toBe(true); - await confirmBtn!.trigger("click"); await waitForExpect(() => { expect(mockNotification).toHaveBeenCalledWith( - expect.objectContaining({ title: "Image not copied", state: "warning" }), + expect.objectContaining({ title: "Successfully duplicated" }), ); }); // eslint-disable-next-line @typescript-eslint/no-explicit-any const vm: any = wrapper.vm; - expect(vm.editableParent.imageData.fileCollections).toEqual([]); + expect(vm.editableParent.imageData.fileCollections.length).toBeGreaterThan(0); + expect(vm.editableParent.imageData.duplicateFrom).toBe(mockData.mockPostDto._id); + expect(mockNotification).not.toHaveBeenCalledWith( + expect.objectContaining({ title: "Image not copied" }), + ); }, 15000); it("does not warn about the image when the source has a storage bucket", async () => { diff --git a/cms/src/components/content/util/buildContentDuplicate.ts b/cms/src/components/content/util/buildContentDuplicate.ts index 093980d43b..4c0ebadccf 100644 --- a/cms/src/components/content/util/buildContentDuplicate.ts +++ b/cms/src/components/content/util/buildContentDuplicate.ts @@ -8,21 +8,17 @@ import { } from "luminary-shared"; import * as _ from "lodash"; -/** - * What happened to the source image when building the duplicate. - * `noSourceBucket` is a failure the caller must surface — the source has image files but no - * bucket to copy them from, so the duplicate is created without an image. - */ -export type DuplicateImageOutcome = "copied" | "skipped" | "none" | "noSourceBucket"; +/** What happened to the source image when building the duplicate. */ +export type DuplicateImageOutcome = "copied" | "skipped" | "none"; /** * Build unsaved duplicate clones of a content parent and its translations: fresh ids, * stripped `_rev`, drafted + "(Copy)"/"-copy"-suffixed children. Returns new objects; * the inputs are not mutated. * - * `duplicateImage` carries over a copyable image collection — only possible when the - * source actually has an image bucket; otherwise the collection is cleared and the - * returned `imageOutcome` says why. + * `duplicateImage` names the source on the clone so the API can copy its image across. The + * source bucket and files are resolved server-side, so a stale or missing local bucket + * reference cannot lose the image. */ export function buildContentDuplicate( parent: ContentParentDto, @@ -37,22 +33,16 @@ export function buildContentDuplicate( let imageOutcome: DuplicateImageOutcome = "none"; if (clonedParent.imageData) { - const imageData = clonedParent.imageData as typeof clonedParent.imageData & { - duplicate?: boolean; - }; - delete clonedParent.imageData.uploadData; + const imageData = clonedParent.imageData; + delete imageData.uploadData; delete imageData.duplicate; - if (imageData.fileCollections?.length > 0) { - if (!options.duplicateImage) { - imageData.fileCollections = []; - imageOutcome = "skipped"; - } else if (parent.imageBucketId) { - imageData.duplicate = true; - imageOutcome = "copied"; - } else { - imageData.fileCollections = []; - imageOutcome = "noSourceBucket"; - } + delete imageData.duplicateFrom; + if (imageData.fileCollections?.length > 0 && options.duplicateImage) { + imageData.duplicateFrom = parent._id; + imageOutcome = "copied"; + } else if (imageData.fileCollections?.length > 0) { + imageData.fileCollections = []; + imageOutcome = "skipped"; } else if (imageData.fileCollections) { imageData.fileCollections = []; } diff --git a/shared/src/types/dto.ts b/shared/src/types/dto.ts index bf5e4f0f7b..66984b1383 100644 --- a/shared/src/types/dto.ts +++ b/shared/src/types/dto.ts @@ -183,6 +183,12 @@ export type GroupDto = BaseDocumentDto & { export type ImageDto = { fileCollections: ImageFileCollectionDto[]; uploadData?: ImageUploadDto[]; + /** + * Id of the parent this image is being copied from. The API resolves the source bucket and + * files from that document, so the copy does not depend on the client's own state. + */ + duplicateFrom?: Uuid; + /** @deprecated Superseded by `duplicateFrom`; still accepted from older clients. */ duplicate?: boolean; }; From eb63a20612d5b77456bcf9d4dfe571f93b0b4faa Mon Sep 17 00:00:00 2001 From: Dirk Date: Thu, 3 Sep 2026 08:30:01 +0200 Subject: [PATCH 4/4] fix(validateChangeRequestAccess): handle image duplication source access checks --- .../processPostTagDto.spec.ts | 2 + .../validateChangeRequestAccess.spec.ts | 51 ++++++++----------- 2 files changed, 24 insertions(+), 29 deletions(-) diff --git a/api/src/changeRequests/documentProcessing/processPostTagDto.spec.ts b/api/src/changeRequests/documentProcessing/processPostTagDto.spec.ts index 242439332e..9cf84ccd39 100644 --- a/api/src/changeRequests/documentProcessing/processPostTagDto.spec.ts +++ b/api/src/changeRequests/documentProcessing/processPostTagDto.spec.ts @@ -281,6 +281,7 @@ describe("processPostTagDto", () => { db, (changeRequest.doc as PostDto).imageBucketId, undefined, + undefined, // no duplication source bucket ); }); @@ -440,6 +441,7 @@ describe("processPostTagDto", () => { db, undefined, // no bucketId undefined, // no prevBucketId + undefined, // no duplication source bucket ); expect(result.warnings).toContain("Bucket is not specified for image processing."); }); diff --git a/api/src/changeRequests/validateChangeRequestAccess.spec.ts b/api/src/changeRequests/validateChangeRequestAccess.spec.ts index 4de27d5835..e71c4891a9 100644 --- a/api/src/changeRequests/validateChangeRequestAccess.spec.ts +++ b/api/src/changeRequests/validateChangeRequestAccess.spec.ts @@ -810,55 +810,48 @@ describe("validateChangeRequestAccess", () => { }); }); describe("image duplication source access", () => { - it("rejects a duplicate naming a source the user cannot view", async () => { - const changeReq = plainToClass(ChangeReqDto, { + // An existing public post is saved so the request clears the edit and group assign checks, + // leaving the duplication source as the only difference between these tests. + const duplicateChangeReq = (duplicateFrom: string) => + plainToClass(ChangeReqDto, { doc: { - _id: "post-duplicate-of-private", + _id: "post-page1", type: "post", memberOf: ["group-public-content"], image: "", tags: [], - imageData: { fileCollections: [], duplicateFrom: "post-blog2" }, + imageData: { fileCollections: [], duplicateFrom }, }, }); - const res = await validateChangeRequestAccess(changeReq, ["group-public-editors"], db); + it("rejects a duplicate naming a source the user cannot view", async () => { + const res = await validateChangeRequestAccess( + duplicateChangeReq("post-blog2"), + ["group-public-editors"], + db, + ); expect(res.validated).toBe(false); expect(res.error).toBe("No 'View' access to the document the image is duplicated from"); }); it("rejects a duplicate naming a source that does not exist", async () => { - const changeReq = plainToClass(ChangeReqDto, { - doc: { - _id: "post-duplicate-of-missing", - type: "post", - memberOf: ["group-public-content"], - image: "", - tags: [], - imageData: { fileCollections: [], duplicateFrom: "post-does-not-exist" }, - }, - }); - - const res = await validateChangeRequestAccess(changeReq, ["group-public-editors"], db); + const res = await validateChangeRequestAccess( + duplicateChangeReq("post-does-not-exist"), + ["group-public-editors"], + db, + ); expect(res.validated).toBe(false); expect(res.error).toBe("No 'View' access to the document the image is duplicated from"); }); it("accepts a duplicate naming a source the user can view", async () => { - const changeReq = plainToClass(ChangeReqDto, { - doc: { - _id: "post-duplicate-of-blog1", - type: "post", - memberOf: ["group-public-content"], - image: "", - tags: [], - imageData: { fileCollections: [], duplicateFrom: "post-blog1" }, - }, - }); - - const res = await validateChangeRequestAccess(changeReq, ["group-public-editors"], db); + const res = await validateChangeRequestAccess( + duplicateChangeReq("post-blog1"), + ["group-public-editors"], + db, + ); expect(res.validated).toBe(true); });