Bug 9809

Summary: Get rid of reserveconstraints
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: Architecture, internals, and plumbingAssignee: Kyle M Hall <kyle>
Status: CLOSED FIXED QA Contact:
Severity: enhancement    
Priority: P5 - low CC: gmcharlt, gwilliams, jonathan.druart, kyle, m.de.rooy, tomascohen
Version: master   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14526
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14337
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14155
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14097
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13930
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13903
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12632
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7376
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=5144
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7957
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 12632, 14337    
Attachments: Bug 9809: Get rid of reserveconstraints
Bug 9809 - DB Updates
Bug 9809 - Update Schema files
Bug 9809 - Remove DBIC module for reserveconstraints
Bug 9809 - Update unit tests
Bug 9809 - Remove reserveconstraints references from C4::Reserves
Bug 9809 - Update AddReserve prototype to remove constraint parameter
Bug 9809 - DB Updates
Bug 9809 - Update Schema files
Bug 9809 - Remove DBIC module for reserveconstraints
Bug 9809 - Update unit tests
Bug 9809 - Remove reserveconstraints references from C4::Reserves
Bug 9809 - Update AddReserve prototype to remove constraint parameter
Bug 9809: [QA Follow-up] Remove warnings from Hold.pm
Bug 9809: [QA Follow-up] Remove an erroneous call to GetReserveFee
[SIGNED-OFF] Bug 9809 - DB Updates
[SIGNED-OFF] Bug 9809 - Update Schema files
[SIGNED-OFF] Bug 9809 - Remove DBIC module for reserveconstraints
[SIGNED-OFF] Bug 9809 - Update unit tests
[SIGNED-OFF] Bug 9809 - Remove reserveconstraints references from C4::Reserves
[SIGNED-OFF] Bug 9809 - Update AddReserve prototype to remove constraint parameter
[SIGNED-OFF] Bug 9809: [QA Follow-up] Remove warnings from Hold.pm
[SIGNED-OFF] Bug 9809: [QA Follow-up] Remove an erroneous call to GetReserveFee
Bug 9809 - DB Updates
Bug 9809 - Update Schema files
Bug 9809 - Remove DBIC module for reserveconstraints
Bug 9809 - Update unit tests
Bug 9809 - Remove reserveconstraints references from C4::Reserves
Bug 9809 - Update AddReserve prototype to remove constraint parameter
Bug 9809: [QA Follow-up] Remove warnings from Hold.pm
Bug 9809: [QA Follow-up] Remove an erroneous call to GetReserveFee
Bug 9809: [QA Follow-up] Remove constrainttype from 14464 tests
Bug 9809: [QA Follow-up] Still found some remains
Bug 9809 - Update AddReserve prototype to remove constraint parameter
Bug 9809: [QA Follow-up] Remove warnings from Hold.pm
Bug 9809: [QA Follow-up] Remove an erroneous call to GetReserveFee
Bug 9809: [QA Follow-up] Remove constrainttype from 14464 tests
Bug 9809: [QA Follow-up] Still found some remains
Bug 9809 - DB Updates
Bug 9809 - Update Schema files
Bug 9809 - Remove DBIC module for reserveconstraints
Bug 9809 - Update unit tests
Bug 9809 - Remove reserveconstraints references from C4::Reserves
Bug 9809 - Update AddReserve prototype to remove constraint parameter
Bug 9809: [QA Follow-up] Remove warnings from Hold.pm
Bug 9809: [QA Follow-up] Remove an erroneous call to GetReserveFee
Bug 9809: [QA Follow-up] Remove constrainttype from 14464 tests
Bug 9809: [QA Follow-up] Still found some remains
Bug 9809: Fix pod errors
Bug 9809: Remove one more occurrence of reserveconstraints

Description Marcel de Rooy 2013-03-13 15:12:36 UTC
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 :)
Comment 1 Marcel de Rooy 2013-03-13 15:14:33 UTC
Include the code around reqbib in reserve/placerequest.pl ?
Comment 2 Jonathan Druart 2015-04-10 10:06:02 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2015-06-30 12:56:31 UTC Comment hidden (obsolete)
Comment 4 Kyle M Hall 2015-06-30 12:56:37 UTC Comment hidden (obsolete)
Comment 5 Kyle M Hall 2015-06-30 12:56:39 UTC Comment hidden (obsolete)
Comment 6 Kyle M Hall 2015-06-30 12:56:42 UTC Comment hidden (obsolete)
Comment 7 Kyle M Hall 2015-06-30 12:56:44 UTC Comment hidden (obsolete)
Comment 8 Kyle M Hall 2015-06-30 12:56:46 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2015-07-02 10:34:56 UTC
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.
Comment 10 Kyle M Hall 2015-07-07 12:31:43 UTC
(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 11 Katrin Fischer 2015-07-07 12:40:15 UTC
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?
Comment 12 Jonathan Druart 2015-08-05 08:28:21 UTC
Yes it is!
Comment 13 Marcel de Rooy 2015-08-21 07:39:42 UTC
Testing this one now..
Comment 14 Marcel de Rooy 2015-08-21 08:59:50 UTC Comment hidden (obsolete)
Comment 15 Marcel de Rooy 2015-08-21 08:59:55 UTC Comment hidden (obsolete)
Comment 16 Marcel de Rooy 2015-08-21 08:59:58 UTC Comment hidden (obsolete)
Comment 17 Marcel de Rooy 2015-08-21 09:00:02 UTC Comment hidden (obsolete)
Comment 18 Marcel de Rooy 2015-08-21 09:00:07 UTC Comment hidden (obsolete)
Comment 19 Marcel de Rooy 2015-08-21 09:00:11 UTC Comment hidden (obsolete)
Comment 20 Marcel de Rooy 2015-08-21 09:00:15 UTC Comment hidden (obsolete)
Comment 21 Marcel de Rooy 2015-08-21 09:00:19 UTC Comment hidden (obsolete)
Comment 22 Marcel de Rooy 2015-08-21 09:01:43 UTC
Looks good to me! Thanks, Kyle.
I added two small follow-ups and a see-also to a new report for GetReserveFee.
Comment 23 Marcel de Rooy 2015-08-21 10:14:06 UTC
(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?
Comment 24 Kyle M Hall 2015-08-21 10:22:32 UTC Comment hidden (obsolete)
Comment 25 Kyle M Hall 2015-08-21 10:22:39 UTC Comment hidden (obsolete)
Comment 26 Kyle M Hall 2015-08-21 10:22:42 UTC Comment hidden (obsolete)
Comment 27 Kyle M Hall 2015-08-21 10:22:44 UTC Comment hidden (obsolete)
Comment 28 Kyle M Hall 2015-08-21 10:22:47 UTC Comment hidden (obsolete)
Comment 29 Kyle M Hall 2015-08-21 10:22:49 UTC Comment hidden (obsolete)
Comment 30 Kyle M Hall 2015-08-21 10:22:52 UTC Comment hidden (obsolete)
Comment 31 Kyle M Hall 2015-08-21 10:22:54 UTC Comment hidden (obsolete)
Comment 32 Kyle M Hall 2015-08-21 10:24:01 UTC
(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!
Comment 33 Jonathan Druart 2015-08-21 10:34:04 UTC
Bug 14464 is passed QA and will be in conflict with this patch. Please provide a followup build on top of it.
Comment 34 Marcel de Rooy 2015-08-21 10:51:08 UTC
(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.
Comment 35 Marcel de Rooy 2015-08-24 06:57:53 UTC Comment hidden (obsolete)
Comment 36 Marcel de Rooy 2015-08-24 06:57:57 UTC Comment hidden (obsolete)
Comment 37 Marcel de Rooy 2015-08-24 06:58:01 UTC Comment hidden (obsolete)
Comment 38 Marcel de Rooy 2015-08-24 06:58:05 UTC Comment hidden (obsolete)
Comment 39 Marcel de Rooy 2015-08-24 06:58:09 UTC Comment hidden (obsolete)
Comment 40 Marcel de Rooy 2015-08-24 06:58:13 UTC Comment hidden (obsolete)
Comment 41 Marcel de Rooy 2015-08-24 06:58:17 UTC Comment hidden (obsolete)
Comment 42 Marcel de Rooy 2015-08-24 06:58:21 UTC Comment hidden (obsolete)
Comment 43 Marcel de Rooy 2015-08-24 06:58:25 UTC Comment hidden (obsolete)
Comment 44 Marcel de Rooy 2015-08-24 07:00:48 UTC
Rebased (after 14464)
Comment 45 Marcel de Rooy 2015-08-24 12:09:36 UTC Comment hidden (obsolete)
Comment 46 Marcel de Rooy 2015-08-24 12:39:34 UTC Comment hidden (obsolete)
Comment 47 Marcel de Rooy 2015-08-24 12:39:38 UTC Comment hidden (obsolete)
Comment 48 Marcel de Rooy 2015-08-24 12:39:42 UTC Comment hidden (obsolete)
Comment 49 Marcel de Rooy 2015-08-24 12:39:45 UTC Comment hidden (obsolete)
Comment 50 Marcel de Rooy 2015-08-24 12:39:49 UTC Comment hidden (obsolete)
Comment 51 Jonathan Druart 2015-08-25 11:37:02 UTC
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>
Comment 52 Jonathan Druart 2015-08-25 11:37:06 UTC
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>
Comment 53 Jonathan Druart 2015-08-25 11:37:10 UTC
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>
Comment 54 Jonathan Druart 2015-08-25 11:37:13 UTC
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>
Comment 55 Jonathan Druart 2015-08-25 11:37:16 UTC
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>
Comment 56 Jonathan Druart 2015-08-25 11:37:20 UTC
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>
Comment 57 Jonathan Druart 2015-08-25 11:37:23 UTC
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>
Comment 58 Jonathan Druart 2015-08-25 11:37:26 UTC
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>
Comment 59 Jonathan Druart 2015-08-25 11:37:30 UTC
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>
Comment 60 Jonathan Druart 2015-08-25 11:37:33 UTC
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>
Comment 61 Jonathan Druart 2015-08-25 11:37:36 UTC
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>
Comment 62 Jonathan Druart 2015-08-25 11:37:40 UTC
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>
Comment 63 Tomás Cohen Arazi 2015-08-26 14:09:36 UTC
Patches pushed to master.

Thanks guys!