Skip to content

GP - Post migration validation feature #29395

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.

Sign up for GitHub

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

Jump to bottom
Open
jaymckinney-enavate wants to merge 17 commits into
base: main
Choose a base branch
from
Open

GP - Post migration validation feature #29395

jaymckinney-enavate wants to merge 17 commits into from

Conversation

@jaymckinney-enavate
Copy link
Contributor

@jaymckinney-enavate jaymckinney-enavate commented Nov 10, 2025

Fixes #579005

@jaymckinney-enavate jaymckinney-enavate requested a review from a team as a code owner November 10, 2025 18:54
@github-actions
Copy link
Contributor

github-actions bot commented Nov 10, 2025

Issue #579005 is not valid. Please make sure you link an issue that exists, is open and is approved.

nikolakukrika
nikolakukrika reviewed Nov 13, 2025
View reviewed changes
@JesperSchulz JesperSchulz added the Integration GitHub request for Integration area label Nov 14, 2025
nikolakukrika
nikolakukrika reviewed Jan 10, 2026
View reviewed changes
Apps/W1/HybridBaseDeployment/app/src/codeunits/MigrationValidatorWarning.Codeunit.al
@@ -0,0 +1,60 @@
namespace Microsoft.DataMigration;

codeunit 40031 "Migration Validator Warning" implements "Cloud Migration Warning"
Copy link
Contributor

@nikolakukrika nikolakukrika Jan 10, 2026

Choose a reason for hiding this comment

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

We already have warnings system, it would be good to align the 2 existing systems.

Copy link
Contributor Author

@jaymckinney-enavate jaymckinney-enavate Jan 12, 2026

Choose a reason for hiding this comment

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

This is in the base. I wanted to align with the Cloud Migration Warning feature there. I think eventually the GP warnings will move toward this new approach.

nikolakukrika
nikolakukrika reviewed Jan 27, 2026
View reviewed changes
Apps/W1/HybridBaseDeployment/app/src/codeunits/HybridCloudManagement.Codeunit.al Outdated
ClearCompanyMigrationValidation(DataMigrationStatus."Migration Type");
end;

[EventSubscriber(ObjectType::Codeunit, Codeunit::"Data Migration Mgt.", OnValidateMigration, '', false, false)]
Copy link
Contributor

@nikolakukrika nikolakukrika Jan 27, 2026

Choose a reason for hiding this comment

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

This is not a correct event to subscribe to.
This is the code where the event is defined:
OnValidateMigration(DataMigrationStatus, DataCreationFailed);
if DataCreationFailed then
exit;

OnAfterMigrationFinished(DataMigrationStatus, false, StartTime, Retry);

OnAfterMigrationFinished will move additional data. E.g. for GP IRS1099, Historical data, PostGLTransactions... Subscribing to the event before will run some of the tests too early.

The issue is that some processes are running Async, so I'm not sure if we can automatically subscribe to the validation. Maybe we will need to trigger these tests manually or via specific subscribers of each implementation engine.

Copy link
Contributor Author

@jaymckinney-enavate jaymckinney-enavate Jan 27, 2026

Choose a reason for hiding this comment

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

IRS 1099 and others should be using OnCreatePostMigrationData, which is right before OnValidateMigration.
In my view, OnAfterMigrationFinished is after all transforms and validation is complete.

Copy link
Contributor

@nikolakukrika nikolakukrika Jan 27, 2026

Choose a reason for hiding this comment

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

We may need a new event after OnAfterMigrationFinished, something OnValidateAfterMigrationFinished.
I simply do not know how this will work. Some processes are triggered as the job queue or a scheduled task.
Maybe it is the best to have the subscriber to invoke this functionality per implementation for now?
GP knows when the migration is complete as it knows which tasks are running

nikolakukrika
nikolakukrika reviewed Jan 27, 2026
View reviewed changes
Apps/W1/HybridBaseDeployment/app/src/codeunits/HybridCloudManagement.Codeunit.al Outdated

MigrationValidation.StartValidation(IntelligentCloudSetup."Product ID", true);

MigrationValidationError.SetRange("Migration Type", IntelligentCloudSetup."Product ID");
Copy link
Contributor

@nikolakukrika nikolakukrika Jan 27, 2026

Choose a reason for hiding this comment

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

This code can impact the other data that is created, as it will stop the transformations.

Copy link
Contributor Author

@jaymckinney-enavate jaymckinney-enavate Jan 27, 2026

Choose a reason for hiding this comment

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

I don't think this is the case. Repeating my previous response. IRS 1099 and others should be using OnCreatePostMigrationData, which is right before OnValidateMigration.
In my view, OnAfterMigrationFinished is after all transforms and validation is complete.

nikolakukrika
nikolakukrika reviewed Jan 27, 2026
View reviewed changes
Apps/W1/HybridBaseDeployment/app/src/codeunits/MigrationValidation.Codeunit.al
Clear(CloudMigrationWarning);
CloudMigrationWarning."Entry No." := 0;
CloudMigrationWarning."Warning Type" := CloudMigrationWarning."Warning Type"::"Migration Validator";
CloudMigrationWarning.Message := CopyStr(StrSubstNo(CloudMigrationWarningErr, MigrationValidatorRegistry."Validator Code", GetLastErrorText()), 1, MaxStrLen(CloudMigrationWarning.Message));
Copy link
Contributor

@nikolakukrika nikolakukrika Jan 27, 2026

Choose a reason for hiding this comment

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

Should we list these as errors if the codeunit has failed running?
We may need a callstack too.

Copy link
Contributor Author

@jaymckinney-enavate jaymckinney-enavate Jan 27, 2026

Choose a reason for hiding this comment

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

I don't know. I just wanted to reuse the existing Cloud Migration Warning functionality to show a failed validator at least somewhere.

nikolakukrika
nikolakukrika reviewed Jan 27, 2026
View reviewed changes
Apps/W1/HybridBaseDeployment/app/src/codeunits/MigrationValidation.Codeunit.al
FailoverToSession := not CanStartBackgroundJob();

if not FailoverToSession then begin
SendStartValidationResultMessage('', StrSubstNo(TelemetryValidationToBeScheduledMsg, JobQueueLbl), false, false);
Copy link
Contributor

@nikolakukrika nikolakukrika Jan 27, 2026

Choose a reason for hiding this comment

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

There are many blank values, is this intentional?

Copy link
Contributor Author

@jaymckinney-enavate jaymckinney-enavate Jan 27, 2026

Choose a reason for hiding this comment

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

I need a telemetry event Id. I forgot to bring that up.

nikolakukrika
nikolakukrika reviewed Jan 27, 2026
View reviewed changes
Apps/W1/HybridBaseDeployment/app/src/codeunits/MigrationValidatorWarning.Codeunit.al
// The issue should be reported.
end;

procedure ShowWarning(var CloudMigrationWarning: Record "Cloud Migration Warning"): Text
Copy link
Contributor

@nikolakukrika nikolakukrika Jan 27, 2026

Choose a reason for hiding this comment

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

Nit - The method is not showing the warning it is returning text, the name should be udpated

Copy link
Contributor Author

@jaymckinney-enavate jaymckinney-enavate Jan 27, 2026

Choose a reason for hiding this comment

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

That is part of the interface, I'm conforming to it. Is there a better way to implement it?

nikolakukrika
nikolakukrika reviewed Jan 27, 2026
View reviewed changes
Apps/W1/HybridBaseDeployment/app/src/tables/MigrationValidationError.Table.al
Caption = 'Entity Type';
NotBlank = true;
}
field(7; Context; Text[250])
Copy link
Contributor

@nikolakukrika nikolakukrika Jan 27, 2026

Choose a reason for hiding this comment

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

As stated above, it would really be good to get the tooltips here.
What is context used for?
We need a field to store the additional information about the record that is specific, maybe even a callstack if it fails. These should be blobs.

250 characters for context is a bit short.

Copy link
Contributor Author

@jaymckinney-enavate jaymckinney-enavate Jan 27, 2026

Choose a reason for hiding this comment

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

Tooltips added.
The Context field is meant to be a text display of the entity Id. The UI isn't meant to drill down to the entity staging record being tested. Issues should most often be resolved in the old system, not in the staging tables.

nikolakukrika
nikolakukrika reviewed Jan 27, 2026
View reviewed changes
Apps/W1/HybridBaseDeployment/app/src/tables/MigrationValidationTest.Table.al
{
field(1; "Code"; Code[30])
{
Caption = 'Code';
Copy link
Contributor

@nikolakukrika nikolakukrika Jan 27, 2026

Choose a reason for hiding this comment

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

Do we need field(2; "Migration Type"; Text[250]) here too?

Copy link
Contributor Author

@jaymckinney-enavate jaymckinney-enavate Jan 27, 2026

Choose a reason for hiding this comment

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

Maybe not, it's using Validator Code as part of the key.
The Validator Registry table has the Migration Type.

nikolakukrika
nikolakukrika reviewed Jan 27, 2026
View reviewed changes
Apps/W1/HybridBaseDeployment/app/src/tables/MigrationValidationTest.Table.al
@@ -0,0 +1,48 @@
namespace Microsoft.DataMigration;

table 40044 "Migration Validation Test"
Copy link
Contributor

@nikolakukrika nikolakukrika Jan 27, 2026

Choose a reason for hiding this comment

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

What is the difference between this table and Migration Validator Registry?
They look similar, for what should each of the tables be used for?

Copy link
Contributor Author

@jaymckinney-enavate jaymckinney-enavate Jan 27, 2026

Choose a reason for hiding this comment

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

Migration Validator Registry is to register a validator Codeunit and to give the user control over if validation is automatic and/or if validation errors should fail the migration.

Migration Validation Test is a detail table for each of the validator's tests.

Copy link
Contributor

@nikolakukrika nikolakukrika Jan 27, 2026

Choose a reason for hiding this comment

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

Can we merge these in a single table?
Ideally users should be able to pick the tests they want to run. Validator should be grouping of the tests. Having 2 tables seem excessive.

Copy link
Contributor Author

@jaymckinney-enavate jaymckinney-enavate Jan 27, 2026

Choose a reason for hiding this comment

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

Users should not need to select which tests to run. They will be preset by the app. For instance, the GP validators are currently responsible for 116 total tests. The user doesn't need to know that and/or need to select all 116.

nikolakukrika
nikolakukrika requested changes Jan 27, 2026
View reviewed changes
Copy link
Contributor

@nikolakukrika nikolakukrika left a comment

Choose a reason for hiding this comment

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

Left feedback

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nikolakukrika nikolakukrika nikolakukrika requested changes

@pborring pborring Awaiting requested review from pborring pborring is a code owner automatically assigned from microsoft/dynamics-365-business-central

Requested changes must be addressed to merge this pull request.

Assignees

@nikolakukrika nikolakukrika

Labels

Integration GitHub request for Integration area

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jaymckinney-enavate @nikolakukrika @JesperSchulz @jaymckinney