Bug 18887

Summary: Introduce new table 'circulation_rules', use for 'max_holds' rules
Product: Koha Reporter: Kyle M Hall <kyle>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Josef Moravec <josef.moravec>
Severity: enhancement    
Priority: P5 - low CC: alexbuckley, bibliwho, black23, brendan, jonathan.druart, josef.moravec, jzairo, koha, lisettepalouse+koha, lucie.gay, martin.renvoize, nick, pianohacker, veron
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 15524    
Bug Blocks: 18886, 18925, 22526, 22847    
Attachments: Bug 18887 - Update database
Bug 18887 - Add new schema file
Bug 18887 - Update existing schema files
Bug 18887 - Add Koha::CirculationRule(s)
Bug 18887 - Port max_holds rules to new CirculationRules system
Bug 18887: (follow-up) Fix behavior for default branch/category
Bug 18887 - Update database
Bug 18887 - Add new schema file
Bug 18887 - Update existing schema files
Bug 18887 - Add Koha::CirculationRule(s)
Bug 18887 - Port max_holds rules to new CirculationRules system
Bug 18887: (follow-up) Fix behavior for default branch/category
Bug 18887: (follow-up) Fix null/empty behavior
Bug 18887: (follow-up) Fix null/empty behavior
Bug 18887 - Update database
Bug 18887 - Add new schema file
Bug 18887 - Update existing schema files
Bug 18887 - Add Koha::CirculationRule(s)
Bug 18887 - Port max_holds rules to new CirculationRules system
Bug 18887: (follow-up) Fix behavior for default branch/category
Bug 18887: (follow-up) Fix null/empty behavior
Bug 18887 - Update database
Bug 18887 - Add new schema file
Bug 18887 - Update existing schema files
Bug 18887 - Add Koha::CirculationRule(s)
Bug 18887 - Port max_holds rules to new CirculationRules system
Bug 18887: (follow-up) Fix behavior for default branch/category
Bug 18887: (follow-up) Fix null/empty behavior
Bug 18887: Fix issues caused by rebase
Bug 18887: Update database
Bug 18887: Add new schema file
Bug 18887: Update existing schema files
Bug 18887: Add Koha::CirculationRule(s)
Bug 18887: Port max_holds rules to new CirculationRules system
Bug 18887: (follow-up) Fix behavior for default branch/category
Bug 18887: (follow-up) Fix null/empty behavior
Bug 18887: Fix issues caused by rebase
Bug 18887: Fix DB structure issues
Bug 18887: Insert undef instead of '*'
Bug 18887: Use get_effective_rule instead of search
Bug 18887: Update database
Bug 18887: Add new schema file
Bug 18887: Update existing schema files
Bug 18887: Add Koha::CirculationRule(s)
Bug 18887: Port max_holds rules to new CirculationRules system
Bug 18887: (follow-up) Fix behavior for default branch/category
Bug 18887: (follow-up) Fix null/empty behavior
Bug 18887: Fix issues caused by rebase
Bug 18887: Fix DB structure issues
Bug 18887: Insert undef instead of '*'
Bug 18887: Use get_effective_rule instead of search
Bug 18887: Update database
Bug 18887: Add new schema file
Bug 18887: Update existing schema files
Bug 18887: Add Koha::CirculationRule(s)
Bug 18887: Port max_holds rules to new CirculationRules system
Bug 18887: (follow-up) Fix behavior for default branch/category
Bug 18887: (follow-up) Fix null/empty behavior
Bug 18887: Fix issues caused by rebase
Bug 18887: Fix DB structure issues
Bug 18887: Insert undef instead of '*'
Bug 18887: Use get_effective_rule instead of search
Bug 18887: Update database
Bug 18887: Add new schema file
Bug 18887: Update existing schema files
Bug 18887: Add Koha::CirculationRule(s)
Bug 18887: Port max_holds rules to new CirculationRules system
Bug 18887: (follow-up) Fix behavior for default branch/category
Bug 18887: (follow-up) Fix null/empty behavior
Bug 18887: Fix issues caused by rebase
Bug 18887: Fix DB structure issues
Bug 18887: Insert undef instead of '*'
Bug 18887: Use get_effective_rule instead of search
Bug 18887: Update database
Bug 18887: Add new schema file
Bug 18887: Update existing schema files
Bug 18887: Add Koha::CirculationRule(s)
Bug 18887: Port max_holds rules to new CirculationRules system
Bug 18887: (follow-up) Fix behavior for default branch/category
Bug 18887: (follow-up) Fix null/empty behavior
Bug 18887: Fix issues caused by rebase
Bug 18887: Fix DB structure issues
Bug 18887: Insert undef instead of '*'
Bug 18887: Use get_effective_rule instead of search
Bug 18887: Add 2 missing html filters
Looks good. Test are all successful. -Jesse
Bug 18887: Update database
Bug 18887: Add Koha::CirculationRule(s)
Bug 18887: Port max_holds rules to new CirculationRules system
Bug 18887: (follow-up) Fix behavior for default branch/category
Bug 18887: (follow-up) Fix null/empty behavior
Bug 18887: Fix issues caused by rebase
Bug 18887: Fix DB structure issues
Bug 18887: Insert undef instead of '*'
Bug 18887: Use get_effective_rule instead of search
Bug 18887: Add 2 missing html filters
Bug 18887: Add foreign keys
Bug 18887: Add new schema file
Bug 18887: Update existing schema files
Bug 18887: Fix POD c/p issues
Bug 18887: Replace croak with exceptions
Bug 18887: Add tests
Bug 18887: Update database
Bug 18887: Add Koha::CirculationRule(s)
Bug 18887: Port max_holds rules to new CirculationRules system
Bug 18887: (follow-up) Fix behavior for default branch/category
Bug 18887: (follow-up) Fix null/empty behavior
Bug 18887: Fix issues caused by rebase
Bug 18887: Fix DB structure issues
Bug 18887: Insert undef instead of '*'
Bug 18887: Use get_effective_rule instead of search
Bug 18887: Add 2 missing html filters
Bug 18887: Add foreign keys
Bug 18887: Add new schema file
Bug 18887: Update existing schema files
Bug 18887: Fix POD c/p issues
Bug 18887: Replace croak with exceptions
Bug 18887: Add tests
Bug 18887: (QA follow-up) Remove last occurences of old max_holds
Bug 18887: Make DB update idempotent

Description Kyle M Hall 2017-06-30 16:48:24 UTC

    
Comment 1 Kyle M Hall 2017-07-25 15:07:44 UTC
Created attachment 65233 [details] [review]
Bug 18887 - Update database
Comment 2 Kyle M Hall 2017-07-25 15:07:50 UTC
Created attachment 65234 [details] [review]
Bug 18887 - Add new schema file
Comment 3 Kyle M Hall 2017-07-25 15:07:53 UTC
Created attachment 65235 [details] [review]
Bug 18887 - Update existing schema files
Comment 4 Kyle M Hall 2017-07-25 15:07:55 UTC
Created attachment 65236 [details] [review]
Bug 18887 - Add Koha::CirculationRule(s)
Comment 5 Kyle M Hall 2017-07-25 15:07:58 UTC
Created attachment 65237 [details] [review]
Bug 18887 - Port max_holds rules to new CirculationRules system

This is the first step in the circulation rules revamp as detailed
in the RFF https://wiki.koha-community.org/wiki/Circulation_Rules_Interface_and_Backend_Revamp_RFC

This patch moves the recent max_holds rule to the new circulation_rules table.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Go to the circ rules editor, note the new max holds rules
   by patron category in the "Checkout limit by patron category".
   ( Should we rename this section? )
4) Create find a patron that is allowed to place a hold, count the
   number of holds that patron has. Lets make that number 'X'.
5) Set the new max holds rule to X for "All libraries"
6) Note the patron can no longer place another hold
7) Set the new max holds rule to X + 1 for the patron's home library
8) Note the patron can again place another hold
9) Set the new max holds rule to X for the patron's home library
10) Note the patron can no longer place another hold
Comment 6 Jesse Weaver 2017-08-10 19:48:24 UTC
Comment on attachment 65233 [details] [review]
Bug 18887 - Update database

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

::: installer/data/mysql/atomicupdate/bug_18887.perl
@@ +24,5 @@
> +    });
> +
> +    $dbh->do(q{
> +        INSERT INTO circulation_rules ( branchcode, categorycode, itemtype, rule_name, rule_value )
> +        SELECT NULL, categorycode, NULL, 'max_holds', max_holds FROM branch_borrower_circ_rules

Shouldn't this SELECT be pulling from `default_borrower_circ_rules`?
Comment 7 Jesse Weaver 2017-08-10 19:52:59 UTC
Comment on attachment 65236 [details] [review]
Bug 18887 - Add Koha::CirculationRule(s)

I'm a bit confused as to the intended API; the new code in Reserves.pm uses CirculationRules->find, the template plugin uses CirculationRules->search, and there's an unused(?) method called get_effective_rule that seems like it implements the inheritance logic correctly.
Comment 8 Josef Moravec 2017-09-18 11:59:58 UTC
 FAIL	Koha/CirculationRules.pm
   FAIL	  forbidden patterns
		forbidden pattern: Data::Dumper::Dumper (line 139)
   FAIL	  pod coverage
		POD is missing for 'object_class'


Setting the max_holds rule for some library is ok and getting used when placing hold.

But when I try to set default rule for all libraries, the rule is saved to the database (with branchcode = NULL), but is not shown on the circulation rules page.
Comment 9 Jesse Weaver 2017-10-04 22:54:03 UTC
Created attachment 67620 [details] [review]
Bug 18887: (follow-up) Fix behavior for default branch/category

Also, fix a couple small QA issues.
Comment 10 Alex Buckley 2017-10-04 23:13:23 UTC
Hi Kyle

Can you please rebase against master because when trying to apply the patches up to and including the 'port max_holds_rules to new CirculationRules systems' (after applying the dependency: patch 15524) I got the below merge error. 

The issue is in Koha/Schema/Result/Branch.pm file.

Apply? [(y)es, (n)o, (i)nteractive] i
Applying: Bug 18887 - Update database
Applying: Bug 18887 - Add new schema file
Applying: Bug 18887 - Update existing schema files
Using index info to reconstruct a base tree...
M	Koha/Schema/Result/Branch.pm
Falling back to patching base and 3-way merge...
Auto-merging Koha/Schema/Result/Branch.pm
CONFLICT (content): Merge conflict in Koha/Schema/Result/Branch.pm
Failed to merge in the changes.
Patch failed at 0001 Bug 18887 - Update existing schema files
The copy of the patch that failed is found in:
   /home/vagrant/kohaclone/.git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-18887---Update-existing-schema-files-74PCpN.patch
Comment 11 Jesse Weaver 2018-01-26 21:38:38 UTC
Created attachment 70986 [details] [review]
Bug 18887 - Update database
Comment 12 Jesse Weaver 2018-01-26 21:38:40 UTC
Created attachment 70987 [details] [review]
Bug 18887 - Add new schema file
Comment 13 Jesse Weaver 2018-01-26 21:38:43 UTC
Created attachment 70988 [details] [review]
Bug 18887 - Update existing schema files
Comment 14 Jesse Weaver 2018-01-26 21:38:47 UTC
Created attachment 70989 [details] [review]
Bug 18887 - Add Koha::CirculationRule(s)
Comment 15 Jesse Weaver 2018-01-26 21:38:50 UTC
Created attachment 70990 [details] [review]
Bug 18887 - Port max_holds rules to new CirculationRules system

This is the first step in the circulation rules revamp as detailed
in the RFF https://wiki.koha-community.org/wiki/Circulation_Rules_Interface_and_Backend_Revamp_RFC

This patch moves the recent max_holds rule to the new circulation_rules table.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Go to the circ rules editor, note the new max holds rules
   by patron category in the "Checkout limit by patron category".
   ( Should we rename this section? )
4) Create find a patron that is allowed to place a hold, count the
   number of holds that patron has. Lets make that number 'X'.
5) Set the new max holds rule to X for "All libraries"
6) Note the patron can no longer place another hold
7) Set the new max holds rule to X + 1 for the patron's home library
8) Note the patron can again place another hold
9) Set the new max holds rule to X for the patron's home library
10) Note the patron can no longer place another hold
Comment 16 Jesse Weaver 2018-01-26 21:38:52 UTC
Created attachment 70991 [details] [review]
Bug 18887: (follow-up) Fix behavior for default branch/category

Also, fix a couple small QA issues.
Comment 17 Jesse Weaver 2018-01-29 23:07:02 UTC
Created attachment 71031 [details] [review]
Bug 18887: (follow-up) Fix null/empty behavior
Comment 18 Jesse Weaver 2018-01-29 23:11:45 UTC
Created attachment 71042 [details] [review]
Bug 18887: (follow-up) Fix null/empty behavior
Comment 19 Jesse Weaver 2018-02-26 18:25:00 UTC
Created attachment 72218 [details] [review]
Bug 18887 - Update database
Comment 20 Jesse Weaver 2018-02-26 18:25:03 UTC
Created attachment 72219 [details] [review]
Bug 18887 - Add new schema file
Comment 21 Jesse Weaver 2018-02-26 18:25:06 UTC
Created attachment 72220 [details] [review]
Bug 18887 - Update existing schema files
Comment 22 Jesse Weaver 2018-02-26 18:25:09 UTC
Created attachment 72221 [details] [review]
Bug 18887 - Add Koha::CirculationRule(s)
Comment 23 Jesse Weaver 2018-02-26 18:25:12 UTC
Created attachment 72222 [details] [review]
Bug 18887 - Port max_holds rules to new CirculationRules system

This is the first step in the circulation rules revamp as detailed
in the RFF https://wiki.koha-community.org/wiki/Circulation_Rules_Interface_and_Backend_Revamp_RFC

This patch moves the recent max_holds rule to the new circulation_rules table.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Go to the circ rules editor, note the new max holds rules
   by patron category in the "Checkout limit by patron category".
   ( Should we rename this section? )
4) Create find a patron that is allowed to place a hold, count the
   number of holds that patron has. Lets make that number 'X'.
5) Set the new max holds rule to X for "All libraries"
6) Note the patron can no longer place another hold
7) Set the new max holds rule to X + 1 for the patron's home library
8) Note the patron can again place another hold
9) Set the new max holds rule to X for the patron's home library
10) Note the patron can no longer place another hold
Comment 24 Jesse Weaver 2018-02-26 18:25:15 UTC
Created attachment 72223 [details] [review]
Bug 18887: (follow-up) Fix behavior for default branch/category

Also, fix a couple small QA issues.
Comment 25 Jesse Weaver 2018-02-26 18:25:18 UTC
Created attachment 72224 [details] [review]
Bug 18887: (follow-up) Fix null/empty behavior
Comment 26 Lucie Gay 2018-03-13 13:48:29 UTC
DBD::mysql::db do failed: Unknown column 'max_holds' in 'field list' [for Statement "
        INSERT INTO circulation_rules ( branchcode, categorycode, itemtype, rule_name, rule_value )
        SELECT branchcode, categorycode, NULL, 'max_holds', COALESCE( max_holds, '' ) FROM branch_borrower_circ_rules
    "] at (eval 1179) line 21.
DBD::mysql::db do failed: Unknown column 'max_holds' in 'field list' [for Statement "
        INSERT INTO circulation_rules ( branchcode, categorycode, itemtype, rule_name, rule_value )
        SELECT NULL, categorycode, NULL, 'max_holds', COALESCE( max_holds, '' ) FROM default_borrower_circ_rules
    "] at (eval 1179) line 26.
DBD::mysql::db do failed: Can't DROP 'max_holds'; check that column/key exists [for Statement "
        ALTER TABLE branch_borrower_circ_rules DROP COLUMN max_holds
    "] at (eval 1179) line 31.
Upgrade to XXX done (Bug 18887 - Introduce new table 'circulation_rules', use for 'max_holds' rules)
DBD::mysql::db do failed: Can't DROP 'max_holds'; check that column/key exists [for Statement "
        ALTER TABLE default_borrower_circ_rules DROP COLUMN max_holds
    "] at (eval 1179) line 35.
Comment 27 Lucie Gay 2018-03-14 14:33:29 UTC
Tested on Unimarc sandbox.

Patch applied but there are error messages : 

-----

The sandbox you've requested is now ready.
The patches attached to bugzilla 18887 have been applied, and the result is:

Updatedatabase:
Upgrade to 17.12.00.000 done (Tē tōia, tē haumatia)
Upgrade to 17.12.00.001 done (Bug 19724 - Add [deleted]biblio_metadata.timestamp)
Upgrade to 17.12.00.002 done (Bug 19096 - Make Default authoritative for Koha to MARC mappings)
Upgrade to 17.12.00.003 done (Bug 10021 - Drop notifys-related table and columns)
Upgrade to 17.12.00.004 done (Bug 19278 - Add a configurable default page size for REST endpoints)
Upgrade to 17.12.00.005 done (Bug 18915 - Correct CHECKOUT_NOTE notice template)
Upgrade to 17.12.00.006 done (Bug 17682 - Update URL for Google Scholar in OPACSearchForTitleIn)
Upgrade to 17.12.00.007 done (Bug 15707 - Add new table library_groups)
Upgrade to 17.12.00.008 done (Bug 16735 - Migrate library search groups into the new hierarchical groups)
Upgrade to 17.12.00.009 done (Bug 18403 - Add the view_borrower_infos_from_any_libraries permission )
Upgrade to 17.12.00.010 done (Bug 20133 - Add library_groups.ft_hide_patron_info)
Upgrade to 17.12.00.011 done (Bug 20157 - Use group 'features' to decide which groups to use for group searching functionality)
Upgrade to 17.12.00.012 done (Bug 15752 - Add system preference AutoSwitchPatron)
Upgrade to 17.12.00.013 done (Bug 20175 - Set DEFAULT NULL value for club_enrollments.date_created)
Upgrade to 17.12.00.014 done (Bug 19790 - Remove additionalauthors.author from installer files)
Upgrade to 17.12.00.015 done (Bug 20144 - Adapt DB structure to work with new SQL modes)
Upgrade to 17.12.00.016 done (Bug 18336 - Convert DB tables to utf8mb4 ��)
Upgrade to 17.12.00.017 done (Bug 17672 - Add damaged_on to items and deleteditems tables)
Upgrade to 17.12.00.018 done (Bug 19290 - Add system preference BrowseResultSelection)
Upgrade to 17.12.00.019 done (Bug 20074 - Auth_subfield_structure changes hidden attribute)
Upgrade to 17.12.00.020 done (Bug 20082 - Update descriptions of Vietnamese language)
Upgrade to 17.12.00.021 done (Bug 13287 - Add system preference PurgeSuggestionsOlderThan)
DEV atomic update: bug_18887.perl
DBD::mysql::db do failed: Unknown column 'max_holds' in 'field list' [for Statement "
        INSERT INTO circulation_rules ( branchcode, categorycode, itemtype, rule_name, rule_value )
        SELECT branchcode, categorycode, NULL, 'max_holds', COALESCE( max_holds, '' ) FROM branch_borrower_circ_rules
    "] at (eval 1179) line 21.
DBD::mysql::db do failed: Unknown column 'max_holds' in 'field list' [for Statement "
        INSERT INTO circulation_rules ( branchcode, categorycode, itemtype, rule_name, rule_value )
        SELECT NULL, categorycode, NULL, 'max_holds', COALESCE( max_holds, '' ) FROM default_borrower_circ_rules
    "] at (eval 1179) line 26.
DBD::mysql::db do failed: Can't DROP 'max_holds'; check that column/key exists [for Statement "
        ALTER TABLE branch_borrower_circ_rules DROP COLUMN max_holds
    "] at (eval 1179) line 31.
DBD::mysql::db do failed: Can't DROP 'max_holds'; check that column/key exists [for Statement "
        ALTER TABLE default_borrower_circ_rules DROP COLUMN max_holds
    "] at (eval 1179) line 35.
Upgrade to XXX done (Bug 18887 - Introduce new table 'circulation_rules', use for 'max_holds' rules)

------

Staff interface works, but when I go in admin > Circulation and fines rules (cgi-bin/koha/admin/smart-rules.pl), there are error messages :

Global symbol "$max_holds" requires explicit package name (did you forget to declare "my $max_holds"?) at /home/koha/src/admin/smart-rules.pl line 283.
Global symbol "$max_holds" requires explicit package name (did you forget to declare "my $max_holds"?) at /home/koha/src/admin/smart-rules.pl line 284.
Global symbol "$max_holds" requires explicit package name (did you forget to declare "my $max_holds"?) at /home/koha/src/admin/smart-rules.pl line 284.
Global symbol "$max_holds" requires explicit package name (did you forget to declare "my $max_holds"?) at /home/koha/src/admin/smart-rules.pl line 315.
Global symbol "$max_holds" requires explicit package name (did you forget to declare "my $max_holds"?) at /home/koha/src/admin/smart-rules.pl line 347.
Global symbol "$max_holds" requires explicit package name (did you forget to declare "my $max_holds"?) at /home/koha/src/admin/smart-rules.pl line 405.
Execution of /home/koha/src/admin/smart-rules.pl aborted due to compilation errors.
Comment 28 Kyle M Hall 2018-07-18 18:02:25 UTC
Created attachment 77080 [details] [review]
Bug 18887 - Update database
Comment 29 Kyle M Hall 2018-07-18 18:02:45 UTC
Created attachment 77081 [details] [review]
Bug 18887 - Add new schema file
Comment 30 Kyle M Hall 2018-07-18 18:02:50 UTC
Created attachment 77082 [details] [review]
Bug 18887 - Update existing schema files
Comment 31 Kyle M Hall 2018-07-18 18:02:54 UTC
Created attachment 77083 [details] [review]
Bug 18887 - Add Koha::CirculationRule(s)
Comment 32 Kyle M Hall 2018-07-18 18:02:58 UTC
Created attachment 77084 [details] [review]
Bug 18887 - Port max_holds rules to new CirculationRules system

This is the first step in the circulation rules revamp as detailed
in the RFF https://wiki.koha-community.org/wiki/Circulation_Rules_Interface_and_Backend_Revamp_RFC

This patch moves the recent max_holds rule to the new circulation_rules table.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Go to the circ rules editor, note the new max holds rules
   by patron category in the "Checkout limit by patron category".
   ( Should we rename this section? )
4) Create find a patron that is allowed to place a hold, count the
   number of holds that patron has. Lets make that number 'X'.
5) Set the new max holds rule to X for "All libraries"
6) Note the patron can no longer place another hold
7) Set the new max holds rule to X + 1 for the patron's home library
8) Note the patron can again place another hold
9) Set the new max holds rule to X for the patron's home library
10) Note the patron can no longer place another hold
Comment 33 Kyle M Hall 2018-07-18 18:03:03 UTC
Created attachment 77085 [details] [review]
Bug 18887: (follow-up) Fix behavior for default branch/category

Also, fix a couple small QA issues.
Comment 34 Kyle M Hall 2018-07-18 18:03:07 UTC
Created attachment 77086 [details] [review]
Bug 18887: (follow-up) Fix null/empty behavior
Comment 35 Kyle M Hall 2018-07-18 18:03:11 UTC
Created attachment 77087 [details] [review]
Bug 18887: Fix issues caused by rebase
Comment 36 Jonathan Druart 2018-07-23 23:29:51 UTC
Note for QA (does not block signoff):
* update DB entry is not idempotent
* Last of tests and POD for the new module
* merge markers in first patch
* This change is odd and need more attention 
-    $max_holds = undef if $max_holds !~ /^\d+/;
+    $max_holds = '' if $max_holds !~ /^\d+/;

Especially because it is different than others.
Comment 37 Jonathan Druart 2018-07-23 23:34:29 UTC
Also
* The table circulation_rules does not have the current charset collate (see bug 18336)
* Copyright lines come from Koha/IssuingRules.pm
Comment 38 Jonathan Druart 2018-07-24 12:06:48 UTC
Created attachment 77207 [details] [review]
Bug 18887: Update database
Comment 39 Jonathan Druart 2018-07-24 12:06:53 UTC
Created attachment 77208 [details] [review]
Bug 18887: Add new schema file
Comment 40 Jonathan Druart 2018-07-24 12:06:58 UTC
Created attachment 77209 [details] [review]
Bug 18887: Update existing schema files
Comment 41 Jonathan Druart 2018-07-24 12:07:03 UTC
Created attachment 77210 [details] [review]
Bug 18887: Add Koha::CirculationRule(s)
Comment 42 Jonathan Druart 2018-07-24 12:07:08 UTC
Created attachment 77211 [details] [review]
Bug 18887: Port max_holds rules to new CirculationRules system

This is the first step in the circulation rules revamp as detailed
in the RFF https://wiki.koha-community.org/wiki/Circulation_Rules_Interface_and_Backend_Revamp_RFC

This patch moves the recent max_holds rule to the new circulation_rules table.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Go to the circ rules editor, note the new max holds rules
   by patron category in the "Checkout limit by patron category".
   ( Should we rename this section? )
4) Create find a patron that is allowed to place a hold, count the
   number of holds that patron has. Lets make that number 'X'.
5) Set the new max holds rule to X for "All libraries"
6) Note the patron can no longer place another hold
7) Set the new max holds rule to X + 1 for the patron's home library
8) Note the patron can again place another hold
9) Set the new max holds rule to X for the patron's home library
10) Note the patron can no longer place another hold
Comment 43 Jonathan Druart 2018-07-24 12:07:13 UTC
Created attachment 77212 [details] [review]
Bug 18887: (follow-up) Fix behavior for default branch/category

Also, fix a couple small QA issues.
Comment 44 Jonathan Druart 2018-07-24 12:07:18 UTC
Created attachment 77213 [details] [review]
Bug 18887: (follow-up) Fix null/empty behavior
Comment 45 Jonathan Druart 2018-07-24 12:07:23 UTC
Created attachment 77214 [details] [review]
Bug 18887: Fix issues caused by rebase
Comment 46 Jonathan Druart 2018-07-24 12:07:28 UTC
Created attachment 77215 [details] [review]
Bug 18887: Fix DB structure issues

- Make atomic update idempotent
- Correct COLLAT (atomic update)
- Add index on rule_name (kohastructure.sql)
- Add rule_name to the unique constraint (kohastructure.sql)
- Remove max_holds columns (kohastructure.sql)
Comment 47 Jonathan Druart 2018-07-24 14:07:12 UTC
Created attachment 77218 [details] [review]
Bug 18887: Insert undef instead of '*'

Koha::CirculationRules->get_effective_rule will become the method to
call to retrieve a specific rule, we should start using it when
possible.

Moreover undef could replace '*' to mean 'any', that way we will be able
to add FK on circulation_rules

TODO: Add more tests
Comment 48 Jonathan Druart 2018-07-24 14:11:57 UTC
(In reply to Jonathan Druart from comment #47)
> Created attachment 77218 [details] [review] [review]
> Bug 18887: Insert undef instead of '*'
> 
> Koha::CirculationRules->get_effective_rule will become the method to
> call to retrieve a specific rule, we should start using it when
> possible.
> 
> Moreover undef could replace '*' to mean 'any', that way we will be able
> to add FK on circulation_rules
> 
> TODO: Add more tests

Kyle, does this last patch make sense to you?
Comment 49 Jonathan Druart 2018-07-24 15:25:25 UTC
Created attachment 77238 [details] [review]
Bug 18887: Use get_effective_rule instead of search
Comment 50 Jonathan Druart 2018-08-10 13:05:20 UTC
Created attachment 77649 [details] [review]
Bug 18887: Update database
Comment 51 Jonathan Druart 2018-08-10 13:05:25 UTC
Created attachment 77650 [details] [review]
Bug 18887: Add new schema file
Comment 52 Jonathan Druart 2018-08-10 13:05:31 UTC
Created attachment 77651 [details] [review]
Bug 18887: Update existing schema files
Comment 53 Jonathan Druart 2018-08-10 13:05:36 UTC
Created attachment 77652 [details] [review]
Bug 18887: Add Koha::CirculationRule(s)
Comment 54 Jonathan Druart 2018-08-10 13:05:42 UTC
Created attachment 77653 [details] [review]
Bug 18887: Port max_holds rules to new CirculationRules system

This is the first step in the circulation rules revamp as detailed
in the RFF https://wiki.koha-community.org/wiki/Circulation_Rules_Interface_and_Backend_Revamp_RFC

This patch moves the recent max_holds rule to the new circulation_rules table.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Go to the circ rules editor, note the new max holds rules
   by patron category in the "Checkout limit by patron category".
   ( Should we rename this section? )
4) Create find a patron that is allowed to place a hold, count the
   number of holds that patron has. Lets make that number 'X'.
5) Set the new max holds rule to X for "All libraries"
6) Note the patron can no longer place another hold
7) Set the new max holds rule to X + 1 for the patron's home library
8) Note the patron can again place another hold
9) Set the new max holds rule to X for the patron's home library
10) Note the patron can no longer place another hold
Comment 55 Jonathan Druart 2018-08-10 13:05:48 UTC
Created attachment 77654 [details] [review]
Bug 18887: (follow-up) Fix behavior for default branch/category

Also, fix a couple small QA issues.
Comment 56 Jonathan Druart 2018-08-10 13:05:53 UTC
Created attachment 77655 [details] [review]
Bug 18887: (follow-up) Fix null/empty behavior
Comment 57 Jonathan Druart 2018-08-10 13:05:59 UTC
Created attachment 77656 [details] [review]
Bug 18887: Fix issues caused by rebase
Comment 58 Jonathan Druart 2018-08-10 13:06:04 UTC
Created attachment 77657 [details] [review]
Bug 18887: Fix DB structure issues

- Make atomic update idempotent
- Correct COLLAT (atomic update)
- Add index on rule_name (kohastructure.sql)
- Add rule_name to the unique constraint (kohastructure.sql)
- Remove max_holds columns (kohastructure.sql)
Comment 59 Jonathan Druart 2018-08-10 13:06:10 UTC
Created attachment 77658 [details] [review]
Bug 18887: Insert undef instead of '*'

Koha::CirculationRules->get_effective_rule will become the method to
call to retrieve a specific rule, we should start using it when
possible.

Moreover undef could replace '*' to mean 'any', that way we will be able
to add FK on circulation_rules

TODO: Add more tests
Comment 60 Jonathan Druart 2018-08-10 13:06:15 UTC
Created attachment 77659 [details] [review]
Bug 18887: Use get_effective_rule instead of search
Comment 61 Cab Vinton 2018-08-30 04:38:11 UTC
Patch did not apply to PTFS Sandbox 0.

The sandbox you've requested is not ready.
Some problems occurred applying patches from bug 18887:
<h1>Something went wrong !</h1>Applying: Bug 18887: Update database
Applying: Bug 18887: Add new schema file
Applying: Bug 18887: Update existing schema files
Applying: Bug 18887: Add Koha::CirculationRule(s)
Applying: Bug 18887: Port max_holds rules to new CirculationRules system
error: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt).
Patch failed at 0001 Bug 18887: Port max_holds rules to new CirculationRules system
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem run git bz apply --continue.
If you would prefer to skip this patch, instead run git bz apply --skip.
To restore the original branch and stop patching run git bz apply --abort.
error: could not build fake ancestor
Bug 18887 - Introduce new table 'circulation_rules', use for 'max_holds' rules

77649 - Bug 18887: Update database
77650 - Bug 18887: Add new schema file
77651 - Bug 18887: Update existing schema files
77652 - Bug 18887: Add Koha::CirculationRule(s)
77653 - Bug 18887: Port max_holds rules to new CirculationRules system
77654 - Bug 18887: (follow-up) Fix behavior for default branch/category
77655 - Bug 18887: (follow-up) Fix null/empty behavior
77656 - Bug 18887: Fix issues caused by rebase
77657 - Bug 18887: Fix DB structure issues
77658 - Bug 18887: Insert undef instead of '*'
77659 - Bug 18887: Use get_effective_rule instead of search

Apply? [(y)es, (n)o, (i)nteractive] Patch left in /tmp/Bug-18887-Port-maxholds-rules-to-new-CirculationRu-_cXRQk.patch .
Comment 62 Jonathan Druart 2018-08-31 18:13:37 UTC
Created attachment 78346 [details] [review]
Bug 18887: Update database
Comment 63 Jonathan Druart 2018-08-31 18:13:42 UTC
Created attachment 78347 [details] [review]
Bug 18887: Add new schema file
Comment 64 Jonathan Druart 2018-08-31 18:13:47 UTC
Created attachment 78348 [details] [review]
Bug 18887: Update existing schema files
Comment 65 Jonathan Druart 2018-08-31 18:13:52 UTC
Created attachment 78349 [details] [review]
Bug 18887: Add Koha::CirculationRule(s)
Comment 66 Jonathan Druart 2018-08-31 18:13:56 UTC
Created attachment 78350 [details] [review]
Bug 18887: Port max_holds rules to new CirculationRules system

This is the first step in the circulation rules revamp as detailed
in the RFF https://wiki.koha-community.org/wiki/Circulation_Rules_Interface_and_Backend_Revamp_RFC

This patch moves the recent max_holds rule to the new circulation_rules table.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Go to the circ rules editor, note the new max holds rules
   by patron category in the "Checkout limit by patron category".
   ( Should we rename this section? )
4) Create find a patron that is allowed to place a hold, count the
   number of holds that patron has. Lets make that number 'X'.
5) Set the new max holds rule to X for "All libraries"
6) Note the patron can no longer place another hold
7) Set the new max holds rule to X + 1 for the patron's home library
8) Note the patron can again place another hold
9) Set the new max holds rule to X for the patron's home library
10) Note the patron can no longer place another hold
Comment 67 Jonathan Druart 2018-08-31 18:14:01 UTC
Created attachment 78351 [details] [review]
Bug 18887: (follow-up) Fix behavior for default branch/category

Also, fix a couple small QA issues.
Comment 68 Jonathan Druart 2018-08-31 18:14:06 UTC
Created attachment 78352 [details] [review]
Bug 18887: (follow-up) Fix null/empty behavior
Comment 69 Jonathan Druart 2018-08-31 18:14:11 UTC
Created attachment 78353 [details] [review]
Bug 18887: Fix issues caused by rebase
Comment 70 Jonathan Druart 2018-08-31 18:14:16 UTC
Created attachment 78354 [details] [review]
Bug 18887: Fix DB structure issues

- Make atomic update idempotent
- Correct COLLAT (atomic update)
- Add index on rule_name (kohastructure.sql)
- Add rule_name to the unique constraint (kohastructure.sql)
- Remove max_holds columns (kohastructure.sql)
Comment 71 Jonathan Druart 2018-08-31 18:14:21 UTC
Created attachment 78355 [details] [review]
Bug 18887: Insert undef instead of '*'

Koha::CirculationRules->get_effective_rule will become the method to
call to retrieve a specific rule, we should start using it when
possible.

Moreover undef could replace '*' to mean 'any', that way we will be able
to add FK on circulation_rules

TODO: Add more tests
Comment 72 Jonathan Druart 2018-08-31 18:14:26 UTC
Created attachment 78356 [details] [review]
Bug 18887: Use get_effective_rule instead of search
Comment 73 Cab Vinton 2018-09-01 07:01:08 UTC
Software error on PTFS sandbox 0:

Template process failed: undef error - DBIx::Class::Storage::DBI::_dbh_execute(): Table 'koha.circulation_rules' doesn't exist at /home/koha/src/Koha/Objects.pm line 191
Comment 74 Jonathan Druart 2018-09-04 18:08:11 UTC
(In reply to Cab Vinton from comment #73)
> Software error on PTFS sandbox 0:
> 
> Template process failed: undef error -
> DBIx::Class::Storage::DBI::_dbh_execute(): Table 'koha.circulation_rules'
> doesn't exist at /home/koha/src/Koha/Objects.pm line 191

On which page do you get this error? The circulation_rules should have been created during the upgrade process, do you see any errors displayed in the news section on the Koha home page?
Comment 75 Cab Vinton 2018-09-05 01:09:58 UTC
(In reply to Jonathan Druart from comment #74)
> (In reply to Cab Vinton from comment #73)
> > Software error on PTFS sandbox 0:
> > 
> > Template process failed: undef error -
> > DBIx::Class::Storage::DBI::_dbh_execute(): Table 'koha.circulation_rules'
> > doesn't exist at /home/koha/src/Koha/Objects.pm line 191
> 
> On which page do you get this error? The circulation_rules should have been
> created during the upgrade process, do you see any errors displayed in the
> news section on the Koha home page?

Bugger. Can't recreate the error, but I believe the error came when I tried to place a hold on an item. (One or more settings isn't allowing me to place a hold, but haven't been able to isolate which.)

Fwiw, I get a similar error when simply trying to check out an item (both with and without the patch applied):

Undefined subroutine &C4::Circulation::getnextacctno called at /home/koha/src/C4/Circulation.pm line 3194.

Have tried a number of the Biblibre sandboxes as well, but several of those also have issues w/ checking out & placing holds, even w/ master :-(
Comment 76 Koha Team University Lyon 3 2018-09-14 17:27:18 UTC Comment hidden (obsolete)
Comment 77 Koha Team University Lyon 3 2018-09-14 17:29:57 UTC
CONFLICT (content): Merge conflict in t/db_dependent/Holds.t Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt
Comment 78 Jonathan Druart 2018-09-14 17:50:44 UTC
Created attachment 78674 [details] [review]
Bug 18887: Update database
Comment 79 Jonathan Druart 2018-09-14 17:50:51 UTC
Created attachment 78675 [details] [review]
Bug 18887: Add new schema file
Comment 80 Jonathan Druart 2018-09-14 17:50:57 UTC
Created attachment 78676 [details] [review]
Bug 18887: Update existing schema files
Comment 81 Jonathan Druart 2018-09-14 17:51:02 UTC
Created attachment 78677 [details] [review]
Bug 18887: Add Koha::CirculationRule(s)
Comment 82 Jonathan Druart 2018-09-14 17:51:08 UTC
Created attachment 78678 [details] [review]
Bug 18887: Port max_holds rules to new CirculationRules system

This is the first step in the circulation rules revamp as detailed
in the RFF https://wiki.koha-community.org/wiki/Circulation_Rules_Interface_and_Backend_Revamp_RFC

This patch moves the recent max_holds rule to the new circulation_rules table.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Go to the circ rules editor, note the new max holds rules
   by patron category in the "Checkout limit by patron category".
   ( Should we rename this section? )
4) Create find a patron that is allowed to place a hold, count the
   number of holds that patron has. Lets make that number 'X'.
5) Set the new max holds rule to X for "All libraries"
6) Note the patron can no longer place another hold
7) Set the new max holds rule to X + 1 for the patron's home library
8) Note the patron can again place another hold
9) Set the new max holds rule to X for the patron's home library
10) Note the patron can no longer place another hold
Comment 83 Jonathan Druart 2018-09-14 17:51:14 UTC
Created attachment 78679 [details] [review]
Bug 18887: (follow-up) Fix behavior for default branch/category

Also, fix a couple small QA issues.
Comment 84 Jonathan Druart 2018-09-14 17:51:20 UTC
Created attachment 78680 [details] [review]
Bug 18887: (follow-up) Fix null/empty behavior
Comment 85 Jonathan Druart 2018-09-14 17:51:26 UTC
Created attachment 78681 [details] [review]
Bug 18887: Fix issues caused by rebase
Comment 86 Jonathan Druart 2018-09-14 17:51:31 UTC
Created attachment 78682 [details] [review]
Bug 18887: Fix DB structure issues

- Make atomic update idempotent
- Correct COLLAT (atomic update)
- Add index on rule_name (kohastructure.sql)
- Add rule_name to the unique constraint (kohastructure.sql)
- Remove max_holds columns (kohastructure.sql)
Comment 87 Jonathan Druart 2018-09-14 17:51:36 UTC
Created attachment 78683 [details] [review]
Bug 18887: Insert undef instead of '*'

Koha::CirculationRules->get_effective_rule will become the method to
call to retrieve a specific rule, we should start using it when
possible.

Moreover undef could replace '*' to mean 'any', that way we will be able
to add FK on circulation_rules

TODO: Add more tests
Comment 88 Jonathan Druart 2018-09-14 17:51:40 UTC
Created attachment 78684 [details] [review]
Bug 18887: Use get_effective_rule instead of search
Comment 89 Lisette Scheer 2018-09-14 21:34:39 UTC
Patch applied but exploded with the following error when I tried to go to the circulation rules page. 
Template process failed: undef error - DBIx::Class::Schema::source(): Can't find source for CirculationRule at /home/vagrant/kohaclone/Koha/Objects.pm line 341
 at /home/vagrant/kohaclone/C4/Templates.pm line 122 

Lisette
Comment 90 Lisette Scheer 2018-09-14 21:59:30 UTC
Created attachment 78743 [details] [review]
Bug 18887: Update database
Comment 91 Lisette Scheer 2018-09-14 21:59:38 UTC
Created attachment 78744 [details] [review]
Bug 18887: Add new schema file
Comment 92 Lisette Scheer 2018-09-14 21:59:43 UTC
Created attachment 78745 [details] [review]
Bug 18887: Update existing schema files
Comment 93 Lisette Scheer 2018-09-14 21:59:47 UTC
Created attachment 78746 [details] [review]
Bug 18887: Add Koha::CirculationRule(s)
Comment 94 Lisette Scheer 2018-09-14 21:59:52 UTC
Created attachment 78747 [details] [review]
Bug 18887: Port max_holds rules to new CirculationRules system

This is the first step in the circulation rules revamp as detailed
in the RFF https://wiki.koha-community.org/wiki/Circulation_Rules_Interface_and_Backend_Revamp_RFC

This patch moves the recent max_holds rule to the new circulation_rules table.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Go to the circ rules editor, note the new max holds rules
   by patron category in the "Checkout limit by patron category".
   ( Should we rename this section? )
4) Create find a patron that is allowed to place a hold, count the
   number of holds that patron has. Lets make that number 'X'.
5) Set the new max holds rule to X for "All libraries"
6) Note the patron can no longer place another hold
7) Set the new max holds rule to X + 1 for the patron's home library
8) Note the patron can again place another hold
9) Set the new max holds rule to X for the patron's home library
10) Note the patron can no longer place another hold
Comment 95 Lisette Scheer 2018-09-14 21:59:57 UTC
Created attachment 78748 [details] [review]
Bug 18887: (follow-up) Fix behavior for default branch/category

Also, fix a couple small QA issues.
Comment 96 Lisette Scheer 2018-09-14 22:00:02 UTC
Created attachment 78749 [details] [review]
Bug 18887: (follow-up) Fix null/empty behavior
Comment 97 Lisette Scheer 2018-09-14 22:00:11 UTC
Created attachment 78750 [details] [review]
Bug 18887: Fix issues caused by rebase
Comment 98 Lisette Scheer 2018-09-14 22:00:22 UTC
Created attachment 78751 [details] [review]
Bug 18887: Fix DB structure issues

- Make atomic update idempotent
- Correct COLLAT (atomic update)
- Add index on rule_name (kohastructure.sql)
- Add rule_name to the unique constraint (kohastructure.sql)
- Remove max_holds columns (kohastructure.sql)
Comment 99 Lisette Scheer 2018-09-14 22:00:28 UTC
Created attachment 78752 [details] [review]
Bug 18887: Insert undef instead of '*'

Koha::CirculationRules->get_effective_rule will become the method to
call to retrieve a specific rule, we should start using it when
possible.

Moreover undef could replace '*' to mean 'any', that way we will be able
to add FK on circulation_rules

TODO: Add more tests
Comment 100 Lisette Scheer 2018-09-14 22:00:35 UTC
Created attachment 78753 [details] [review]
Bug 18887: Use get_effective_rule instead of search

Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>
Comment 101 Jonathan Druart 2018-09-15 20:08:31 UTC
Created attachment 78879 [details] [review]
Bug 18887: Add 2 missing html filters
Comment 102 Jesse Maseto 2018-09-19 15:01:25 UTC
Created attachment 79119 [details] [review]
Looks good. Test are all successful.   -Jesse

Bug 18887: Update database

Signed-off-by: Jesse Maseto <jesse@bywatersolution.com>
Comment 103 Josef Moravec 2018-09-20 09:00:54 UTC
Comment on attachment 78744 [details] [review]
Bug 18887: Add new schema file

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

I started reading the code and found few issues:

In the schema files I can see the foreign keys for branchcode, categorycode and itemtype, but they aren't defined in db update nor kohastructure.sql. But I think we want them.
Comment 104 Josef Moravec 2018-09-20 09:10:41 UTC
Comment on attachment 78746 [details] [review]
Bug 18887: Add Koha::CirculationRule(s)

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

::: Koha/CirculationRule.pm
@@ +26,5 @@
> +use Koha::ItemTypes;
> +
> +=head1 NAME
> +
> +Koha::Hold - Koha Hold object class

C & P mistake

::: Koha/CirculationRules.pm
@@ +27,5 @@
> +use base qw(Koha::Objects);
> +
> +=head1 NAME
> +
> +Koha::IssuingRules - Koha IssuingRule Object set class

C & P mistake

@@ +47,5 @@
> +    my $categorycode = $params->{categorycode};
> +    my $itemtype     = $params->{itemtype};
> +    my $branchcode   = $params->{branchcode};
> +
> +    croak q{No rule name passed in!} unless $rule_name;

Please use Exception instead of croak

@@ +85,5 @@
> +      unless exists $params->{itemtype};
> +    croak q{set_rule requires the parameter 'rule_name'!}
> +      unless exists $params->{rule_name};
> +    croak q{set_rule requires the parameter 'rule_value'!}
> +      unless exists $params->{rule_value};

Please use Exception instead of croak
Comment 105 Jonathan Druart 2018-09-29 21:18:48 UTC
Created attachment 79626 [details] [review]
Bug 18887: Update database

Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>

Signed-off-by: Jesse Maseto <jesse@bywatersolution.com>
Comment 106 Jonathan Druart 2018-09-29 21:18:55 UTC
Created attachment 79627 [details] [review]
Bug 18887: Add Koha::CirculationRule(s)

Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>

Signed-off-by: Jesse Maseto <jesse@bywatersolution.com>
Comment 107 Jonathan Druart 2018-09-29 21:19:01 UTC
Created attachment 79628 [details] [review]
Bug 18887: Port max_holds rules to new CirculationRules system

This is the first step in the circulation rules revamp as detailed
in the RFF https://wiki.koha-community.org/wiki/Circulation_Rules_Interface_and_Backend_Revamp_RFC

This patch moves the recent max_holds rule to the new circulation_rules table.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Go to the circ rules editor, note the new max holds rules
   by patron category in the "Checkout limit by patron category".
   ( Should we rename this section? )
4) Create find a patron that is allowed to place a hold, count the
   number of holds that patron has. Lets make that number 'X'.
5) Set the new max holds rule to X for "All libraries"
6) Note the patron can no longer place another hold
7) Set the new max holds rule to X + 1 for the patron's home library
8) Note the patron can again place another hold
9) Set the new max holds rule to X for the patron's home library
10) Note the patron can no longer place another hold

Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>

Signed-off-by: Jesse Maseto <jesse@bywatersolution.com>
Comment 108 Jonathan Druart 2018-09-29 21:19:08 UTC
Created attachment 79629 [details] [review]
Bug 18887: (follow-up) Fix behavior for default branch/category

Also, fix a couple small QA issues.

Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>

Signed-off-by: Jesse Maseto <jesse@bywatersolution.com>
Comment 109 Jonathan Druart 2018-09-29 21:19:15 UTC
Created attachment 79630 [details] [review]
Bug 18887: (follow-up) Fix null/empty behavior

Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>

Signed-off-by: Jesse Maseto <jesse@bywatersolution.com>
Comment 110 Jonathan Druart 2018-09-29 21:19:21 UTC
Created attachment 79631 [details] [review]
Bug 18887: Fix issues caused by rebase

Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>

Signed-off-by: Jesse Maseto <jesse@bywatersolution.com>
Comment 111 Jonathan Druart 2018-09-29 21:19:28 UTC
Created attachment 79632 [details] [review]
Bug 18887: Fix DB structure issues

- Make atomic update idempotent
- Correct COLLAT (atomic update)
- Add index on rule_name (kohastructure.sql)
- Add rule_name to the unique constraint (kohastructure.sql)
- Remove max_holds columns (kohastructure.sql)

Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>

Signed-off-by: Jesse Maseto <jesse@bywatersolution.com>
Comment 112 Jonathan Druart 2018-09-29 21:19:35 UTC
Created attachment 79633 [details] [review]
Bug 18887: Insert undef instead of '*'

Koha::CirculationRules->get_effective_rule will become the method to
call to retrieve a specific rule, we should start using it when
possible.

Moreover undef could replace '*' to mean 'any', that way we will be able
to add FK on circulation_rules

TODO: Add more tests

Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>

Signed-off-by: Jesse Maseto <jesse@bywatersolution.com>
Comment 113 Jonathan Druart 2018-09-29 21:19:41 UTC
Created attachment 79634 [details] [review]
Bug 18887: Use get_effective_rule instead of search

Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>

Signed-off-by: Jesse Maseto <jesse@bywatersolution.com>
Comment 114 Jonathan Druart 2018-09-29 21:19:48 UTC
Created attachment 79635 [details] [review]
Bug 18887: Add 2 missing html filters

Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>

Signed-off-by: Jesse Maseto <jesse@bywatersolution.com>
Comment 115 Jonathan Druart 2018-09-29 21:20:00 UTC
Created attachment 79636 [details] [review]
Bug 18887: Add foreign keys
Comment 116 Jonathan Druart 2018-09-29 21:20:08 UTC
Created attachment 79637 [details] [review]
Bug 18887: Add new schema file
Comment 117 Jonathan Druart 2018-09-29 21:20:15 UTC
Created attachment 79638 [details] [review]
Bug 18887: Update existing schema files
Comment 118 Jonathan Druart 2018-09-29 21:20:23 UTC
Created attachment 79639 [details] [review]
Bug 18887: Fix POD c/p issues
Comment 119 Jonathan Druart 2018-09-29 21:20:30 UTC
Created attachment 79640 [details] [review]
Bug 18887: Replace croak with exceptions
Comment 120 Jonathan Druart 2018-09-29 21:20:37 UTC
Created attachment 79641 [details] [review]
Bug 18887: Add tests
Comment 121 Jonathan Druart 2018-09-29 21:24:55 UTC
Several things here:

1. I have reattached the patch in the correct order and add the signed-off-by lines for Lisette and Jesse (only last patch was signed off)

2. (In reply to Josef Moravec from comment #103)
> Comment on attachment 78744 [details] [review] [review]
> In the schema files I can see the foreign keys for branchcode, categorycode
> and itemtype, but they aren't defined in db update nor kohastructure.sql.
> But I think we want them.

I have reworked the schema files (md5sum was wrong which caused the script to fail). Foreign keys have been added to both kohastructure.sql and atomic update file.

3. 
(In reply to Josef Moravec from comment #104)
> Comment on attachment 78746 [details] [review] [review]
> > +Koha::Hold - Koha Hold object class
> 
> C & P mistake

Fixed (x2)

4.
> @@ +47,5 @@
> > +    my $categorycode = $params->{categorycode};
> > +    my $itemtype     = $params->{itemtype};
> > +    my $branchcode   = $params->{branchcode};
> > +
> > +    croak q{No rule name passed in!} unless $rule_name;
> 
> Please use Exception instead of croak

Removed and exceptions are now thrown

5. Tests have been provided, finally!
Comment 122 Josef Moravec 2018-10-01 19:32:53 UTC
Created attachment 79761 [details] [review]
Bug 18887: Update database

Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>

Signed-off-by: Jesse Maseto <jesse@bywatersolution.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 123 Josef Moravec 2018-10-01 19:33:05 UTC
Created attachment 79762 [details] [review]
Bug 18887: Add Koha::CirculationRule(s)

Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>

Signed-off-by: Jesse Maseto <jesse@bywatersolution.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 124 Josef Moravec 2018-10-01 19:33:18 UTC
Created attachment 79763 [details] [review]
Bug 18887: Port max_holds rules to new CirculationRules system

This is the first step in the circulation rules revamp as detailed
in the RFF https://wiki.koha-community.org/wiki/Circulation_Rules_Interface_and_Backend_Revamp_RFC

This patch moves the recent max_holds rule to the new circulation_rules table.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Go to the circ rules editor, note the new max holds rules
   by patron category in the "Checkout limit by patron category".
   ( Should we rename this section? )
4) Create find a patron that is allowed to place a hold, count the
   number of holds that patron has. Lets make that number 'X'.
5) Set the new max holds rule to X for "All libraries"
6) Note the patron can no longer place another hold
7) Set the new max holds rule to X + 1 for the patron's home library
8) Note the patron can again place another hold
9) Set the new max holds rule to X for the patron's home library
10) Note the patron can no longer place another hold

Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>

Signed-off-by: Jesse Maseto <jesse@bywatersolution.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 125 Josef Moravec 2018-10-01 19:33:34 UTC
Created attachment 79764 [details] [review]
Bug 18887: (follow-up) Fix behavior for default branch/category

Also, fix a couple small QA issues.

Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>

Signed-off-by: Jesse Maseto <jesse@bywatersolution.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 126 Josef Moravec 2018-10-01 19:33:50 UTC
Created attachment 79765 [details] [review]
Bug 18887: (follow-up) Fix null/empty behavior

Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>

Signed-off-by: Jesse Maseto <jesse@bywatersolution.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 127 Josef Moravec 2018-10-01 19:34:02 UTC
Created attachment 79766 [details] [review]
Bug 18887: Fix issues caused by rebase

Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>

Signed-off-by: Jesse Maseto <jesse@bywatersolution.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 128 Josef Moravec 2018-10-01 19:34:20 UTC
Created attachment 79767 [details] [review]
Bug 18887: Fix DB structure issues

- Make atomic update idempotent
- Correct COLLAT (atomic update)
- Add index on rule_name (kohastructure.sql)
- Add rule_name to the unique constraint (kohastructure.sql)
- Remove max_holds columns (kohastructure.sql)

Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>

Signed-off-by: Jesse Maseto <jesse@bywatersolution.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 129 Josef Moravec 2018-10-01 19:34:34 UTC
Created attachment 79768 [details] [review]
Bug 18887: Insert undef instead of '*'

Koha::CirculationRules->get_effective_rule will become the method to
call to retrieve a specific rule, we should start using it when
possible.

Moreover undef could replace '*' to mean 'any', that way we will be able
to add FK on circulation_rules

TODO: Add more tests

Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>

Signed-off-by: Jesse Maseto <jesse@bywatersolution.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 130 Josef Moravec 2018-10-01 19:34:56 UTC
Created attachment 79769 [details] [review]
Bug 18887: Use get_effective_rule instead of search

Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>

Signed-off-by: Jesse Maseto <jesse@bywatersolution.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 131 Josef Moravec 2018-10-01 19:35:22 UTC
Created attachment 79770 [details] [review]
Bug 18887: Add 2 missing html filters

Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>

Signed-off-by: Jesse Maseto <jesse@bywatersolution.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 132 Josef Moravec 2018-10-01 19:35:34 UTC
Created attachment 79771 [details] [review]
Bug 18887: Add foreign keys

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 133 Josef Moravec 2018-10-01 19:35:49 UTC
Created attachment 79772 [details] [review]
Bug 18887: Add new schema file

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 134 Josef Moravec 2018-10-01 19:35:59 UTC
Created attachment 79773 [details] [review]
Bug 18887: Update existing schema files

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 135 Josef Moravec 2018-10-01 19:36:12 UTC
Created attachment 79774 [details] [review]
Bug 18887: Fix POD c/p issues

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 136 Josef Moravec 2018-10-01 19:36:22 UTC
Created attachment 79775 [details] [review]
Bug 18887: Replace croak with exceptions

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 137 Josef Moravec 2018-10-01 19:36:36 UTC
Created attachment 79776 [details] [review]
Bug 18887: Add tests

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 138 Josef Moravec 2018-10-01 19:37:01 UTC
Created attachment 79778 [details] [review]
Bug 18887: (QA follow-up) Remove last occurences of old max_holds

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 139 Josef Moravec 2018-10-01 19:37:59 UTC
(In reply to Jonathan Druart from comment #121)
> Several things here:
> 
> 1. I have reattached the patch in the correct order and add the
> signed-off-by lines for Lisette and Jesse (only last patch was signed off)
> 
> 2. (In reply to Josef Moravec from comment #103)
> > Comment on attachment 78744 [details] [review] [review] [review]
> > In the schema files I can see the foreign keys for branchcode, categorycode
> > and itemtype, but they aren't defined in db update nor kohastructure.sql.
> > But I think we want them.
> 
> I have reworked the schema files (md5sum was wrong which caused the script
> to fail). Foreign keys have been added to both kohastructure.sql and atomic
> update file.
> 
> 3. 
> (In reply to Josef Moravec from comment #104)
> > Comment on attachment 78746 [details] [review] [review] [review]
> > > +Koha::Hold - Koha Hold object class
> > 
> > C & P mistake
> 
> Fixed (x2)
> 
> 4.
> > @@ +47,5 @@
> > > +    my $categorycode = $params->{categorycode};
> > > +    my $itemtype     = $params->{itemtype};
> > > +    my $branchcode   = $params->{branchcode};
> > > +
> > > +    croak q{No rule name passed in!} unless $rule_name;
> > 
> > Please use Exception instead of croak
> 
> Removed and exceptions are now thrown
> 
> 5. Tests have been provided, finally!

Great job Jonathan, thanks!
Comment 140 Jonathan Druart 2018-10-03 14:05:34 UTC
Created attachment 79891 [details] [review]
Bug 18887: Make DB update idempotent
Comment 141 Nick Clemens 2018-10-05 16:27:33 UTC
Awesome work all!

Pushed to master for 18.11
Comment 142 Martin Renvoize 2018-10-08 08:59:39 UTC
Enhancement, will not be backported to 18.05.x series.
Comment 143 Jonathan Druart 2018-10-25 13:30:22 UTC
I have found a bug in the structure of the table.

circulation_rules:
  PRIMARY KEY (`id`),
  UNIQUE KEY `branchcode` (`branchcode`,`categorycode`,`itemtype`,`rule_name`),

issuingrules:
PRIMARY KEY (`branchcode`,`categorycode`,`itemtype`),

The difference between UNIQUE and PRIMARY keys is that UNIQUE accept duplicated NULL values.
So the DBMS will not raised an error if we try to insert several times the same rule.
Comment 144 Martin Renvoize 2018-10-31 14:57:25 UTC
Are you working on a fix for this Jonathan.. or have you opened a followup bug?
Comment 145 Jonathan Druart 2018-10-31 15:10:10 UTC
(In reply to Martin Renvoize from comment #144)
> Are you working on a fix for this Jonathan.. or have you opened a followup
> bug?

I do not know what would be the best way to fix it, any ideas?
Comment 146 Martin Renvoize 2018-11-01 09:47:05 UTC
Ack, of course.. this is where NULL being meaningful is bad :(.

Hmm, in a few other projects I enforced via relations and made sure I had a 'hidden' default to replace anywhere I'd want a NULL.. but I think that would be a pretty drastic change..  I'll ponder for a while.