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 ).
Created attachment 28328 [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
Patch tested with a sandbox, by Coralie Barsacq <c.barsacq@mairie-laciotat.fr>
Created attachment 28634 [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>
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--; }
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--; > }
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>
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>
(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)
Patches pushed to master. Thanks Kyle and Jonathan!