microsoft / ALAppExtensions

Repository for collaboration on Microsoft AL application add-on and localization extensions for Microsoft Dynamics 365 Business Central.
MIT License
791 stars 622 forks source link

OnBeforeCheckPeriodFormVendUniqueness trigger in table 10035 "IRS 1099 Form Doc. Header" #27659

Open Kelly-cn opened 2 days ago

Kelly-cn commented 2 days ago

Describe the request

In table 10035 "IRS 1099 Form Doc. Header", we need to have integration event in the "CheckPeriodFormVendUniqueness" procedure. Adding an event where we can skip the checking. We will need IncludeSender and IsHandled as a parameter. local procedure CheckPeriodFormVendUniqueness() var IRS1099FormDocHeader: Record "IRS 1099 Form Doc. Header"; IsHandled: Boolean; begin if Rec."Vendor No." = '' then exit; if Rec."Form No." = '' then exit; OnBeforeCheckPeriodFormVendUniqueness(IsHandled); if IsHandled then exit; IRS1099FormDocHeader.SetFilter(Id, '<>%1', ID); IRS1099FormDocHeader.SetRange("Period No.", "Period No."); IRS1099FormDocHeader.SetRange("Vendor No.", "Vendor No."); IRS1099FormDocHeader.SetRange("Form No.", "Form No."); if not IRS1099FormDocHeader.IsEmpty() then Error(CannotCreateFormDocSamePeriodVendorFormErr); end;

[IntegrationEvent(true, false)] local procedure OnBeforeCheckPeriodFormVendUniqueness(var IsHandled: Boolean) begin end;

Additional context

We need to support multiple dimensions for the same Reporting Period, Vendor and Form No. because each dimension may have different "Federal ID No." which needs to submit separately.

Internal work item: AB#558145

nikolakukrika commented 1 day ago

@Kelly-cn Skipping these kinds of validations is not a good design. Instead of using our own tables in a way that they were not designed, I would recommend introducing new tables to hold the functionality that you added or to restructure the Microsoft code to support your scenario (contribution). This kind of an event will be fragile, we have introduced this validation for a reason. If we change base functionality, there is a risk that we will break your event or other code in your solution. From this handled event we have no idea how you have extended the solution, how you are using these records and how it will work.

nikolakukrika commented 1 day ago

Rejecting because of Handled event and include sender. We do not want to introduce more events with include sender, this makes maintenance on our end hard and it is also leading to complex code. IsHandled is not a proper event. Please revise the solution as current solution is too fragile. Please reopen the request or submit a new one with a better design.

nikolakukrika commented 1 day ago

@Kelly-cn Additional feedback:

This validation was introduced for a reason local procedure CheckPeriodFormVendUniqueness() var IRS1099FormDocHeader: Record "IRS 1099 Form Doc. Header"; begin if Rec."Vendor No." = '' then exit; if Rec."Form No." = '' then exit; IRS1099FormDocHeader.SetFilter(Id, '<>%1', ID); IRS1099FormDocHeader.SetRange("Period No.", "Period No."); IRS1099FormDocHeader.SetRange("Vendor No.", "Vendor No."); IRS1099FormDocHeader.SetRange("Form No.", "Form No."); if not IRS1099FormDocHeader.IsEmpty() then Error(CannotCreateFormDocSamePeriodVendorFormErr); end;

It was not designed to be extensible. This is a clear design intent - we want the records to be unique under these criteria. IsHandled event here is bad, because we will not know what the subscriber is doing, it is not reusable by others, we may break the solutions in the future without users realizing. I have seen this happen many times.

We can introduce an event to populate your own records and an event to extend our reporting to include extra data. This is a better design, because we see that we have an extensibility point for additional reporting. Other partners can use it. Events added have a clear purpose - provide additional data for reporting.

Is it possible for you to restructure the code and to change the event request?

dylanbinarystream commented 1 day ago

@nikolakukrika Thank you for your feedback.

My understanding of the reason behind this validation is that typically a single Business Central company will have a single Federal ID Number. Based on this there should only be one document per Period/Vendor/IRS 1099 Form, since a company with a single Federal ID number should only have one 1099 form of each type at most for a tax year which makes sense for this validation to be present.

However, with our ISV extension we allow a customer to maintain multiple entities with different Federal ID Numbers within a single Business Central company. So this validation is no longer valid, as the validation should instead be that each entity or potentially tax group (i.e. unique Federal ID Number) should have at most one 1099 Form of each type for each tax year.

Really we just want to modify this validation to make it applicable for our scenario. Based on your feedback I have re-written the integration request instead to just give access to the IRS1099FormDocHeader so we can set an additional range on it prior to the validation, as this will also work for our needs. Please let me know if this is sufficient or if any further adjustment is needed.

We do already have many customers live using both the previous 1099 functionality for a number of years as well as the IRS 1096 app since its release which we have extended to match the same concept described above. We need to be able to apply the same concept to be able to continue to support our customers for the 1099 functionality as the base functionality is being refactored and we would like to avoid needing to re-do the tables of the new 1099 extension just to work around this one piece of validation code which is currently not able to be bypassed otherwise.


local procedure CheckPeriodFormVendUniqueness()
var
    IRS1099FormDocHeader: Record "IRS 1099 Form Doc. Header";
begin
    if Rec."Vendor No." = '' then
        exit;
    if Rec."Form No." = '' then
        exit;
    IRS1099FormDocHeader.SetFilter(Id, '<>%1', ID);
    IRS1099FormDocHeader.SetRange("Period No.", "Period No.");
    IRS1099FormDocHeader.SetRange("Vendor No.", "Vendor No.");
    IRS1099FormDocHeader.SetRange("Form No.", "Form No.");
    OnBeforeCheckPeriodFormVendUniqueness(IRS1099FormDocHeader);
    if not IRS1099FormDocHeader.IsEmpty() then
        Error(CannotCreateFormDocSamePeriodVendorFormErr);
end;

[IntegrationEvent(false, false)]
local procedure OnBeforeCheckPeriodFormVendUniqueness(var IRS1099FormDocHeader: Record "IRS 1099 Form Doc. Header")
begin
end;
nikolakukrika commented 20 hours ago

@dylanbinarystream This is much better, thanks for the update. Now we are not skiping the check but we can add additional filters to the request. I would recommend placing a comment above the event stating that one of the usage of the events is to set additional filters. Can you update the request above?

A better approach would be to update the event to get additional filters that takes in the current view and returns a new one, but It can be skipped.

grobyns commented 17 hours ago

@dylanbinarystream I would suggest adding verification after the event that a subscriber did not change the 3 filters set before the event. A subscriber should not change vendor or form no. right?

something like: if (IRS1099FormDocHeader.GetFilter("Period No.") <> Rec."Period No.") or (..) or (..) then error(FiltersModifiedBySubscriberError);

dylanbinarystream commented 14 hours ago

@nikolakukrika @grobyns Thank you for your feedback. I have updated the request below. I used a sub-procedure to make the code flow a bit more readable, but let me know if you would me to avoid using the sub-procedure or make any other modifications.

var
    FiltersModifiedBySubscriberErr: Label 'An installed extension has modified the period, vendor or form uniqueness filter which is not allowed.';

local procedure CheckPeriodFormVendUniqueness()
var
    IRS1099FormDocHeader: Record "IRS 1099 Form Doc. Header";
begin
    if Rec."Vendor No." = '' then
        exit;
    if Rec."Form No." = '' then
        exit;
    IRS1099FormDocHeader.SetFilter(Id, '<>%1', ID);
    IRS1099FormDocHeader.SetRange("Period No.", "Period No.");
    IRS1099FormDocHeader.SetRange("Vendor No.", "Vendor No.");
    IRS1099FormDocHeader.SetRange("Form No.", "Form No.");
    AddPeriodFormVendUniquenessFilters(IRS1099FormDocHeader);
    if not IRS1099FormDocHeader.IsEmpty() then
        Error(CannotCreateFormDocSamePeriodVendorFormErr);
end;

/// <summary>
/// Provides an integration event that can be used to add additional filters to the period, vendor, and form uniqueness validation.
/// </summary>
/// <param name="IRS1099FormDocHeader">The form doc header record being filtered for the uniqueness validation.</param>
local procedure AddPeriodFormVendUniquenessFilters(var IRS1099FormDocHeader: Record "IRS 1099 Form Doc. Header")
begin
    OnAddPeriodFormVendUniquenessFilters(IRS1099FormDocHeader);
    if (IRS1099FormDocHeader.GetFilter("Period No.") <> Rec."Period No.") or (IRS1099FormDocHeader.GetFilter("Vendor No.") <> Rec."Vendor No.") or (IRS1099FormDocHeader.GetFilter("Form No.") <> Rec."Form No.") then
        Error(FiltersModifiedBySubscriberErr);
end;

[IntegrationEvent(false, false)]
local procedure OnAddPeriodFormVendUniquenessFilters(var IRS1099FormDocHeader: Record "IRS 1099 Form Doc. Header")
begin
end;