http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8369 --- Comment #1 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 12166 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=12166&action=edit Bug 8369: Merge tables for circulation rules. This patch adds 3 new tables: - circ_rules - borrower_circ_rules - item_circ_rules It removes 6 tables: - default_borrower_circ_rules - default_branch_circ_rules - default_branch_item_rules - default_circ_rules - branch_borrower_circ_rules - branch_item_rules The goal is to merge circulations rules into 3 tables instead of 6 without add or remove features. Previous behaviours do not change. The wildcard '*' character is a rule for the default values. Then it is possible to merge 2 tables into one if we consider that the default value is the value with a branchcode eq '*'. I removed the foreign key constraint with branchcode and added 3 queries in the C4::Branch::DelBranch routine in order to remove records about a deleted branch. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8369 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff CC| |jonathan.druart@biblibre.co | |m Assignee|koha-bugs@lists.koha-commun |jonathan.druart@biblibre.co |ity.org |m -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8369 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff --- Comment #4 from Jonathan Druart <jonathan.druart@biblibre.com> --- (In reply to comment #3)

The "Default checkout, hold and return policy for all libraries" and "Checkout limit by patron category for all libraries" rules do not appear to be working for me. I tried both with a limit of 1, and was able to check out 2 items to a patron with no warnings.

Hi Kyle, Thank you for testing. I can't reproduce your issue. I retested with "Total current checkouts allowed" = 1 for all libraries and I got a message "Too many checked out. 1 checked out, only 1 are allowed." after my second checkout. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8369 Jared Camins-Esakov <jcamins@cpbibliography.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version|master |rel_3_12 --- Comment #6 from Jared Camins-Esakov <jcamins@cpbibliography.com> --- This seems like too large a change for this close to the 3.10 release. Will hold until after the release. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8369 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #12166|0 |1 is obsolete| | Attachment #12167|0 |1 is obsolete| | --- Comment #8 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 13229 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=13229&action=edit Bug 8369: Merge tables for circulation rules. This patch adds 3 new tables: - circ_rules - borrower_circ_rules - item_circ_rules It removes 6 tables: - default_borrower_circ_rules - default_branch_circ_rules - default_branch_item_rules - default_circ_rules - branch_borrower_circ_rules - branch_item_rules The goal is to merge circulations rules into 3 tables instead of 6 without add or remove features. Previous behaviours do not change. The wildcard '*' character is a rule for the default values. Then it is possible to merge 2 tables into one if we consider that the default value is the value with a branchcode eq '*'. I removed the foreign key constraint with branchcode and added 3 queries in the C4::Branch::DelBranch routine in order to remove records about a deleted branch. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8369 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8369 Mason James <mtj@kohaaloha.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mtj@kohaaloha.com --- Comment #11 from Mason James <mtj@kohaaloha.com> --- (In reply to comment #10)

Created attachment 13416 [details] Bug 8369: Merge tables for circulation rules.

i rebased this against master, after some merge conflicts 3b8a750ee2646b Merge branch 'bug_9039' into 3.12-master -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8369 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #13230|0 |1 is obsolete| | --- Comment #13 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 13930 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=13930&action=edit [SIGNED-OFF] Bug 8369: Followup: indent sql queries in admin/smart-rules.pl Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8369 Mason James <mtj@kohaaloha.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8369 Mason James <mtj@kohaaloha.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #14929|0 |1 is obsolete| | --- Comment #15 from Mason James <mtj@kohaaloha.com> --- Created attachment 14930 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=14930&action=edit Bug 8369: Merge tables for circulation rules. This patch adds 3 new tables: - circ_rules - borrower_circ_rules - item_circ_rules It removes 6 tables: - default_borrower_circ_rules - default_branch_circ_rules - default_branch_item_rules - default_circ_rules - branch_borrower_circ_rules - branch_item_rules The goal is to merge circulations rules into 3 tables instead of 6 without add or remove features. Previous behaviours do not change. The wildcard '*' character is a rule for the default values. Then it is possible to merge 2 tables into one if we consider that the default value is the value with a branchcode eq '*'. I removed the foreign key constraint with branchcode and added 3 queries in the C4::Branch::DelBranch routine in order to remove records about a deleted branch. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Mason James <mtj@kohaaloha.com> -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8369 --- Comment #17 from Mason James <mtj@kohaaloha.com> --- (In reply to comment #16)

Created attachment 14931 [details] Bug 8369: Followup: indent sql queries in admin/smart-rules.pl

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Mason James <mtj@kohaaloha.com>

passing QA... i've had a look at this, and had a bit of a circ test it looks good to me (applied to f8686e9 'Bug 9116: Remove some useless tmp var') -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8369 --- Comment #19 from Jonathan Druart <jonathan.druart@biblibre.com> --- (In reply to comment #18)

Is there a reason we couldn't keep the foreign key constraint and use NULL to mean all branches/all patron types/all item types? It seems a shame to lose the database-enforced consistency.

Yes, it is not possible to have a primary key on branchcode, categorycode and itemtype if they can be NULL :-/ -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8369 --- Comment #21 from Jonathan Druart <jonathan.druart@biblibre.com> --- The '*' is used for the issuingrules table, so why don't use it for theses 3 tables? Don't you think it is more consistent? -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8369 --- Comment #22 from Jonathan Druart <jonathan.druart@biblibre.com> --- Jared, do you agree with my consistent argument? -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8369 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #14931|0 |1 is obsolete| | --- Comment #24 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 20533 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=20533&action=edit Bug 8369: Followup: indent sql queries in admin/smart-rules.pl Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Mason James <mtj@kohaaloha.com> -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8369 Galen Charlton <gmcharlt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |In Discussion --- Comment #26 from Galen Charlton <gmcharlt@gmail.com> --- (In reply to Jonathan Druart from comment #25)

Galen, I change the status to Passed QA. Like that, you will show this report in your list.

I can provide unit tests for changed routines but before, I would like to get your opinion on comment 18-22 and on this change in general. Thanks!

My general opinion of the change as a whole: I would hate to lose the foreign key constraints, the change doesn't seem to have any user-visible benefit, and I don't see that the goal of reducing the number of tables involved is worth the risk of regressions. I'm not fond of the use of '*' in issuing rules, either -- I'd rather that NULL be usable to signal "don't care/matches any" and that brancode, itemtype, and categorycode be FKs -- but composite UKs don't enforce uniqueness if one of the values is NULL. We could dispense with the composite PK on issuingrules, perhaps, but then would have to write more code to ensure that duplicate rows couldn't slip in. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8369 Jacek Ablewicz <abl@biblos.pk.edu.pl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |abl@biblos.pk.edu.pl -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8369 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|jonathan.druart@biblibre.co |koha-bugs@lists.koha-commun |m |ity.org Version|3.12 |master --- Comment #29 from Jonathan Druart <jonathan.druart@biblibre.com> --- (In reply to Katrin Fischer from comment #28)

This is marked 3.12 and hasn't been touched since 08-2013. Should this have another status? Should the patches be obsoleted?

The patches are still a good start and they work. I don't plan to work on this in the near future. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.