Bug 11999 - Add two checks in CanBookBeReserved and CanItemBeReserved
Summary: Add two checks in CanBookBeReserved and CanItemBeReserved
Status: Patch doesn't apply
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Arthur Suzuki
QA Contact: Testopia
URL:
Keywords:
: 14155 (view as bug list)
Depends on: 13113 17740
Blocks: 11641
  Show dependency treegraph
 
Reported: 2014-03-26 15:37 UTC by Julian Maurice
Modified: 2023-06-06 19:24 UTC (History)
21 users (show)

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


Attachments
Bug 11999: Add two checks in CanBookBeReserved and CanItemBeReserved (18.90 KB, patch)
2014-03-26 15:37 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 11999: Add two checks in CanBookBeReserved and CanItemBeReserved (20.62 KB, patch)
2014-06-19 10:01 UTC, Julian Maurice
Details | Diff | Splinter Review
Payment Thanks value entered into accountlines.description (896.17 KB, application/zip)
2014-09-11 10:28 UTC, Nick Clemens
Details
Bug 11999: Add two checks in CanBookBeReserved and CanItemBeReserved (12.12 KB, patch)
2015-02-06 13:21 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 11999: Add two checks in CanBookBeReserved and CanItemBeReserved (12.12 KB, patch)
2015-03-03 08:09 UTC, Julian Maurice
Details | Diff | Splinter Review
Signed patch (12.27 KB, patch)
2015-03-03 13:45 UTC, Alex Sassmannshausen
Details | Diff | Splinter Review
Revised patch with correct author (12.20 KB, patch)
2015-03-05 17:00 UTC, Alex Sassmannshausen
Details | Diff | Splinter Review
Bug 11999: Check for age restriction in CanBookBeReserved (2.35 KB, patch)
2015-04-23 09:59 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 11999: Add two checks in CanBookBeReserved and CanItemBeReserved (11.77 KB, patch)
2015-08-26 08:04 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 11999: Check for age restriction in CanBookBeReserved (2.35 KB, patch)
2015-08-26 08:04 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 11999: Add two checks in CanBookBeReserved and CanItemBeReserved (11.54 KB, patch)
2015-11-09 10:39 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 11999: Check for age restriction in CanBookBeReserved (2.37 KB, patch)
2015-11-09 10:39 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 11999: Add two checks in CanBookBeReserved and CanItemBeReserved (11.82 KB, patch)
2016-01-05 15:03 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 11999: Check for age restriction in CanBookBeReserved (2.65 KB, patch)
2016-01-05 15:03 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 11999: Add two checks in CanBookBeReserved and CanItemBeReserved (11.83 KB, patch)
2016-04-14 09:10 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 11999: Check for age restriction in CanBookBeReserved (2.64 KB, patch)
2016-04-14 09:11 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 11999: Add two checks in CanBookBeReserved and CanItemBeReserved (10.24 KB, patch)
2017-01-10 12:21 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 11999: Check for age restriction in CanBookBeReserved (2.20 KB, patch)
2017-01-10 12:22 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 11999: Remove 'alreadyReserved' check in CanBookBeReserved (2.41 KB, patch)
2017-01-10 12:22 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 11999: Add two checks in CanBookBeReserved and CanItemBeReserved (10.27 KB, patch)
2017-05-22 07:10 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 11999: Check for age restriction in CanBookBeReserved (2.38 KB, patch)
2017-05-22 07:10 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 11999: Remove 'alreadyReserved' check in CanBookBeReserved (2.03 KB, patch)
2017-05-22 07:10 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 11999: Add two checks in CanBookBeReserved and CanItemBeReserved (10.76 KB, patch)
2017-08-21 12:13 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 11999: Check for age restriction in CanBookBeReserved (2.34 KB, patch)
2017-08-21 12:14 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 11999: Remove 'alreadyReserved' check in CanBookBeReserved (1.86 KB, patch)
2017-08-21 12:14 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 11999: Remove the "biblio already reserved" check in CanItemBeReserved (1.07 KB, patch)
2017-08-21 12:14 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 11999: Add two checks in CanBookBeReserved and CanItemBeReserved (11.85 KB, patch)
2019-04-18 14:50 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 11999: Check for age restriction in CanBookBeReserved (4.45 KB, patch)
2019-04-18 14:50 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 11999: Remove 'alreadyReserved' check in CanBookBeReserved (1.54 KB, patch)
2019-04-18 14:51 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 11999: Remove the "biblio already reserved" check in CanItemBeReserved (1.09 KB, patch)
2019-04-18 14:51 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 11999: Add two checks in CanBookBeReserved and CanItemBeReserved (9.56 KB, patch)
2019-05-28 08:17 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 11999: Add two checks in CanBookBeReserved and CanItemBeReserved (9.56 KB, patch)
2019-05-28 08:36 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 11999: Check for age restriction in CanBookBeReserved (3.44 KB, patch)
2019-05-28 08:37 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 11999: Remove 'alreadyReserved' check in CanBookBeReserved (1.54 KB, patch)
2019-05-28 08:38 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 11999: Remove the "biblio already reserved" check in CanItemBeReserved (1.09 KB, patch)
2019-05-28 08:38 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 11999: QA follow-up (1.79 KB, patch)
2019-06-27 08:52 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 11999: QA follow-up (9.49 KB, patch)
2019-07-08 08:56 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 11999: QA follow-up [18.11] (9.54 KB, patch)
2019-07-08 09:03 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 11999: Remove 'alreadyReserved' check in CanBookBeReserved (1.73 KB, patch)
2019-07-12 08:43 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 11999: QA follow-up (9.52 KB, patch)
2019-07-12 09:29 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 11999: QA follow-up (10.27 KB, patch)
2019-07-12 14:42 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 11999: Add two checks in CanBookBeReserved and CanItemBeReserved (4.89 KB, patch)
2019-07-18 13:27 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 11999: Check for age restriction in CanBookBeReserved (1.03 KB, patch)
2019-07-18 13:27 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 11999: Test improvements (8.82 KB, patch)
2019-07-18 13:28 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 11999: Test improvements (7.96 KB, patch)
2019-07-18 14:48 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 11999: Add maxreserves check in CanBookBeReserved and CanItemBeReserved (4.47 KB, patch)
2019-07-18 14:48 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 11999: Test improvements (6.97 KB, patch)
2019-07-18 14:51 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 11999: Test improvements (6.97 KB, patch)
2019-07-18 15:03 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 11999: Add maxreserves check in CanBookBeReserved and CanItemBeReserved (4.20 KB, patch)
2020-03-02 15:16 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 11999: Test improvements (1.30 KB, patch)
2020-03-02 15:17 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 11999: Test improvements + check maxreserves (3.14 KB, patch)
2020-07-03 00:34 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 11999: QA follow-up (4.61 KB, patch)
2020-07-03 10:02 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 11999: Add maxreserves check in CanBookBeReserved and CanItemBeReserved (4.78 KB, patch)
2020-10-15 09:38 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 11999: Test improvements + check maxreserves (3.31 KB, patch)
2020-10-15 09:38 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 11999: (QA follow-up) (5.54 KB, patch)
2020-10-15 09:38 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 11999: Add maxreserves check in CanBookBeReserved and CanItemBeReserved (5.11 KB, patch)
2020-11-02 21:06 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 11999: Test improvements + check maxreserves (3.35 KB, patch)
2020-11-02 21:07 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 11999: (QA follow-up) (5.76 KB, patch)
2020-11-02 21:07 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 11999: (QA follow-up) (5.34 KB, patch)
2020-11-02 21:24 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 11999: Add maxreserves check in CanBookBeReserved and CanItemBeReserved (4.20 KB, patch)
2021-05-10 12:53 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 11999: Test improvements + check maxreserves (3.13 KB, patch)
2021-05-10 12:53 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 11999: Test improvements + check maxreserves (3.13 KB, patch)
2021-05-10 12:54 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 11999: Add maxreserves check in CanBookBeReserved and CanItemBeReserved (4.17 KB, patch)
2021-05-10 12:55 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 11999: Test improvements + check maxreserves (3.13 KB, patch)
2021-05-10 12:55 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 11999: Test improvements + check maxreserves (3.27 KB, patch)
2021-06-01 12:18 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Test fails for holds - 2021-06-04 (16.51 KB, text/plain)
2021-06-03 22:42 UTC, David Nind
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Julian Maurice 2014-03-26 15:37:12 UTC
- Check if borrower already has reserved the same biblio (or item).
- Check if borrower has reached the maximum number of holds allowed (syspref maxreserves)
Comment 1 Julian Maurice 2014-03-26 15:37:52 UTC Comment hidden (obsolete)
Comment 2 Julian Maurice 2014-03-26 15:38:56 UTC
Increasing severity from enh to normal, as the patch fixes a bug in ILS-DI
Comment 3 Mark Tompsett 2014-03-29 04:29:21 UTC
Comment on attachment 26589 [details] [review]
Bug 11999: Add two checks in CanBookBeReserved and CanItemBeReserved

Review of attachment 26589 [details] [review]:
-----------------------------------------------------------------

::: C4/ILSDI/Services.pm
@@ +609,4 @@
>      my $title = $$biblio{title};
>  
>      # Check if the biblio can be reserved
> +    my ($canbookbereserved) = CanBookBeReserved($borrowernumber, $biblionumber);

The change in interface that passes back reasons is lost, when you don't include a variable for it. Also, it makes this look like a pointless variable. I suppose it's just hard to read and not really incorrect.

@@ +686,5 @@
>      return { code => 'RecordNotFound' } if $$item{biblionumber} ne $$biblio{biblionumber};
>  
>      # Check for item disponibility
> +    my ($canitembereserved) = C4::Reserves::CanItemBeReserved( $borrowernumber, $itemnumber );
> +    my ($canbookbereserved) = C4::Reserves::CanBookBeReserved( $borrowernumber, $biblionumber );

Why is there a C4::Reserves::CanBookBeReserved call here, but a CanBookBeReserved in the earlier code?
Comment 4 Julian Maurice 2014-03-29 12:12:57 UTC
> The change in interface that passes back reasons is lost, when you don't
> include a variable for it. Also, it makes this look like a pointless
> variable. I suppose it's just hard to read and not really incorrect.
I don't understand. CanBookBeReserved now returns a list so the return value is affected to a list. Why does the variable look pointless ? I just don't need the $reasons here, so I don't retrieve it.

> Why is there a C4::Reserves::CanBookBeReserved call here, but a
> CanBookBeReserved in the earlier code?
I don't know, but it was like that before the patch, so I didn't change this.
Comment 5 Julian Maurice 2014-06-19 10:01:15 UTC Comment hidden (obsolete)
Comment 6 Nick Clemens 2014-06-29 13:40:19 UTC
Pardon if I am confused, but CanBooksBeReserved can return three different reasons  [alreadyreserved, maxreseres, noholdableitems]

But the check in request.pl checks for alreadyreserved and assumes maxreserves otherwise

   my $alreadyreserved = 0;
190	        $maxreserves = 1;	
191	    my ($can_book_be_reserved, $reasons) =
192	      CanBookBeReserved( $borrowerinfo->{borrowernumber}, $biblionumber );
193	
194	    unless ($can_book_be_reserved) {
195	        if ($reasons->{alreadyreserved}) {
196	            $alreadyreserved = 1;
197	            $biblioloopiter{warn} = 1;
198	            $biblioloopiter{alreadyres} = 1;
199	        } else {
200	            $maxreserves = 1;
201	        }

This seems to be the same issue as Bug 10912

To my mind, maxreserves shouldn't be flagged unless the patron is over their number of reserves.
Comment 7 Nick Clemens 2014-09-11 10:28:12 UTC Comment hidden (obsolete)
Comment 8 Nick Clemens 2014-09-11 10:30:07 UTC
Comment on attachment 31524 [details]
Payment Thanks value entered into accountlines.description

Wrong bug# sorry
Comment 9 Olli-Antti Kivilahti 2014-10-24 15:26:15 UTC
Hi!
It looks like this duplicates Bug 13116.

Could you take a look at Comment 11 of Bug 13116 and give me some ideas?

Also, could you change your title to something more descriptive?
Like "Propagating error messages out from CanBookBeReserved"?
Comment 10 Olli-Antti Kivilahti 2014-10-29 16:09:21 UTC
Hi there!

Apparently Bug 13116 is already signed off. Can you refactor your feature to supplement mine?
Comment 11 Marcel de Rooy 2015-01-15 14:04:16 UTC
CONFLICT (content): Merge conflict in t/db_dependent/Holds.t
Auto-merging reserve/request.pl
CONFLICT (content): Merge conflict in reserve/request.pl
Auto-merging opac/opac-reserve.pl
CONFLICT (content): Merge conflict in opac/opac-reserve.pl
Auto-merging C4/Reserves.pm
CONFLICT (content): Merge conflict in C4/Reserves.pm
Auto-merging C4/ILSDI/Services.pm
CONFLICT (content): Merge conflict in C4/ILSDI/Services.pm
Comment 12 Julian Maurice 2015-02-06 13:21:35 UTC Comment hidden (obsolete)
Comment 13 Julian Maurice 2015-02-06 13:24:35 UTC
Patch rebased on master (and consequently on top of bug 13116)

There is one unit test that fails in t/db_dependent/Holds.t (line 331), but it fails on master too.
Should I open a separate bug for this ?
Comment 14 Marcel de Rooy 2015-02-09 07:39:35 UTC
(In reply to Julian Maurice from comment #13)
> Patch rebased on master (and consequently on top of bug 13116)
> 
> There is one unit test that fails in t/db_dependent/Holds.t (line 331), but
> it fails on master too.
> Should I open a separate bug for this ?

Current master e5b834a1c474f761c9f9c8c01dd6abeb99ee5eac does not fail this test with me.
Comment 15 Julian Maurice 2015-02-10 11:40:35 UTC
(In reply to M. de Rooy from comment #14)
> Current master e5b834a1c474f761c9f9c8c01dd6abeb99ee5eac does not fail this
> test with me.

Maybe this is the kind of test that depends on some configuration in database that you have and I have not.
Could you try to apply the patch and see if the test pass ?
Comment 16 Alex Sassmannshausen 2015-03-02 15:53:41 UTC
Hi Julian,

I've just tried to sign this one off using a sandbox, but there was a problem applying the patch.

I think it may need to be rebased again?

I'll try again when you've had a look.

Cheers,

Alex
Comment 17 Julian Maurice 2015-03-03 08:09:12 UTC Comment hidden (obsolete)
Comment 18 Alex Sassmannshausen 2015-03-03 13:45:41 UTC Comment hidden (obsolete)
Comment 19 Jonathan Druart 2015-03-05 16:01:00 UTC
Alex, to change the patch's author, you can use
  git commit --amend --author "Author Name <authoremail@example.org>"
Comment 20 Alex Sassmannshausen 2015-03-05 16:11:59 UTC
(In reply to Jonathan Druart from comment #19)
> Alex, to change the patch's author, you can use
>   git commit --amend --author "Author Name <authoremail@example.org>"

Sorry Jonathan, I'm afraid I'm confused: do you need me to change the name of the contributor on the patch?
Comment 21 Jonathan Druart 2015-03-05 16:32:19 UTC
(In reply to Alex Sassmannshausen from comment #20)
> (In reply to Jonathan Druart from comment #19)
> > Alex, to change the patch's author, you can use
> >   git commit --amend --author "Author Name <authoremail@example.org>"
> 
> Sorry Jonathan, I'm afraid I'm confused: do you need me to change the name
> of the contributor on the patch?

Actually it could be done by the QAer, just wanted to share with you the trick if you didn't know.
Comment 22 Alex Sassmannshausen 2015-03-05 17:00:00 UTC Comment hidden (obsolete)
Comment 23 Katrin Fischer 2015-03-22 20:04:34 UTC
This seems to break one of the existing tests, please check:

not ok 52 - Reserving a 'PEGI 16' Biblio by a 15 year old borrower fails
#   Failed test 'Reserving a 'PEGI 16' Biblio by a 15 year old borrower fails'
#   at t/db_dependent/Reserves.t line 498.
#          got: 'none_available'
#     expected: 'ageRestricted'
ok 53 - Reserving a 'PEGI 16' Biblio by a 30 year old borrower succeeds
# Looks like you failed 1 test of 53.

Please also include a short explanation in your new tests:

ok 35 - cannot request item if policy that matches on bib-level item type forbids it (bug 9532)
ok 36
ok 37
ok 38
ok 39 - Waiting reserve beyond max pickup delay *not* canceled on holiday
Comment 24 Julian Maurice 2015-04-23 09:59:25 UTC Comment hidden (obsolete)
Comment 25 Julian Maurice 2015-04-23 10:00:31 UTC
This patch should make the "age restriction" test pass
Comment 26 Mark Tompsett 2015-04-23 12:11:16 UTC
(In reply to Julian Maurice from comment #25)
> This patch should make the "age restriction" test pass

This may totally be tangential and unrelated, but I vaguely recall hitting an age restriction problem in some test file, and it turned out to be a timezone mismatch problem somewhere.
Comment 27 Julian Maurice 2015-08-26 08:04:18 UTC Comment hidden (obsolete)
Comment 28 Julian Maurice 2015-08-26 08:04:29 UTC Comment hidden (obsolete)
Comment 29 Mark Tompsett 2015-08-26 11:31:42 UTC
(In reply to Katrin Fischer from comment #23)
> This seems to break one of the existing tests, please check:
> 
> not ok 52 - Reserving a 'PEGI 16' Biblio by a 15 year old borrower fails
> #   Failed test 'Reserving a 'PEGI 16' Biblio by a 15 year old borrower
> fails'
> #   at t/db_dependent/Reserves.t line 498.
> #          got: 'none_available'
> #     expected: 'ageRestricted'
> ok 53 - Reserving a 'PEGI 16' Biblio by a 30 year old borrower succeeds
> # Looks like you failed 1 test of 53.
> 
> Please also include a short explanation in your new tests:
> 
> ok 35 - cannot request item if policy that matches on bib-level item type
> forbids it (bug 9532)
> ok 36
> ok 37
> ok 38
> ok 39 - Waiting reserve beyond max pickup delay *not* canceled on holiday

You ran it at the magical failure hour for your timezone.
See bug 14362.
Comment 30 Liz Rea 2015-11-06 01:41:56 UTC
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/Holds.t
CONFLICT (content): Merge conflict in t/db_dependent/Holds.t
Auto-merging reserve/request.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt
Comment 31 Julian Maurice 2015-11-09 10:39:26 UTC Comment hidden (obsolete)
Comment 32 Julian Maurice 2015-11-09 10:39:36 UTC Comment hidden (obsolete)
Comment 33 Alex Arnaud 2016-01-05 15:03:21 UTC Comment hidden (obsolete)
Comment 34 Alex Arnaud 2016-01-05 15:03:28 UTC Comment hidden (obsolete)
Comment 35 Alex Arnaud 2016-01-05 15:56:02 UTC
*** Bug 14155 has been marked as a duplicate of this bug. ***
Comment 36 Jonathan Druart 2016-01-07 12:06:14 UTC
Can we get tests for the age restriction check please?
Comment 37 Alex Arnaud 2016-03-15 09:51:09 UTC
Hello Jonathan,

Why should we add tests for the age restriction ? Code about checking age restriction has not chenged and sub C4::Circulation::GetAgeRestriction() is already tested int/Circulation/AgeRestrictionMarkers.t.
Comment 38 Alex Arnaud 2016-03-15 10:01:00 UTC
Also, i can see:
t/db_dependent/Reserves.t:is( C4::Reserves::CanBookBeReserved($borrowernumber, $biblionumber) , 'OK', "Reserving an ageRestricted Biblio without a borrower dateofbirth succeeds" );
t/db_dependent/Reserves.t:is( C4::Reserves::CanBookBeReserved($borrowernumber, $biblionumber) , 'ageRestricted', "Reserving a 'PEGI 16' Biblio by a 15 year old borrower fails");
Comment 39 Jonathan Druart 2016-04-04 13:25:35 UTC
(In reply to Alex Arnaud from comment #37)
> Hello Jonathan,
> 
> Why should we add tests for the age restriction ? Code about checking age
> restriction has not chenged and sub C4::Circulation::GetAgeRestriction() is
> already tested int/Circulation/AgeRestrictionMarkers.t.

The changes about the age restriction in CanBookBeReserved is not covered by tests.
Comment 40 Julian Maurice 2016-04-14 09:10:59 UTC
Created attachment 50224 [details] [review]
Bug 11999: Add two checks in CanBookBeReserved and CanItemBeReserved

- Check if borrower already has reserved the same biblio (or item).
- Check if borrower has reached the maximum number of holds allowed
  (syspref maxreserves)

The goal of this patch is to do these checks also when using ILS-DI
services HoldTitle and HoldItem

Test plan:
----------
Before patch:
1/ Set syspref maxreserves to 1
2/ Place some holds through ILS-DI and note that you can place more than
   1 hold for a borrower
3/ Try to hold the same biblio/item with the same borrower through
   ILS-DI and note that you can

Apply the patch
4/ Place some holds through ILS-DI, you shouldn't be able to place more
   than 1 hold for a borrower
5/ Try to hold the same biblio/item with the same borrower through
   ILS-DI. You shouldn't be able to do that.
6/ Try to place holds on staff interface and OPAC. The behaviour must be
   identical than before the patch.

Maxreserves and alreadyreserved works on ILSDI
Maxreserves works also on staff and opac
Already reserves works also on staff and opac
tests on t/db_dependent/Reserves.t and t/db_dependent/Holds.t passe (using koha_ut db)

Signed-off-by: Alex Sassmannshausen <alex.sassmannshausen@ptfs-europe.com>
Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 41 Julian Maurice 2016-04-14 09:11:03 UTC
Created attachment 50225 [details] [review]
Bug 11999: Check for age restriction in CanBookBeReserved

+ improve t/db_dependent/Reserves.t to make tests pass on UNIMARC
installation

Maxreserves and alreadyreserved works on ILSDI
Maxreserves works also on staff and opac
Already reserves works also on staff and opac
tests on t/db_dependent/Reserves.t and t/db_dependent/Holds.t passe (using koha_ut db)

Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 42 Julian Maurice 2016-10-11 14:43:03 UTC
I forgot to change the bug status last time.
Back to Signed off
Comment 43 Marcel de Rooy 2016-11-25 07:40:00 UTC
Patch does not apply and needs adjustments in view of the pushed "multiple holds per record"-functionality.
Comment 44 Julian Maurice 2017-01-10 12:21:58 UTC
Created attachment 58741 [details] [review]
Bug 11999: Add two checks in CanBookBeReserved and CanItemBeReserved

- Check if borrower already has reserved the same biblio (or item).
- Check if borrower has reached the maximum number of holds allowed
  (syspref maxreserves)

The goal of this patch is to do these checks also when using ILS-DI
services HoldTitle and HoldItem

Test plan:
----------
Before patch:
1/ Set syspref maxreserves to 1
2/ Place some holds through ILS-DI and note that you can place more than
   1 hold for a borrower
3/ Try to hold the same biblio/item with the same borrower through
   ILS-DI and note that you can

Apply the patch
4/ Place some holds through ILS-DI, you shouldn't be able to place more
   than 1 hold for a borrower
5/ Try to hold the same biblio/item with the same borrower through
   ILS-DI. You shouldn't be able to do that.
6/ Try to place holds on staff interface and OPAC. The behaviour must be
   identical than before the patch.

Maxreserves and alreadyreserved works on ILSDI
Maxreserves works also on staff and opac
Already reserves works also on staff and opac
tests on t/db_dependent/Reserves.t and t/db_dependent/Holds.t passe (using koha_ut db)

Signed-off-by: Alex Sassmannshausen <alex.sassmannshausen@ptfs-europe.com>
Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 45 Julian Maurice 2017-01-10 12:22:07 UTC
Created attachment 58742 [details] [review]
Bug 11999: Check for age restriction in CanBookBeReserved

+ improve t/db_dependent/Reserves.t to make tests pass on UNIMARC
installation

Maxreserves and alreadyreserved works on ILSDI
Maxreserves works also on staff and opac
Already reserves works also on staff and opac
tests on t/db_dependent/Reserves.t and t/db_dependent/Holds.t passe (using koha_ut db)

Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 46 Julian Maurice 2017-01-10 12:22:15 UTC
Created attachment 58743 [details] [review]
Bug 11999: Remove 'alreadyReserved' check in CanBookBeReserved

Since bug 14695 it is possible to reserve more than one item per biblio

Also fix bad use of $item
Comment 47 Marc Véron 2017-04-12 13:02:47 UTC
WAnted to test, but does not apply:

Bug 11999: Check for age restriction in CanBookBeReserved
fatal: sha1 information is lacking or useless (C4/Reserves.pm).
Comment 48 Fridolin Somers 2017-05-17 07:16:25 UTC
Bug 13113 added check for age restriction on CanItemBeReserved
Comment 49 Julian Maurice 2017-05-22 07:10:23 UTC
Created attachment 63593 [details] [review]
Bug 11999: Add two checks in CanBookBeReserved and CanItemBeReserved

- Check if borrower already has reserved the same biblio (or item).
- Check if borrower has reached the maximum number of holds allowed
  (syspref maxreserves)

The goal of this patch is to do these checks also when using ILS-DI
services HoldTitle and HoldItem

Test plan:
----------
Before patch:
1/ Set syspref maxreserves to 1
2/ Place some holds through ILS-DI and note that you can place more than
   1 hold for a borrower
3/ Try to hold the same biblio/item with the same borrower through
   ILS-DI and note that you can

Apply the patch
4/ Place some holds through ILS-DI, you shouldn't be able to place more
   than 1 hold for a borrower
5/ Try to hold the same biblio/item with the same borrower through
   ILS-DI. You shouldn't be able to do that.
6/ Try to place holds on staff interface and OPAC. The behaviour must be
   identical than before the patch.

Maxreserves and alreadyreserved works on ILSDI
Maxreserves works also on staff and opac
Already reserves works also on staff and opac
tests on t/db_dependent/Reserves.t and t/db_dependent/Holds.t passe (using koha_ut db)

Signed-off-by: Alex Sassmannshausen <alex.sassmannshausen@ptfs-europe.com>
Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 50 Julian Maurice 2017-05-22 07:10:28 UTC
Created attachment 63594 [details] [review]
Bug 11999: Check for age restriction in CanBookBeReserved

+ improve t/db_dependent/Reserves.t to make tests pass on UNIMARC
installation

Maxreserves and alreadyreserved works on ILSDI
Maxreserves works also on staff and opac
Already reserves works also on staff and opac
tests on t/db_dependent/Reserves.t and t/db_dependent/Holds.t passe (using koha_ut db)

Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 51 Julian Maurice 2017-05-22 07:10:33 UTC
Created attachment 63595 [details] [review]
Bug 11999: Remove 'alreadyReserved' check in CanBookBeReserved

Since bug 14695 it is possible to reserve more than one item per biblio

Also fix bad use of $item
Comment 52 Josef Moravec 2017-05-22 08:07:29 UTC
Test t/db_dependent/Holds.t is failing:

#   Failed test 'Patron cannot reserve item with hold limit of 1, 1 bib level hold placed'
#   at t/db_dependent/Holds.t line 472.
#          got: 'alreadyReserved'
#     expected: 'tooManyReserves'
Comment 53 Julian Maurice 2017-08-21 12:13:58 UTC
Created attachment 66267 [details] [review]
Bug 11999: Add two checks in CanBookBeReserved and CanItemBeReserved

- Check if borrower already has reserved the same biblio (or item).
- Check if borrower has reached the maximum number of holds allowed
  (syspref maxreserves)

The goal of this patch is to do these checks also when using ILS-DI
services HoldTitle and HoldItem

Test plan:
----------
Before patch:
1/ Set syspref maxreserves to 1
2/ Place some holds through ILS-DI and note that you can place more than
   1 hold for a borrower
3/ Try to hold the same biblio/item with the same borrower through
   ILS-DI and note that you can

Apply the patch
4/ Place some holds through ILS-DI, you shouldn't be able to place more
   than 1 hold for a borrower
5/ Try to hold the same biblio/item with the same borrower through
   ILS-DI. You shouldn't be able to do that.
6/ Try to place holds on staff interface and OPAC. The behaviour must be
   identical than before the patch.

Maxreserves and alreadyreserved works on ILSDI
Maxreserves works also on staff and opac
Already reserves works also on staff and opac
tests on t/db_dependent/Reserves.t and t/db_dependent/Holds.t passe (using koha_ut db)

Signed-off-by: Alex Sassmannshausen <alex.sassmannshausen@ptfs-europe.com>
Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 54 Julian Maurice 2017-08-21 12:14:03 UTC
Created attachment 66268 [details] [review]
Bug 11999: Check for age restriction in CanBookBeReserved

+ improve t/db_dependent/Reserves.t to make tests pass on UNIMARC
installation

Maxreserves and alreadyreserved works on ILSDI
Maxreserves works also on staff and opac
Already reserves works also on staff and opac
tests on t/db_dependent/Reserves.t and t/db_dependent/Holds.t passe (using koha_ut db)

Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 55 Julian Maurice 2017-08-21 12:14:07 UTC
Created attachment 66269 [details] [review]
Bug 11999: Remove 'alreadyReserved' check in CanBookBeReserved

Since bug 14695 it is possible to reserve more than one item per biblio

Also fix bad use of $item
Comment 56 Julian Maurice 2017-08-21 12:14:11 UTC
Created attachment 66270 [details] [review]
Bug 11999: Remove the "biblio already reserved" check in CanItemBeReserved
Comment 57 Julian Maurice 2017-08-21 12:15:01 UTC
All patches rebased on master.
The last patch should fix comment 52
Comment 58 Josef Moravec 2017-08-22 11:29:17 UTC
Some comments from QA test tools, sorry

 FAIL	C4/Reserves.pm
   FAIL	  valid
		"my" variable $holds masks earlier declaration in same scope 

 FAIL	reserve/request.pl
   FAIL	  valid
		"my" variable $holds masks earlier declaration in same scope 

 FAIL	t/db_dependent/Holds.t
   FAIL	  valid
		"my" variable $holds masks earlier declaration in same scope 

 FAIL	t/db_dependent/Reserves.t
   FAIL	  valid
		"my" variable $holds masks earlier declaration in same scope
Comment 59 Fridolin Somers 2017-11-08 11:35:12 UTC
Looks like CanBookBeReserved may be called with undef biblionumber : when at intranet clicking on holds tab of a record.
This generates error :
  Can't call method "holds" on an undefined value
Comment 60 Fridolin Somers 2017-11-08 12:39:37 UTC
(In reply to Fridolin SOMERS from comment #59)
> Looks like CanBookBeReserved may be called with undef biblionumber : when at
> intranet clicking on holds tab of a record.
> This generates error :
>   Can't call method "holds" on an undefined value

Ok in master, CanBookBeReserved is not called if no borrower.
Sorry
Comment 61 Arthur Suzuki 2019-04-18 14:50:32 UTC
Created attachment 88300 [details] [review]
Bug 11999: Add two checks in CanBookBeReserved and CanItemBeReserved

- Check if borrower already has reserved the same biblio (or item).
- Check if borrower has reached the maximum number of holds allowed
  (syspref maxreserves)

The goal of this patch is to do these checks also when using ILS-DI
services HoldTitle and HoldItem

Test plan:
----------
Before patch:
1/ Set syspref maxreserves to 1
2/ Place some holds through ILS-DI and note that you can place more than
   1 hold for a borrower
3/ Try to hold the same biblio/item with the same borrower through
   ILS-DI and note that you can

Apply the patch
4/ Place some holds through ILS-DI, you shouldn't be able to place more
   than 1 hold for a borrower
5/ Try to hold the same biblio/item with the same borrower through
   ILS-DI. You shouldn't be able to do that.
6/ Try to place holds on staff interface and OPAC. The behaviour must be
   identical than before the patch.

Maxreserves and alreadyreserved works on ILSDI
Maxreserves works also on staff and opac
Already reserves works also on staff and opac
tests on t/db_dependent/Reserves.t and t/db_dependent/Holds.t passe (using koha_ut db)

Signed-off-by: Alex Sassmannshausen <alex.sassmannshausen@ptfs-europe.com>
Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 62 Arthur Suzuki 2019-04-18 14:50:48 UTC
Created attachment 88301 [details] [review]
Bug 11999: Check for age restriction in CanBookBeReserved

+ improve t/db_dependent/Reserves.t to make tests pass on UNIMARC
installation

Maxreserves and alreadyreserved works on ILSDI
Maxreserves works also on staff and opac
Already reserves works also on staff and opac
tests on t/db_dependent/Reserves.t and t/db_dependent/Holds.t passe (using koha_ut db)

Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 63 Arthur Suzuki 2019-04-18 14:51:06 UTC
Created attachment 88302 [details] [review]
Bug 11999: Remove 'alreadyReserved' check in CanBookBeReserved

Since bug 14695 it is possible to reserve more than one item per biblio

Also fix bad use of $item
Comment 64 Arthur Suzuki 2019-04-18 14:51:24 UTC
Created attachment 88303 [details] [review]
Bug 11999: Remove the "biblio already reserved" check in CanItemBeReserved
Comment 65 Arthur Suzuki 2019-04-18 14:52:17 UTC
rebased on master, plz sign me ^^
Comment 66 Magnus Enger 2019-05-23 13:07:39 UTC
attachment 88300 [details] [review] has merge markers (<<<<) in it. Maybe the rebase was not complete?
Comment 67 Arthur Suzuki 2019-05-28 08:17:46 UTC
Created attachment 90142 [details] [review]
Bug 11999: Add two checks in CanBookBeReserved and CanItemBeReserved

- Check if borrower already has reserved the same biblio (or item).
- Check if borrower has reached the maximum number of holds allowed
  (syspref maxreserves)

The goal of this patch is to do these checks also when using ILS-DI
services HoldTitle and HoldItem

Test plan:
----------
Before patch:
1/ Set syspref maxreserves to 1
2/ Place some holds through ILS-DI and note that you can place more than
   1 hold for a borrower
3/ Try to hold the same biblio/item with the same borrower through
   ILS-DI and note that you can

Apply the patch
4/ Place some holds through ILS-DI, you shouldn't be able to place more
   than 1 hold for a borrower
5/ Try to hold the same biblio/item with the same borrower through
   ILS-DI. You shouldn't be able to do that.
6/ Try to place holds on staff interface and OPAC. The behaviour must be
   identical than before the patch.

Maxreserves and alreadyreserved works on ILSDI
Maxreserves works also on staff and opac
Already reserves works also on staff and opac
tests on t/db_dependent/Reserves.t and t/db_dependent/Holds.t passe (using koha_ut db)

Signed-off-by: Alex Sassmannshausen <alex.sassmannshausen@ptfs-europe.com>
Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 68 Arthur Suzuki 2019-05-28 08:36:36 UTC
Created attachment 90143 [details] [review]
Bug 11999: Add two checks in CanBookBeReserved and CanItemBeReserved

- Check if borrower already has reserved the same biblio (or item).
- Check if borrower has reached the maximum number of holds allowed
  (syspref maxreserves)

The goal of this patch is to do these checks also when using ILS-DI
services HoldTitle and HoldItem

Test plan:
----------
Before patch:
1/ Set syspref maxreserves to 1
2/ Place some holds through ILS-DI and note that you can place more than
   1 hold for a borrower
3/ Try to hold the same biblio/item with the same borrower through
   ILS-DI and note that you can

Apply the patch
4/ Place some holds through ILS-DI, you shouldn't be able to place more
   than 1 hold for a borrower
5/ Try to hold the same biblio/item with the same borrower through
   ILS-DI. You shouldn't be able to do that.
6/ Try to place holds on staff interface and OPAC. The behaviour must be
   identical than before the patch.

Maxreserves and alreadyreserved works on ILSDI
Maxreserves works also on staff and opac
Already reserves works also on staff and opac
tests on t/db_dependent/Reserves.t and t/db_dependent/Holds.t passe (using koha_ut db)

Signed-off-by: Alex Sassmannshausen <alex.sassmannshausen@ptfs-europe.com>
Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 69 Arthur Suzuki 2019-05-28 08:37:29 UTC
Created attachment 90144 [details] [review]
Bug 11999: Check for age restriction in CanBookBeReserved

+ improve t/db_dependent/Reserves.t to make tests pass on UNIMARC
installation

Maxreserves and alreadyreserved works on ILSDI
Maxreserves works also on staff and opac
Already reserves works also on staff and opac
tests on t/db_dependent/Reserves.t and t/db_dependent/Holds.t passe (using koha_ut db)

Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 70 Arthur Suzuki 2019-05-28 08:38:14 UTC
Created attachment 90146 [details] [review]
Bug 11999: Remove 'alreadyReserved' check in CanBookBeReserved

Since bug 14695 it is possible to reserve more than one item per biblio

Also fix bad use of $item
Comment 71 Arthur Suzuki 2019-05-28 08:38:53 UTC
Created attachment 90147 [details] [review]
Bug 11999: Remove the "biblio already reserved" check in CanItemBeReserved
Comment 72 Arthur Suzuki 2019-05-28 08:42:58 UTC
(In reply to Magnus Enger from comment #66)
> attachment 88300 [details] [review] [review] has merge markers (<<<<) in it. Maybe
> the rebase was not complete?

Thanks for pointing that out :) did a re-rebase and re-upload, everything applies fine now (still asking for QA though)
Comment 73 Katrin Fischer 2019-05-28 08:46:49 UTC
"Pushed for QA" still doesn't mean what you think ;)
Comment 74 Marcel de Rooy 2019-05-31 07:41:52 UTC
Run QA tools please, and fix
Comment 75 Arthur Suzuki 2019-06-27 08:52:25 UTC
Created attachment 91046 [details] [review]
Bug 11999: QA follow-up
Comment 76 Fridolin Somers 2019-07-05 10:09:20 UTC
Arg there are conflict signes in patches :

+<<<<<<< HEAD
     CanItemBeReserved( $borrowernumbers[0], $itemnumber)->{status}, 'tooManyReserves',
+=======
+    CanItemBeReserved( $borrowernumbers[0], $itemnumber2), 'tooManyReserves',
+>>>>>>> Bug 11999: Add two checks in CanBookBeReserved and CanItemBeReserved
Comment 77 Arthur Suzuki 2019-07-08 08:56:33 UTC
Created attachment 91402 [details] [review]
Bug 11999: QA follow-up
Comment 78 Arthur Suzuki 2019-07-08 09:03:53 UTC
Created attachment 91403 [details] [review]
Bug 11999: QA follow-up [18.11]
Comment 79 Fridolin Somers 2019-07-09 12:01:26 UTC
I see test "alreadyReserved" on same biblio still exists inCanBookBeReserved. 
It should be removed says "Remove 'alreadyReserved' check in CanBookBeReserved".
Comment 80 Arthur Suzuki 2019-07-12 08:43:52 UTC
Created attachment 91479 [details] [review]
Bug 11999: Remove 'alreadyReserved' check in CanBookBeReserved

Since bug 14695 it is possible to reserve more than one item per biblio

Also fix bad use of $item
Comment 81 Arthur Suzuki 2019-07-12 09:29:16 UTC
Created attachment 91481 [details] [review]
Bug 11999: QA follow-up
Comment 82 Nick Clemens 2019-07-12 13:48:29 UTC
I think you need to obsolete some of these, not sure why one followup is 18.11?
Comment 83 Arthur Suzuki 2019-07-12 14:42:40 UTC
Created attachment 91494 [details] [review]
Bug 11999: QA follow-up
Comment 84 Arthur Suzuki 2019-07-16 12:51:20 UTC
(In reply to Nick Clemens from comment #82)
> I think you need to obsolete some of these, not sure why one followup is
> 18.11?

Indeed... that one was to prepare backporting to 18.11.
Comment 85 Nick Clemens 2019-07-16 19:54:27 UTC
I tried cleaning these up, but hit some snags, a few comments:
1 - Please squash these - removing the merge markers at the end is messy
2 - We now have a routine to create helper bibs 'build_sample_biblio' - please use that
3 - you change some tests to expect 'tooManyHoldsForThisRecord' but are allowing 99 so these are failing - it would be nice to have those cases covered, but I think this change may have been accidental?
Comment 86 Arthur Suzuki 2019-07-18 13:27:00 UTC
Created attachment 91580 [details] [review]
Bug 11999: Add two checks in CanBookBeReserved and CanItemBeReserved

- Check if borrower already has reserved the same item.
- Check if borrower has reached the maximum number of holds allowed
  (syspref maxreserves)

The goal of this patch is to do these checks also when using ILS-DI
services HoldTitle and HoldItem

Test plan:
----------
Before patch:
1/ Set syspref maxreserves to 1
2/ Place some holds through ILS-DI and note that you can place more than
   1 hold for a borrower
3/ Try to hold the same item with the same borrower through
   ILS-DI and note that you can

Apply the patch
4/ Place some holds through ILS-DI, you shouldn't be able to place more
   than 1 hold for a borrower
5/ Try to hold the same item with the same borrower through
   ILS-DI. You shouldn't be able to do that.
6/ Try to place holds on staff interface and OPAC. The behaviour must be
   identical than before the patch.

Maxreserves and alreadyreserved works on ILSDI
Maxreserves works also on staff and opac
Already reserves works also on staff and opac
tests on t/db_dependent/Reserves.t and t/db_dependent/Holds.t passe (using koha_ut db)

Signed-off-by: Alex Sassmannshausen <alex.sassmannshausen@ptfs-europe.com>
Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 87 Arthur Suzuki 2019-07-18 13:27:37 UTC
Created attachment 91581 [details] [review]
Bug 11999: Check for age restriction in CanBookBeReserved
Comment 88 Arthur Suzuki 2019-07-18 13:28:03 UTC
Created attachment 91582 [details] [review]
Bug 11999: Test improvements

improve t/db_dependent/Reserves.t to make tests pass on UNIMARC installation
Comment 89 Arthur Suzuki 2019-07-18 14:48:30 UTC
Created attachment 91590 [details] [review]
Bug 11999: Test improvements

improve t/db_dependent/Reserves.t to make tests pass on UNIMARC installation
Comment 90 Arthur Suzuki 2019-07-18 14:48:52 UTC
Created attachment 91591 [details] [review]
Bug 11999: Add maxreserves check in CanBookBeReserved and CanItemBeReserved

- Check if borrower has reached the maximum number of holds allowed
  (syspref maxreserves)

The goal of this patch is to do these checks also when using ILS-DI
services HoldTitle and HoldItem

Test plan:
----------
Before patch:
1/ Set syspref maxreserves to 1
2/ Place some holds through ILS-DI and note that you can place more than
   1 hold for a borrower

Apply the patch
3/ Place some holds through ILS-DI, you shouldn't be able to place more
   than 1 hold for a borrower
4/ Try to place holds on staff interface and OPAC. The behaviour must be
   identical than before the patch.

Maxreserves and alreadyreserved works on ILSDI
Maxreserves works also on staff and opac
Already reserves works also on staff and opac
tests on t/db_dependent/Reserves.t and t/db_dependent/Holds.t passe (using koha_ut db)

Signed-off-by: Alex Sassmannshausen <alex.sassmannshausen@ptfs-europe.com>
Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 91 Arthur Suzuki 2019-07-18 14:51:46 UTC
Created attachment 91592 [details] [review]
Bug 11999: Test improvements

improve t/db_dependent/Reserves.t to make tests pass on UNIMARC installation
Comment 92 Arthur Suzuki 2019-07-18 15:03:28 UTC
Created attachment 91593 [details] [review]
Bug 11999: Test improvements

improve t/db_dependent/Reserves.t to make tests pass on UNIMARC installation
Comment 93 David Nind 2020-02-23 17:05:27 UTC
Patch no longer applies:

root@e83d8895749e:koha(bz11999)$ git bz apply 11999
Bug 11999 - Add two checks in CanBookBeReserved and CanItemBeReserved

91581 - Bug 11999: Check for age restriction in CanBookBeReserved
91591 - Bug 11999: Add maxreserves check in CanBookBeReserved and CanItemBeReserved
91593 - Bug 11999: Test improvements

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 11999: Check for age restriction in CanBookBeReserved
error: sha1 information is lacking or useless (C4/Reserves.pm).
error: could not build fake ancestor
Patch failed at 0001 Bug 11999: Check for age restriction in CanBookBeReserved
Comment 94 David Nind 2020-02-23 17:10:26 UTC
Similar to Bug 17229 you get an error on the patron's page when following the test plan (step 2 before patch), but before trying to apply the patch:

1) For the patron, the holds tab heading shows that there is a hold (1).

2) When you click on the patron's holds tab a message pops up with "DataTables warning: table id=holds-table - Ajax error. For more information about this error, please see http://datatables.net/tn/7"

3) The bibliographic record shows that there is a hold.

I'm assuming that is because the DataTables feature for displaying results is a more recent addition to Koha.

(Test URL I used for ILS_DI was http://127.0.0.1:8080/cgi-bin/koha/ilsdi.pl?service=HoldTitle&patron_id=19&bib_id=15&request_location='127.0.0.1' using koha-testing-docker)
Comment 95 Arthur Suzuki 2020-03-02 14:11:51 UTC
Just realized Age Restriction issue has been fixed in BZ13113.
Will re-post patches and change tests accordingly.
Comment 96 Arthur Suzuki 2020-03-02 15:16:33 UTC
Created attachment 99927 [details] [review]
Bug 11999: Add maxreserves check in CanBookBeReserved and CanItemBeReserved

- Check if borrower has reached the maximum number of holds allowed
  (syspref maxreserves)

The goal of this patch is to do these checks also when using ILS-DI
services HoldTitle and HoldItem

Test plan:
----------
Before patch:
1/ Set syspref maxreserves to 1
2/ Place some holds through ILS-DI and note that you can place more than
   1 hold for a borrower

Apply the patch
3/ Place some holds through ILS-DI, you shouldn't be able to place more
   than 1 hold for a borrower
4/ Try to place holds on staff interface and OPAC. The behaviour must be
   identical than before the patch.

Maxreserves and alreadyreserved works on ILSDI
Maxreserves works also on staff and opac
Already reserves works also on staff and opac
tests on t/db_dependent/Reserves.t and t/db_dependent/Holds.t passe (using koha_ut db)

Signed-off-by: Alex Sassmannshausen <alex.sassmannshausen@ptfs-europe.com>
Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 97 Arthur Suzuki 2020-03-02 15:17:54 UTC
Created attachment 99928 [details] [review]
Bug 11999: Test improvements

improve t/db_dependent/Reserves.t to make tests pass on UNIMARC installation
Comment 98 Arthur Suzuki 2020-03-02 15:27:35 UTC
(In reply to David Nind from comment #94)
> Similar to Bug 17229 you get an error on the patron's page when following
> the test plan (step 2 before patch), but before trying to apply the patch:
> 
> 1) For the patron, the holds tab heading shows that there is a hold (1).
> 
> 2) When you click on the patron's holds tab a message pops up with
> "DataTables warning: table id=holds-table - Ajax error. For more information
> about this error, please see http://datatables.net/tn/7"
> 
> 3) The bibliographic record shows that there is a hold.
> 
> I'm assuming that is because the DataTables feature for displaying results
> is a more recent addition to Koha.
> 
> (Test URL I used for ILS_DI was
> http://127.0.0.1:8080/cgi-bin/koha/ilsdi.
> pl?service=HoldTitle&patron_id=19&bib_id=15&request_location='127.0.0.1'
> using koha-testing-docker)

Hi David,
Thanks again for testing, I've posted new patches with conflicts fixed + tests green + qa tools green.
I cannot reproduce the issue you've seen with DataTable.
Kr,
Arthur
Comment 99 David Nind 2020-03-03 18:21:16 UTC
Hi Arthur. Have tested again, even though it is already signed off now. Everything works fine, except for the DataTables/Ajax warning. Bug 24769 created for that.
Comment 100 Marcel de Rooy 2020-03-06 10:17:32 UTC
QAing
Comment 101 Marcel de Rooy 2020-03-06 10:51:12 UTC
Sorry, but I cannot pass QA on this patch set:

You add changes to Reserves.pm but you only change a test script for updating or inserting age restriction in marc subfields. We need solid test cases for the changes you make.

But looking further, do we really want this change here? We already check for maxreserves in opac-reserve.pl and reserve/request.pl. So if you would need it for ILS-DI, I would expect you add it there? If we want the check in the module, we should also consider the scripts mentioned. And what about performance? If you come from CanBook, why would you test it again and again in CanItem? This check is only needed for the first item?

+         { status => alreadyReserved }, if the borrower has already reserved this item.
Where do you find that one ?

Failed QA
Comment 102 Katrin Fischer 2020-03-09 11:33:52 UTC
I agree about the tests, but with all the API work - Doesn't it makes sense to centralize all the checks needed instead of repeating (and forgetting them) all over the place? The general idea makes sense to me.
Comment 103 Arthur Suzuki 2020-03-10 13:17:24 UTC
(In reply to Katrin Fischer from comment #102)
> I agree about the tests, but with all the API work - Doesn't it makes sense
> to centralize all the checks needed instead of repeating (and forgetting
> them) all over the place? The general idea makes sense to me.

Thanks for the +1 Katrin, that was definitely the idea behind this patch.
While working with library frontend software we (@BibLibre) have stumbled across several checks and behaviors which are different from OPAC to ILS-DI.

Centralizing those checks and behaviors makes a lot of sense to me as well with also the Rest API coming if we want to have a bit of coherence across the different entry points of Koha.

Also makes the whole thing much easier to maintain I guess.

Still I do have a bit of work ahead for the tests.
I will try to solve this before the NoHackFest / CanceledHackFest
Comment 104 Arthur Suzuki 2020-07-03 00:34:29 UTC
Created attachment 106481 [details] [review]
Bug 11999: Test improvements + check maxreserves

improve t/db_dependent/Reserves.t to make tests pass on UNIMARC installation
Comment 105 Arthur Suzuki 2020-07-03 00:49:36 UTC
Daag Marcel,
from your comment :
"And what about performance? If you come from CanBook, why would you test it again and again in CanItem? This check is only needed for the first item?"

No performance issues involved.
There is a test in CanBook before the item loop.
Then if test is false it won't enter the loop.
The check in CanItem is there to have the syspref checked even when using ItemLevelHold (like ILSDI-HoldItem for example).

Mvg,
Arthur
Comment 106 Marcel de Rooy 2020-07-03 06:27:45 UTC
(In reply to Arthur Suzuki from comment #105)
> Daag Marcel,
Goedemorgen Arthur :)


> There is a test in CanBook before the item loop.
> Then if test is false it won't enter the loop.
> The check in CanItem is there to have the syspref checked even when using
> ItemLevelHold (like ILSDI-HoldItem for example).

I do not see the change? You test in CanBook and if it's green, you will test it again for every item?
Maybe you need to tell CanItem to test once?
Comment 107 Arthur Suzuki 2020-07-03 10:02:14 UTC
Created attachment 106494 [details] [review]
Bug 11999: QA follow-up
Comment 108 Arthur Suzuki 2020-07-03 10:10:30 UTC
> You test in CanBook and if it's green, you will
> test it again for every item?
> Maybe you need to tell CanItem to test once?

I told CanItem to test only if caller is different than CanBook, should solve.
I think $patron->holds do not actually execute SQL,
only if queried with $holds->count right?
then if first condition is false, it is not queried

eet smakelijk een goeie namiddag :)
Comment 109 David Nind 2020-09-17 19:25:21 UTC
Hi Arthur, I thought I would test again but the patch no longer applies 8-( :

root@kohadevbox:koha(bz11999)$ git bz apply 11999
Bug 11999 - Add two checks in CanBookBeReserved and CanItemBeReserved

99927 - Bug 11999: Add maxreserves check in CanBookBeReserved and CanItemBeReserved
106481 - Bug 11999: Test improvements + check maxreserves
106494 - Bug 11999: QA follow-up

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 11999: Add maxreserves check in CanBookBeReserved and CanItemBeReserved
Using index info to reconstruct a base tree...
M	C4/Reserves.pm
M	reserve/request.pl
Falling back to patching base and 3-way merge...
Auto-merging reserve/request.pl
Auto-merging C4/Reserves.pm
CONFLICT (content): Merge conflict in C4/Reserves.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 11999: Add maxreserves check in CanBookBeReserved and CanItemBeReserved
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-11999-Add-maxreserves-check-in-CanBookBeReserv-WtX_N_.patch
Comment 110 Arthur Suzuki 2020-10-15 09:38:05 UTC
Created attachment 111687 [details] [review]
Bug 11999: Add maxreserves check in CanBookBeReserved and CanItemBeReserved

- Check if borrower has reached the maximum number of holds allowed
  (syspref maxreserves)

The goal of this patch is to do these checks also when using ILS-DI
services HoldTitle and HoldItem

Test plan:
----------
Before patch:
1/ Set syspref maxreserves to 1
2/ Place some holds through ILS-DI and note that you can place more than
   1 hold for a borrower

Apply the patch
3/ Place some holds through ILS-DI, you shouldn't be able to place more
   than 1 hold for a borrower
4/ Try to place holds on staff interface and OPAC. The behaviour must be
   identical than before the patch.

Maxreserves and alreadyreserved works on ILSDI
Maxreserves works also on staff and opac
Already reserves works also on staff and opac
tests on t/db_dependent/Reserves.t and t/db_dependent/Holds.t passe (using koha_ut db)

Signed-off-by: Alex Sassmannshausen <alex.sassmannshausen@ptfs-europe.com>
Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 111 Arthur Suzuki 2020-10-15 09:38:24 UTC
Created attachment 111688 [details] [review]
Bug 11999: Test improvements + check maxreserves

improve t/db_dependent/Reserves.t to make tests pass on UNIMARC installation
Comment 112 Arthur Suzuki 2020-10-15 09:38:30 UTC
Created attachment 111689 [details] [review]
Bug 11999: (QA follow-up)
Comment 113 Arthur Suzuki 2020-10-15 09:41:18 UTC
Attached new patches rebased on current master, just in time for KohaCon ;)
Comment 114 David Nind 2020-10-30 10:31:27 UTC
Everything seems to work as expected.

However, prove t/db_dependent/Holds.t  fails:

t/db_dependent/Holds.t (Wstat: 3072 Tests: 67 Failed: 12)
  Failed tests:  37-39, 42, 47, 53, 55-58, 62, 64
  Non-zero exit status: 12
Files=1, Tests=67,  7 wallclock secs ( 0.04 usr  0.01 sys +  5.38 cusr  0.41 csys =  5.84 CPU)
Result: FAIL

Let me know if you want a verbose log.
Comment 115 Arthur Suzuki 2020-11-01 14:16:52 UTC
Huuum...
Hi David,
Sorry, can't reproduce, this is what I got.
Can you post verbose log? (in the meantime i'll try to clear those warnings)

root@kohadevbox:koha(bz11999)$ prove t/db_dependent/Holds.t 
t/db_dependent/Holds.t .. 1/67 Use of uninitialized value $caller in index at /kohadevbox/koha/C4/Reserves.pm line 429.
Use of uninitialized value $caller in index at /kohadevbox/koha/C4/Reserves.pm line 429.
Use of uninitialized value $caller in index at /kohadevbox/koha/C4/Reserves.pm line 429.
Use of uninitialized value $caller in index at /kohadevbox/koha/C4/Reserves.pm line 429.
Use of uninitialized value $caller in index at /kohadevbox/koha/C4/Reserves.pm line 429.
Use of uninitialized value $caller in index at /kohadevbox/koha/C4/Reserves.pm line 429.
Use of uninitialized value $caller in index at /kohadevbox/koha/C4/Reserves.pm line 429.
Use of uninitialized value $caller in index at /kohadevbox/koha/C4/Reserves.pm line 429.
Use of uninitialized value $caller in index at /kohadevbox/koha/C4/Reserves.pm line 429.
Use of uninitialized value $caller in index at /kohadevbox/koha/C4/Reserves.pm line 429.
Use of uninitialized value $caller in index at /kohadevbox/koha/C4/Reserves.pm line 429.
Use of uninitialized value $caller in index at /kohadevbox/koha/C4/Reserves.pm line 429.
Use of uninitialized value $caller in index at /kohadevbox/koha/C4/Reserves.pm line 429.
Use of uninitialized value $caller in index at /kohadevbox/koha/C4/Reserves.pm line 429.
Use of uninitialized value $caller in index at /kohadevbox/koha/C4/Reserves.pm line 429.
t/db_dependent/Holds.t .. ok     
All tests successful.
Files=1, Tests=67,  5 wallclock secs ( 0.03 usr  0.01 sys +  3.68 cusr  0.38 csys =  4.10 CPU)
Result: PASS
Comment 116 David Nind 2020-11-02 18:31:33 UTC
Patches no longer apply 8-(
Comment 117 Arthur Suzuki 2020-11-02 21:06:41 UTC
Created attachment 112871 [details] [review]
Bug 11999: Add maxreserves check in CanBookBeReserved and CanItemBeReserved

- Check if borrower has reached the maximum number of holds allowed
  (syspref maxreserves)

The goal of this patch is to do these checks also when using ILS-DI
services HoldTitle and HoldItem

Test plan:
----------
Before patch:
1/ Set syspref maxreserves to 1
2/ Place some holds through ILS-DI and note that you can place more than
   1 hold for a borrower

Apply the patch
3/ Place some holds through ILS-DI, you shouldn't be able to place more
   than 1 hold for a borrower
4/ Try to place holds on staff interface and OPAC. The behaviour must be
   identical than before the patch.

Maxreserves and alreadyreserved works on ILSDI
Maxreserves works also on staff and opac
Already reserves works also on staff and opac
tests on t/db_dependent/Reserves.t and t/db_dependent/Holds.t passe (using koha_ut db)

Signed-off-by: Alex Sassmannshausen <alex.sassmannshausen@ptfs-europe.com>
Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 118 Arthur Suzuki 2020-11-02 21:07:02 UTC
Created attachment 112872 [details] [review]
Bug 11999: Test improvements + check maxreserves

improve t/db_dependent/Reserves.t to make tests pass on UNIMARC installation
Comment 119 Arthur Suzuki 2020-11-02 21:07:08 UTC
Created attachment 112873 [details] [review]
Bug 11999: (QA follow-up)
Comment 120 Arthur Suzuki 2020-11-02 21:16:38 UTC
Thanks david :)
Patch did no longer apply because one of my other patch actually got into master, hurray! \o/
Re-posted all patches.
Comment 121 Arthur Suzuki 2020-11-02 21:24:45 UTC
Created attachment 112874 [details] [review]
Bug 11999: (QA follow-up)
Comment 122 Joonas Kylmälä 2020-11-03 07:52:25 UTC
The attached patch contains git merge conflict resolution hints. Also, if I understand correctly maxreserves is already checked in some other places in some other way, it would be really good idea to have a patch then to convert those calls to just use CanBookBeReserved().
Comment 123 Joonas Kylmälä 2020-11-03 08:08:09 UTC
I didn't realize the follow-ups here removed those git merge conflict resolution hints. It would still probably be better if the conflicts were solved in the patches they happen. If I review the patch "Test improvements + check maxreserves" it contains merge resolution and test improvements, so two things, git commits should do just one thing. Also "Bug 11999: (QA follow-up)" doesn't say what it does.
Comment 124 Fridolin Somers 2021-05-10 12:53:25 UTC
Created attachment 120776 [details] [review]
Bug 11999: Add maxreserves check in CanBookBeReserved and CanItemBeReserved

- Check if borrower has reached the maximum number of holds allowed
  (syspref maxreserves)

The goal of this patch is to do these checks also when using ILS-DI
services HoldTitle and HoldItem

Test plan:
----------
Before patch:
1/ Set syspref maxreserves to 1
2/ Place some holds through ILS-DI and note that you can place more than
   1 hold for a borrower

Apply the patch
3/ Place some holds through ILS-DI, you shouldn't be able to place more
   than 1 hold for a borrower
4/ Try to place holds on staff interface and OPAC. The behaviour must be
   identical than before the patch.

Maxreserves and alreadyreserved works on ILSDI
Maxreserves works also on staff and opac
Already reserves works also on staff and opac
tests on t/db_dependent/Reserves.t and t/db_dependent/Holds.t passe (using koha_ut db)

Signed-off-by: Alex Sassmannshausen <alex.sassmannshausen@ptfs-europe.com>
Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 125 Fridolin Somers 2021-05-10 12:53:48 UTC
Created attachment 120777 [details] [review]
Bug 11999: Test improvements + check maxreserves

improve t/db_dependent/Reserves.t to make tests pass on UNIMARC installation
Comment 126 Fridolin Somers 2021-05-10 12:54:13 UTC
Created attachment 120778 [details] [review]
Bug 11999: Test improvements + check maxreserves

improve t/db_dependent/Reserves.t to make tests pass on UNIMARC installation
Comment 127 Fridolin Somers 2021-05-10 12:55:25 UTC
Created attachment 120779 [details] [review]
Bug 11999: Add maxreserves check in CanBookBeReserved and CanItemBeReserved

- Check if borrower has reached the maximum number of holds allowed
  (syspref maxreserves)

The goal of this patch is to do these checks also when using ILS-DI
services HoldTitle and HoldItem

Test plan:
----------
Before patch:
1/ Set syspref maxreserves to 1
2/ Place some holds through ILS-DI and note that you can place more than
   1 hold for a borrower

Apply the patch
3/ Place some holds through ILS-DI, you shouldn't be able to place more
   than 1 hold for a borrower
4/ Try to place holds on staff interface and OPAC. The behaviour must be
   identical than before the patch.

Maxreserves and alreadyreserved works on ILSDI
Maxreserves works also on staff and opac
Already reserves works also on staff and opac
tests on t/db_dependent/Reserves.t and t/db_dependent/Holds.t passe (using koha_ut db)

Signed-off-by: Alex Sassmannshausen <alex.sassmannshausen@ptfs-europe.com>
Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 128 Fridolin Somers 2021-05-10 12:55:44 UTC
Created attachment 120780 [details] [review]
Bug 11999: Test improvements + check maxreserves

improve t/db_dependent/Reserves.t to make tests pass on UNIMARC installation
Comment 129 Fridolin Somers 2021-05-10 12:58:34 UTC
(In reply to Joonas Kylmälä from comment #123)
> I didn't realize the follow-ups here removed those git merge conflict
> resolution hints. It would still probably be better if the conflicts were
> solved in the patches they happen. If I review the patch "Test improvements
> + check maxreserves" it contains merge resolution and test improvements, so
> two things, git commits should do just one thing. Also "Bug 11999: (QA
> follow-up)" doesn't say what it does.

I've squashed fixes from QA follow-up inside patches.
And rebase master.

I set back to Needs Signoff
Comment 130 David Nind 2021-05-13 11:54:09 UTC
Test failed - if I change the number of tests to 68, everything works fine:

t/db_dependent/Reserves.t (Wstat: 65280 Tests: 68 Failed: 1)
  Failed test:  68
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 67 tests but ran 68.

Also, while not covered in the test plan, prove t/db_dependent/Holds.t fails (it passed before the patch was applied):

t/db_dependent/Holds.t (Wstat: 4352 Tests: 71 Failed: 17)
  Failed tests:  37-39, 42, 47, 53-60, 63, 65, 67, 71
  Non-zero exit status: 17
Comment 131 Arthur Suzuki 2021-06-01 12:18:49 UTC
Created attachment 121531 [details] [review]
Bug 11999: Test improvements + check maxreserves

improve t/db_dependent/Reserves.t to make tests pass on UNIMARC installation
Comment 132 Arthur Suzuki 2021-06-01 12:20:07 UTC
Hi David,
I've not reproduced the issue with t/db_dependent/Holds.t
number of tests in Reserves.t has been updated.
Kr,
Comment 133 David Nind 2021-06-03 22:42:35 UTC
Created attachment 121612 [details]
Test fails for holds - 2021-06-04

I've attached the test results for prove -v t/db_dependent/Holds.t after the patch is applied - it passed before the patch was applied.

I'm testing on a MARC21 installation, does this need to be a UNIMARC setup?
Comment 134 David Cook 2021-06-04 00:33:02 UTC
Checking hold limits has been driving me crazy lately.

It seems to me that these functions should check all the different hold limits and report back specifically why the book or item can't be reserved. For example, exceeded max reserves, exceeded total holds for the day, exceeded total holds for record, etc.
Comment 135 Fridolin Somers 2022-03-11 23:20:05 UTC
Needs rebase
Comment 136 Arthur Suzuki 2022-03-28 08:39:34 UTC
(In reply to David Cook from comment #134)
> Checking hold limits has been driving me crazy lately.
> 
> It seems to me that these functions should check all the different hold
> limits and report back specifically why the book or item can't be reserved.
> For example, exceeded max reserves, exceeded total holds for the day,
> exceeded total holds for record, etc.

This seems to be BZ 17712.
I've been trying to rebase that one with not much success so far.
Git branch available here : https://git.biblibre.com/biblibre/kohac/src/branch/bz17712
Lots of tests are failing and needs rework.
Comment 137 Fridolin Somers 2023-06-06 19:24:37 UTC
FYI here is our patch for 22.11

https://git.biblibre.com/biblibre/kohac/commit/2fd63a2a5916e3e19ba9b0aad90caca0225a1212