Bug 34308

Summary: Item->store tests on itemlost>0
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: Architecture, internals, and plumbingAssignee: Bugs List <koha-bugs>
Status: NEW --- QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: andrew, jonathan.druart, martin.renvoize, nick
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:

Description Marcel de Rooy 2023-07-19 14:49:56 UTC 
Should store test itemlost > 0 or !itemlost (truth test) ?

In most code it seems to be tested with truth already.

It looks like we do not have any special meaning for negative itemlost values. There is normally just 1 to 5.
So I think that the >0 test just stems from seeing values 1 and higher. So imo we can just test truth to see if something is lost.

Do you have any feedback about these negative lost values? Please add it here.
Comment 1 Marcel de Rooy 2023-07-19 14:52:35 UTC 
Discussion comes actually from 33608 btw.
Comment 2 Andrew Fuerste-Henry 2023-07-19 14:59:12 UTC 
I can't recall ever running into any lost item functionality that depended on positive vs negative itemlost values.
Comment 3 Jonathan Druart 2023-07-19 15:02:05 UTC 
History is:

commit dd1eec2715b465
commit 0005f6bad36319

which does not make a lot of sense at the end.
Comment 4 Marcel de Rooy 2023-07-19 15:02:29 UTC 
This is current MASTER:

        # If item was lost and has now been found,
        # reverse any list item charges if necessary.
        if (    exists $updated_columns{itemlost}
            and $updated_columns{itemlost} <= 0
            and $pre_mod_item->itemlost > 0 )
        {
            $self->_set_found_trigger($pre_mod_item);
        }

I actually do not understand the test here for <= 0 too. Imo should be =0 only.
Comment 5 Marcel de Rooy 2023-07-19 15:09:37 UTC 
(In reply to Jonathan Druart from comment #3)
> History is:
> 
> commit dd1eec2715b465
> commit 0005f6bad36319
> 
> which does not make a lot of sense at the end.

$charge eq $lostvalue 
Strange test?

Not reading the definitive answer here for making the distinction?
Comment 6 Jonathan Druart 2023-07-20 11:17:18 UTC 
(In reply to Marcel de Rooy from comment #5)
> $charge eq $lostvalue 
> Strange test?

This seems correct.

misc/cronjobs/longoverdue.pl
60     'c|charge=s'        => \$charge,

146 =item B<--charge | -c>
147 
148 This specifies what lost value triggers Koha to charge the account for the lost item.

238   $PERL5LIB/misc/cronjobs/longoverdue.pl --lost 60=2 --charge 2
239     Would set LOST=2 after 60 days (up to one year), and charge the account when setting LOST=2.
Comment 7 Marcel de Rooy 2023-07-20 11:40:17 UTC 
(In reply to Jonathan Druart from comment #6)
> (In reply to Marcel de Rooy from comment #5)
> > $charge eq $lostvalue 
> > Strange test?
> 
> This seems correct.

Ah thanks. Bit of confusing parameter. I was thinking of an amount or so.