Bug 12287 - Hold priority for new hold incorrect if record has holds in transit
Summary: Hold priority for new hold incorrect if record has holds in transit
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-05-19 11:01 UTC by Kyle M Hall
Modified: 2015-06-04 23:32 UTC (History)
4 users (show)

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


Attachments
Bug 12287 - Hold priority for new hold incorrect if record has holds in transit (1.19 KB, patch)
2014-05-19 11:05 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12287 - Hold priority for new hold incorrect if record has holds in transit (1.25 KB, patch)
2014-06-03 08:14 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 12287 - Hold priority for new hold incorrect if record has holds in transit (1.31 KB, patch)
2014-07-21 08:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12287: At the moment, found is 'W', 'T' or NULL (851 bytes, patch)
2014-07-21 08:05 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2014-05-19 11:01:44 UTC
When placing a new hold, the listed priority for the hold will be incorrect if there are any holds with items in transit ( i.e. found = T ).
Comment 1 Kyle M Hall 2014-05-19 11:05:37 UTC Comment hidden (obsolete)
Comment 2 Biblibre Sandboxes 2014-06-03 08:13:49 UTC
Patch tested with a sandbox, by Coralie Barsacq <c.barsacq@mairie-laciotat.fr>
Comment 3 Biblibre Sandboxes 2014-06-03 08:14:07 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2014-07-08 15:06:31 UTC
Don't you think it's safer to test the 'T' value?
Something like

    if ( defined $res->{found}
        and ( $res->{found} eq 'W' or $res->{found} eq 'T' ) ) 
    {   
        $count--;
    }
Comment 5 Kyle M Hall 2014-07-18 13:33:32 UTC
I don't think so. That could cause future regressions if new found statuses are added. For example, there used to be a found status of 'F' for 'Finished' ( from before the existence of the old_reserves table iirc ). We never want to include any type of found hold when calculating a new hold's priority, set there is no need to list everything as a special case.

(In reply to Jonathan Druart from comment #4)
> Don't you think it's safer to test the 'T' value?
> Something like
> 
>     if ( defined $res->{found}
>         and ( $res->{found} eq 'W' or $res->{found} eq 'T' ) ) 
>     {   
>         $count--;
>     }
Comment 6 Jonathan Druart 2014-07-21 08:05:31 UTC
Created attachment 29911 [details] [review]
Bug 12287 - Hold priority for new hold incorrect if record has holds in transit

When placing a new hold, the listed priority for the hold will be
incorrect if there are any holds with items in transit ( i.e. found = T ).

Test Plan:
1) Place 3 holds on a record with at least 2 items
2) Fill one hold so it is marked as waiting
3) Fill on hold so it is marked as in transit
4) Begin placing a new hold
5) Note the expected priority would be 2, but is actually 3
6) Apply this patch
7) Reload the patch, now the expected priority shows correctly

Signed-off-by: Coralie Barsacq <c.barsacq@mairie-laciotat.fr>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 7 Jonathan Druart 2014-07-21 08:05:35 UTC
Created attachment 29912 [details] [review]
Bug 12287: At the moment, found is 'W', 'T' or NULL

Just add a small comment to avoid any further ambiguity.

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 8 Jonathan Druart 2014-07-21 08:08:02 UTC
(In reply to Kyle M Hall from comment #5)
> I don't think so. That could cause future regressions if new found statuses
> are added. For example, there used to be a found status of 'F' for
> 'Finished' ( from before the existence of the old_reserves table iirc ). We
> never want to include any type of found hold when calculating a new hold's
> priority, set there is no need to list everything as a special case.

Looking at the code in C4/Reserves.pm and reserves/*, the 'found' value is often compared to 'W' or 'T'. Both values are listed explicitly. That's why I supposed it is safer to use the same way.
I added a small comment in the line (in order to get return if someone grep found.*W.*T)
Comment 9 Tomás Cohen Arazi 2014-08-16 12:15:30 UTC
Patches pushed to master.

Thanks Kyle and Jonathan!