[Koha-bugs] [Bug 11336] Priority is not updated on deleting holds

[bugzilla-daemon at bugs.koha-community.org]

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11336

Galen Charlton <gmcharlt at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|Passed QA                   |In Discussion

--- Comment #18 from Galen Charlton <gmcharlt at gmail.com> ---
(In reply to Jonathan Druart from comment #7)
> (In reply to Kyle M Hall from comment #6)
> > I agree this is a bit of a dirty fix. Wouldn't it be better to modify
> > GetReserve to look for the reserve in reserves first, and then old_reserves
> > if it doesn't find it there?
> 
> Hum... It could work. But how to be sure this won't introduce a regression?
> For ex. a logic like: if GetReserve returns undef, the reserve is deleted.

Upon review, I prefer Jonathan's original approach.  Yes, it's a bit "dirty",
but that's mostly because _FixPriority() does not have all that great an
interface: if it's passed a single $reserve_id, it's really meant to fix the
priority for all holds on that hold's bib; if it's passed a rank and a
$reserve_id, it touches that specific hold first.

No other accessor routine currently falls back to looking in the old_* or
deleted* table if it can't find a row in the main table, and I don't see a good
reason to make GetReserve() different when all that is needed is to pass the
biblionumber to _FixPriority.

Also, making hold priority updates after cancellation contingent on there being
a row in old_reserves opens  up the possibility (admittedly, a remote one) of a
race condition if anybody ever writes a cronjob that purges old_reserves.

I suggest going back to Jonathan's original approach -- most of the test cases
and the follow-up can be used as is, I suspect.  For extra credit, it might be
a good time to make _FixPriority accept a hashref, so that you can say:

_FixPriority({ biblionumber => $foo});

vs.

_FixPriority({ reserve_id => $foo, rank => 'del' })

-- 
You are receiving this mail because:
You are watching all bug changes.