Bug 14155 - Placing holds via ILSDI bypass some checks
Summary: Placing holds via ILSDI bypass some checks
Status: RESOLVED DUPLICATE of bug 11999
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Alex Arnaud
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-05-06 12:43 UTC by Alex Arnaud
Modified: 2016-01-05 15:56 UTC (History)
8 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
Add maxreserves and alreadyreserved checks when placing hold via ILSDI (9.38 KB, patch)
2015-05-06 13:03 UTC, Alex Arnaud
Details | Diff | Splinter Review
[Follow-up] Bug 14155 - Add tests in t/db_dependent/Reserves.t (3.73 KB, patch)
2015-06-16 09:50 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 14155 - Add tests in t/db_dependent/Reserves.t (3.79 KB, patch)
2015-06-16 10:48 UTC, Indranil Das Gupta
Details | Diff | Splinter Review
Add maxreserves and alreadyreserved checks when placing hold via ILSDI (9.35 KB, patch)
2015-06-16 10:52 UTC, Indranil Das Gupta
Details | Diff | Splinter Review
Bug 14155 - Add tests in t/db_dependent/Reserves.t (3.79 KB, patch)
2015-06-16 10:54 UTC, Indranil Das Gupta
Details | Diff | Splinter Review
(fix typo) AddBiblio call in Reserves.t (1.07 KB, patch)
2015-06-17 04:34 UTC, Indranil Das Gupta
Details | Diff | Splinter Review
(qa followup) variable declaration and cleanup (3.66 KB, patch)
2015-06-17 05:03 UTC, Indranil Das Gupta
Details | Diff | Splinter Review
(qa followup) variable declaration and cleanup (3.73 KB, patch)
2015-06-17 13:17 UTC, Alex Arnaud
Details | Diff | Splinter Review
(fix typo) AddBiblio call in Reserves.t (1.14 KB, patch)
2015-06-17 13:18 UTC, Alex Arnaud
Details | Diff | Splinter Review
(qa followup) variable declaration and cleanup (3.73 KB, patch)
2015-06-17 13:18 UTC, Alex Arnaud
Details | Diff | Splinter Review
Add maxreserves and alreadyreserved checks when placing hold via ILSDI (9.33 KB, patch)
2015-10-27 09:49 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 14155 - Add tests in t/db_dependent/Reserves.t (3.79 KB, patch)
2015-10-27 09:50 UTC, Alex Arnaud
Details | Diff | Splinter Review
(fix typo) AddBiblio call in Reserves.t (1.14 KB, patch)
2015-10-27 09:50 UTC, Alex Arnaud
Details | Diff | Splinter Review
(qa followup) variable declaration and cleanup (3.73 KB, patch)
2015-10-27 09:50 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 14155 - Update tests and fix failures (4.96 KB, patch)
2015-10-27 13:45 UTC, Alex Arnaud
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Arnaud 2015-05-06 12:43:59 UTC
If you make a hold request using ILSDI webservices (HoldItem or HoldTitle) 2 important checks are skipped while they are done using staff user interface.

1) maxreserves system preference is ignored so you can place as many hold as issuingrules allow you even if maxreserves is set.

2) nothing checks if the borrower already reserved the biblio.

Patch is coming.
Comment 1 Alex Arnaud 2015-05-06 13:03:04 UTC Comment hidden (obsolete)
Comment 2 Indranil Das Gupta 2015-06-14 13:41:47 UTC
Comment on attachment 38884 [details] [review]
Add maxreserves and alreadyreserved checks when placing hold via ILSDI

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

Alex,

I think the following :

::: C4/Reserves.pm
@@ +1117,5 @@
> +        return 1 if $borrowernumber eq $res->{borrowernumber};
> +    }
> +
> +    return 0;
> +}

needs added test in t/db_dependent/Reserves.t

@@ +1139,5 @@
> +        return 1;
> +    }
> +
> +    return 0;
> +}

needs added test in t/db_dependent/Reserves.t
Comment 3 Indranil Das Gupta 2015-06-14 13:46:42 UTC
(In reply to Indranil Das Gupta from comment #2)
> Comment on attachment 38884 [details] [review] [review]
> Add maxreserves and alreadyreserved checks when placing hold via ILSDI
> 
> Review of attachment 38884 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> Alex,
> 
> I think the following :
> 
> ::: C4/Reserves.pm

C4::Reserves::AlreadyReserved()
C4::Reserves::ReachedMaxReserves()

needs added test in t/db_dependent/Reserves.t
Comment 4 Alex Arnaud 2015-06-16 09:50:55 UTC Comment hidden (obsolete)
Comment 5 Alex Arnaud 2015-06-16 09:53:31 UTC
(In reply to Indranil Das Gupta from comment #3)
> (In reply to Indranil Das Gupta from comment #2)
> > Comment on attachment 38884 [details] [review] [review] [review]
> > Add maxreserves and alreadyreserved checks when placing hold via ILSDI
> > 
> > Review of attachment 38884 [details] [review] [review] [review]:
> > -----------------------------------------------------------------
> > 
> > Alex,
> > 
> > I think the following :
> > 
> > ::: C4/Reserves.pm
> 
> C4::Reserves::AlreadyReserved()
> C4::Reserves::ReachedMaxReserves()
> 
> needs added test in t/db_dependent/Reserves.t
Hello,

Thank for having had a look at this bug. Just added tests for ttthis 2 sub?
Comment 6 Indranil Das Gupta 2015-06-16 10:48:20 UTC Comment hidden (obsolete)
Comment 7 Indranil Das Gupta 2015-06-16 10:52:38 UTC Comment hidden (obsolete)
Comment 8 Indranil Das Gupta 2015-06-16 10:54:09 UTC Comment hidden (obsolete)
Comment 9 Indranil Das Gupta 2015-06-17 04:34:43 UTC Comment hidden (obsolete)
Comment 10 Indranil Das Gupta 2015-06-17 05:03:26 UTC Comment hidden (obsolete)
Comment 11 Indranil Das Gupta 2015-06-17 05:05:09 UTC
Setting this back to "Needs Signoff"
Comment 12 Alex Arnaud 2015-06-17 13:17:18 UTC Comment hidden (obsolete)
Comment 13 Alex Arnaud 2015-06-17 13:18:11 UTC Comment hidden (obsolete)
Comment 14 Alex Arnaud 2015-06-17 13:18:18 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2015-08-21 10:35:29 UTC
Be care, AddReserve prototype changed by bug 14526 (contrainst param removed).
Comment 16 Jonathan Druart 2015-08-21 10:40:34 UTC
I meant bug 9809.
Comment 17 Katrin Fischer 2015-09-07 21:33:33 UTC
I have some problems with the conflict in the test files - can you please take a look? If I just fix the number of tests, I get some output like this:

bumblebee:~/kohaclone (14155-ilsdi|AM 1/1) $ perl t/db_dependent/Reserves.t 
1..71
ok 1 - use C4::Reserves;
"my" variable $title masks earlier declaration in same scope at t/db_dependent/Reserves.t line 85.
"my" variable $title masks earlier declaration in same scope at t/db_dependent/Reserves.t line 98.
"my" variable $bibitemnum masks earlier declaration in same scope at t/db_dependent/Reserves.t line 98.
...
Comment 18 Alex Arnaud 2015-10-27 09:49:56 UTC
Created attachment 44047 [details] [review]
Add maxreserves and alreadyreserved checks when placing hold via ILSDI

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

Signed-off-by: Indranil Das Gupta (L2C2 Technologies) <indradg@gmail.com>
Comment 19 Alex Arnaud 2015-10-27 09:50:04 UTC
Created attachment 44048 [details] [review]
Bug 14155 - Add tests in t/db_dependent/Reserves.t

- add tests for C4::Reserves::AlreadyReserved(),
- add tests for C4::Reserves::ReachedMaxReserves(),

Signed-off-by: Indranil Das Gupta (L2C2 Technologies) <indradg@gmail.com>
Comment 20 Alex Arnaud 2015-10-27 09:50:17 UTC
Created attachment 44049 [details] [review]
(fix typo) AddBiblio call in Reserves.t

Fix for the typo, it read :

my ($broken_id, $title, $bibitemnum) = AddBiblio($miniaturist, '');

it should read :

my ($broken_id, $title, $bibitemnum) = AddBiblio($broken, '');

Signed-off-by: Alex Arnaud (Biblibre) <alex.arnaud@biblibre.com>
Comment 21 Alex Arnaud 2015-10-27 09:50:22 UTC
Created attachment 44050 [details] [review]
(qa followup) variable declaration and cleanup

1/ running koha qa tools showed up valid FAIL. This patch fix
   that error.

   FAIL	t/db_dependent/Reserves.t
     OK	   critic
     OK	   forbidden patterns
     OK	   pod
     FAIL  valid
	        "my" variable $title masks earlier declaration in same scope
		"my" variable $bibitemnum masks earlier declaration in same scope

2/ the following redundant line was removed as it had no use:

   t::lib::Mocks::mock_preference('maxreserves', 15);

Signed-off-by: Alex Arnaud (Biblibre) <alex.arnaud@biblibre.com>
Comment 22 Alex Arnaud 2015-10-27 13:45:20 UTC
Created attachment 44064 [details] [review]
Bug 14155 - Update tests and fix failures
Comment 23 Liz Rea 2015-12-23 19:59:24 UTC
Hi,

I'm happy to have a guess at how to test this, please in the future may we have a test plan? It makes things go a lot faster. :)

Thanks very much,
Liz
Comment 24 Alex Arnaud 2016-01-05 15:56:02 UTC

*** This bug has been marked as a duplicate of bug 11999 ***