Description
Kyle M Hall (khall)
2017-07-10 15:35:18 UTC
Created attachment 65238 [details] [review] Bug 18925 - Move maxissueqty and maxonsiteissueqty to circulation_rules This patch set moves maxissueqty and maxonsiteissueqty to the circulation_rules table. Test Plan: 1) Apply this patch 2) Run updatedatabase 3) prove t/db_dependent/Circulation.t 4) prove t/db_dependent/Circulation/Branch.t 5) prove t/db_dependent/Circulation/GetHardDueDate.t 6) prove t/db_dependent/Circulation/Returns.t 7) prove t/db_dependent/Circulation/SwitchOnSiteCheckouts.t 8) prove t/db_dependent/Circulation/TooMany.t 9) prove t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t 10) prove t/db_dependent/Reserves.t 11) Note no changes in circulation behavior related to check out limis both on and off site Created attachment 65239 [details] [review] Bug 18925 - Update Schema Files Created attachment 65240 [details] [review] Bug 18925 - Remove schema files for deleted tables Created attachment 67024 [details] [review] Bug 18925 - Move maxissueqty and maxonsiteissueqty to circulation_rules This patch set moves maxissueqty and maxonsiteissueqty to the circulation_rules table. Test Plan: 1) Apply this patch 2) Run updatedatabase 3) prove t/db_dependent/Circulation.t 4) prove t/db_dependent/Circulation/Branch.t 5) prove t/db_dependent/Circulation/GetHardDueDate.t 6) prove t/db_dependent/Circulation/Returns.t 7) prove t/db_dependent/Circulation/SwitchOnSiteCheckouts.t 8) prove t/db_dependent/Circulation/TooMany.t 9) prove t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t 10) prove t/db_dependent/Reserves.t 11) Note no changes in circulation behavior related to check out limis both on and off site Created attachment 67025 [details] [review] Bug 18925 - Update Schema Files Created attachment 67026 [details] [review] Bug 18925 - Remove schema files for deleted tables Rebased. Created attachment 67621 [details] [review] Bug 18925: (follow-up) Change name of rule to fix ambiguity There was previously an ambiguity between the branch/category/itemtype specific max{,onsite}issueqty and the total-per-patron max{,onsite}issueqty. The latter has been renamed to patron_max{,onsite}issueqty. Created attachment 70992 [details] [review] Bug 18925 - Move maxissueqty and maxonsiteissueqty to circulation_rules This patch set moves maxissueqty and maxonsiteissueqty to the circulation_rules table. Test Plan: 1) Apply this patch 2) Run updatedatabase 3) prove t/db_dependent/Circulation.t 4) prove t/db_dependent/Circulation/Branch.t 5) prove t/db_dependent/Circulation/GetHardDueDate.t 6) prove t/db_dependent/Circulation/Returns.t 7) prove t/db_dependent/Circulation/SwitchOnSiteCheckouts.t 8) prove t/db_dependent/Circulation/TooMany.t 9) prove t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t 10) prove t/db_dependent/Reserves.t 11) Note no changes in circulation behavior related to check out limis both on and off site Created attachment 70993 [details] [review] Bug 18925 - Update Schema Files Created attachment 70994 [details] [review] Bug 18925 - Remove schema files for deleted tables Created attachment 70995 [details] [review] Bug 18925: (follow-up) Change name of rule to fix ambiguity There was previously an ambiguity between the branch/category/itemtype specific max{,onsite}issueqty and the total-per-patron max{,onsite}issueqty. The latter has been renamed to patron_max{,onsite}issueqty. Created attachment 70996 [details] [review] Bug 18925: (follow-up) Change name of rule to fix ambiguity There was previously an ambiguity between the branch/category/itemtype specific max{,onsite}issueqty and the total-per-patron max{,onsite}issueqty. The latter has been renamed to patron_max{,onsite}issueqty. Created attachment 71032 [details] [review] Bug 18925 - Move maxissueqty and maxonsiteissueqty to circulation_rules This patch set moves maxissueqty and maxonsiteissueqty to the circulation_rules table. Test Plan: 1) Apply this patch 2) Run updatedatabase 3) prove t/db_dependent/Circulation.t 4) prove t/db_dependent/Circulation/Branch.t 5) prove t/db_dependent/Circulation/GetHardDueDate.t 6) prove t/db_dependent/Circulation/Returns.t 7) prove t/db_dependent/Circulation/SwitchOnSiteCheckouts.t 8) prove t/db_dependent/Circulation/TooMany.t 9) prove t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t 10) prove t/db_dependent/Reserves.t 11) Note no changes in circulation behavior related to check out limis both on and off site Created attachment 71033 [details] [review] Bug 18925: (follow-up) Fix null/empty behavior Created attachment 71043 [details] [review] Bug 18925: Move maxissueqty and maxonsiteissueqty to circulation_rules This patch set moves maxissueqty and maxonsiteissueqty to the circulation_rules table. Test Plan: 1) Apply this patch 2) Run updatedatabase 3) prove t/db_dependent/Circulation.t 4) prove t/db_dependent/Circulation/Branch.t 5) prove t/db_dependent/Circulation/GetHardDueDate.t 6) prove t/db_dependent/Circulation/Returns.t 7) prove t/db_dependent/Circulation/SwitchOnSiteCheckouts.t 8) prove t/db_dependent/Circulation/TooMany.t 9) prove t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t 10) prove t/db_dependent/Reserves.t 11) Note no changes in circulation behavior related to check out limis both on and off site Created attachment 71044 [details] [review] Bug 18925: Update schema files Created attachment 71045 [details] [review] Bug 18925: Remove schema files for deleted tables Created attachment 71046 [details] [review] Bug 18925: (follow-up) Change name of rule to fix ambiguity There was previously an ambiguity between the branch/category/itemtype specific max{,onsite}issueqty and the total-per-patron max{,onsite}issueqty. The latter has been renamed to patron_max{,onsite}issueqty. Created attachment 71047 [details] [review] Bug 18925: (follow-up) Fix null/empty behavior Created attachment 72225 [details] [review] Bug 18925: Move maxissueqty and maxonsiteissueqty to circulation_rules This patch set moves maxissueqty and maxonsiteissueqty to the circulation_rules table. Test Plan: 1) Apply this patch 2) Run updatedatabase 3) prove t/db_dependent/Circulation.t 4) prove t/db_dependent/Circulation/Branch.t 5) prove t/db_dependent/Circulation/GetHardDueDate.t 6) prove t/db_dependent/Circulation/Returns.t 7) prove t/db_dependent/Circulation/SwitchOnSiteCheckouts.t 8) prove t/db_dependent/Circulation/TooMany.t 9) prove t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t 10) prove t/db_dependent/Reserves.t 11) Note no changes in circulation behavior related to check out limis both on and off site Created attachment 72226 [details] [review] Bug 18925: Update schema files Created attachment 72227 [details] [review] Bug 18925: Remove schema files for deleted tables Created attachment 72228 [details] [review] Bug 18925: (follow-up) Change name of rule to fix ambiguity There was previously an ambiguity between the branch/category/itemtype specific max{,onsite}issueqty and the total-per-patron max{,onsite}issueqty. The latter has been renamed to patron_max{,onsite}issueqty. Created attachment 72229 [details] [review] Bug 18925: (follow-up) Fix null/empty behavior Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 18925: Move maxissueqty and maxonsiteissueqty to circulation_rules fatal: sha1 information is lacking or useless (Koha/CirculationRules.pm). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 Bug 18925: Move maxissueqty and maxonsiteissueqty to circulation_rules The copy of the patch that failed is found in: /var/repositories/koha/.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-18925-Move-maxissueqty-and-maxonsiteissueqty-t-lf7fNZ.patch Created attachment 77225 [details] [review] Bug 18925: Move maxissueqty and maxonsiteissueqty to circulation_rules This patch set moves maxissueqty and maxonsiteissueqty to the circulation_rules table. Test Plan: 1) Apply this patch 2) Run updatedatabase 3) prove t/db_dependent/Circulation.t 4) prove t/db_dependent/Circulation/Branch.t 5) prove t/db_dependent/Circulation/GetHardDueDate.t 6) prove t/db_dependent/Circulation/Returns.t 7) prove t/db_dependent/Circulation/SwitchOnSiteCheckouts.t 8) prove t/db_dependent/Circulation/TooMany.t 9) prove t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t 10) prove t/db_dependent/Reserves.t 11) Note no changes in circulation behavior related to check out limis both on and off site Created attachment 77226 [details] [review] Bug 18925: Update schema files Created attachment 77227 [details] [review] Bug 18925: Remove schema files for deleted tables Created attachment 77228 [details] [review] Bug 18925: (follow-up) Change name of rule to fix ambiguity There was previously an ambiguity between the branch/category/itemtype specific max{,onsite}issueqty and the total-per-patron max{,onsite}issueqty. The latter has been renamed to patron_max{,onsite}issueqty. Created attachment 77229 [details] [review] Bug 18925: (follow-up) Fix null/empty behavior Rebased. Blocked for now. I will submit a follow-up when I will get an answer on bug 18887 comment 48. Note: C4::Circulation::GetBranchBorrowerCircRule 1640 my $rule = Koha::CirculationRules->search( should certainly be replaced by Koha::CirculationRules->get_effective_rule Created attachment 78881 [details] [review] Bug 18925: Move maxissueqty and maxonsiteissueqty to circulation_rules This patch set moves maxissueqty and maxonsiteissueqty to the circulation_rules table. Test Plan: 1) Apply this patch 2) Run updatedatabase 3) prove t/db_dependent/Circulation.t 4) prove t/db_dependent/Circulation/Branch.t 5) prove t/db_dependent/Circulation/GetHardDueDate.t 6) prove t/db_dependent/Circulation/Returns.t 7) prove t/db_dependent/Circulation/SwitchOnSiteCheckouts.t 8) prove t/db_dependent/Circulation/TooMany.t 9) prove t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t 10) prove t/db_dependent/Reserves.t 11) Note no changes in circulation behavior related to check out limis both on and off site Created attachment 78882 [details] [review] Bug 18925: Update schema files Created attachment 78883 [details] [review] Bug 18925: Remove schema files for deleted tables Created attachment 78884 [details] [review] Bug 18925: (follow-up) Change name of rule to fix ambiguity There was previously an ambiguity between the branch/category/itemtype specific max{,onsite}issueqty and the total-per-patron max{,onsite}issueqty. The latter has been renamed to patron_max{,onsite}issueqty. Created attachment 78885 [details] [review] Bug 18925: (follow-up) Fix null/empty behavior Created attachment 78886 [details] [review] Bug 18925: Replace logic in GetBranchBorrowerCircRule with get_effective_rule Created attachment 79642 [details] [review] Bug 18925: Move maxissueqty and maxonsiteissueqty to circulation_rules This patch set moves maxissueqty and maxonsiteissueqty to the circulation_rules table. Test Plan: 1) Apply this patch 2) Run updatedatabase 3) prove t/db_dependent/Circulation.t 4) prove t/db_dependent/Circulation/Branch.t 5) prove t/db_dependent/Circulation/GetHardDueDate.t 6) prove t/db_dependent/Circulation/Returns.t 7) prove t/db_dependent/Circulation/SwitchOnSiteCheckouts.t 8) prove t/db_dependent/Circulation/TooMany.t 9) prove t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t 10) prove t/db_dependent/Reserves.t 11) Note no changes in circulation behavior related to check out limis both on and off site Created attachment 79643 [details] [review] Bug 18925: (follow-up) Change name of rule to fix ambiguity There was previously an ambiguity between the branch/category/itemtype specific max{,onsite}issueqty and the total-per-patron max{,onsite}issueqty. The latter has been renamed to patron_max{,onsite}issueqty. Created attachment 79644 [details] [review] Bug 18925: (follow-up) Fix null/empty behavior Created attachment 79645 [details] [review] Bug 18925: Replace logic in GetBranchBorrowerCircRule with get_effective_rule Created attachment 79646 [details] [review] Bug 18925: Update schema files Rebased on top of new patches submitted on bug 18887. Created attachment 81091 [details] [review] Bug 18925: Move maxissueqty and maxonsiteissueqty to circulation_rules This patch set moves maxissueqty and maxonsiteissueqty to the circulation_rules table. Test Plan: 1) Apply this patch 2) Run updatedatabase 3) prove t/db_dependent/Circulation.t 4) prove t/db_dependent/Circulation/Branch.t 5) prove t/db_dependent/Circulation/GetHardDueDate.t 6) prove t/db_dependent/Circulation/Returns.t 7) prove t/db_dependent/Circulation/SwitchOnSiteCheckouts.t 8) prove t/db_dependent/Circulation/TooMany.t 9) prove t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t 10) prove t/db_dependent/Reserves.t 11) Note no changes in circulation behavior related to check out limis both on and off site Created attachment 81092 [details] [review] Bug 18925: (follow-up) Change name of rule to fix ambiguity There was previously an ambiguity between the branch/category/itemtype specific max{,onsite}issueqty and the total-per-patron max{,onsite}issueqty. The latter has been renamed to patron_max{,onsite}issueqty. Created attachment 81093 [details] [review] Bug 18925: (follow-up) Fix null/empty behavior Created attachment 81094 [details] [review] Bug 18925: Replace logic in GetBranchBorrowerCircRule with get_effective_rule Created attachment 81095 [details] [review] Bug 18925: Update schema files Created attachment 82927 [details] [review] Bug 18925: Move maxissueqty and maxonsiteissueqty to circulation_rules This patch set moves maxissueqty and maxonsiteissueqty to the circulation_rules table. Test Plan: 1) Apply this patch 2) Run updatedatabase 3) prove t/db_dependent/Circulation.t 4) prove t/db_dependent/Circulation/Branch.t 5) prove t/db_dependent/Circulation/GetHardDueDate.t 6) prove t/db_dependent/Circulation/Returns.t 7) prove t/db_dependent/Circulation/SwitchOnSiteCheckouts.t 8) prove t/db_dependent/Circulation/TooMany.t 9) prove t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t 10) prove t/db_dependent/Reserves.t 11) Note no changes in circulation behavior related to check out limis both on and off site Created attachment 82928 [details] [review] Bug 18925: (follow-up) Change name of rule to fix ambiguity There was previously an ambiguity between the branch/category/itemtype specific max{,onsite}issueqty and the total-per-patron max{,onsite}issueqty. The latter has been renamed to patron_max{,onsite}issueqty. Created attachment 82929 [details] [review] Bug 18925: (follow-up) Fix null/empty behavior Created attachment 82930 [details] [review] Bug 18925: Replace logic in GetBranchBorrowerCircRule with get_effective_rule Created attachment 82931 [details] [review] Bug 18925: Update schema files Attempted rebase due to bug 21753 - got tests failures after that Created attachment 85480 [details] [review] Bug 18925: Move maxissueqty and maxonsiteissueqty to circulation_rules This patch set moves maxissueqty and maxonsiteissueqty to the circulation_rules table. Test Plan: 1) Apply this patch 2) Run updatedatabase 3) prove t/db_dependent/Circulation.t 4) prove t/db_dependent/Circulation/Branch.t 5) prove t/db_dependent/Circulation/GetHardDueDate.t 6) prove t/db_dependent/Circulation/Returns.t 7) prove t/db_dependent/Circulation/SwitchOnSiteCheckouts.t 8) prove t/db_dependent/Circulation/TooMany.t 9) prove t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t 10) prove t/db_dependent/Reserves.t 11) Note no changes in circulation behavior related to check out limis both on and off site Created attachment 85481 [details] [review] Bug 18925: (follow-up) Change name of rule to fix ambiguity There was previously an ambiguity between the branch/category/itemtype specific max{,onsite}issueqty and the total-per-patron max{,onsite}issueqty. The latter has been renamed to patron_max{,onsite}issueqty. Created attachment 85482 [details] [review] Bug 18925: (follow-up) Fix null/empty behavior Created attachment 85483 [details] [review] Bug 18925: Replace logic in GetBranchBorrowerCircRule with get_effective_rule Created attachment 85484 [details] [review] Bug 18925: Update schema files Created attachment 85485 [details] [review] Bug 18925: Fix null/empty behavior, again Created attachment 85486 [details] [review] Bug 18925: Update new occurrences Do we need a fix for bug 18887 comment 143 in order to move on with this? All looks good to me. The only doubt is why aren't the maxissueqty and maxonsiteissureqty columns removed from issuingrules. Created attachment 85939 [details] [review] Bug 18925: Move maxissueqty and maxonsiteissueqty to circulation_rules This patch set moves maxissueqty and maxonsiteissueqty to the circulation_rules table. Test Plan: 1) Apply this patch 2) Run updatedatabase 3) prove t/db_dependent/Circulation.t 4) prove t/db_dependent/Circulation/Branch.t 5) prove t/db_dependent/Circulation/GetHardDueDate.t 6) prove t/db_dependent/Circulation/Returns.t 7) prove t/db_dependent/Circulation/SwitchOnSiteCheckouts.t 8) prove t/db_dependent/Circulation/TooMany.t 9) prove t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t 10) prove t/db_dependent/Reserves.t 11) Note no changes in circulation behavior related to check out limis both on and off site Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 85940 [details] [review] Bug 18925: (follow-up) Change name of rule to fix ambiguity There was previously an ambiguity between the branch/category/itemtype specific max{,onsite}issueqty and the total-per-patron max{,onsite}issueqty. The latter has been renamed to patron_max{,onsite}issueqty. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 85941 [details] [review] Bug 18925: (follow-up) Fix null/empty behavior Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 85942 [details] [review] Bug 18925: Replace logic in GetBranchBorrowerCircRule with get_effective_rule Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 85943 [details] [review] Bug 18925: Update schema files Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 85944 [details] [review] Bug 18925: Fix null/empty behavior, again Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 85945 [details] [review] Bug 18925: Update new occurrences Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 85946 [details] [review] Bug 18925: (QA follow-up) Make DB update idempotent Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 85947 [details] [review] Bug 18925: (QA follow-up) Fix indentation characters in smart-rules.tt Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Everything looks great. Minor stuffs: - The question about issuingrules. - Related to issuingrules: basic_workflow.t needs to be adjusted to use the new set_rule method for maxissueqty Created attachment 85948 [details] [review] Bug 18925: (QA follow-up) Fix basic_workflow.t Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 85949 [details] [review] Bug 18925: Move existing rules Created attachment 85950 [details] [review] Bug 18925: (follow-up) DBIC schema changes Created attachment 85951 [details] [review] Bug 18925: Fix 'Default checkout, hold policy by patron category' block We want ->Search to return undef if the rule does not exist. (In reply to Tomás Cohen Arazi from comment #75) > Everything looks great. Minor stuffs: > > - The question about issuingrules. > - Related to issuingrules: basic_workflow.t needs to be adjusted to use the > new set_rule method for maxissueqty Indeed, thanks for catching that! Created attachment 85953 [details] [review] Bug 18925: Move maxissueqty and maxonsiteissueqty to circulation_rules This patch set moves maxissueqty and maxonsiteissueqty to the circulation_rules table. Test Plan: 1) Apply this patch 2) Run updatedatabase 3) prove t/db_dependent/Circulation.t 4) prove t/db_dependent/Circulation/Branch.t 5) prove t/db_dependent/Circulation/GetHardDueDate.t 6) prove t/db_dependent/Circulation/Returns.t 7) prove t/db_dependent/Circulation/SwitchOnSiteCheckouts.t 8) prove t/db_dependent/Circulation/TooMany.t 9) prove t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t 10) prove t/db_dependent/Reserves.t 11) Note no changes in circulation behavior related to check out limis both on and off site Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 85954 [details] [review] Bug 18925: (follow-up) Change name of rule to fix ambiguity There was previously an ambiguity between the branch/category/itemtype specific max{,onsite}issueqty and the total-per-patron max{,onsite}issueqty. The latter has been renamed to patron_max{,onsite}issueqty. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 85955 [details] [review] Bug 18925: (follow-up) Fix null/empty behavior Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 85956 [details] [review] Bug 18925: Replace logic in GetBranchBorrowerCircRule with get_effective_rule Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 85957 [details] [review] Bug 18925: Update schema files Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 85958 [details] [review] Bug 18925: Fix null/empty behavior, again Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 85959 [details] [review] Bug 18925: Update new occurrences Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 85960 [details] [review] Bug 18925: (QA follow-up) Make DB update idempotent Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 85961 [details] [review] Bug 18925: (QA follow-up) Fix indentation characters in smart-rules.tt Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 85962 [details] [review] Bug 18925: (QA follow-up) Fix basic_workflow.t Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 85963 [details] [review] Bug 18925: Move existing rules Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 85964 [details] [review] Bug 18925: (follow-up) DBIC schema changes Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 85965 [details] [review] Bug 18925: Fix 'Default checkout, hold policy by patron category' block We want ->Search to return undef if the rule does not exist. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 86068 [details] [review] Bug 18925: Move maxissueqty and maxonsiteissueqty to circulation_rules This patch set moves maxissueqty and maxonsiteissueqty to the circulation_rules table. Test Plan: 1) Apply this patch 2) Run updatedatabase 3) prove t/db_dependent/Circulation.t 4) prove t/db_dependent/Circulation/Branch.t 5) prove t/db_dependent/Circulation/GetHardDueDate.t 6) prove t/db_dependent/Circulation/Returns.t 7) prove t/db_dependent/Circulation/SwitchOnSiteCheckouts.t 8) prove t/db_dependent/Circulation/TooMany.t 9) prove t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t 10) prove t/db_dependent/Reserves.t 11) Note no changes in circulation behavior related to check out limis both on and off site Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 86069 [details] [review] Bug 18925: (follow-up) Change name of rule to fix ambiguity There was previously an ambiguity between the branch/category/itemtype specific max{,onsite}issueqty and the total-per-patron max{,onsite}issueqty. The latter has been renamed to patron_max{,onsite}issueqty. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 86070 [details] [review] Bug 18925: (follow-up) Fix null/empty behavior Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 86071 [details] [review] Bug 18925: Replace logic in GetBranchBorrowerCircRule with get_effective_rule Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 86072 [details] [review] Bug 18925: Update schema files Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 86073 [details] [review] Bug 18925: Fix null/empty behavior, again Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 86074 [details] [review] Bug 18925: Update new occurrences Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 86075 [details] [review] Bug 18925: (QA follow-up) Make DB update idempotent Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 86076 [details] [review] Bug 18925: (QA follow-up) Fix indentation characters in smart-rules.tt Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 86077 [details] [review] Bug 18925: (QA follow-up) Fix basic_workflow.t Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 86078 [details] [review] Bug 18925: Move existing rules Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 86079 [details] [review] Bug 18925: (follow-up) DBIC schema changes Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 86080 [details] [review] Bug 18925: Fix 'Default checkout, hold policy by patron category' block We want ->Search to return undef if the rule does not exist. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 86081 [details] [review] Bug 18925: (QA follow-up) Fix Circulation.t Test plan: t/db_dependent/Circulation.t Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Just a note: The patron_maxissueqty, and maxissueqty rules represents the same thing: it is number of max issues possible. There is no setting from "Default checkout, hold policy by patron category" you can't make in the main circulation rules settings table. But as this is the same on master and as this development leads to revamp circulation rules setting UI, this could be fixed in the future. Hopefully the solution arise during developing/testing... (In reply to Josef Moravec from comment #108) > Just a note: The patron_maxissueqty, and maxissueqty rules represents the > same thing: it is number of max issues possible. > > There is no setting from "Default checkout, hold policy by patron category" > you can't make in the main circulation rules settings table. But as this is > the same on master and as this development leads to revamp circulation rules > setting UI, this could be fixed in the future. Hopefully the solution arise > during developing/testing... Reading your discussion with Joubu in IRC I think this is incorrect. Let say Branch A has rules for 'Patron' catgory: itype = BOOK, Current checkouts allowed = 5 itype = All, Current checkouts allowed = 5 This patron can checkout 5 books and 5 of anything else. If you also set "Default checkout, hold policy by patron category" for 'Patron' to 5 then the patron can only checkout 5 items total (e.g. 3 books, 2 anything else) The top section always checks the most specific rule and 'All' rules are really 'All not defined elsewhere' or 'Default' or 'Generic' - All is really a bad name when you look at it this way Created attachment 86121 [details] [review] Bug 18925: (RM follow-up) Remove issuingrules columns for new installs QA tool complaint: FAIL koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt FAIL filters missing_filter at line 12 ( [% categorycodes.push( pc.id ) %]) Is this a false positive? Or how can we filter on pushing to an array? (In reply to Nick Clemens from comment #111) > QA tool complaint: > FAIL koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt > FAIL filters > missing_filter at line 12 ( [% categorycodes.push( pc.id ) %]) > > Is this a false positive? Or how can we filter on pushing to an array? It is! We shouldn't mess with this cases. Only when displaying. Awesome work all! Pushed to master for 19.05 (In reply to Nick Clemens from comment #113) > Awesome work all! > > Pushed to master for 19.05 \o/ (In reply to Tomás Cohen Arazi from comment #114) > (In reply to Nick Clemens from comment #113) > > Awesome work all! > > > > Pushed to master for 19.05 > > \o/ \o/ from me as well! Created attachment 86165 [details] [review] Bug 18925: (QA follow-up) Remove weird 'Null' values for integer column Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> 2 new commits added to misc4dev to insert the circ rules correctly: commit 8eb9c2489ce376e719fcc0a9a1549c83207a8a20 Fix insert circ rules since bug 18925 And its followup... commit 02e6701eb923947a3939e27884c354dc4cb05401 (follow-up) Fix insert circ rules since bug 18925 (In reply to Tomás Cohen Arazi from comment #116) > Created attachment 86165 [details] [review] [review] > Bug 18925: (QA follow-up) Remove weird 'Null' values for integer column > > Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> As 0 is the default value defined at DBMS level we could have removed the lines :) (In reply to Jonathan Druart from comment #118) > (In reply to Tomás Cohen Arazi from comment #116) > > Created attachment 86165 [details] [review] [review] [review] > > Bug 18925: (QA follow-up) Remove weird 'Null' values for integer column > > > > Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> > > As 0 is the default value defined at DBMS level we could have removed the > lines :) Sure, I prefered to make it explicit. (In reply to Nick Clemens from comment #111) > QA tool complaint: > FAIL koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt > FAIL filters > missing_filter at line 12 ( [% categorycodes.push( pc.id ) %]) > > Is this a false positive? Or how can we filter on pushing to an array? See bug 22466. Created attachment 86206 [details] [review] Bug 18925: (RM follow-up) Remove schema files for deletedtables Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 86207 [details] [review] Bug 18925: (RM follow-up) Remove maxissueqty from issuingrule in SIP tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Enhancement will not be backported to 18.11.x series. After updating to 19.05 with this patch included, circulation rules treated "Unlimited" in "Current checkouts allowed" and "Current on-site checkouts allowed" as 0 (zero) instead of infinite. Attempting to checkout items with an "Unlimited" maxissueqty or maxonsiteissueqty resulted in the error "• Too many checked out. 0 checked out, only are allowed.• This patron can't check out this item per library circulation policy" Circulation was allowed again after changing "Unlimited" to a numerical value. Currently, leaving the "Current checkouts allowed"/maxissueqty and "Current on-site checkouts allowed"/maxonsiteissueqty blank defaults to "Unlimited". Should either the default value be changed, or the code changed to treat the default value as infinite again, instead of zero? (In reply to hannah.co from comment #124) > After updating to 19.05 with this patch included, circulation rules treated > "Unlimited" in "Current checkouts allowed" and "Current on-site checkouts > allowed" as 0 (zero) instead of infinite. Attempting to checkout items with > an "Unlimited" maxissueqty or maxonsiteissueqty resulted in the error "• Too > many checked out. 0 checked out, only are allowed.• This patron can't check > out this item per library circulation policy" > > Circulation was allowed again after changing "Unlimited" to a numerical > value. > > Currently, leaving the "Current checkouts allowed"/maxissueqty and "Current > on-site checkouts allowed"/maxonsiteissueqty blank defaults to "Unlimited". > Should either the default value be changed, or the code changed to treat the > default value as infinite again, instead of zero? I have opened bug 23104 |