FINDSET ISSUE

MrWho

Hello!

We have a strange situation I hope someone can help us sort out. It seems like the cursors go "bananas" when we modify certain record in the purchase line, we know we have set filter on the value we are modifying so that is why we are using a second variable to modify the record in question.

We have tried FINDSET,FINDSET(TRUE) and FINDSET(TRUE,TRUE) with the same error.

After setting the filter, there is about 43 records to modify. After looping through about 5 records, it comes to this record that gives the error message. The 5 previous records have also been modified , so why does the "tblPurchaseLine.NEXT = 0" get confused and retrieves the same record one more time?

If we had modified the "tblPurchaseLine" instead of the "tblPurchaseLine2" record variable I would have understood, but this?

IF _Date = 0D THEN
  dGlobalDate := TODAY
ELSE
  dGlobalDate := _Date;

tblPurchaseLine.SETCURRENTKEY("Document Type",Type,"No.","Variant Code","Drop Shipment","Location Code","Expected Receipt Date");
tblPurchaseLine.SETRANGE("Document Type",tblPurchaseLine."Document Type"::Order);
tblPurchaseLine.SETRANGE(Type,tblPurchaseLine.Type::Item);
tblPurchaseLine.SETFILTER("No.",'<>%1','');
tblPurchaseLine.SETFILTER("Expected Receipt Date",'<=%1',dGlobalDate);
tblPurchaseLine.SETFILTER("Outstanding Quantity",'<>%1',0);
tblPurchaseLine.SETRANGE("Quantity Received",0);

iModified := 0;

dNewDate := GetNewDate(dGlobalDate);

tblPurchaseLineTemp.RESET;
tblPurchaseLineTemp.DELETEALL;
iLines := tblPurchaseLine.COUNT;

IF tblPurchaseLine.FINDSET(TRUE,TRUE) THEN REPEAT

  tblPurchaseHeader.GET(tblPurchaseLine."Document Type",tblPurchaseLine."Document No.");
  IF tblPurchaseHeader.Status = tblPurchaseHeader.Status::Released THEN
    cduReleasePurchDocument.Reopen(tblPurchaseHeader);

  tblPurchaseLine2.GET(tblPurchaseLine."Document Type",tblPurchaseLine."Document No.",tblPurchaseLine."Line No.");
  tblPurchaseLine2.VALIDATE("Expected Receipt Date",dNewDate);
  tblPurchaseLine2."Chng. Exp. Receipt Date" := dGlobalDate;
  //tblPurchaseLine2.MODIFY(TRUE); //These modify lines is the reason why the tblPurchaseLine.NEXT retrieves the same record twice!!!
  tblPurchaseLine2.MODIFY;
  iModified += 1;
  
  tblPurchaseLineTemp.INIT;
  tblPurchaseLineTemp.TRANSFERFIELDS(tblPurchaseLine);
  tblPurchaseLineTemp.INSERT; //This line cause the attached error message picture.
  iLines -= 1;
UNTIL tblPurchaseLine.NEXT = 0;

IF iModified > 0 THEN BEGIN
  COMMIT;
  NotifyPurchaser(tblPurchaseLineTemp);
END;

So, if we leave out the "tblPurchaseLine2.MODIFY" it loops throught fine with all 43 records and insert them into the TEMP table.

mohana_cse06

Did you set Temporary field to yes for tblPurchaseLineTemp variable?

mohana_cse06

Did you set Temporary field to yes for tblPurchaseLineTemp variable?

MrWho

mohana_cse06 wrote:

Did you set Temporary field to yes for tblPurchaseLineTemp variable?

Yes, we have. Otherwise we would a big problem with the delete line further up

Marije_Brummel

Why do you make the change in the loop?

The idea is to create the Temporary table first, then loop through that table and change the values of the orriginal table

And could you please stop with this ugly Hungarian Notation? It makes your code impossible to read...

MrWho

Mark Brummel wrote:

Why do you make the change in the loop?

The idea is to create the Temporary table first, then loop through that table and change the values of the orriginal table

And could you please stop with this ugly Hungarian Notation? It makes your code impossible to read...

Hello Mark, nice presentation at the TechDays btw regarding PRS. We will have an internal session about it for all developers.

The temp table is used in the notify function to send e-mail to each purchaser to whom the purchase order have been modified. We would like to commit the change even if the mail should fail for any reason that is why we use the temp table afterward.

By Hungarian you mean this tbl and d and so on ? These are company guidelines for development, it is more readable for us and it is easier to distinguish out of std. NAV code lines in std. functions. But, any good input is always appreciated.

Marije_Brummel

When you run into issues when changing a filtered value in a loop, I would use the TEMP table to loop through.

I haven't been doing much SQL stuff lately but wasn't the second variable trick only for sorting? Not for filtering values.

I might be wrong.

So your solution should first loop through the filtered values of the purchase line, store them in the temp table.

Then loop though the temp table and change the value in the real table.

This should solve the issue.

As for the Hungarian Notation. I hear this feedback a lot. IMHO if the requirement for partners is to see what their code is compared to standard NAV code this is something that Microsoft should support.

We (PRS Team) discussed that with Microsoft. If this is a common requirement then I think they should support this somehow.

MrWho

Yes, the second variable is needed when modifying any value that is used within the "SETCURRENTKEY", correct. But I believe SETCURRENTKEY issue cause the cursor to go more to EOF (End Of File) type of behaviour rather than this example causing the NEXT statement to give the same record once more before going further.

Never had any issue with the filtering before, but I think it sounds logical so I go to comfort with that rather then believing there is a cursor bug in the client and modify our code accordingly if no one else has any comment on this?

But MS, get this in the training books!!

MrWho

Here is the changed and working code using TEMP table to find the record first, before we can modify any of the filtered values.

IF _Date = 0D THEN
  dGlobalDate := TODAY
ELSE
  dGlobalDate := _Date;

tblPurchaseLine.SETCURRENTKEY("Document Type",Type,"No.","Variant Code","Drop Shipment","Location Code","Expected Receipt Date");
tblPurchaseLine.SETRANGE("Document Type",tblPurchaseLine."Document Type"::Order);
tblPurchaseLine.SETRANGE(Type,tblPurchaseLine.Type::Item);
tblPurchaseLine.SETFILTER("No.",'<>%1','');
tblPurchaseLine.SETFILTER("Expected Receipt Date",'<=%1',dGlobalDate);
tblPurchaseLine.SETFILTER("Outstanding Quantity",'<>%1',0);
tblPurchaseLine.SETRANGE("Quantity Received",0);

iModified := 0;

dNewDate := GetNewDate(dGlobalDate);

tblPurchaseLineTemp.RESET;
tblPurchaseLineTemp.DELETEALL;
iLines := tblPurchaseLine.COUNT;

IF tblPurchaseLine.FINDSET THEN REPEAT
  tblPurchaseLineTemp.INIT;
  tblPurchaseLineTemp.TRANSFERFIELDS(tblPurchaseLine);
  tblPurchaseLineTemp.INSERT;
UNTIL tblPurchaseLine.NEXT = 0;

tblPurchaseLine.RESET;

IF tblPurchaseLineTemp.FINDSET THEN REPEAT
  tblPurchaseLine.GET(tblPurchaseLineTemp."Document Type",tblPurchaseLineTemp."Document No.",tblPurchaseLineTemp."Line No.");
  
  tblPurchaseHeader.GET(tblPurchaseLine."Document Type",tblPurchaseLine."Document No.");
  IF tblPurchaseHeader.Status = tblPurchaseHeader.Status::Released THEN
    cduReleasePurchDocument.Reopen(tblPurchaseHeader);

  tblPurchaseLine.VALIDATE("Expected Receipt Date",dNewDate);
  tblPurchaseLine."Chng. Exp. Receipt Date" := dGlobalDate;
  tblPurchaseLine.MODIFY(TRUE);
  iModified += 1;

UNTIL tblPurchaseLineTemp.NEXT = 0;

IF iModified > 0 THEN BEGIN
  COMMIT;
  NotifyPurchaser(tblPurchaseLineTemp);
END;

Marije_Brummel

Yep, that's what I meant.

Last step is to remove the Hungarian Notation...

pdj

No, keep whatever notation you have in your company. I would rather recommend you focused on optimizing the code, if it is frequently executed :-k

1) You are going to modify all the records you loop through, so you should have a LOCKTABLE before the first loop. This increases the speed of the MODIFY later, as it doesn't have to get and lock each record before the update. (both the line and the header)

2) You should never use TRANSFERFIELDS with records of same type. Instead simply assign them:
tblPurchaseLineTemp := tblPurchaseLine

3) You should never use COUNT unless you really have to. Here you could put iLines := iLines + 1 into the first loop. (or iLines += 1 which you seem to prefer)

4) There is no need to tblPurchaseLine.GET(..), since you already have it the temp record. Again, simple assign it "back" like this: tblPurchaseLine := tblPurchaseLineTemp

5) Unless you are sure that the filters (almost) always results in less records than specified in the RecordsetSize, you should replace all your FINDSETs with FIND('-').

6) Assuming most orders have multiple lines, you should only get each header once. Simply add IF tblPurchaseHeader."No." <> tblPurchaseLine."Document No." THEN..

A rough guess, say at least a factor 10 performance improvement. with these 6 changes.

7) And shouldn't you Release the orders again, which you just opened?

Edit: Added no. 6 :-)

David_Singleton

MrWho wrote:

By Hungarian you mean this tbl and d and so on ? These are company guidelines for development, it is more readable for us and it is easier to distinguish out of std. NAV code lines in std. functions. But, any good input is always appreciated.

Maybe a better idea would be to write all your custom changes in Norwegian this would achive all the things you aim to achieve with Hungarian notation, i.e. it will be easier for YOU to read, it will be explicit which is standard Navision code, and which is added by you, and most importantly no one else will be able to read it (which I assume is the main reason for doing it this way) and thus the customer would have a hard time changing partners.

pdj

David Singleton wrote:

...(which I assume is the main reason for doing it this way)...

#-o
Edit: Deleted comment I wrote before having counted to 10... :-#

MrWho

pdj wrote:

No, keep whatever notation you have in your company. I would rather recommend you focused on optimizing the code, if it is frequently executed :-k

1) You are going to modify all the records you loop through, so you should have a LOCKTABLE before the first loop. This increases the speed of the MODIFY later, as it doesn't have to get and lock each record before the update. (both the line and the header)

2) You should never use TRANSFERFIELDS with records of same type. Instead simply assign them:
tblPurchaseLineTemp := tblPurchaseLine

3) You should never use COUNT unless you really have to. Here you could put iLines := iLines + 1 into the first loop. (or iLines += 1 which you seem to prefer)

4) There is no need to tblPurchaseLine.GET(..), since you already have it the temp record. Again, simple assign it "back" like this: tblPurchaseLine := tblPurchaseLineTemp

5) Unless you are sure that the filters (almost) always results in less records than specified in the RecordsetSize, you should replace all your FINDSETs with FIND('-').

6) Assuming most orders have multiple lines, you should only get each header once. Simply add IF tblPurchaseHeader."No." <> tblPurchaseLine."Document No." THEN..

A rough guess, say at least a factor 10 performance improvement. with these 6 changes.

7) And shouldn't you Release the orders again, which you just opened?

Edit: Added no. 6 :-)

Thanks alot for all your feedback, this has become a real good discussion. The hungarian part, I think we shall leave it, as is with no further discussion

1.) Regarding the locking, totally agree.
2.) Thanks for the tips regarding transferfields.
3.) The iLines , count thing was just there while debugging to see if it looped through all or not.
4.) I believe assigning it back and modifying it is more appropiate if you use the locktable correctly in the begining.
5.) This I'm aware of..
6.) The setcurrentkey doesn't contain document no. so I assumed the result would be randomly and would need to be retrieved pretty often anyway, but less is better
7.) The customer doesn't really use the release status to anything in particular on the purchase order, so no. It is just set released on partially received purchase order.

I agree that you should always be aware of performance issue and code at the best. But this code is only run every saturday evening by Job Queue and it is normally only about 50 to 100 lines they haven't received in time. They have good purchasers that constantley call their vendors and set correct dates in NAV, since this is very essential for them to have the rest of the planning worksheet and req. worksheet to work properly.

pdj

MrWho wrote:

6.) The setcurrentkey doesn't contain document no. so I assumed the result would be randomly and would need to be retrieved pretty often anyway, but less is better

You will never get any dataset randomly in NAV. Since you don't specify a key for the temporary table, it will be sorted by the primary key, which starts with the Document No. 8)

And I'm glad you can use my comments. I fully understand you might not implement them since the job isn't executed that often.