Bug 8369 - default_branch_circ_rule and default_circ_rules tables useless
Summary: default_branch_circ_rule and default_circ_rules tables useless
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Bugs List
QA Contact:
URL:
Keywords:
Depends on:
Blocks: 8362
  Show dependency treegraph
 
Reported: 2012-07-05 13:51 UTC by Paul Poulain
Modified: 2021-06-14 21:29 UTC (History)
9 users (show)

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


Attachments
Bug 8369: Merge tables for circulation rules. (31.95 KB, patch)
2012-09-12 13:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8369: Followup: indent sql queries in admin/smart-rules.pl (12.47 KB, patch)
2012-09-12 13:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8369: Merge tables for circulation rules. (31.61 KB, patch)
2012-11-02 14:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8369: Followup: indent sql queries in admin/smart-rules.pl (12.47 KB, patch)
2012-11-02 14:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8369: Merge tables for circulation rules. (31.64 KB, patch)
2012-11-13 04:57 UTC, Mason James
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8369: Merge tables for circulation rules. (31.66 KB, patch)
2012-12-07 15:19 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8369: Followup: indent sql queries in admin/smart-rules.pl (12.52 KB, patch)
2012-12-07 15:19 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9116: Remove some useless tmp variables (5.51 KB, patch)
2013-01-30 06:01 UTC, Mason James
Details | Diff | Splinter Review
Bug 8369: Merge tables for circulation rules. (32.03 KB, patch)
2013-01-30 06:03 UTC, Mason James
Details | Diff | Splinter Review
Bug 8369: Followup: indent sql queries in admin/smart-rules.pl (12.63 KB, patch)
2013-01-30 06:03 UTC, Mason James
Details | Diff | Splinter Review
Bug 8369: Merge tables for circulation rules. (32.03 KB, patch)
2013-08-21 10:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8369: Followup: indent sql queries in admin/smart-rules.pl (12.66 KB, patch)
2013-08-21 10:14 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Paul Poulain 2012-07-05 13:51:31 UTC
Issuingrules uses '*' as a wildcard to say "default".
The branch_borrower_circ_rules and default_branch_circ_rules could use the same method to define the default behaviour.

It means the tables default_branch_circ_rule and default_circ_rules would become useless and the behaviour more consistent
Comment 1 Jonathan Druart 2012-09-12 13:14:42 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2012-09-12 13:14:47 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2012-09-21 14:16:09 UTC
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.
Comment 4 Jonathan Druart 2012-10-16 09:45:41 UTC
(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.
Comment 5 Kyle M Hall 2012-10-19 12:33:21 UTC
> 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.

It's likely a configuration issue on my end then. I'll do a clean install and retry.
Comment 6 Jared Camins-Esakov 2012-11-01 19:49:32 UTC
This seems like too large a change for this close to the 3.10 release. Will hold until after the release.
Comment 7 Kyle M Hall 2012-11-02 14:02:56 UTC
CONFLICT (content): Merge conflict in admin/smart-rules.pl
Comment 8 Jonathan Druart 2012-11-02 14:21:39 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2012-11-02 14:21:44 UTC Comment hidden (obsolete)
Comment 10 Mason James 2012-11-13 04:57:04 UTC Comment hidden (obsolete)
Comment 11 Mason James 2012-11-13 04:58:35 UTC
(In reply to comment #10)
> Created attachment 13416 [details] [review]
> 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
Comment 12 Kyle M Hall 2012-12-07 15:19:36 UTC Comment hidden (obsolete)
Comment 13 Kyle M Hall 2012-12-07 15:19:52 UTC Comment hidden (obsolete)
Comment 14 Mason James 2013-01-30 06:01:09 UTC Comment hidden (obsolete)
Comment 15 Mason James 2013-01-30 06:03:08 UTC Comment hidden (obsolete)
Comment 16 Mason James 2013-01-30 06:03:24 UTC Comment hidden (obsolete)
Comment 17 Mason James 2013-01-30 06:07:50 UTC
(In reply to comment #16)
> Created attachment 14931 [details] [review]
> 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')
Comment 18 Jared Camins-Esakov 2013-02-01 01:23:42 UTC
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.
Comment 19 Jonathan Druart 2013-02-01 15:03:25 UTC
(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 :-/
Comment 20 Jonathan Druart 2013-02-01 15:08:15 UTC
(In reply to comment #19)
> (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 :-/

But perhaps I can use a multiple unique key column on them
Comment 21 Jonathan Druart 2013-02-01 15:15:35 UTC
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?
Comment 22 Jonathan Druart 2013-02-19 09:35:31 UTC
Jared, do you agree with my consistent argument?
Comment 23 Jonathan Druart 2013-08-21 10:14:11 UTC
Created attachment 20532 [details] [review]
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>
Comment 24 Jonathan Druart 2013-08-21 10:14:17 UTC
Created attachment 20533 [details] [review]
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>
Comment 25 Jonathan Druart 2013-08-21 10:18:04 UTC
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!
Comment 26 Galen Charlton 2013-08-21 15:21:28 UTC
(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.
Comment 27 Jonathan Druart 2013-08-22 08:02:47 UTC
(In reply to Galen Charlton from comment #26)
> 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.

The goal of this patch is not to offer something to the final user.
With this approach, the code and sql queries in the smart-rules script are more readable and cleaner. It is enough for me to provide a patch.

> 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.

I will try to propose a followup.

Thanks for your feedback!
Comment 28 Katrin Fischer 2015-06-06 22:46:04 UTC
This is marked 3.12 and hasn't been touched since 08-2013. Should this have another status? Should the patches be obsoleted?
Comment 29 Jonathan Druart 2015-06-16 11:23:04 UTC
(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.
Comment 30 Hayley Pelham 2019-03-12 03:51:59 UTC
It appears that the default_circ_rules table is no longer used in the current development version. Should this table be removed from the database?
Comment 31 Jonathan Druart 2019-04-24 14:03:57 UTC
(In reply to Hayley Mapley from comment #30)
> It appears that the default_circ_rules table is no longer used in the
> current development version. Should this table be removed from the database?

% git grep default_circ_rules
returns a lot of occurrences. This table is still in used ;)

It's going to be removed by bug 15522.
Comment 32 Katrin Fischer 2019-04-24 16:29:01 UTC
Should we close the bug? Or mark as dependency?
Comment 33 Jonathan Druart 2019-04-25 22:10:57 UTC
(In reply to Katrin Fischer from comment #32)
> Should we close the bug? Or mark as dependency?

I would mark as fixed once done.
Comment 34 Jonathan Druart 2020-08-06 10:34:54 UTC
Tables removed by bug 18928.