Skip to content
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

New System App Module for easy file systems access #663

Open
wants to merge 45 commits into
base: main
Choose a base branch
from

Conversation

IceOnly
Copy link

@IceOnly IceOnly commented Feb 29, 2024

Migration of microsoft/ALAppExtensions#23225

Now that we have made several clients Universal code ready.
One of the main problems turned out to be the replacement of the file record and the file object (File.Create, File.Exists).
Some of our customers now use Azure Blob Service from the System app. Others don't have a good internet and now use a local microservice with REST API, others use Azure File Shares.
The problem is that every integration is different. I think many other partners will have the same problem.
To simplify access I have adopted the email module and created new file accounts.
With just one codeunit, a developer can now connect to various file services without having to know how they actually work. The code unit "File System" delviers everything taht is needed.

An additional PR contains three connector apps:
microsoft/ALAppExtensions#23225

This Apps will conenct:

  • Azure Blob Storage
  • Azure File Share
  • SharePoint Online

All three service can be access over one unified interface!

New provider in the future could be:

  • a OneDrive Provider
  • Third Party file services

An example that shows how simple it is to use the new modules can be found here:
https://github.com/IceOnly/BC_FileSystem_Example

But before I go round the whole thing, I'm interested in whether there is any interest in the module at all.

Here are some Screenshots:
image
image
image
image
image
image

I have decided in favour of some restrictions. Paths must not start with a /. The only supported path separator is /. If services that require a \ are to be connected, this must be translated by the connector app.

These restrictions should ensure that the file service can be exchanged without upgrading data.

Copy link

Could not find linked issues in the pull request description. Please make sure the pull request description contains a line that contains 'Fixes #' followed by the issue number being fixed. Use that pattern for every issue you want to link.

Copy link
Contributor

@JesperSchulz JesperSchulz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR needs some work before the real code review can conducted.

@IceOnly IceOnly marked this pull request as draft February 29, 2024 13:40
@JesperSchulz JesperSchulz changed the title New System App Module for easy file systems access [DRAFT] New System App Module for easy file systems access Feb 29, 2024
@IceOnly IceOnly marked this pull request as ready for review March 1, 2024 15:08
@IceOnly IceOnly changed the title [DRAFT] New System App Module for easy file systems access New System App Module for easy file systems access Mar 1, 2024
@IceOnly
Copy link
Author

IceOnly commented Sep 17, 2024

@JesperSchulz done!

@JesperSchulz
Copy link
Contributor

If we are to accept this module, we want to have some uptake of it in the Base Application. We're currently looking into possible scenarios!

Background: ideally, we no longer want to have modules without any uptake - for 2 reasons:

  1. When we don't have uptake, we don't test the features in manual E2E tests, and hence often don't catch bugs
  2. Even though considerable work went into developing the module, the standard Business Central customer doesn't get any value out of it, as the capability doesn't surface in a vanilla implementation.

Work is in progress!

JesperSchulz
JesperSchulz previously approved these changes Nov 15, 2024
src/System Application/App/File System/app.json Outdated Show resolved Hide resolved
@JesperSchulz
Copy link
Contributor

@IceOnly, those were the wrong app.jsons 😉 You should pull main into your branch, and then you should update your app.jsons to the latest version. Then I hope your code compiles and all tests pass, and if they do, we can merge.

@IceOnly
Copy link
Author

IceOnly commented Nov 15, 2024

@JesperSchulz should be correct now 🙈

Copy link
Contributor

@StefanSosic StefanSosic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commented stuff for reimplementation.

@JesperSchulz @IceOnly
Due very large number of comments for smaller things, especially spellings, but also some sorting, app.json, I created new PR, from my fork to IceOnly fork.
PR: IceOnly#1

@IceOnly, before reimplementation, review my changes on PR, approve/merge to this branch. Otherwise it would have conflicts and you would do the fixes by yourself 😁

Let's get this rolling into new release 🚀

InStream: InStream;
ConnectorLogoDescriptionTxt: Label '%1 Logo', Locked = true;
begin
ConnectorLogoBase64 := Connector.GetLogoAsBase64();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be better to have GetLogoAsBase64 parameter for Logo and return parameter for if Success, so you would end with implementation like this:

if Connector.GetLogoAsBase64(ConnectorLogoBase64) then
exit;

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it were an internal procedure, I would agree. However, since it is an interface that is implemented multiple times, it makes little sense to implement this redundant code in every implementation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't have anything against it, doesn't do anything except esthetic :) Close this one than

Caption = 'Specify the type of file account to add';
}

repeater(Connectors)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the warning isn't true NavogatePages supports repeaters in webclient. And it is used more than once in Business Central, like the Email Accoutn Wizard or the Compant Creation Wizard.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now 😁 Let it be than, Microsoft will than later do reimplementation on all places, we could just been one step ahead.

PathCannotStartWithSlashErr: Label 'The path %1 can not start with /.', Comment = '%1 - Path';

begin
if Path.StartsWith('/') then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about error here, maybe it would be smarter to trim if it starts with /?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Smarter, but intransparent. I like to define all paths over all connectors have to be in the same format.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, just wanted to point out that and to make sure it's intended. Close this one also than :)

@IceOnly
Copy link
Author

IceOnly commented Nov 16, 2024

Commented stuff for reimplementation.

@JesperSchulz @IceOnly Due very large number of comments for smaller things, especially spellings, but also some sorting, app.json, I created new PR, from my fork to IceOnly fork. PR: IceOnly#1

@IceOnly, before reimplementation, review my changes on PR, approve/merge to this branch. Otherwise it would have conflicts and you would do the fixes by yourself 😁

Let's get this rolling into new release 🚀

Thank you @StefanSosic!
I acccepted all changes, except the renaming of the codeunit "File Scenario". Renaming it to "File Scenario Mgt." will break the Facade Naming Pattern.

Copy link
Contributor

@StefanSosic StefanSosic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@IceOnly, thank you for the fast addressing of everything! Amazing!!!

@JesperSchulz As agreed, stepped in, all reviewed. From my side green light 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AL: System Application From Fork Pull request is coming from a fork Integration GitHub request for Integration area
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants