Bug 636342: External Storage - Document Attachments does not delete blobs on attachment delete

src/Apps/W1/External Storage - Document Attachments/Test/src/DAExtStorageImplTests.Codeunit.al

@@ -368,6 +368,132 @@ codeunit 136820 "DA Ext. Storage Impl. Tests"
 
     #endregion
 
+    #region OnAfterDelete Subscriber Tests
+
+    [Test]
+    [HandlerFunctions('ConfirmYesHandler')]
+    procedure RecordDeleteRemovesBlobFromExternalStorage()
+    var
+        DocumentAttachment: Record "Document Attachment";
+        DAExternalStorageImpl: Codeunit "DA External Storage Impl.";
+        ExternalFilePath: Text;
+    begin
+        // [SCENARIO] Deleting a Document Attachment row must delete its blob via the OnAfterDelete subscriber.
+        // Regression test for the bug where the subscriber called DeleteFromExternalStorage(Rec), which
+        // started with Rec.Find() and exited because the row was already gone, leaving the blob orphaned.
+        Initialize();
+        SetupFileScenarioWithTestConnector();
+        EnableFeatureWithDelete();
+
+        // [GIVEN] A document attachment that has been uploaded to external storage
+        CreateDocumentAttachmentWithContent(DocumentAttachment);
+        DAExternalStorageImpl.UploadToExternalStorage(DocumentAttachment);
+        DocumentAttachment.SetRecFilter();
+        DocumentAttachment.FindFirst();
+        ExternalFilePath := DocumentAttachment."External File Path";
+        Assert.AreNotEqual('', ExternalFilePath, 'Precondition: upload should set the External File Path');
+
+        // [WHEN] The Document Attachment row is deleted (fires OnAfterDeleteEvent)
+        DocumentAttachment.Delete(true);
+
+        // [THEN] The subscriber invoked DeleteFile against the external connector with the stored path
+        Assert.AreEqual(ExternalFilePath, FileConnectorMock.GetLastDeletedPath(),
+            'External connector DeleteFile should be invoked with the stored External File Path when the attachment row is deleted');
+    end;
+
+    [Test]
+    [HandlerFunctions('ConfirmYesHandler')]
+    procedure RecordDeleteKeepsBlobWhenSkipDeleteOnCopyIsSet()
+    var
+        DocumentAttachment: Record "Document Attachment";
+        DAExternalStorageImpl: Codeunit "DA External Storage Impl.";
+        ExternalFilePath: Text;
+    begin
+        // [SCENARIO] When the row was created by an attachment copy, the blob is shared with the source
+        // and must NOT be deleted when the copy is removed.
+        Initialize();
+        SetupFileScenarioWithTestConnector();
+        EnableFeatureWithDelete();
+
+        // [GIVEN] An externally-stored attachment flagged as a copy
+        CreateDocumentAttachmentWithContent(DocumentAttachment);
+        DAExternalStorageImpl.UploadToExternalStorage(DocumentAttachment);
+        DocumentAttachment.SetRecFilter();
+        DocumentAttachment.FindFirst();
+        DocumentAttachment."Skip Delete On Copy" := true;
+        DocumentAttachment.Modify();
+        ExternalFilePath := DocumentAttachment."External File Path";
+
+        // [WHEN] The row is deleted
+        DocumentAttachment.Delete(true);
+
+        // [THEN] The subscriber must NOT invoke DeleteFile for this path
+        Assert.AreNotEqual(ExternalFilePath, FileConnectorMock.GetLastDeletedPath(),
+            'External connector DeleteFile should not be invoked when Skip Delete On Copy is set');
+    end;
+
+    [Test]
+    [HandlerFunctions('ConfirmYesHandler')]
+    procedure RecordDeleteKeepsBlobWhenFileIsFromAnotherEnvironment()
+    var
+        DocumentAttachment: Record "Document Attachment";
+        DAExternalStorageImpl: Codeunit "DA External Storage Impl.";
+        ExternalFilePath: Text;
+    begin
+        // [SCENARIO] Files owned by another environment or company must not be deleted
+        // when the local attachment row is removed - the owning environment is responsible
+        // for the blob's lifecycle.
+        Initialize();
+        SetupFileScenarioWithTestConnector();
+        EnableFeatureWithDelete();
+
+        // [GIVEN] An externally-stored attachment carrying a foreign source environment hash
+        CreateDocumentAttachmentWithContent(DocumentAttachment);
+        DAExternalStorageImpl.UploadToExternalStorage(DocumentAttachment);
+        DocumentAttachment.SetRecFilter();
+        DocumentAttachment.FindFirst();
+        DocumentAttachment."Source Environment Hash" := 'DIFFERENTHASH123';
+        DocumentAttachment.Modify();
+        ExternalFilePath := DocumentAttachment."External File Path";
+
+        // [WHEN] The row is deleted
+        DocumentAttachment.Delete(true);
+
+        // [THEN] The subscriber must NOT invoke DeleteFile for this path
+        Assert.AreNotEqual(ExternalFilePath, FileConnectorMock.GetLastDeletedPath(),
+            'External connector DeleteFile should not be invoked when the file belongs to another environment or company');
+    end;
+
+    [Test]
+    [HandlerFunctions('ConfirmYesHandler')]
+    procedure RecordDeleteKeepsBlobWhenDeleteFromExternalStorageDisabled()
+    var
+        DocumentAttachment: Record "Document Attachment";
+        DAExternalStorageImpl: Codeunit "DA External Storage Impl.";
+        ExternalFilePath: Text;
+    begin
+        // [SCENARIO] When the user opted out of automatic deletion, the blob must stay even if the row is deleted.
+        Initialize();
+        SetupFileScenarioWithTestConnector();
+        EnableFeature(); // "Delete from External Storage" = false
+
+        // [GIVEN] An uploaded externally-stored attachment
+        CreateDocumentAttachmentWithContent(DocumentAttachment);
+        DAExternalStorageImpl.UploadToExternalStorage(DocumentAttachment);
+        DocumentAttachment.SetRecFilter();
+        DocumentAttachment.FindFirst();
+        ExternalFilePath := DocumentAttachment."External File Path";
+
+        // [WHEN] The row is deleted
+        DocumentAttachment.Delete(true);
+
+        // [THEN] The subscriber must NOT invoke DeleteFile when the feature setting opts out
+        Assert.AreNotEqual(ExternalFilePath, FileConnectorMock.GetLastDeletedPath(),
+            'External connector DeleteFile should not be invoked when Delete from External Storage is disabled');
+    end;
+
+    #endregion
+
     #region MIME Type Tests
 
     [Test]

src/Apps/W1/External Storage - Document Attachments/app/src/DocumentAttachmentIntegration/DAExternalStorageImpl.Codeunit.al

@@ -364,13 +364,6 @@ codeunit 8751 "DA External Storage Impl." implements "File Scenario"
     /// <param name="DocumentAttachment">The document attachment record to delete from external storage.</param>
     /// <returns>True if deletion was successful, false otherwise.</returns>
     procedure DeleteFromExternalStorage(var DocumentAttachment: Record "Document Attachment"): Boolean
-    var
-        TempFileAccount: Record "File Account";
-        ExternalFileStorage: Codeunit "External File Storage";
-        FileScenarioCU: Codeunit "File Scenario";
-        DAFeatureTelemetry: Codeunit "DA Feature Telemetry";
-        FileScenario: Enum "File Scenario";
-        ExternalFilePath: Text;
     begin
         // Check if feature is enabled
         if not IsFeatureEnabled() then
@@ -395,23 +388,31 @@ codeunit 8751 "DA External Storage Impl." implements "File Scenario"
             exit(true);
         end;
 
-        // Use the stored external file path
-        ExternalFilePath := DocumentAttachment."External File Path";
+        if not DeleteExternalFile(DocumentAttachment."External File Path", DocumentAttachment) then
+            exit(false);
 
-        // Search for External Storage assigned File Scenario
+        DocumentAttachment.MarkAsNotUploadedToExternal();
+        exit(true);
+    end;
+
+    local procedure DeleteExternalFile(ExternalFilePath: Text; DocumentAttachmentForTelemetry: Record "Document Attachment"): Boolean
+    var
+        TempFileAccount: Record "File Account";
+        ExternalFileStorage: Codeunit "External File Storage";
+        FileScenarioCU: Codeunit "File Scenario";
+        DAFeatureTelemetry: Codeunit "DA Feature Telemetry";
+        FileScenario: Enum "File Scenario";
+    begin
         FileScenario := FileScenario::"Doc. Attach. - External Storage";
         if not FileScenarioCU.GetSpecificFileAccount(FileScenario, TempFileAccount) then
             exit(false);
 
-        // Delete the file with connector using the File Account framework
         ExternalFileStorage.Initialize(FileScenario);
-        if ExternalFileStorage.DeleteFile(ExternalFilePath) then begin
-            DocumentAttachment.MarkAsNotUploadedToExternal();
-            DAFeatureTelemetry.LogFileDeleted(DocumentAttachment);
-            exit(true);
-        end;
+        if not ExternalFileStorage.DeleteFile(ExternalFilePath) then
+            exit(false);
 
-        exit(false);
+        DAFeatureTelemetry.LogFileDeleted(DocumentAttachmentForTelemetry);
+        exit(true);
     end;
 
     /// <summary>
@@ -800,7 +801,6 @@ codeunit 8751 "DA External Storage Impl." implements "File Scenario"
     local procedure OnAfterDeleteDocumentAttachment(var Rec: Record "Document Attachment"; RunTrigger: Boolean)
     var
         ExternalStorageSetup: Record "DA External Storage Setup";
-        ExternalStorageImpl: Codeunit "DA External Storage Impl.";
     begin
         // Exit early if trigger is not running
         if not RunTrigger then
@@ -810,7 +810,7 @@ codeunit 8751 "DA External Storage Impl." implements "File Scenario"
         if Rec.IsTemporary() then
             exit;
 
-        // Check if auto upload is enabled
+        // Check if auto delete is enabled
         if not ExternalStorageSetup.Get() then
             exit;
 
@@ -821,13 +821,21 @@ codeunit 8751 "DA External Storage Impl." implements "File Scenario"
         if not Rec."Stored Externally" then
             exit;
 
-        // Delete from external storage
-        ExternalStorageImpl.DeleteFromExternalStorage(Rec);
+        if Rec."External File Path" = '' then
+            exit;
 
-        if Rec."Skip Delete On Copy" then begin
-            Rec."Skip Delete On Copy" := false;
-            Rec.Modify();
-        end;
+        // Copied attachments share the source file - never delete the shared blob
+        if Rec."Skip Delete On Copy" then
+            exit;
+
+        // Files from another environment/company are managed by their owning environment
+        if IsFileFromAnotherEnvironmentOrCompany(Rec) then
+            exit;
+
+        // The Document Attachment row is already deleted at this point, so we cannot
+        // Find() or Modify() it. Delete the blob using the field values still carried
+        // on Rec, bypassing the record-based DeleteFromExternalStorage entry point.
+        DeleteExternalFile(Rec."External File Path", Rec);
     end;
 
     /// <summary>

src/System Application/Test Library/External File Storage/src/Mock/FileConnectorMock.Codeunit.al

@@ -17,6 +17,7 @@ codeunit 135810 "File Connector Mock"
     var
         TestFileAccount: Record "Test File Account";
         TestFileConnectorSetup: Record "Test File Connector Setup";
+        TestFileStorageConnector: Codeunit "Test File Storage Connector";
     begin
         TestFileConnectorSetup.DeleteAll();
         TestFileConnectorSetup.Init();
@@ -27,6 +28,8 @@ codeunit 135810 "File Connector Mock"
         TestFileConnectorSetup.Insert();
 
         TestFileAccount.DeleteAll();
+
+        TestFileStorageConnector.ResetLastDeletedPath();
     end;
 
     procedure GetAccounts(var FileAccount: Record "File Account")
@@ -116,4 +119,11 @@ codeunit 135810 "File Connector Mock"
         TestFileConnectorSetup."Unsuccessful Register" := Fail;
         TestFileConnectorSetup.Modify();
     end;
+
+    procedure GetLastDeletedPath(): Text
+    var
+        TestFileStorageConnector: Codeunit "Test File Storage Connector";
+    begin
+        exit(TestFileStorageConnector.GetLastDeletedPath());
+    end;
 }

src/System Application/Test Library/External File Storage/src/TestFileStorageConnector.Codeunit.al

@@ -9,6 +9,8 @@ using System.ExternalFileStorage;
 
 codeunit 135814 "Test File Storage Connector" implements "External File Storage Connector"
 {
+    SingleInstance = true;
+
     procedure ListFiles(AccountId: Guid; Path: Text; FilePaginationData: Codeunit "File Pagination Data"; var TempFileAccountContent: Record "File Account Content" temporary);
     begin
         TempFileAccountContent.Init();
@@ -44,6 +46,19 @@ codeunit 135814 "Test File Storage Connector" implements "External File Storage
 
     procedure DeleteFile(AccountId: Guid; Path: Text);
     begin
+        // The platform invokes connector callbacks inside a TryFunction, so we cannot
+        // Modify() a table from here. Stash the path in a SingleInstance global instead.
+        LastDeletedFilePath := Path;
+    end;
+
+    internal procedure GetLastDeletedPath(): Text
+    begin
+        exit(LastDeletedFilePath);
+    end;
+
+    internal procedure ResetLastDeletedPath()
+    begin
+        Clear(LastDeletedFilePath);
     end;
 
     procedure ListDirectories(AccountId: Guid; Path: Text; FilePaginationData: Codeunit "File Pagination Data"; var TempFileAccountContent: Record "File Account Content" temporary);
@@ -107,4 +122,5 @@ codeunit 135814 "Test File Storage Connector" implements "External File Storage
 
     var
         FileConnectorMock: Codeunit "File Connector Mock";
+        LastDeletedFilePath: Text;
 }