-
Notifications
You must be signed in to change notification settings - Fork 627
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
Azure Blob Storage, Azure File Share and SharePoint Online Connector Apps #23225
base: main
Are you sure you want to change the base?
Conversation
Apps/W1/File - Azure BLOB Storage Connector/app/src/BlobStorageAccount.Page.al
Outdated
Show resolved
Hide resolved
Apps/W1/File - Azure BLOB Storage Connector/app/src/BlobStorageConnectorImpl.Codeunit.al
Outdated
Show resolved
Hide resolved
Apps/W1/File - Azure BLOB Storage Connector/app/src/BlobStorageConnectorImpl.Codeunit.al
Outdated
Show resolved
Hide resolved
Modules/System/File Access/src/Connector/FileConnector.Interface.al
Outdated
Show resolved
Hide resolved
Modules/System/File Access/src/Connector/FileConnector.Interface.al
Outdated
Show resolved
Hide resolved
Modules/System/File Access/src/Connector/FileConnector.Interface.al
Outdated
Show resolved
Hide resolved
Some general thoughts: Additionally we might need some additional kind of top level selection before List Directories: Further more if I take a look at the current sharepoint client. I have no idea how we should map this to the current provided Sometimes a path isn't enough and we might need more information to download a file. Maybe someone else with more experience has some ideas how this access can be unified... |
Why not listing C: and D: as directory on the top level? However, I am not sure if the module should define the path separator. So all Services uses the same path structure and the implentation need to translate if needed. |
Apps/W1/File - Azure BLOB Storage Connector/app/src/BlobStorageConnectorImpl.Codeunit.al
Outdated
Show resolved
Hide resolved
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.
Looks cool! I have a few small performance and security concerns, which I have addressed, and some minor other stuff, but nothing major. I'm excited for this.
Apps/W1/File - Azure BLOB Storage Connector/app/src/BlobStorageAccount.Table.al
Outdated
Show resolved
Hide resolved
Apps/W1/File - Azure BLOB Storage Connector/app/src/BlobStorageAccount.Table.al
Outdated
Show resolved
Hide resolved
Apps/W1/File - Azure BLOB Storage Connector/app/src/BlobStorageAccountWizard.Page.al
Outdated
Show resolved
Hide resolved
Apps/W1/File - Azure BLOB Storage Connector/app/src/BlobStorageAccountWizard.Page.al
Outdated
Show resolved
Hide resolved
Modules/System/File Access/src/Account/FileAccountImpl.Codeunit.al
Outdated
Show resolved
Hide resolved
Let me amplify this on Twitter/X to get your initial question answered: "before I go round the whole thing, I'm interested in whether there is any interest in the module at all". Let's see what people have to say! 😊 |
I support every effort to improve file handling. |
Wonderful! Please merge this 👍🏻 |
Easily overlooked feature but a needed one. Huge development effort that should not be thrown away. |
Bravo! Love seeing more 'helper' features/functions that eventually will benefit all of us! |
@IceOnly, this PR seems to have stagnated a little (which is perfectly fine). I was just wondering if you want to push this forward at some point, or if you've changed your mind / cannot find the time? There seems to be plenty of interest for this module in the community! |
It is the time. I will try to find some free time to make the last changes. This are my open points:
|
Feel free to do it at your own pace. I didn't mean to stress you. I was just curious if you still intend to continue work. If you do, we'll just leave this PR open. No problem whatsoever. |
…rePoint connectors
var | ||
ErrorOccuredErr: Label 'An error occured.\%1', Comment = '%1 - Error message from sharepoint'; | ||
begin | ||
Error(ErrorOccuredErr, SharePointClient.GetDiagnostics().GetErrorMessage()); |
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.
Thought; could we add ResponseReasonPhrase to the message if ErrorMessage is empty?
Two cases where I came across this were when I misconfigured the Sharepoint URLs, and I was getting "An error occurred." with no details, but when debugging I saw it returned a 403.
Second one was when I tried to create a file in a subfolder that doesn't exist. Error Message is empty, but 404 Not Found was still in the diagnostics.
On the one hand, I know that's not really informative for the user, but otherwise I always have to debug, to understand what went wrong...
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.
Could also be an improvement we add later in a follow-up PR. Doesn't strike me as critical to get this PR on the road. But it's a good idea for sure. We should probably use ErrorInfo to shield the normal user from such details, while providing these important pointers in a different dimension of the error message.
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.
Coming along nicely! I couldn't find anything serious, but would suggest to revisit the permissions.
|
||
Permissions = | ||
table "Blob Storage Account" = X, | ||
codeunit "Blob Storage Connector Impl." = X, |
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.
Why not use inherent permissions/entitlements on most of these objects. Not ever would a system admin be interested in controlling the execute permissions of e.g. an Impl. codeunit. Let's only have the permissions configurable, which an admin actually could care about!
Apps/W1/External File Storage - Azure Blob Service Connector/app/src/BlobStorageAccount.Page.al
Outdated
Show resolved
Hide resolved
Apps/W1/External File Storage - Azure File Service Connector/app/src/FileShareAccount.Table.al
Outdated
Show resolved
Hide resolved
Apps/W1/External File Storage - Azure File Service Connector/app/src/FileShareAuthType.Enum.al
Outdated
Show resolved
Hide resolved
Apps/W1/External File Storage - SharePoint Connector/app/src/SharePointAccount.Table.al
Outdated
Show resolved
Hide resolved
var | ||
ErrorOccuredErr: Label 'An error occured.\%1', Comment = '%1 - Error message from sharepoint'; | ||
begin | ||
Error(ErrorOccuredErr, SharePointClient.GetDiagnostics().GetErrorMessage()); |
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.
Could also be an improvement we add later in a follow-up PR. Doesn't strike me as critical to get this PR on the road. But it's a good idea for sure. We should probably use ErrorInfo to shield the normal user from such details, while providing these important pointers in a different dimension of the error message.
…int Account tables; fix enum value formatting in Ext. File Share Auth. Type
…ePoint connectors; include inherent permissions and entitlements
…ePoint connectors to include "Ext." prefix
@IceOnly, we got an issue to fix to make CI work again. ETA is tomorrow or Wednesday. Once we're done, could you pull from main and see if your PR passes too? I'll ping you when we have a successful build again. |
I tried using the connectors with our apps, and I must again say that this is a great addition! One final thing I'd try to get sorted out for this PR is to make the Blob Storage and File Share tables public as well. The use case is that I'd store files in either of those storage types from BC and then send the file URI to an external service that can pull it (File based integrations). Right now I have to keep Storage Account, Container, and File Share Name in a separate record to build up a URI. |
…lds in Azure Blob, File Share, and SharePoint account tables
I made the other setup tables global as well. |
Found one more case that would probably need to be dealt with within the connector app. Copying a production environment to a sandbox (mabye also copy companies?). We don't want changes in a sandbox environment to be reflected in production at all, so during the copy part, the settings or at the very least the credentials should be dropped. |
…e account tables, Disable on Enviroment Copy
Good idea. I added a disabled field to all account tables. It will be set automaticly on enviroment copy. |
I am super eager to get this one the road. Community, could we get some sign-offs? Once we've got those (and we've fixed our uptake issues 🤦♂️), we'll move this PR into the next stage! My goal is to have this completed before end of this month 😊 |
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 added some comments.
You don't necessary need to change everything, but I wanted to give you my hones opinion on the current PR.
nit: if you have the time or use the known VS Code extensions, then you could sort the properties of the pages.
.../External File Storage - Azure Blob Service Connector/app/src/ExtBlobStorageAuthType.Enum.al
Outdated
Show resolved
Hide resolved
...ternal File Storage - Azure File Service Connector/app/src/ExtFileShareAccountWizard.Page.al
Outdated
Show resolved
Hide resolved
Rec.Init(); | ||
Rec.Insert(); | ||
|
||
if MediaResources.Get(AssistedSetupLogoTok) and (CurrentClientType() = ClientType::Web) then |
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.
@JesperSchulz do we already have to prepare for this upcoming change when the Images are removed from the MediaResources table?
microsoft/BCApps#2391
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 definitely try to do things "the new way" where possible.
...ternal File Storage - Azure File Service Connector/app/src/ExtFileShareAccountWizard.Page.al
Outdated
Show resolved
Hide resolved
|
||
permissionset 4582 "Ext. SharePoint - Edit" | ||
{ | ||
Assignable = false; |
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.
Whats' the use for permission Sets that are Public but not assignable?
Is this really on purpose?
Apps/W1/External File Storage - SharePoint Connector/app/src/ExtSharePointAccount.Table.al
Show resolved
Hide resolved
.../External File Storage - SharePoint Connector/app/src/ExtSharePointConnectorImpl.Codeunit.al
Outdated
Show resolved
Hide resolved
...orage - Azure Blob Service Connector/app/Entitlements/ExtBlobStorageConnector.Entitlement.al
Outdated
Show resolved
Hide resolved
// ------------------------------------------------------------------------------------------------ | ||
|
||
namespace System.ExternalFileStorage; | ||
|
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.
Will this permissionset extension allow partners to set this up as delegated admin agent? (@JesperSchulz could you maybe forward this to someone with knowledge on this topic?)
.../W1/External File Storage - Azure File Service Connector/app/src/ExtFileShareAccount.Page.al
Outdated
Show resolved
Hide resolved
…zure Blob, File Share, and SharePoint account pages
…hare, and SharePoint account tables
…lob, File Share, and SharePoint
…nto feature/FileAccess
This PR contains three new connector apps, to connect Azure Blob Storage, Azure File Share and SharePoint Online with the New File System Module in the System App.
File System Module PR:
microsoft/BCApps#663
Here are some Screenshots:
Fixes #22691
Fixes AB#559148