Description
Paul Poulain
2012-07-05 13:51:31 UTC
Created attachment 12166 [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. Created attachment 12167 [details] [review] Bug 8369: Followup: indent sql queries in admin/smart-rules.pl 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. (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. > 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.
This seems like too large a change for this close to the 3.10 release. Will hold until after the release. CONFLICT (content): Merge conflict in admin/smart-rules.pl Created attachment 13229 [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. Created attachment 13230 [details] [review] Bug 8369: Followup: indent sql queries in admin/smart-rules.pl Created attachment 13416 [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. (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 Created attachment 13929 [details] [review] [SIGNED-OFF] 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> Created attachment 13930 [details] [review] [SIGNED-OFF] Bug 8369: Followup: indent sql queries in admin/smart-rules.pl Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 14929 [details] [review] Bug 9116: Remove some useless tmp variables Some variables are useless in C4::Items::PrepareItemrecordDisplay. This patch removes its. Test plan: Syspref AcqCreateItem = "receive an order" and try to receive an order. Check there is no regression. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Created attachment 14930 [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> 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> (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') 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. (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 :-/ (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 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? Jared, do you agree with my consistent argument? 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> 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> 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! (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. (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! This is marked 3.12 and hasn't been touched since 08-2013. Should this have another status? Should the patches be obsoleted? (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. 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? (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. Should we close the bug? Or mark as dependency? (In reply to Katrin Fischer from comment #32) > Should we close the bug? Or mark as dependency? I would mark as fixed once done. Tables removed by bug 18928. |