[Suggestion] Add code action 'OnAfterPublisher'

[fvet]

While making our code extensible, we often have to provide new integrationevents to execute some extra code after normal function logic. We do this by providing OnAfter publishers in the function body.

Similar to how the CRS / waldo codeunit method pattern works (although we don't create method codeunits)

it would be good if we had a code action 'Add OnAfterPublisher' on 'Procedure' level to:

• Create a new IntegrationEvent, named OnAfterFunctionName
• IntegrationEvent should copy all function parameters + add a var parameter for the functions return value
• Add a line at the end of the function to call the IntegrationEvent / publishers with all vars + return value

If the functions return value (AccessKey) would not be named, either add a message / alert to first assign a name (preferred). Either add a local variable, that is only used in the exit statement.

internal procedure GetAccessKey() AccessKey: Text
    var
        Handled: boolean;
        AccessKeyNotFoundErr: Label 'Access key not found';
    begin
        if IsNullGuid("Access Key") then
            exit('');

        OnAfterGetAccessKey(AccessKey);
....

[IntegrationEvent(false, false)]
    local procedure OnAfterGetAccessKey(var AccessKey: Text)
    begin
    end;

PS: Maybe we should be able to apply #118 and #119 at once?

[DavidFeldhoff]

As written in #118, I'll implement it most probably. I hope I'll get to it quite soon.

[DavidFeldhoff]

I'm struggeling with what's the best solution currently and I would like to know what you think, so I'm giving a few more examples:

1. procedure ParametersVarSectionUnnamedReturn(CustomerNo: Code[20]): Record Customer
   var
       Customer2: Record Customer;
   begin
       Customer2.Get(CustomerNo);
       exit(Customer2);
   end;

   results in

   procedure ParametersVarSectionUnnamedReturn(CustomerNo: Code[20]): Record Customer
   var
       Customer2: Record Customer;
   begin
       Customer2.Get(CustomerNo);
       OnAfterParametersVarSectionUnnamedReturn(CustomerNo, Customer2); //=do not show message that a named return value is required as Customer2 could be used as well
       exit(Customer2);
   end;

2. procedure ParametersVarSectionNamedReturnDifferentExit(CustomerNo: Code[20]) Customer3: Record Customer
   var
       Customer2: Record Customer;
   begin
       Customer2.Get(CustomerNo);
       exit(Customer2);
   end;

   results in

   procedure ParametersVarSectionNamedReturnDifferentExit(CustomerNo: Code[20]) Customer3: Record Customer
   var
       Customer2: Record Customer;
   begin
       Customer2.Get(CustomerNo);
       OnAfterParametersVarSectionNamedReturnDifferentExit(CustomerNo, Customer2); //=do not use named return in this case
       exit(Customer2);
   end;

3. procedure ParametersVarSectionUnnamedReturnMemberAccess(CustomerNo: Code[20]): Decimal
   var
       Customer2: Record Customer;
   begin
       Customer2.Get(CustomerNo);
       exit(Customer2.Amount);
   end;

   results in

   procedure ParametersVarSectionUnnamedReturnMemberAccess(CustomerNo: Code[20]): Decimal
   var
       Customer2: Record Customer;
   begin
       Customer2.Get(CustomerNo);
       OnAfterParametersVarSectionUnnamedReturnMemberAccess(CustomerNo, Customer2);
       exit(Customer2.Amount);
   end;

4. procedure ParametersVarSectionUnnamedReturnConstant(CustomerNo: Code[20]): Integer
   var
       myInt: Integer;
   begin
       myInt := 5;
       exit(6);
   end;

   results in

   Message: Please change the value in the exit statement to a variable, so that it can be passed to the publisher

What do you think of my current ideas? I think if I just look at these examples it looks quite okay.

But I start to dislike these ideas if I think about adding onBefore publishers, because it feels like it's getting inconsistent. Let's say I want to add an onBefore publisher in case 1 or 3 then I tend to say that a named return value is required. Or would you expect that it works like this?

OnBeforeParametersVarSectionUnnamedReturn(CustomerNo, Customer2, IsHandled);
if isHandled then
    exit(Customer2); //case 1
    exit(Customer2.Amount) //case 3

Or case 2: Would you expect this

OnBefore(CustomerNo, Customer2, IsHandled);
if IsHandled then
    exit(Customer2);

or would you expect here as well that the named return value is required, so that we could do this?

OnBefore(CustomerNo, Customer3, IsHandled);
if IsHandled then
    exit;

Personally I would like to work as often with named return values as possible, but if the users get the error that they first have to specify a named return most of the time before they can add the publishers, then I think it's annoying as well. I hope you can help me finding a nice, consistent solution.

Cheers, David

[DavidFeldhoff]

Another idea would be to always modify the exit Statement to use a named return variable instead (which would be added as well), so that afterwards always an OnAfterxx(returnVariable) could be added and then the named return variable which was just added can be renamed by the user directly like I'm doing it for the "extract procedure" code action as well. What do you think of that approach? Then it would be consistent with OnBefore and OnAfter as we always use the named return variable. But I don't know if everyone wants that behaviour..

    procedure ParametersVarSectionUnnamedReturnConstant(CustomerNo: Code[20]): Integer
    var
        myInt: Integer;
    begin
        myInt := 5;
        exit(6);
    end;

results in

    procedure ParametersVarSectionUnnamedReturnConstant(CustomerNo: Code[20]) returnVar : Integer
    var
        myInt: Integer;
    begin
        myInt := 5;
        returnVar := 6;
        OnAfterParametersVarSectionUnnamedReturnConstant(CustomerNo, returnVar);
    end;

[fvet]

Will re-read your examples later today, but after reading the links below, wouldn't it make sense always applying named return variables instead? This would make all your samples more consistent, and at least give you a variable to store the return type (in case of the exit(6)).

https://www.waldo.be/2021/07/06/the-complexity-of-complex-return-types/

https://twitter.com/KarolakNatalie/status/1412698747163844609

[fvet]

Looked into some standard BC examples for more scenarios ;)

exit(true) makes sense here (instead of replacing by Result := true first) The OnAfter should not touch the return value.

local procedure RevertWarehouseEntry(var TempWhseJnlLine: Record "Warehouse Journal Line" temporary; JobNo: Code[20]; PostJobConsumptionBeforePurch: Boolean): Boolean
    var
        IsHandled: Boolean;
        Result: Boolean;
    begin
        IsHandled := false;
        OnBeforeRevertWarehouseEntry(TempWhseJnlLine, JobNo, PostJobConsumptionBeforePurch, Result, IsHandled);
        if IsHandled then
            exit(Result);

        if PostJobConsumptionBeforePurch or (JobNo = '') or PositiveWhseEntrycreated then
            exit(false);

        TempWhseJnlLine."Entry Type" := TempWhseJnlLine."Entry Type"::"Negative Adjmt.";
        TempWhseJnlLine.Quantity := -TempWhseJnlLine.Quantity;
        TempWhseJnlLine."Qty. (Base)" := -TempWhseJnlLine."Qty. (Base)";
        TempWhseJnlLine."From Bin Code" := TempWhseJnlLine."To Bin Code";
        TempWhseJnlLine."To Bin Code" := '';

        OnAfterRevertWarehouseEntry(TempWhseJnlLine);
        exit(true);
    end;

OnAfter should only act on ItemJnlPostLine, the exit takes care of what to return.

procedure PostItemJnlLine(PurchHeader: Record "Purchase Header"; PurchLine: Record "Purchase Line"; QtyToBeReceived: Decimal; QtyToBeReceivedBase: Decimal; QtyToBeInvoiced: Decimal; QtyToBeInvoicedBase: Decimal; ItemLedgShptEntryNo: Integer; ItemChargeNo: Code[20]; TrackingSpecification: Record "Tracking Specification"): Integer
    var
        ItemJnlLine: Record "Item Journal Line";
        OriginalItemJnlLine: Record "Item Journal Line";
        TempWhseJnlLine: Record "Warehouse Journal Line" temporary;
        TempWhseTrackingSpecification: Record "Tracking Specification" temporary;
        TempTrackingSpecificationChargeAssmt: Record "Tracking Specification" temporary;
        CurrExchRate: Record "Currency Exchange Rate";
        TempReservationEntry: Record "Reservation Entry" temporary;
        Factor: Decimal;
        PostWhseJnlLine: Boolean;
        CheckApplToItemEntry: Boolean;
        PostJobConsumptionBeforePurch: Boolean;
        IsHandled: Boolean;
    begin
        ...
        OnAfterPostItemJnlLine(ItemJnlLine, PurchLine, PurchHeader, ItemJnlPostLine);

        exit(ItemJnlLine."Item Shpt. Entry No.");
    end;

OnAfter should only act on NoSeriesCode, the exit takes care of what to return.

procedure GetNoSeriesCode(): Code[20]
    var
        NoSeriesCode: Code[20];
        IsHandled: Boolean;
    begin
        SalesSetup.Get();
        IsHandled := false;
        OnBeforeGetNoSeriesCode(Rec, SalesSetup, NoSeriesCode, IsHandled);
        if IsHandled then
            exit;

        NoSeriesCode := SalesSetup."Reminder Nos.";

        OnAfterGetNoSeriesCode(Rec, SalesSetup, NoSeriesCode);
        exit(NoSeriesMgt.GetNoSeriesWithCheck(NoSeriesCode, SelectNoSeriesAllowed, "No. Series"));
    end;

OnAfter should only act on IdenticalArray, the exit takes care of what to return.

OnAfterEntriesAreIdentical(ReservEntry1, ReservEntry2, IdenticalArray);

        exit(IdenticalArray[1] and IdenticalArray[2]);

OnAfter should only act on QtyToHandleColumnIsHidden, the exit takes care of what to return.

OnAfterGetHandleSource(TrackingSpecification, QtyToHandleColumnIsHidden);
        exit(not QtyToHandleColumnIsHidden);

[fvet]

Thinking about this part of the requirement (for OnAfter):

- IntegrationEvent should copy all function parameters + add a var parameter for the functions return value

It definitely makes sense in case of OnBefore, since it gives you all context to overrule the entire function. For OnAfter, the main pattern is often to work on the result of the function, often working on new local variables and no longer requiring to act on the original function parameters. So an (easier?) option could be not to include any variables at all (except for the named return variables) to OnAfter publishers and later add them via 'Add Parameters' on demand?

Looking at ...

procedure PostItemJnlLine(PurchHeader: Record "Purchase Header"; PurchLine: Record "Purchase Line"; QtyToBeReceived: Decimal; QtyToBeReceivedBase: Decimal; QtyToBeInvoiced: Decimal; QtyToBeInvoicedBase: Decimal; ItemLedgShptEntryNo: Integer; ItemChargeNo: Code[20]; TrackingSpecification: Record "Tracking Specification")

... I 'don't need most of these vars as part of my OnAfter publisher

Result of your scenarios could then be

1. Add OnAfterParametersVarSectionUnnamedReturn() and user can add Customer2 variable to OnAfter publisher if needed
2. Add OnAfterParametersVarSectionNamedReturnDifferentExit(Customer3) and user can remove Customer3 variable to OnAfter publisher if needed
3. Add OnAfterParametersVarSectionUnnamedReturnMemberAccess() and user can add Customer2 variable to OnAfter publisher if needed
4. Add OnAfterParametersVarSectionUnnamedReturnConstant() and user can name the returnvalue and add returnVar variable to OnAfter publisher if needed

For OnBefore, i'd expect at least ...

1. OnBeforeParametersVarSectionUnnamedReturn(CustomerNo, IsHandled) and user can add Customer2 / named return variable to OnBefore publisher if needed
2. OnBeforeParametersVarSectionNamedReturnDifferentExit(CustomerNo, Customer3, IsHandled);
3. OnBeforeParametersVarSectionUnnamedReturnMemberAccess(CustomerNo, IsHandled) and user can add Customer2 / named return variable to OnBefore publisher if needed
4. OnBeforeParametersVarSectionUnnamedReturnConstant(CustomerNo, IsHandled) and user can add named return variable to OnBefore publisher if needed

... but indeed maybe by default always include a named return variable if missing (ask for a name, to avoid having to rename afterwards)

[DavidFeldhoff]

Thanks for your huge input so far! It confirms my assumption that there is not the golden path. Sometimes you want all the parameters sometimes not and sometimes it's just "personal preference" if you want to work with named return variables and want to have them changed or not. But I got another idea after your feedback, let me prototype that one and let me come back to you then.

[DavidFeldhoff]

What do you think of this approach where you can

• add a named return variable on the fly
• decide which parameters to add in a fast way
• select/unselect all local variables/parameters/return variables
• add global variables afterwards (I think the list would become too big/unclear if I would list them as well in the dropdown).
• skip adding parameters and adding them "manually" instead.

If you "escape" the input boxes away, then the process stops. If you "enter" the input boxes away, then the process continuous with skipping the adding of a named return variable or adding the parameters.

I hope the video is not too fast :/ But I created it as a video by intent to be able to stop and look at something a bit more instead of making a gif.

I'm looking forward to your honest feedback if this is too much.

https://user-images.githubusercontent.com/53570297/127767658-58568f49-b6f1-45a8-8642-f4a9848fd559.mp4

[fvet]

@DavidFeldhoff This looks great! I think it covers all scenarios and gives total controls and is guiding the user as much as possible. Can't think of any other additions.

[DavidFeldhoff]

I'll publish it most probably tomorrow. I've done some more automated testing over the weekend and will check it once again tomorrow, but then it'll be published

[DavidFeldhoff]

While testing I noticed that the settings "alCodeActions.varParameters" and "alCodeActions.publisherHasVarParametersOnly" were not respected. I added the support for that as well. And now it's pushed in version 1.0.5. Looking forward for the feedback and thanks for the good and valuable discussion!

Cheers, David

[fvet]

Made use of the new feature today and it was dead-simple! Will further follow-up and raise new issues if needed.

Thanks for this nice addition!