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

Discussion: Introduce company initialize in System Application #2317

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// ------------------------------------------------------------------------------------------------
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License. See License.txt in the project root for license information.
// ------------------------------------------------------------------------------------------------

namespace System.Environment.Configuration;

codeunit 900 "Company Initialize"
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be moved to the platform system events. Whenever a new company is created they should trigger the initialize company event.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a nice opportunity to improve the flows.

{
[IntegrationEvent(false, false)]
internal procedure InitializeCompanySetup()
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer On for the events, it increases readability

begin
end;

[IntegrationEvent(false, false)]
internal procedure InitializeCompany()
begin
end;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
// ------------------------------------------------------------------------------------------------
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License. See License.txt in the project root for license information.
// ------------------------------------------------------------------------------------------------

namespace System.Environment.Configuration;

table 900 "Company Initialize"
{
DataClassification = SystemMetadata;

fields
{
field(1; "Primary Key"; Integer)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be moved to the company table so we track for each company.
Keep in mind that it is by design that we can run it multiple times.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you explain why we want to run this multiple times?

{
}
field(2; "Initialized Time"; DateTime)
{
}
field(3; "Initialized Version"; Code[20])
{
}
Copy link
Contributor

Choose a reason for hiding this comment

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

One thing I'd add here is a field: IsCompanyInitialized (or something like that)

If you look at some code in BaseApp, we are leveraging the existences of GLSetup to determine this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I was using the fact that the table is empty as it not being initialized. Do you have a suggestion for when we want to insert a record but state that the company has not been initialized?

}

keys
{
key(Key1; "Primary Key")
{
Clustered = true;
}
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
// ------------------------------------------------------------------------------------------------
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License. See License.txt in the project root for license information.
// ------------------------------------------------------------------------------------------------

namespace System.Environment.Configuration;

using System.Environment;

codeunit 901 "Company Initialize Impl."
{
Access = Internal;

procedure InitializeCompany()
var
CompanyInitialize: Record "Company Initialize";
CompanyInitializeCodeunit: Codeunit "Company Initialize";
ModuleInfo: ModuleInfo;
begin
CompanyInitialize."Initialized Time" := CurrentDateTime();

if NavApp.GetCurrentModuleInfo(ModuleInfo) then
CompanyInitialize."Initialized Version" := format(ModuleInfo.AppVersion());
CompanyInitialize.Insert();
Copy link
Contributor

Choose a reason for hiding this comment

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

We can initialize company many times, the codeunit 2 must be safe to run many times. This logic is also wrong, it should be moved after Initialize company not before. We could partially commit and keep the record.


CompanyInitializeCodeunit.InitializeCompanySetup();

CompanyInitializeCodeunit.InitializeCompany();
Comment on lines +26 to +28
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the gain of raising two identical events (except for their name) in a row?
And shouldn't their names start with On*?

Copy link
Contributor Author

@AndreasMoth AndreasMoth Nov 5, 2024

Choose a reason for hiding this comment

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

True, they should start with On. My thought on two events is that we have a lot of setup tables that are standalone and doesn't depend on anything else being setup and then we have other tables that does depend on the initial setup tables being filled in. With this you always subscribe to the first event if you're independent of any other setup and second if you are dependent. Of course this does not account for third or fourth level dependencies and there are also many other ways to deal with this kind of problem. It's simply trying to help a very complicated problem a little bit.

end;

[EventSubscriber(ObjectType::Codeunit, Codeunit::"System Initialization", 'OnAfterLogin', '', false, false)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
[EventSubscriber(ObjectType::Codeunit, Codeunit::"System Initialization", 'OnAfterLogin', '', false, false)]
[EventSubscriber(ObjectType::Codeunit, Codeunit::"System Initialization", OnAfterLogin, '', false, false)]

local procedure OnLoginInitializeCompany()
var
CompanyInitialize: Record "Company Initialize";
ClientTypeManagement: Codeunit "Client Type Management";
begin
if not GuiAllowed() then
exit;

if ClientTypeManagement.GetCurrentClientType() = ClientType::Background then
Copy link
Contributor

Choose a reason for hiding this comment

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

The most important check that needs to be refactored is the check that we have on the GLSetup.Get(). We should introduce concepts of force initialization and a better check if it was implemented or not.

exit;

if GetExecutionContext() <> ExecutionContext::Normal then
exit;

if CompanyInitialize.Get() then
exit;

InitializeCompany();
end;
}
Loading