Created attachment 65233 [details] [review] Bug 18887 - Update database
Created attachment 65234 [details] [review] Bug 18887 - Add new schema file
Created attachment 65235 [details] [review] Bug 18887 - Update existing schema files
Created attachment 65236 [details] [review] Bug 18887 - Add Koha::CirculationRule(s)
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 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 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.
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.
Created attachment 67620 [details] [review] Bug 18887: (follow-up) Fix behavior for default branch/category Also, fix a couple small QA issues.
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
Created attachment 70986 [details] [review] Bug 18887 - Update database
Created attachment 70987 [details] [review] Bug 18887 - Add new schema file
Created attachment 70988 [details] [review] Bug 18887 - Update existing schema files
Created attachment 70989 [details] [review] Bug 18887 - Add Koha::CirculationRule(s)
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
Created attachment 70991 [details] [review] Bug 18887: (follow-up) Fix behavior for default branch/category Also, fix a couple small QA issues.
Created attachment 71031 [details] [review] Bug 18887: (follow-up) Fix null/empty behavior
Created attachment 71042 [details] [review] Bug 18887: (follow-up) Fix null/empty behavior
Created attachment 72218 [details] [review] Bug 18887 - Update database
Created attachment 72219 [details] [review] Bug 18887 - Add new schema file
Created attachment 72220 [details] [review] Bug 18887 - Update existing schema files
Created attachment 72221 [details] [review] Bug 18887 - Add Koha::CirculationRule(s)
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
Created attachment 72223 [details] [review] Bug 18887: (follow-up) Fix behavior for default branch/category Also, fix a couple small QA issues.
Created attachment 72224 [details] [review] Bug 18887: (follow-up) Fix null/empty behavior
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.
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.
Created attachment 77080 [details] [review] Bug 18887 - Update database
Created attachment 77081 [details] [review] Bug 18887 - Add new schema file
Created attachment 77082 [details] [review] Bug 18887 - Update existing schema files
Created attachment 77083 [details] [review] Bug 18887 - Add Koha::CirculationRule(s)
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
Created attachment 77085 [details] [review] Bug 18887: (follow-up) Fix behavior for default branch/category Also, fix a couple small QA issues.
Created attachment 77086 [details] [review] Bug 18887: (follow-up) Fix null/empty behavior
Created attachment 77087 [details] [review] Bug 18887: Fix issues caused by rebase
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.
Also * The table circulation_rules does not have the current charset collate (see bug 18336) * Copyright lines come from Koha/IssuingRules.pm
Created attachment 77207 [details] [review] Bug 18887: Update database
Created attachment 77208 [details] [review] Bug 18887: Add new schema file
Created attachment 77209 [details] [review] Bug 18887: Update existing schema files
Created attachment 77210 [details] [review] Bug 18887: Add Koha::CirculationRule(s)
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
Created attachment 77212 [details] [review] Bug 18887: (follow-up) Fix behavior for default branch/category Also, fix a couple small QA issues.
Created attachment 77213 [details] [review] Bug 18887: (follow-up) Fix null/empty behavior
Created attachment 77214 [details] [review] Bug 18887: Fix issues caused by rebase
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)
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
(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?
Created attachment 77238 [details] [review] Bug 18887: Use get_effective_rule instead of search
Created attachment 77649 [details] [review] Bug 18887: Update database
Created attachment 77650 [details] [review] Bug 18887: Add new schema file
Created attachment 77651 [details] [review] Bug 18887: Update existing schema files
Created attachment 77652 [details] [review] Bug 18887: Add Koha::CirculationRule(s)
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
Created attachment 77654 [details] [review] Bug 18887: (follow-up) Fix behavior for default branch/category Also, fix a couple small QA issues.
Created attachment 77655 [details] [review] Bug 18887: (follow-up) Fix null/empty behavior
Created attachment 77656 [details] [review] Bug 18887: Fix issues caused by rebase
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)
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
Created attachment 77659 [details] [review] Bug 18887: Use get_effective_rule instead of search
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 .
Created attachment 78346 [details] [review] Bug 18887: Update database
Created attachment 78347 [details] [review] Bug 18887: Add new schema file
Created attachment 78348 [details] [review] Bug 18887: Update existing schema files
Created attachment 78349 [details] [review] Bug 18887: Add Koha::CirculationRule(s)
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
Created attachment 78351 [details] [review] Bug 18887: (follow-up) Fix behavior for default branch/category Also, fix a couple small QA issues.
Created attachment 78352 [details] [review] Bug 18887: (follow-up) Fix null/empty behavior
Created attachment 78353 [details] [review] Bug 18887: Fix issues caused by rebase
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)
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
Created attachment 78356 [details] [review] Bug 18887: Use get_effective_rule instead of search
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
(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?
(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 :-(
CONFLICT (content): Merge conflict in t/db_dependent/Holds.t Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt
Created attachment 78674 [details] [review] Bug 18887: Update database
Created attachment 78675 [details] [review] Bug 18887: Add new schema file
Created attachment 78676 [details] [review] Bug 18887: Update existing schema files
Created attachment 78677 [details] [review] Bug 18887: Add Koha::CirculationRule(s)
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
Created attachment 78679 [details] [review] Bug 18887: (follow-up) Fix behavior for default branch/category Also, fix a couple small QA issues.
Created attachment 78680 [details] [review] Bug 18887: (follow-up) Fix null/empty behavior
Created attachment 78681 [details] [review] Bug 18887: Fix issues caused by rebase
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)
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
Created attachment 78684 [details] [review] Bug 18887: Use get_effective_rule instead of search
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
Created attachment 78743 [details] [review] Bug 18887: Update database
Created attachment 78744 [details] [review] Bug 18887: Add new schema file
Created attachment 78745 [details] [review] Bug 18887: Update existing schema files
Created attachment 78746 [details] [review] Bug 18887: Add Koha::CirculationRule(s)
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
Created attachment 78748 [details] [review] Bug 18887: (follow-up) Fix behavior for default branch/category Also, fix a couple small QA issues.
Created attachment 78749 [details] [review] Bug 18887: (follow-up) Fix null/empty behavior
Created attachment 78750 [details] [review] Bug 18887: Fix issues caused by rebase
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)
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
Created attachment 78753 [details] [review] Bug 18887: Use get_effective_rule instead of search Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>
Created attachment 78879 [details] [review] Bug 18887: Add 2 missing html filters
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 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 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
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
Created attachment 79636 [details] [review] Bug 18887: Add foreign keys
Created attachment 79637 [details] [review] Bug 18887: Add new schema file
Created attachment 79638 [details] [review] Bug 18887: Update existing schema files
Created attachment 79639 [details] [review] Bug 18887: Fix POD c/p issues
Created attachment 79640 [details] [review] Bug 18887: Replace croak with exceptions
Created attachment 79641 [details] [review] Bug 18887: Add tests
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!
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
Created attachment 79771 [details] [review] Bug 18887: Add foreign keys Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 79772 [details] [review] Bug 18887: Add new schema file Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 79773 [details] [review] Bug 18887: Update existing schema files Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 79774 [details] [review] Bug 18887: Fix POD c/p issues Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 79775 [details] [review] Bug 18887: Replace croak with exceptions Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 79776 [details] [review] Bug 18887: Add tests Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
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>
(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!
Created attachment 79891 [details] [review] Bug 18887: Make DB update idempotent
Awesome work all! Pushed to master for 18.11
Enhancement, will not be backported to 18.05.x series.
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.
Are you working on a fix for this Jonathan.. or have you opened a followup bug?
(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?
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.