The GetReserveStatus subroutine doesn't cover all needed use cases and the change done by bug 9367 need to be reverted for that reason.
(In reply to Katrin Fischer from comment #0) > The GetReserveStatus subroutine doesn't cover all needed use cases and the > change done by bug 9367 need to be reverted for that reason. Do you have more specific information?
Yes, it's on bug 10663. Current state of things is that GetReserveStatus does: - not return Filled (Finished) correctly, because it checks the wrong table. - not return Reserved correctly because it leaves out lots of the logic that CheckReserves has to determine if an item can actually fill a hold and totally ignores title level holds when only called with itemnumber. - not return Waiting correctly, because the condition is wrong (see obsoleted partial fix patch on bug 10663) I have spent hours on this as have others and a revert is IMO the best option to fix the regression introduced by bug 9367. It was too late to revert it from git, so doing it in smaller steps accompanied by unit tests.
Ah I see now, that I got a little confused - Waiting works fine, which is why the patch here won't make much difference in most cases. I still think reverting is the safer option and maybe we should just remove GetReserveStatus and instead create something new and light weight that just checks for the 'Waiting' status.
Created attachment 20164 [details] [review] Bug 10697 - Adding more unit tests to CheckReserves Adding more unit tests for CheckReserves to cover more use cases.
Created attachment 20165 [details] [review] Bug 10697: Revert from using GetReserveStatus to use CheckReserves Patch reverts switch from CheckReserves to GetReserveStatus done by 9367 in various places. Note: You will need the patches from bug 10663 to check renewal related functionality. Note: When the check is only done for "Waiting" it will work the same before and after applying this patch as this part of GetReserveStatus works fine. In order to test this, create item and title level holds on some records, make sure that there are holds not yet pulled and items waiting for pickup in the library. Search.pm - searchResult XSLT.pm - buildKohaItemsNamespace Check item status in search result list are shown correctly using XSLT and non-XSLT views. Only checked for 'Waiting', so verify no regressions. opac-detail.pl Check on item status is shown correctly on detail pages in OPAC. if( $reserve_status eq "Reserved"){ $itm->{'onhold'} = 1; } seems to be not used, verify no regressions. circulation.pl - build_issue_data opac-user.pl Check items on hold are properly marked in the list of checkouts and can not be renewed for - patron account pages in the staff interface - patron account pages in the OPAC $it->{'renew_error'} = ( $restype eq "Waiting" or $restype eq "Reserved" ) ? 1 : 0; seems to be not used, verify no regressions. Run unit tests from first patch and any additional tests you can think of. Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
I know that more tests contain references to patron type S and home branch MPL but if my test database does not contain them, these kind of tests will always fail..
I submitted a counterpatch on bug 10663 to resolve problems with GetReserveStatus. Could you tell me how that impacts this report?
Applying: Bug 10697 - Adding more unit tests to CheckReserves Using index info to reconstruct a base tree... M t/db_dependent/Reserves.t Falling back to patching base and 3-way merge... Auto-merging t/db_dependent/Reserves.t CONFLICT (content): Merge conflict in t/db_dependent/Reserves.t Patch failed at 0001 Bug 10697 - Adding more unit tests to CheckReserves The copy of the patch that failed is found in: /home/christopher/git/koha/.git/rebase-apply/patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort".
I had a go at resolving this conflict, but can't, Katrin can you take a look please
(In reply to Chris Cormack from comment #9) > I had a go at resolving this conflict, but can't, Katrin can you take a look > please I too have tried and failed.
I think this needs to be redone probably... should we put a big fat NOTE in the docs of GetReserveStatus meanwhile?
Scary!
Comment on attachment 20164 [details] [review] Bug 10697 - Adding more unit tests to CheckReserves Review of attachment 20164 [details] [review]: ----------------------------------------------------------------- Is it still valid? ::: t/db_dependent/Reserves.t @@ +130,5 @@ > + $constraint, $bibitems, $priority, $resdate, $expdate, $notes, > + $title, undef, $found); > + > +($status, $reserve, $all_reserves) = CheckReserves($itemnumber2); > +is($status,"Reserved", "Reserved - title level hold, still on shelf"); Why are we expecting "Reserved"? If I understand correctly the tests, there is not hold at this point.
Too old - will get obsolete with API work.