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.
Created attachment 38884 [details] [review] Add maxreserves and alreadyreserved checks when placing hold via ILSDI
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
(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
Created attachment 40170 [details] [review] [Follow-up] Bug 14155 - Add tests in t/db_dependent/Reserves.t - add tests for C4::Reserves::AlreadyReserved(), - add tests for C4::Reserves::ReachedMaxReserves(),
(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?
Created attachment 40174 [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>
Created attachment 40175 [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>
Created attachment 40176 [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>
Created attachment 40208 [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, '');
Created attachment 40210 [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);
Setting this back to "Needs Signoff"
Created attachment 40239 [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>
Created attachment 40240 [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>
Created attachment 40241 [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>
Be care, AddReserve prototype changed by bug 14526 (contrainst param removed).
I meant bug 9809.
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. ...
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>
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>
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>
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>
Created attachment 44064 [details] [review] Bug 14155 - Update tests and fix failures
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
*** This bug has been marked as a duplicate of bug 11999 ***