-
Notifications
You must be signed in to change notification settings - Fork 318
External Storage - Document Attachments #4495
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
External Storage - Document Attachments #4495
Conversation
|
@StefanSosic could maybe elaborate a little bit on your events. Couldn't we solve this by adding an well designed interface to the FileScenario.. Maybe something like the following interface FileScenario
procedure HasAdditionalSetup(): Boolean
procedure OpenAdditionalSetup(TBDParameters)
procedure CheckScenarioBeforeDeletion()
...Maybe you could show some example code how you would like to subscribe to this in the Microsoft BaseApp. |
The events created are providing possibility for additional control, which is not mandatory. That's why it's by design with events, not interface. This module is specific and we wanted to put disclaimer message before adding scenario and so on, but I think lot of scenarios won't use that feature. Setup action is using event as discovery method. Again, not mandatory to have and implement setup, that's why didn't went with interfaces. I did had a call to align with the concept, and events were mentioned. Let me try to bring more context to it, implementation of File Accounts with Scenarios shouldn't be limited to Base App and internal development, but give partners possibility to adjust and work on their own scenarios. GetAdditionalScenarioSetup - Discovery event if there is additional setup for a scenario, must not be existing For scenarios, we already have default one. In this case, this are optional features, which I see more that they won't end being used then they will, if they will it will be implemented one of these three for example. Don't want to implement interface and to use just one of the procedures, rest needs to be handled unnecessary. But I got your point, interface is nice and good, but just for this case I don't see it fitting, just because of type of procedures to be used. My scenario is for Document Attachments, but it could be just anything, Email, Sms, Backup, anything, in those case this interface procedure wouldn't make any sense. We need to look for a full picture and wide spectrum of implementation of File Scenarios and that could be anything. Module is generally done and PR's created, Jesper will take it for a spin on Monday. Base Application adjustments for External Storage Module: AL App Extension - Full Module: I am open for suggestions, keep in mind that this is first version, this is just handful of functionality, no need to introduce more and more, since we plan to push it to preview asap. And as you can see, point is to provide this as separate extension, and this should be also an demo for partners, how to implement their own File Scenario for any need. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am voting against these changes due to security risks. It's similar to discussion a few years back when similar events were discussed for Email Scenarios. This would allow anyone to block deletion or disabling of custom connectors and exposing too much control to PTE and ISV solutions for custom File Connectors.
If we want to have this ability in place, it should use interfaces so extensions are able to manage only own implementations not implementations from other partners.
These events basically cancel the generic approach for scenarios where the scenarios should be independent and secure from third-party injections. This is considered as one of the main advantages of scenarios where the implementation enforce some level of security of what is exposed and what other extensions can change. This implementation basically removes this control from partners and introduce (in my opinion) bad IsHandled pattern in place where it should not be used.
@JesperSchulz Please review this design before adding it to the product, these IsHandled in System Apps are not a good choice especially for modular apps. It was discussed in the past how "bad" the IsHandled pattern is and adding it to generic scenario extension is just wrong.
We should be advocating for more sophisticated code, especially in these main apps. Otherwise we end up with a lot of "IsHandled" patterns in PTEs, without thinking about the repercussions. |
...Document Attachments/app/src/DocumentAttachmentIntegration/DAExternalStorageImpl.Codeunit.al
Outdated
Show resolved
Hide resolved
|
@TKapitan @pri-kise @tonyabriccomeske |
pri-kise
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should have some test coverage for this new functionality.
I don't know if we already have a mocking Storage in the System Application, but we could add this here.
For starters I'm thinking about the following test cases
// [GIVEN] Mocked External Storage (Maybe via a internal table or single instance codeunit)
// [GIVEN] Valid Setup for Document Attachments Exteranl Storage provided
// [GIVEN] Document Attachment is created
// [WHEN] Document Attachment sync runs
// [THEN] Verify Document Attachment is updated correctlyThe following test case is an example for one procedure of the document attachment.
// [GIVEN] Mocked External Storage
// [GIVEN] Valid Setup for Document Attachments Exteranl Storage provided
// [GIVEN] Document Attachment is created
// [GIVEN] Document Attachment synced
// {WHEN] TemplBlob is retrieved via procedure from Document Attachment
// [THEN] Verify TempBlobWe should have tests for all the procedures that we handle.
src/System Application/App/External File Storage/src/Scenario/FileScenario.Interface.al
Show resolved
Hide resolved
...ment Attachments/app/src/DocumentAttachmentIntegration/DocumentAttachmentExtStor.TableExt.al
Outdated
Show resolved
Hide resolved
pri-kise
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might need to condsider to add extensions for some Entitlements.
...xternal Storage - Document Attachments/app/src/AutomaticSync/DAExternalStorageSync.Report.al
Outdated
Show resolved
Hide resolved
...Document Attachments/app/src/DocumentAttachmentIntegration/DAExternalStorageImpl.Codeunit.al
Show resolved
Hide resolved
|
I'd like to point out that all comments from the review have been addressed. Managed to accomplish copy company/environment/tenant |
pri-kise
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should have some test coverage for this new functionality.
I don't know if we already have a mocking Storage in the System Application, but we could add this here.
For starters I'm thinking about the following test cases
// [GIVEN] Mocked External Storage (Maybe via a internal table or single instance codeunit)
// [GIVEN] Valid Setup for Document Attachments Exteranl Storage provided
// [GIVEN] Document Attachment is created
// [WHEN] Document Attachment sync runs
// [THEN] Verify Document Attachment is updated correctlyThe following test case is an example for one procedure of the document attachment.
// [GIVEN] Mocked External Storage
// [GIVEN] Valid Setup for Document Attachments Exteranl Storage provided
// [GIVEN] Document Attachment is created
// [GIVEN] Document Attachment synced
// {WHEN] TemplBlob is retrieved via procedure from Document Attachment
// [THEN] Verify TempBlobWe should have tests for all the procedures that we handle.
...s/W1/External Storage - Document Attachments/app/src/Telemetry/DATelemetryLogger.Codeunit.al
Outdated
Show resolved
Hide resolved
…/sso/externalStorageApp
...xternal Storage - Document Attachments/app/src/AutomaticSync/DAExternalStorageSync.Report.al
Show resolved
Hide resolved
...Document Attachments/app/src/DocumentAttachmentIntegration/DAExternalStorageImpl.Codeunit.al
Outdated
Show resolved
Hide resolved
...Apps/W1/External Storage - Document Attachments/app/src/Setup/DAExternalStorageSetup.Page.al
Outdated
Show resolved
Hide resolved
...Document Attachments/app/src/DocumentAttachmentIntegration/DAExternalStorageImpl.Codeunit.al
Show resolved
Hide resolved
...Document Attachments/app/src/DocumentAttachmentIntegration/DAExternalStorageImpl.Codeunit.al
Outdated
Show resolved
Hide resolved
…nSosic/BCApps into dev/sso/externalStorageApp
…/sso/externalStorageApp


Summary
Document Attachments External Storage for Microsoft Dynamics 365 Business Central
Overview
The External Storage extension provides seamless integration between Microsoft Dynamics 365 Business Central and external storage systems such as Azure Blob Storage, SharePoint, and File Shares. This extension automatically manages document attachments by storing them in external storage systems while maintaining full functionality within Business Central.
Key Features
Automatic Upload
Flexible Deletion Policies
Bulk Operations
Installation & Setup
Prerequisites
Installation Steps
Configure File Account
Configure External Storage
Configuration Options
Auto Upload Settings
Usage
Automatic Mode
When Auto Upload is enabled:
Manual Operations
Individual File Operations
From Document Attachment - External page:
Bulk Operations
From External Storage Synchronize report:
File Access
© 2025 Microsoft Corporation. All rights reserved.
Work Item(s)
Fixes AB#592686
Fixes #4494