diff --git a/src/Apps/W1/External Storage - Document Attachments/Test/src/DAExtStorageImplTests.Codeunit.al b/src/Apps/W1/External Storage - Document Attachments/Test/src/DAExtStorageImplTests.Codeunit.al index 8eb6669c20..8cb55920e1 100644 --- a/src/Apps/W1/External Storage - Document Attachments/Test/src/DAExtStorageImplTests.Codeunit.al +++ b/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] diff --git a/src/Apps/W1/External Storage - Document Attachments/app/src/DocumentAttachmentIntegration/DAExternalStorageImpl.Codeunit.al b/src/Apps/W1/External Storage - Document Attachments/app/src/DocumentAttachmentIntegration/DAExternalStorageImpl.Codeunit.al index e1938182b9..b392fb8c56 100644 --- a/src/Apps/W1/External Storage - Document Attachments/app/src/DocumentAttachmentIntegration/DAExternalStorageImpl.Codeunit.al +++ b/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" /// The document attachment record to delete from external storage. /// True if deletion was successful, false otherwise. procedure DeleteFromExternalStorage(var DocumentAttachment: Record "Document Attachment"): Boolean - var - FileAccount: 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 + FileAccount: 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, FileAccount) 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; /// @@ -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; /// diff --git a/src/System Application/Test Library/External File Storage/src/Mock/FileConnectorMock.Codeunit.al b/src/System Application/Test Library/External File Storage/src/Mock/FileConnectorMock.Codeunit.al index 9a9b580e0c..aed2edee61 100644 --- a/src/System Application/Test Library/External File Storage/src/Mock/FileConnectorMock.Codeunit.al +++ b/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; } \ No newline at end of file diff --git a/src/System Application/Test Library/External File Storage/src/TestFileStorageConnector.Codeunit.al b/src/System Application/Test Library/External File Storage/src/TestFileStorageConnector.Codeunit.al index dd52da94dc..9dfe8b9806 100644 --- a/src/System Application/Test Library/External File Storage/src/TestFileStorageConnector.Codeunit.al +++ b/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; } \ No newline at end of file