Bug 10697 - Revert from using GetReserveStatus to use CheckReserves
Summary: Revert from using GetReserveStatus to use CheckReserves
Status: CLOSED WONTFIX
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-08-08 08:06 UTC by Katrin Fischer
Modified: 2018-06-04 20:09 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 10697 - Adding more unit tests to CheckReserves (5.39 KB, patch)
2013-08-08 10:33 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 10697: Revert from using GetReserveStatus to use CheckReserves (7.08 KB, patch)
2013-08-08 10:33 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2013-08-08 08:06:32 UTC
The GetReserveStatus subroutine doesn't cover all needed use cases and the change done by bug 9367 need to be reverted for that reason.
Comment 1 Marcel de Rooy 2013-08-08 08:14:06 UTC
(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?
Comment 2 Katrin Fischer 2013-08-08 08:25:55 UTC
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.
Comment 3 Katrin Fischer 2013-08-08 10:20:32 UTC
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.
Comment 4 Katrin Fischer 2013-08-08 10:33:03 UTC
Created attachment 20164 [details] [review]
Bug 10697 - Adding more unit tests to CheckReserves

Adding more unit tests for CheckReserves to cover more use cases.
Comment 5 Katrin Fischer 2013-08-08 10:33:16 UTC
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>
Comment 6 Marcel de Rooy 2013-08-15 09:34:57 UTC
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..
Comment 7 Marcel de Rooy 2013-08-15 12:16:42 UTC
I submitted a counterpatch on bug 10663 to resolve problems with GetReserveStatus.
Could you tell me how that impacts this report?
Comment 8 I'm just a bot 2013-09-18 11:10:41 UTC
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".
Comment 9 Chris Cormack 2013-12-01 21:51:59 UTC
I had a go at resolving this conflict, but can't, Katrin can you take a look please
Comment 10 Kyle M Hall 2014-07-30 14:34:06 UTC
(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.
Comment 11 Katrin Fischer 2014-07-30 14:36:28 UTC
I think this needs to be redone probably... should we put a big fat NOTE in the docs of GetReserveStatus meanwhile?
Comment 12 Mark Tompsett 2015-04-04 21:41:21 UTC
Scary!
Comment 13 Jonathan Druart 2016-03-04 11:02:19 UTC
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.
Comment 14 Katrin Fischer 2017-08-19 08:26:26 UTC
Too old - will get obsolete with API work.