http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11336 Galen Charlton <gmcharlt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |In Discussion --- Comment #18 from Galen Charlton <gmcharlt@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.