https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40062
Emily Lamancusa (emlam) <emily.lamanc...@montgomerycountymd.gov> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |emily.lamancusa@montgomeryc
| |ountymd.gov
--- Comment #5 from Emily Lamancusa (emlam)
<emily.lamanc...@montgomerycountymd.gov> ---
Yikes, I see multiple bugs here, in the subs and one in the tests. Thanks for
catching all this, Tomas!!
1. AllowHoldDateInFuture was turned on in a previous test and not rolled back,
so _ShiftPriority gets called in these tests too.
2. AddReserves calls _ShiftPriority and passes the priority *of* the new hold,
but _ShiftPriority shifts holds to open up a spot with priority *1 greater
than* the value it was passed, and then returns that value to be the priority
of the new hold. So if a hold is added when one or more holds already exists,
the new hold ends up with a priority 1 greater than it is supposed to. On the
other hand, if an undefined priority is passed to _ShiftPriority, it does
nothing.
3. If no priority is specified, the correct priority is never actually
calculated anywhere. According to the POD for CalculatePriority, AddReserves
used to call CalculatePriority at the time that POD was written, but it doesn't
anymore (presumably once it was rebased to call Hold->new instead).
Koha::Hold->store doesn't have any checks regarding the priority at all, so the
newly created hold is inserted with priority 1 anyway! (But we still end up
with different end results because of bugs #1 and #2)
4. If _ShiftPriority didn't shift anything (or wasn't called), AddReserves adds
the hold at priority => 1 while another hold already has priority => 1. It then
calls _FixPriority passing only the biblionumber as a parameter, but
_FixPriority does weird things because it isn't meant to be used that way,
according to the POD:
> In the second form, where a biblionumber is passed, the holds on that
> bib (that are not captured) are sorted in order of increasing priority,
> then have reserves.priority set so that the first non-captured hold
> has its priority set to 1, the second non-captured hold has its priority
> set to 2, and so forth.
Simply changing the SQL in _FixPriority to "ORDER BY priority ASC, reserve_id
ASC" wouldn't wholly fix it either, because it would produce unexpected
behavior if the user had already changed hold priorities, so that the hold
priorities no longer reflected the order in which they were placed.
5. _ShiftPriority also has a FIXME right in the middle saying "This whole sub
must be rewritten, especially to highlight what is done when reserve_id is not
given"
The reason we don't see these bugs in production is because placerequest.pl
explicitly passes the hold rank, and there's no way to specify a priority other
than last when placing a hold in the UI, so none of the other holds need to be
adjusted. (I didn't look thoroughly at the other places AddReserve is called,
so if it's possible to place a hold with priority other than last through some
other route, the bugs may show up there)
So...we are probably better off adding new methods to Koha::Holds to cover this
logic and removing the old ones, rather than debugging all this.
--
You are receiving this mail because:
You are the assignee for the bug.
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
