Skip to content

[RED] Failing tests: migrating a document attachment deletes shared Tenant Media - #10055

Draft
Magnus Hartvig Grønbech (Groenbech96) wants to merge 1 commit into
mainfrom
copilot/da-shared-tenant-media
Draft

[RED] Failing tests: migrating a document attachment deletes shared Tenant Media#10055
Magnus Hartvig Grønbech (Groenbech96) wants to merge 1 commit into
mainfrom
copilot/da-shared-tenant-media

Conversation

@Groenbech96

@Groenbech96 Magnus Hartvig Grønbech (Groenbech96) commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Red run - these tests are expected to FAIL

This commit changes test code only. The production code is untouched, exactly as it is on main. That is deliberate: the failures are attributable to the defect alone, not to any restructuring of the code under test. The fix follows in a separate commit on this PR.

The defect

DA External Storage Impl.DeleteFromInternalStorage deletes the Tenant Media row unconditionally after moving an attachment to external storage:

if TenantMedia.Get(DocumentAttachment."Document Reference ID".MediaId()) then begin
    TenantMedia.Delete();                        // <-- global act, decided per record
    DocumentAttachment.MarkAsDeletedInternally();

Tenant Media is shared storage. Document Attachment Mgmt.CopyAttachments / CopyAttachmentsForPostedDocs copy attachments onto posted documents with TransferFields + Insert, which copies the media reference rather than the bytes, and Document Attachment.OnInsert never re-imports content. So Vendor -> Purchase Header -> Purch. Inv. Header attachments all end up referencing a single Tenant Media row.

Deleting that row on behalf of one attachment destroys the content of every attachment copied from it. Those attachments are then skipped silently by UploadToExternalStorage:

if not DocumentAttachment."Document Reference ID".HasValue() then
    exit(false);        // no telemetry - this guard sits above LogFeatureUsed()

Media.HasValue() also verifies the media exists in the database, so it returns false and the record is left with Stored Externally = false, a blank External File Path, and no internal content. Opening it then raises Tenant Media does not exist. ID='{00000000-0000-0000-0000-000000000000}' from GetAsTempBlob.

Note the app already guards the external blob against exactly this hazard - "Skip Delete On Copy" := ToDocumentAttachment."Stored Externally" protects DeleteFromExternalStorage. The equivalent protection was never applied to internal Tenant Media.

Field impact

Observed on a production tenant migrating ~5,200 attachments in one company:

Records iterated by report 8752 5,226
Uploaded successfully 4,810
Silently skipped, content destroyed 416

Skips were 0/min while processing Vendor (23), Item (27), Sales Header (36), Purchase Header (38) and Sales Invoice Header (112), then rose to 30-60/min for the entire Purch. Inv. Header (122) block - i.e. the copy targets started failing only once their sources had been migrated.

Tests added

DA Ext. Storage Impl. Tests:

Test Asserts Expected on this commit
DeleteFromInternalKeepsMediaSharedWithCopiedAttachment Shared Tenant Media survives; the copy keeps its content FAIL
DeleteFromInternalRemovesMediaWhenNotShared Space is still reclaimed when the attachment is the sole owner PASS
UploadSucceedsForCopiedAttachmentAfterSourceIsMigrated After the source is migrated, the copy still uploads and gets its own external file FAIL

The middle test is expected to pass both before and after the fix. It is there to prove the fix does not simply stop reclaiming database space, which is the whole point of the Move operation.

A helper CreateCopyOfDocumentAttachment mirrors how Document Attachment Mgmt copies attachments, and asserts the shared-media precondition, so the tests cannot pass for the wrong reason if that copy behaviour ever changes.

The fix (next commit)

OtherDocumentAttachment.SetRange("Document Reference ID", DocumentAttachment."Document Reference ID");
if OtherDocumentAttachment.Count() <= 1 then
    if TenantMedia.Get(DocumentAttachment."Document Reference ID".MediaId()) then
        TenantMedia.Delete();

Index-backed by the existing key(Key2; "Document Reference ID") on table 1173. Count() > 1 rather than excluding self, since the primary key is five fields and "more than one owner" is the actual question.

@github-actions github-actions Bot added AL: Apps (W1) Add-on apps for W1 Integration GitHub request for Integration area labels Aug 7, 2026
@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Could not find a linked ADO work item. Please link one by using the pattern 'AB#' followed by the relevant work item number. You may use the 'Fixes' keyword to automatically resolve the work item when the pull request is merged. E.g. 'Fixes AB#1234'

…ation

Moving a document attachment to external storage deletes its Tenant Media
row unconditionally. Tenant Media is shared storage: Document Attachment
Mgmt copies attachments onto posted documents with TransferFields, which
copies the media reference itself, so several Document Attachment rows can
reference a single Tenant Media row.

Deleting that row on behalf of one attachment therefore destroys the content
of every other attachment copied from it. Those attachments are then skipped
silently by UploadToExternalStorage, because Media.HasValue() also verifies
that the media exists in the database, so they end up with neither internal
content nor an external file.

This commit contains tests only. The production code is unchanged, so the
failures are attributable to the defect alone. The fix follows in a separate
commit.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 45cf5afe-6954-4876-bcec-d2e04bfcb1d8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AL: Apps (W1) Add-on apps for W1 Integration GitHub request for Integration area

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant