microsoft / ALAppExtensions

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

[Event Request] in codeunit 5341 "CRM Int. Table Subscriber" procedure "OnQueryPostFilterIgnoreRecord" #27405

Open JonasPatto opened 6 days ago

JonasPatto commented 6 days ago

Describe the request

Hello Microsoft-Team,

we are currently extending the Dataverse Integration Setup. We are using our own defined processes and thus are a bit off of what the Integration-Feature offers. We need an IsHandled-Pattern in the above mentioned procedure to make our process work.

Additional context

Currently we are facing the issue that Contact and Opportunity-Records are not being synced to Dataverse because they contain or are a Company-Contact. In the above procedure "OnQueryPostFilterIgnoreRecord" the parameter "IgnoreRecord" will always be set to false due to that reason. We looked for other Subscribers to maybe change "IgnoreRecord" to our liking but to no avail.

Code:

[EventSubscriber(ObjectType::Codeunit, Codeunit::"CRM Integration Table Synch.", 'OnQueryPostFilterIgnoreRecord', '', false, false)]
procedure OnQueryPostFilterIgnoreRecord(SourceRecordRef: RecordRef; var IgnoreRecord: Boolean)
var
    isHandled: Boolean; //new
begin
    if IgnoreRecord then
      exit;

    // start new
    isHandled := false;
    OnBeforeHandlePostFilterIgnoreRecord(isHandled);
    if isHandled then
      exit;
    // end new

    case SourceRecordRef.Number() of
        DATABASE::Contact:
            HandleContactQueryPostFilterIgnoreRecord(SourceRecordRef, IgnoreRecord);
        DATABASE::Opportunity:
            HandleOpportunityQueryPostFilterIgnoreRecord(SourceRecordRef, IgnoreRecord);
        DATABASE::"Sales Invoice Line":
            Error(CannotSynchOnlyLinesErr);
        DATABASE::Item:
            HandleItemQueryPostFilterIgnoreRecord(SourceRecordRef, IgnoreRecord);
        DATABASE::Resource:
            HandleResourceQueryPostFilterIgnoreRecord(SourceRecordRef, IgnoreRecord);
        DATABASE::"Sales Invoice Header":
            IgnoreReadOnlyInvoiceOnQueryPostFilterIgnoreRecord(SourceRecordRef, IgnoreRecord);
        Database::"Sales Header":
            IgnoreArchievedSalesOrdersOnQueryPostFilterIgnoreRecord(SourceRecordRef, IgnoreRecord);
        Database::"CRM Salesorder":
            IgnoreArchievedCRMSalesordersOnQueryPostFilterIgnoreRecord(SourceRecordRef, IgnoreRecord);
    end;
end;

    // start new
[IntegrationEvent(false, false)]
local procedure OnBeforeHandlePostFilterIgnoreRecord(var IsHandled: Boolean)
begin
end;
    // end new

Kind Regards

Jonas Internal work item: AB#554795

nikolakukrika commented 6 days ago

@JonasPatto I personally dislike the ask for the IsHandled event. Isn't it possible to implement an event somewhere else to override the IgnoreRecord parameter or an interface maybe? This way we would get an actual event instead of skipping the entire code below.

JonasPatto commented 6 days ago

@JonasPatto I personally dislike the ask for the IsHandled event. Isn't it possible to implement an event somewhere else to override the IgnoreRecord parameter or an interface maybe? This way we would get an actual event instead of skipping the entire code below.

I personally would not create an interface for that because of extensibility reasons. The moment you create an interface you cannot add new procedures to it due to breaking changes and who knows what will happen in the future. And I am also not sure to what enum you would like to bind it.

In my company's case we only care for Contacts and Opportunities (at the moment). Both of these have checks who cannot be altered outside of their own code. We tried to fulfill some statements to not set "IgnoreRecord" to true. With Contacts we managed it by setting an exposed parameter to true making it leave the above shown subscriber early without altering "IgnoreRecord" letting the value stay "false". We are fairly certain that we did not use the Publisher as intened (see OnGetIsContactBusinessRelationOptional in procedure "IsContactBusinessRelationOptional" same Codeunit). But for the Opportunity-Check we did not find an opening. So we thought that a more general way of altering "IgnoreRecord" would be more fitting since we do not know if we or others run across this Issue again with other tables that are handled by Microsoft in the future.

Another problem is that the code itself is already in an EventSubscriber. If it were not then we could create a Publisher before or after the procedure call. I tried to create a subscriber to "OnQueryPostFilterIgnoreRecord" myself but I can't determine the sequence of all subscribers.

What we can do however:

[EventSubscriber(ObjectType::Codeunit, Codeunit::"CRM Integration Table Synch.", 'OnQueryPostFilterIgnoreRecord', '', false, false)]
procedure OnQueryPostFilterIgnoreRecord(SourceRecordRef: RecordRef; var IgnoreRecord: Boolean)
begin
    if IgnoreRecord then
        exit;

        case SourceRecordRef.Number() of
            DATABASE::Contact:
                HandleContactQueryPostFilterIgnoreRecord(SourceRecordRef, IgnoreRecord);
            DATABASE::Opportunity:
                HandleOpportunityQueryPostFilterIgnoreRecord(SourceRecordRef, IgnoreRecord);
            DATABASE::"Sales Invoice Line":
                Error(CannotSynchOnlyLinesErr);
            DATABASE::Item:
                HandleItemQueryPostFilterIgnoreRecord(SourceRecordRef, IgnoreRecord);
            DATABASE::Resource:
                HandleResourceQueryPostFilterIgnoreRecord(SourceRecordRef, IgnoreRecord);
            DATABASE::"Sales Invoice Header":
                IgnoreReadOnlyInvoiceOnQueryPostFilterIgnoreRecord(SourceRecordRef, IgnoreRecord);
            Database::"Sales Header":
                IgnoreArchievedSalesOrdersOnQueryPostFilterIgnoreRecord(SourceRecordRef, IgnoreRecord);
            Database::"CRM Salesorder":
                IgnoreArchievedCRMSalesordersOnQueryPostFilterIgnoreRecord(SourceRecordRef, IgnoreRecord);
        end;

        // start new
        OnAfterHandlePostFilterIgnoreRecord(IgnoreRecord);
        // end new
end;

     // start new
    [IntegrationEvent(false, false)]
    local procedure OnAfterHandlePostFilterIgnoreRecord(var IgnoreRecord: Boolean)
    begin
    end;
    // end new

we can put it at the end so Microsoft-Code can not be skipped. Then everyone who subscribes that Publisher could do an if-statement: "if IgnoreRecord then exit" like in all the procedures.

nikolakukrika commented 6 days ago

@JonasPatto adding event to the end of the method has issue that you do not know when your subscriber will be called. If you want to override IgnoreRecord it needs to be set before the event is raised.

JonasPatto commented 6 days ago

You are quite right. But its only a problem if others also subscribe to the existing publisher. I did not think of that. We would only move this issue from the existing subscriber to the new one. Well, the only possible solutions would be then:

No matter how we look at it, since we are in Event Subscriber-Code we can not guarantee the sequence of Subscriber (which leads to unpredictable scenarios even if you only use Microsoft Code + one of my own subscribers) so I personally would go with the first solution. Thus seperating Microsoft-Code (into actual code outside of a Subscriber) and Partner-Code (into Event Subscriber) again.

onbuyuka commented 6 days ago

@JonasPatto wouldn't it make much more sense to have events HandleContactQueryPostFilterIgnoreRecord and HandleOpportunityQueryPostFilterIgnoreRecord? There you would have the ability to change IgnoreRecord

JonasPatto commented 6 days ago

@JonasPatto wouldn't it make much more sense to have events HandleContactQueryPostFilterIgnoreRecord and HandleOpportunityQueryPostFilterIgnoreRecord? There you would have the ability to change IgnoreRecord

and what if other than Contact or Opportunity needs to be handled? Then we would need a Publisher for each case. I did not see that as necessary. And it would also not solve the above mentioned issues from @nikolakukrika in her first and second response.

onbuyuka commented 6 days ago

An event for each case is still better than having an IsHandled... With IsHandled anyone will have the option to override every single case, not good. I understand that you want only your subscriber to have the power to set the IgnoreRecord value, but what if there's another app depending on Microsoft code to be executed, but your subscriber just blocks any chance of that? An event in the end of OnQueryPostFilterIgnoreRecord or HandleContactQueryPostFilterIgnoreRecord and HandleOpportunityQueryPostFilterIgnoreRecord is better in this case imo

JonasPatto commented 6 days ago

@JonasPatto I personally dislike the ask for the IsHandled event. Isn't it possible to implement an event somewhere else to override the IgnoreRecord parameter or an interface maybe? This way we would get an actual event instead of skipping the entire code below.

I personally would not create an interface for that because of extensibility reasons. The moment you create an interface you cannot add new procedures to it due to breaking changes and who knows what will happen in the future. And I am also not sure to what enum you would like to bind it.

In my company's case we only care for Contacts and Opportunities (at the moment). Both of these have checks who cannot be altered outside of their own code. We tried to fulfill some statements to not set "IgnoreRecord" to true. With Contacts we managed it by setting an exposed parameter to true making it leave the above shown subscriber early without altering "IgnoreRecord" letting the value stay "false". We are fairly certain that we did not use the Publisher as intened (see OnGetIsContactBusinessRelationOptional in procedure "IsContactBusinessRelationOptional" same Codeunit). But for the Opportunity-Check we did not find an opening. So we thought that a more general way of altering "IgnoreRecord" would be more fitting since we do not know if we or others run across this Issue again with other tables that are handled by Microsoft in the future.

Another problem is that the code itself is already in an EventSubscriber. If it were not then we could create a Publisher before or after the procedure call. I tried to create a subscriber to "OnQueryPostFilterIgnoreRecord" myself but I can't determine the sequence of all subscribers.

What we can do however:

[EventSubscriber(ObjectType::Codeunit, Codeunit::"CRM Integration Table Synch.", 'OnQueryPostFilterIgnoreRecord', '', false, false)]
procedure OnQueryPostFilterIgnoreRecord(SourceRecordRef: RecordRef; var IgnoreRecord: Boolean)
begin
    if IgnoreRecord then
        exit;

        case SourceRecordRef.Number() of
            DATABASE::Contact:
                HandleContactQueryPostFilterIgnoreRecord(SourceRecordRef, IgnoreRecord);
            DATABASE::Opportunity:
                HandleOpportunityQueryPostFilterIgnoreRecord(SourceRecordRef, IgnoreRecord);
            DATABASE::"Sales Invoice Line":
                Error(CannotSynchOnlyLinesErr);
            DATABASE::Item:
                HandleItemQueryPostFilterIgnoreRecord(SourceRecordRef, IgnoreRecord);
            DATABASE::Resource:
                HandleResourceQueryPostFilterIgnoreRecord(SourceRecordRef, IgnoreRecord);
            DATABASE::"Sales Invoice Header":
                IgnoreReadOnlyInvoiceOnQueryPostFilterIgnoreRecord(SourceRecordRef, IgnoreRecord);
            Database::"Sales Header":
                IgnoreArchievedSalesOrdersOnQueryPostFilterIgnoreRecord(SourceRecordRef, IgnoreRecord);
            Database::"CRM Salesorder":
                IgnoreArchievedCRMSalesordersOnQueryPostFilterIgnoreRecord(SourceRecordRef, IgnoreRecord);
        end;

        // start new
        OnAfterHandlePostFilterIgnoreRecord(IgnoreRecord);
        // end new
end;

     // start new
    [IntegrationEvent(false, false)]
    local procedure OnAfterHandlePostFilterIgnoreRecord(var IgnoreRecord: Boolean)
    begin
    end;
    // end new

we can put it at the end so Microsoft-Code can not be skipped. Then everyone who subscribes that Publisher could do an if-statement: "if IgnoreRecord then exit" like in all the procedures.

@onbuyuka like the code example here?

onbuyuka commented 6 days ago

@JonasPatto yes I think that's the best. Thinking about it a bit more, this is a little like 'don't add events in interface implementations'... procedure OnQueryPostFilterIgnoreRecord is orchestrating and each case is an implementation. So event in the orchestrator is preferred.