How event hooks are called in CU80 in NAV2016 - is this a bug in standard NAV?

rsaritzky

Hi all,

We discovered something in CU80 in NAV2016. Like all previous versions of NAV, a local copy of the passed REC is assigned to a global variable, and then that variable has updates made to it. For example, The code starts on line 21 where "SalesHeader := Rec". Then, on line 42, function SetShipInvoiceReceiveFlags is called, passing SalesHeader as a parameter. That function updates certain fields in SalesHeader. So after that function is called, the values in REC are different than SalesHeader:

21		SalesHeader := Rec;  //Rec gets copied to SalesHeader here
22		TempServiceItem2.DELETEALL;
23		TempServiceItemComp2.DELETEALL;
24		WITH SalesHeader DO BEGIN
25		  TESTFIELD("On Hold",'');
26		  CheckMandatoryHeaderFields(Rec);
27		  GLSetup.GET;
28		  IF GLSetup."PAC Environment" <> GLSetup."PAC Environment"::Disabled THEN
29		    TESTFIELD("Payment Method Code");
30		
31		  IF GenJnlCheckLine.DateNotAllowed("Posting Date") THEN
32		    FIELDERROR("Posting Date",Text045);
33		
34		  IF "Tax Area Code" = '' THEN
35		    SalesTaxCountry := SalesTaxCountry::NoTax
36		  ELSE BEGIN
37		    TaxArea.GET("Tax Area Code");
38		    SalesTaxCountry := TaxArea."Country/Region";
39		    UseExternalTaxEngine := TaxArea."Use External Tax Engine";
40		  END;
41		
42		  SetShipInvoiceReceiveFlags(SalesHeader);
43		
44		  IF NOT (Ship OR Invoice OR Receive) THEN
45		    ERROR(
46		      Text020,
47		      FIELDCAPTION(Ship),FIELDCAPTION(Invoice),FIELDCAPTION(Receive));

Between lines 47 and 1353, a number of other functions are called that modify SalesHeader.
..subsequently, down on line 1354, REC is then assigned the updated value of SalesHeader.

There are numerous other function calls that update SalesHeader before REC gets the fully-updated values. A couple of examples:
SendICDocument on line 228
ServItemMgt.CreateServItemOnSalesInvoice(SalesHeader) on line 254

Now, here's the problem as we see it. On Line 174, an Event Hook is called - but with REC instead of SalesHeader:

174          OnBeforePostCommitSalesDoc(Rec,GenJnlPostLine,PreviewMode,ModifyHeader);

So, if we create event code that is called by this hook that modifies REC, then those changes will be overwritten with the values of SalesHeader down on line 1354.

Let’s take a very simple example. Let’s say that our event hook code modifies the field "Salesperson Code".
At line 32, let’s say that Salesperson Code is currently “ROBERT”:

SalesHeader := Rec;  // now SalesHeader.”Salesperson Code” = “ROBERT"

Then the OnBeforePostCommitSalesDoc hook is called at Line 174. If the Hook is called with REC, and the event code changes the Salesperson code to “JOHN”, then after the hook is called:

Salesperson.”SalesPerson Code” = “ROBERT” but REC.”Salesperson Code” = “JOHN”

Then, in line 1366:

Rec := SalesHeader;

So, after that line:
Salesperson.”SalesPerson Code” = “ROBERT” and REC.”Salesperson Code” = “ROBERT”

..so the results of the event code update have been overwritten.

We submitted this issue to Microsoft, and their current position is that this is not a bug.
I'm wondering what the NAV community thinks - are we in the wrong here?
We of course worked around it by changing the call the to the OnBeforePostCommitSalesDoc hook to pass SalesHeader instead of REC:

174          OnBeforePostCommitSalesDoc(SalesHeader,GenJnlPostLine,PreviewMode,ModifyHeader);

Currently, this is the only current Integration Event hook in CU80 that has this problem - there are only 3 event hooks in CU80 in this version of NAV, but I believe that additional hooks are planned in future versions (I haven't looked at NAV2017 yet). But the same/similar problem exists in CU90, and I would expect that there are other standard event hooks that do/will have this same issue.

I would be interested in other opinions out there.
Thanks
Ron Saritzky

Kishorm

@rsaritzky I agree with you 100% and it looks like one of the developers at Microsoft does too because this has been fixed in NAV 2017

Kishorm

Hmmm, Robert Miller's response about SalesHeader being "incomplete" doesn't make much sense. Yes, it's being updated as part of the posting routine but that's what you would want to do too - make additional changes to it.

It might be worth checking the lastest CU on 2016 before going back to Microsoft as it may have been fixed in there already.

Kishorm

No problem.

BTW, I love the fact that he says...

...the only possibility is for Rec to be passed

...and then follows on to say...

...you could modify the call to OnBeforePostCommitSalesDoc and pass the SalesHeader variable instead of Rec.

LOL

Kishorm

@rsaritzky I agree with you 100% and it looks like one of the developers at Microsoft does too because this has been fixed in NAV 2017

rsaritzky

Thanks Kishorm,

I was puzzled by Robert Miller's (Microsoft US) response on this item, saying it was not a bug:

I can see the confusion as to why this might appear as a bug, but I don’t believe it is. In fact, the practice of copying Rec to a variable (in this case SalesHeader) and then update the new variable is common practice throughout NAV. Rec is left in a preserved state and any mods are applied to the SalesHeader variable. At the point where the event OnBeforePostCommitSalesDoc is called, the SalesHeader is in an incomplete state, so the only possibility is for Rec to be passed. In fact, SalesHeader is never in a complete state until around line 1366 where Rec is set to SalesHeader. If you need to test a value from SalesHeader, then you could modify the call to OnBeforePostCommitSalesDoc and pass the SalesHeader variable instead of Rec.

He seems to be saying that if you need the current state of REC (including any updates to it by modifying the local copy SalesHeader), change the call to the event (which I did). But it still did not explain why he thought it was not a bug. But if it's changed in 2017, then obviously someone at MS agreed.

We'll pursue it again to see if an official update to 2016 can be created.

Ron

Kishorm

Hmmm, Robert Miller's response about SalesHeader being "incomplete" doesn't make much sense. Yes, it's being updated as part of the posting routine but that's what you would want to do too - make additional changes to it.

It might be worth checking the lastest CU on 2016 before going back to Microsoft as it may have been fixed in there already.

rsaritzky

I had already checked the latest CU update before I posted this question. I had not checked the 2017 code-base, though. Thanks for the reply.

Ron

Kishorm

No problem.

BTW, I love the fact that he says...

...the only possibility is for Rec to be passed

...and then follows on to say...

...you could modify the call to OnBeforePostCommitSalesDoc and pass the SalesHeader variable instead of Rec.

LOL