| Summary: | Hold priority for new hold incorrect if record has holds in transit | ||
|---|---|---|---|
| Product: | Koha | Reporter: | Kyle M Hall (khall) <kyle> |
| Component: | Hold requests | Assignee: | Kyle M Hall (khall) <kyle> |
| Status: | CLOSED FIXED | QA Contact: | Testopia <testopia> |
| Severity: | minor | ||
| Priority: | P5 - low | CC: | gmcharlt, jonathan.druart, sandboxes, tomascohen |
| Version: | Main | ||
| Hardware: | All | ||
| OS: | All | ||
| Change sponsored?: | --- | Patch complexity: | Trivial patch |
| Documentation contact: | Documentation submission: | ||
| Text to go in the release notes: | Version(s) released in: | ||
| Circulation function: | |||
| Attachments: |
Bug 12287 - Hold priority for new hold incorrect if record has holds in transit
Bug 12287 - Hold priority for new hold incorrect if record has holds in transit Bug 12287 - Hold priority for new hold incorrect if record has holds in transit Bug 12287: At the moment, found is 'W', 'T' or NULL |
||
|
Description
Kyle M Hall (khall)
2014-05-19 11:01:44 UTC
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! |