Bug 34308 - Item->store tests on itemlost>0
Summary: Item->store tests on itemlost>0
Status: NEW
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-07-19 14:49 UTC by Marcel de Rooy
Modified: 2023-07-20 11:40 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
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.