This table is actually not used in 3.X. There is still some code referencing, joining this table. Will put it on my list herewith to clean it up. But if you get there before I would, please assign it to yourself :)
Include the code around reqbib in reserve/placerequest.pl ?
Created attachment 37638 [details] [review] Bug 9809: Get rid of reserveconstraints First, to be honest, I am not completely sure this patch won't introduce regression. We need several pairs of eyes, and if possible from someone in place for a long time in the project :) Once again this looks vestige of Koha 2, and comments like "FIXME", "This does not make sense", "probably never reached" appears all around this code. Confirmed, reserveconstraints was in commit d0374d003716dfb40796caad6390a4d69bfb0376 Author: rangi <rangi> Date: Tue Dec 19 23:45:51 2000 +0000 Initial revision This patch is not complete, the reserves.constrainttype should be removed too (to confirm?), and all call to *Reserves using it be updated. What I understood: This feature has never been finished, and has never worked. Developers try not touch the code not to break something. The only place something could be inserted in this table is in C4::Reserves::AddReserve, if the parameter "constraint" starts with an "o" or "e" (hum...). The only place (I found) where it is possible is in the quite muddled serials/routing-preview.pl script (if RoutingListAddReserves is set). This parameter is set to 'o' since the beginning of the history and it's not clear why. I suspect a simple wrong c/p. There are 2 places where something could be retrieved from this table: - C4::Reserves::GetReservesFromBiblionumber. I did not find any calls using something else than the number of reserves or data from the reserves table. Looking at the number of FIXME in this area, not sure it has already worked. - _Findgroupreserve: I let you have a look at the sql query but to me this cannot return anything if the reserveconstraints table is empty. Test plan: 0/ Don't apply this patch 1/ Try to add something in the reserveconstraints table ( :D )
Created attachment 40733 [details] [review] Bug 9809 - DB Updates
Created attachment 40734 [details] [review] Bug 9809 - Update Schema files
Created attachment 40735 [details] [review] Bug 9809 - Remove DBIC module for reserveconstraints
Created attachment 40736 [details] [review] Bug 9809 - Update unit tests
Created attachment 40737 [details] [review] Bug 9809 - Remove reserveconstraints references from C4::Reserves
Created attachment 40738 [details] [review] Bug 9809 - Update AddReserve prototype to remove constraint parameter Test Plan: 1) Apply this patch set 2) prove t/db_dependent/Circulation.t 3) prove t/db_dependent/Holds.t 4) prove t/db_dependent/Holds/LocalHoldsPriority.t 5) prove t/db_dependent/Holds/RevertWaitingStatus.t 6) prove t/db_dependent/HoldsQueue.t 7) prove t/db_dependent/Reserves.t
Kyle, Thanks for these patches, it's a much more complete solution than the one I have proposed. I have a doubt with the changes in GetReserveFee: Before the patches: if ( $const eq "a" ) { push @biblioitems, $data1; } else { some code; } now you always execute some code. But before the patch, $const was sometime equal to "a". I am not sure about the consequences of this change.
(In reply to Jonathan Druart from comment #9) > Kyle, > Thanks for these patches, it's a much more complete solution than the one I > have proposed. > > I have a doubt with the changes in GetReserveFee: > Before the patches: > if ( $const eq "a" ) { > push @biblioitems, $data1; > } > else { > some code; > } > > now you always execute some code. > > But before the patch, $const was sometime equal to "a". > I am not sure about the consequences of this change. Can you add a splinter review? I'm not sure exactly where the code you are referencing is. You can also add a followup to fix it if that's easier. Thanks!
Comment on attachment 40738 [details] [review] Bug 9809 - Update AddReserve prototype to remove constraint parameter Review of attachment 40738 [details] [review]: ----------------------------------------------------------------- ::: C4/Reserves.pm @@ -694,5 @@ > $sth1->execute($biblionumber); > while ( my $data1 = $sth1->fetchrow_hashref ) { > - if ( $const eq "a" ) { > - push @biblioitems, $data1; > - } Could it be this here?
Yes it is!
Testing this one now..
Created attachment 41754 [details] [review] Bug 9809 - DB Updates Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 41755 [details] [review] Bug 9809 - Update Schema files Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 41756 [details] [review] Bug 9809 - Remove DBIC module for reserveconstraints Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 41757 [details] [review] Bug 9809 - Update unit tests Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 41758 [details] [review] Bug 9809 - Remove reserveconstraints references from C4::Reserves Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 41759 [details] [review] Bug 9809 - Update AddReserve prototype to remove constraint parameter Test Plan: 1) Apply this patch set 2) prove t/db_dependent/Circulation.t 3) prove t/db_dependent/Holds.t 4) prove t/db_dependent/Holds/LocalHoldsPriority.t 5) prove t/db_dependent/Holds/RevertWaitingStatus.t 6) prove t/db_dependent/HoldsQueue.t 7) prove t/db_dependent/Reserves.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 41760 [details] [review] Bug 9809: [QA Follow-up] Remove warnings from Hold.pm Resolves two warnings on a uninitialized found column: holds: Use of uninitialized value in string eq at /home/koha/testclone/Koha/Hold.pm line 53., referer: http://test.rijkskoha.nl:28080/cgi-bin/koha/circ/circulation.pl?borrowernumber=152 holds: Use of uninitialized value in string eq at /home/koha/testclone/Koha/Hold.pm line 74., referer: http://test.rijkskoha.nl:28080/cgi-bin/koha/circ/circulation.pl?borrowernumber=152 Test plan: Run t/db_dependent/Hold.t. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 41761 [details] [review] Bug 9809: [QA Follow-up] Remove an erroneous call to GetReserveFee The call to GetReserveFee in opac-reserve.pl is useless in its current form. The first parameter undef takes care of receiving 0. But note that the user is warned correctly for the charge via param variable RESERVE_CHARGE on the opac form. When the hold is placed, AddReserve calls GetReserveFee. So if the routine would work correctly, we would not need this extra call in opac-reserve in the whole place. Unfortunately, the routine is not working correctly. I will submit a fix for GetReserveFee under a new report (14702). Test plan: [1] Add a hold fee to some category. [2] Check the warn for placing a hold on such a book in OPAC. [3] Observe that the actual fee is not charged. This is a current bug and it will be addressed on report 14702. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Looks good to me! Thanks, Kyle. I added two small follow-ups and a see-also to a new report for GetReserveFee.
(In reply to Marcel de Rooy from comment #21) > [2] Check the warn for placing a hold on such a book in OPAC. Actually, this should read: a hold for such a patron ! Maybe we should also think of moving this fee to item types?
Created attachment 41767 [details] [review] [SIGNED-OFF] Bug 9809 - DB Updates Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 41768 [details] [review] [SIGNED-OFF] Bug 9809 - Update Schema files Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 41769 [details] [review] [SIGNED-OFF] Bug 9809 - Remove DBIC module for reserveconstraints Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 41770 [details] [review] [SIGNED-OFF] Bug 9809 - Update unit tests Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 41771 [details] [review] [SIGNED-OFF] Bug 9809 - Remove reserveconstraints references from C4::Reserves Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 41772 [details] [review] [SIGNED-OFF] Bug 9809 - Update AddReserve prototype to remove constraint parameter Test Plan: 1) Apply this patch set 2) prove t/db_dependent/Circulation.t 3) prove t/db_dependent/Holds.t 4) prove t/db_dependent/Holds/LocalHoldsPriority.t 5) prove t/db_dependent/Holds/RevertWaitingStatus.t 6) prove t/db_dependent/HoldsQueue.t 7) prove t/db_dependent/Reserves.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 41773 [details] [review] [SIGNED-OFF] Bug 9809: [QA Follow-up] Remove warnings from Hold.pm Resolves two warnings on a uninitialized found column: holds: Use of uninitialized value in string eq at /home/koha/testclone/Koha/Hold.pm line 53., referer: http://test.rijkskoha.nl:28080/cgi-bin/koha/circ/circulation.pl?borrowernumber=152 holds: Use of uninitialized value in string eq at /home/koha/testclone/Koha/Hold.pm line 74., referer: http://test.rijkskoha.nl:28080/cgi-bin/koha/circ/circulation.pl?borrowernumber=152 Test plan: Run t/db_dependent/Hold.t. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 41774 [details] [review] [SIGNED-OFF] Bug 9809: [QA Follow-up] Remove an erroneous call to GetReserveFee The call to GetReserveFee in opac-reserve.pl is useless in its current form. The first parameter undef takes care of receiving 0. But note that the user is warned correctly for the charge via param variable RESERVE_CHARGE on the opac form. When the hold is placed, AddReserve calls GetReserveFee. So if the routine would work correctly, we would not need this extra call in opac-reserve in the whole place. Unfortunately, the routine is not working correctly. I will submit a fix for GetReserveFee under a new report (14702). Test plan: [1] Add a hold fee to some category. [2] Check the warn for placing a hold on such a book in OPAC. [3] Observe that the actual fee is not charged. This is a current bug and it will be addressed on report 14702. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
(In reply to Marcel de Rooy from comment #22) > Looks good to me! Thanks, Kyle. > I added two small follow-ups and a see-also to a new report for > GetReserveFee. Thanks for the followups Marcel!
Bug 14464 is passed QA and will be in conflict with this patch. Please provide a followup build on top of it.
(In reply to Jonathan Druart from comment #33) > Bug 14464 is passed QA and will be in conflict with this patch. Please > provide a followup build on top of it. Well. maybe we should wait to see it get pushed first.
Created attachment 41809 [details] [review] Bug 9809 - DB Updates Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 41810 [details] [review] Bug 9809 - Update Schema files Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 41811 [details] [review] Bug 9809 - Remove DBIC module for reserveconstraints Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 41812 [details] [review] Bug 9809 - Update unit tests Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 41813 [details] [review] Bug 9809 - Remove reserveconstraints references from C4::Reserves Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 41814 [details] [review] Bug 9809 - Update AddReserve prototype to remove constraint parameter Test Plan: 1) Apply this patch set 2) prove t/db_dependent/Circulation.t 3) prove t/db_dependent/Holds.t 4) prove t/db_dependent/Holds/LocalHoldsPriority.t 5) prove t/db_dependent/Holds/RevertWaitingStatus.t 6) prove t/db_dependent/HoldsQueue.t 7) prove t/db_dependent/Reserves.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 41815 [details] [review] Bug 9809: [QA Follow-up] Remove warnings from Hold.pm Resolves two warnings on a uninitialized found column: holds: Use of uninitialized value in string eq at /home/koha/testclone/Koha/Hold.pm line 53., referer: http://test.rijkskoha.nl:28080/cgi-bin/koha/circ/circulation.pl?borrowernumber=152 holds: Use of uninitialized value in string eq at /home/koha/testclone/Koha/Hold.pm line 74., referer: http://test.rijkskoha.nl:28080/cgi-bin/koha/circ/circulation.pl?borrowernumber=152 Test plan: Run t/db_dependent/Hold.t. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 41816 [details] [review] Bug 9809: [QA Follow-up] Remove an erroneous call to GetReserveFee The call to GetReserveFee in opac-reserve.pl is useless in its current form. The first parameter undef takes care of receiving 0. But note that the user is warned correctly for the charge via param variable RESERVE_CHARGE on the opac form. When the hold is placed, AddReserve calls GetReserveFee. So if the routine would work correctly, we would not need this extra call in opac-reserve in the whole place. Unfortunately, the routine is not working correctly. I will submit a fix for GetReserveFee under a new report (14702). Test plan: [1] Add a hold fee to some category. [2] Check the warn for placing a hold on such a book in OPAC. [3] Observe that the actual fee is not charged. This is a current bug and it will be addressed on report 14702. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 41817 [details] [review] Bug 9809: [QA Follow-up] Remove constrainttype from 14464 tests Some additional unit tests of bug 14464 still included a constraint in the AddReserve calls. Test plan: Run Reserves.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Rebased (after 14464)
Created attachment 41845 [details] [review] Bug 9809: [QA Follow-up] Still found some remains In the staff client we had still some remains referring to the constraint types. Also touched one comment line from SIP. Test plan: Add a hold in the staff client.
Created attachment 41848 [details] [review] Bug 9809 - Update AddReserve prototype to remove constraint parameter Test Plan: 1) Apply this patch set 2) prove t/db_dependent/Circulation.t 3) prove t/db_dependent/Holds.t 4) prove t/db_dependent/Holds/LocalHoldsPriority.t 5) prove t/db_dependent/Holds/RevertWaitingStatus.t 6) prove t/db_dependent/HoldsQueue.t 7) prove t/db_dependent/Reserves.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> AMENDED: An else branch in reserve/placerequest.pl was removed. This had the effect of making it no longer possible to place an any hold in the staff client. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Verified placing a biblio level and an item level hold.
Created attachment 41849 [details] [review] Bug 9809: [QA Follow-up] Remove warnings from Hold.pm Resolves two warnings on a uninitialized found column: holds: Use of uninitialized value in string eq at /home/koha/testclone/Koha/Hold.pm line 53., referer: http://test.rijkskoha.nl:28080/cgi-bin/koha/circ/circulation.pl?borrowernumber=152 holds: Use of uninitialized value in string eq at /home/koha/testclone/Koha/Hold.pm line 74., referer: http://test.rijkskoha.nl:28080/cgi-bin/koha/circ/circulation.pl?borrowernumber=152 Test plan: Run t/db_dependent/Hold.t. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 41850 [details] [review] Bug 9809: [QA Follow-up] Remove an erroneous call to GetReserveFee The call to GetReserveFee in opac-reserve.pl is useless in its current form. The first parameter undef takes care of receiving 0. But note that the user is warned correctly for the charge via param variable RESERVE_CHARGE on the opac form. When the hold is placed, AddReserve calls GetReserveFee. So if the routine would work correctly, we would not need this extra call in opac-reserve in the whole place. Unfortunately, the routine is not working correctly. I will submit a fix for GetReserveFee under a new report (14702). Test plan: [1] Add a hold fee to some category. [2] Check the warn for placing a hold on such a book in OPAC. [3] Observe that the actual fee is not charged. This is a current bug and it will be addressed on report 14702. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 41851 [details] [review] Bug 9809: [QA Follow-up] Remove constrainttype from 14464 tests Some additional unit tests of bug 14464 still included a constraint in the AddReserve calls. Test plan: Run Reserves.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 41852 [details] [review] Bug 9809: [QA Follow-up] Still found some remains In the staff client we had still some remains referring to the constraint types. Also touched one comment line from SIP. Test plan: Add a hold in the staff client.
Created attachment 41892 [details] [review] Bug 9809 - DB Updates Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 41893 [details] [review] Bug 9809 - Update Schema files Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 41894 [details] [review] Bug 9809 - Remove DBIC module for reserveconstraints Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 41895 [details] [review] Bug 9809 - Update unit tests Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 41896 [details] [review] Bug 9809 - Remove reserveconstraints references from C4::Reserves Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 41897 [details] [review] Bug 9809 - Update AddReserve prototype to remove constraint parameter Test Plan: 1) Apply this patch set 2) prove t/db_dependent/Circulation.t 3) prove t/db_dependent/Holds.t 4) prove t/db_dependent/Holds/LocalHoldsPriority.t 5) prove t/db_dependent/Holds/RevertWaitingStatus.t 6) prove t/db_dependent/HoldsQueue.t 7) prove t/db_dependent/Reserves.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> AMENDED: An else branch in reserve/placerequest.pl was removed. This had the effect of making it no longer possible to place an any hold in the staff client. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Verified placing a biblio level and an item level hold. Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 41898 [details] [review] Bug 9809: [QA Follow-up] Remove warnings from Hold.pm Resolves two warnings on a uninitialized found column: holds: Use of uninitialized value in string eq at /home/koha/testclone/Koha/Hold.pm line 53., referer: http://test.rijkskoha.nl:28080/cgi-bin/koha/circ/circulation.pl?borrowernumber=152 holds: Use of uninitialized value in string eq at /home/koha/testclone/Koha/Hold.pm line 74., referer: http://test.rijkskoha.nl:28080/cgi-bin/koha/circ/circulation.pl?borrowernumber=152 Test plan: Run t/db_dependent/Hold.t. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 41899 [details] [review] Bug 9809: [QA Follow-up] Remove an erroneous call to GetReserveFee The call to GetReserveFee in opac-reserve.pl is useless in its current form. The first parameter undef takes care of receiving 0. But note that the user is warned correctly for the charge via param variable RESERVE_CHARGE on the opac form. When the hold is placed, AddReserve calls GetReserveFee. So if the routine would work correctly, we would not need this extra call in opac-reserve in the whole place. Unfortunately, the routine is not working correctly. I will submit a fix for GetReserveFee under a new report (14702). Test plan: [1] Add a hold fee to some category. [2] Check the warn for placing a hold on such a book in OPAC. [3] Observe that the actual fee is not charged. This is a current bug and it will be addressed on report 14702. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 41900 [details] [review] Bug 9809: [QA Follow-up] Remove constrainttype from 14464 tests Some additional unit tests of bug 14464 still included a constraint in the AddReserve calls. Test plan: Run Reserves.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 41901 [details] [review] Bug 9809: [QA Follow-up] Still found some remains In the staff client we had still some remains referring to the constraint types. Also touched one comment line from SIP. Test plan: Add a hold in the staff client. Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 41902 [details] [review] Bug 9809: Fix pod errors FAIL C4/Reserves.pm FAIL pod in file C4/Reserves.pm *** ERROR: Spurious =cut command Test plan: perl -e "use Pod::Checker;podchecker('C4/Reserves.pm');" Should not return any errors. Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 41903 [details] [review] Bug 9809: Remove one more occurrence of reserveconstraints This file is not updated, but no need to keep on occurrence of an nonexistent table in it. Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Patches pushed to master. Thanks guys!