Bug 9809 - Get rid of reserveconstraints
Summary: Get rid of reserveconstraints
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact:
URL:
Keywords:
Depends on:
Blocks: 12632 14337
  Show dependency treegraph
 
Reported: 2013-03-13 15:12 UTC by Marcel de Rooy
Modified: 2016-12-05 21:23 UTC (History)
6 users (show)

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


Attachments
Bug 9809: Get rid of reserveconstraints (10.76 KB, patch)
2015-04-10 10:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9809 - DB Updates (3.23 KB, patch)
2015-06-30 12:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9809 - Update Schema files (2.64 KB, patch)
2015-06-30 12:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9809 - Remove DBIC module for reserveconstraints (2.26 KB, patch)
2015-06-30 12:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9809 - Update unit tests (11.84 KB, patch)
2015-06-30 12:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9809 - Remove reserveconstraints references from C4::Reserves (4.13 KB, patch)
2015-06-30 12:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9809 - Update AddReserve prototype to remove constraint parameter (14.17 KB, patch)
2015-06-30 12:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9809 - DB Updates (3.33 KB, patch)
2015-08-21 08:59 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9809 - Update Schema files (2.73 KB, patch)
2015-08-21 08:59 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9809 - Remove DBIC module for reserveconstraints (2.34 KB, patch)
2015-08-21 08:59 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9809 - Update unit tests (11.96 KB, patch)
2015-08-21 09:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9809 - Remove reserveconstraints references from C4::Reserves (4.22 KB, patch)
2015-08-21 09:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9809 - Update AddReserve prototype to remove constraint parameter (14.27 KB, patch)
2015-08-21 09:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9809: [QA Follow-up] Remove warnings from Hold.pm (1.55 KB, patch)
2015-08-21 09:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9809: [QA Follow-up] Remove an erroneous call to GetReserveFee (1.98 KB, patch)
2015-08-21 09:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9809 - DB Updates (3.35 KB, patch)
2015-08-21 10:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9809 - Update Schema files (2.76 KB, patch)
2015-08-21 10:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9809 - Remove DBIC module for reserveconstraints (2.38 KB, patch)
2015-08-21 10:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9809 - Update unit tests (11.99 KB, patch)
2015-08-21 10:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9809 - Remove reserveconstraints references from C4::Reserves (4.25 KB, patch)
2015-08-21 10:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9809 - Update AddReserve prototype to remove constraint parameter (14.29 KB, patch)
2015-08-21 10:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9809: [QA Follow-up] Remove warnings from Hold.pm (1.58 KB, patch)
2015-08-21 10:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9809: [QA Follow-up] Remove an erroneous call to GetReserveFee (2.02 KB, patch)
2015-08-21 10:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9809 - DB Updates (3.39 KB, patch)
2015-08-24 06:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9809 - Update Schema files (2.79 KB, patch)
2015-08-24 06:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9809 - Remove DBIC module for reserveconstraints (2.39 KB, patch)
2015-08-24 06:58 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9809 - Update unit tests (12.02 KB, patch)
2015-08-24 06:58 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9809 - Remove reserveconstraints references from C4::Reserves (4.24 KB, patch)
2015-08-24 06:58 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9809 - Update AddReserve prototype to remove constraint parameter (14.29 KB, patch)
2015-08-24 06:58 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9809: [QA Follow-up] Remove warnings from Hold.pm (1.61 KB, patch)
2015-08-24 06:58 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9809: [QA Follow-up] Remove an erroneous call to GetReserveFee (2.03 KB, patch)
2015-08-24 06:58 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9809: [QA Follow-up] Remove constrainttype from 14464 tests (1.11 KB, patch)
2015-08-24 06:58 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9809: [QA Follow-up] Still found some remains (2.83 KB, patch)
2015-08-24 12:09 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9809 - Update AddReserve prototype to remove constraint parameter (14.60 KB, patch)
2015-08-24 12:39 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9809: [QA Follow-up] Remove warnings from Hold.pm (1.61 KB, patch)
2015-08-24 12:39 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9809: [QA Follow-up] Remove an erroneous call to GetReserveFee (2.03 KB, patch)
2015-08-24 12:39 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9809: [QA Follow-up] Remove constrainttype from 14464 tests (1.11 KB, patch)
2015-08-24 12:39 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9809: [QA Follow-up] Still found some remains (2.83 KB, patch)
2015-08-24 12:39 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9809 - DB Updates (3.42 KB, patch)
2015-08-25 11:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9809 - Update Schema files (2.81 KB, patch)
2015-08-25 11:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9809 - Remove DBIC module for reserveconstraints (2.42 KB, patch)
2015-08-25 11:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9809 - Update unit tests (12.04 KB, patch)
2015-08-25 11:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9809 - Remove reserveconstraints references from C4::Reserves (4.27 KB, patch)
2015-08-25 11:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9809 - Update AddReserve prototype to remove constraint parameter (14.63 KB, patch)
2015-08-25 11:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9809: [QA Follow-up] Remove warnings from Hold.pm (1.64 KB, patch)
2015-08-25 11:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9809: [QA Follow-up] Remove an erroneous call to GetReserveFee (2.06 KB, patch)
2015-08-25 11:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9809: [QA Follow-up] Remove constrainttype from 14464 tests (1.14 KB, patch)
2015-08-25 11:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9809: [QA Follow-up] Still found some remains (2.85 KB, patch)
2015-08-25 11:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9809: Fix pod errors (1.23 KB, patch)
2015-08-25 11:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9809: Remove one more occurrence of reserveconstraints (1.01 KB, patch)
2015-08-25 11:37 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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!