Group rules based on item type in order to restrict circulation. In other words, not allow more than x items to be checked out across all media type item types (DVD, VHS, Blue Ray) and not just 4 checkouts per type. Currently patrons can bypass the circulation rules and take out 4 DVDs and 4 VHS and still be within the system limitation but in reality have just taken out 8 media items. This development would allow for the grouping of materials in order to keep that 4 media limit across all types. 1 - Extend the itemtypes table to allow an itemtype to have a parent type 2 - When circulating an item the specific itemtype circulation rules will apply unless unset 3 - For itemtypes with a parent the parent rule will apply when child rule is unset
Created attachment 87713 [details] [review] Bug 21946: Database updates
Created attachment 87714 [details] [review] Bug 21946: SCHEMA UPDATES DO NOT PUSH
Created attachment 87715 [details] [review] Bug 21946: Update C4::Circulation->TooMany to check parent itemtypes To test: 1 - prove -v t/db_dependent/Circulation/TooMany.t
Created attachment 87716 [details] [review] Bug 21946: Update Administration->Item types to llow for defining parent types To test: 1 - Browse to Administration -> Item types 2 - Not a new colum for prent type 3 - Edit an existing type and not you can select any type (except the current) as a parent 4 - Select one 5 - Edit the type you selected as parent 6 - Note it cannot have a parent defined 7 - Edit a new type, note you can select a parent
Created attachment 87717 [details] [review] Bug 21946: Fix display of parent types
Created attachment 87718 [details] [review] Bug 21946: Use objects for displaying circulation rules
Created attachment 87719 [details] [review] Bug 21946: Display parent-child relationship on smart-rules.pl To test: 1 - Set some itemtypes to have a parent 2 - Browse to Administration -> Circulation and fines rules 3 - Note new description of parent/child relationships at top of page 4 - Note that itemtype dropdown for circ rules shows child types under parents 5 - Set a rule for a child type 6 - Note it displays as Parent->Child 7 - Have three child types under a parent 8 - Set the parent 'Current checkouts allowed' to 3 9 - Set the children 'Current checkouts allowed' to: type1 = 2 type2 = 1 type3 = 1 10 - Create some items of the type above 11 - Note you can checkout 2 of type 1, and not 3 12 - Note you can checkout 1 of type 2, but not 2 13 - Note that you now cannot checkout any of type3 14 - Note you cannot checkout any of the parent type 15 - Return one of the other items and note you can now checkout an item of type3 16 - Return another item and note you can checkout an item of the parent type 17 - Return all 18 - Set the parent type to 1 19 - Now note you can only checkout 1 of any of the children 20 - Set the parent to 0 21 - Note you cannot checkout any of the child types
Hi Nick, As you probably expected, I have some comments for you. :D On itemtypes.pl: - Would like to see the parent code column be the parent description instead; optionally, as below, put the parent code in parens. - Potentially, would like to see the ability to add many children to one parent itype. A little on the fence about this. To me it is slightly more logical to go to the parent and add all of the children at once (say, a series of addable dropdowns like the advanced search page has for adding additional search terms) than it is to go to each child and define the parent. - As long as the parents and children are grouped (they appear to be), I think we can get away with omitting the parent in the description, for ex: Mixed Materials | MX | (no parent, this is the parent) -- Maps | MP | Mixed Materials (MX) -- Music | MU | Mixed Materials (MX) -- Visual materials | VM | Mixed Materials (MX) I think that's all I have at the moment. I'm going through this one quite slowly :) Cheers, Liz
Created attachment 88392 [details] [review] Bug 21946: (follow-up) Correct dropdown when editing rule
Hi Nick, As we talked about, this mostly works except that it only restricts items per branch hierarchically. I think you know where to look so I won't try and describe it fully bceause that could be very confusing. Cheers, Liz
Created attachment 88574 [details] [review] Bug 21946: Database updates
Created attachment 88575 [details] [review] Bug 21946: SCHEMA UPDATES DO NOT PUSH
Created attachment 88576 [details] [review] Bug 21946: Update C4::Circulation->TooMany to check parent itemtypes To test: 1 - prove -v t/db_dependent/Circulation/TooMany.t
Created attachment 88577 [details] [review] Bug 21946: Update Administration->Item types to allow for defining parent types To test: 1 - Browse to Administration -> Item types 2 - Not a new colum for prent type 3 - Edit an existing type and not you can select any type (except the current) as a parent 4 - Select one 5 - Edit the type you selected as parent 6 - Note it cannot have a parent defined 7 - Edit a new type, note you can select a parent
Created attachment 88578 [details] [review] Bug 21946: Fix display of parent types
Created attachment 88579 [details] [review] Bug 21946: Use objects for displaying circulation rules
Created attachment 88580 [details] [review] Bug 21946: Display parent-child relationship on smart-rules.pl To test: 1 - Set some itemtypes to have a parent 2 - Browse to Administration -> Circulation and fines rules 3 - Note new description of parent/child relationships at top of page 4 - Note that itemtype dropdown for circ rules shows child types under parents 5 - Set a rule for a child type 6 - Note it displays as Parent->Child 7 - Have three child types under a parent 8 - Set the parent 'Current checkouts allowed' to 3 9 - Set the children 'Current checkouts allowed' to: type1 = 2 type2 = 1 type3 = 1 10 - Create some items of the type above 11 - Note you can checkout 2 of type 1, and not 3 12 - Note you can checkout 1 of type 2, but not 2 13 - Note that you now cannot checkout any of type3 14 - Note you cannot checkout any of the parent type 15 - Return one of the other items and note you can now checkout an item of type3 16 - Return another item and note you can checkout an item of the parent type 17 - Return all 18 - Set the parent type to 1 19 - Now note you can only checkout 1 of any of the children 20 - Set the parent to 0 21 - Note you cannot checkout any of the child types
Created attachment 88581 [details] [review] Bug 21946: (follow-up) Correct dropdown when editing rule
Created attachment 88926 [details] [review] Bug 21946: Database updates Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Created attachment 88927 [details] [review] Bug 21946: SCHEMA UPDATES DO NOT PUSH Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Created attachment 88928 [details] [review] Bug 21946: Update C4::Circulation->TooMany to check parent itemtypes To test: 1 - prove -v t/db_dependent/Circulation/TooMany.t Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Created attachment 88929 [details] [review] Bug 21946: Update Administration->Item types to allow for defining parent types To test: 1 - Browse to Administration -> Item types 2 - Not a new colum for prent type 3 - Edit an existing type and not you can select any type (except the current) as a parent 4 - Select one 5 - Edit the type you selected as parent 6 - Note it cannot have a parent defined 7 - Edit a new type, note you can select a parent Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Created attachment 88930 [details] [review] Bug 21946: Fix display of parent types Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Created attachment 88931 [details] [review] Bug 21946: Use objects for displaying circulation rules Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Created attachment 88932 [details] [review] Bug 21946: Display parent-child relationship on smart-rules.pl To test: 1 - Set some itemtypes to have a parent 2 - Browse to Administration -> Circulation and fines rules 3 - Note new description of parent/child relationships at top of page 4 - Note that itemtype dropdown for circ rules shows child types under parents 5 - Set a rule for a child type 6 - Note it displays as Parent->Child 7 - Have three child types under a parent 8 - Set the parent 'Current checkouts allowed' to 3 9 - Set the children 'Current checkouts allowed' to: type1 = 2 type2 = 1 type3 = 1 10 - Create some items of the type above 11 - Note you can checkout 2 of type 1, and not 3 12 - Note you can checkout 1 of type 2, but not 2 13 - Note that you now cannot checkout any of type3 14 - Note you cannot checkout any of the parent type 15 - Return one of the other items and note you can now checkout an item of type3 16 - Return another item and note you can checkout an item of the parent type 17 - Return all 18 - Set the parent type to 1 19 - Now note you can only checkout 1 of any of the children 20 - Set the parent to 0 21 - Note you cannot checkout any of the child types Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Created attachment 88933 [details] [review] Bug 21946: (follow-up) Correct dropdown when editing rule Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Created attachment 88934 [details] [review] Bug 21946: (QA follow-up) - remove console.log To make QA tools happy Signed-off-by: Liz Rea <wizzyrea@gmail.com>
*** Bug 6685 has been marked as a duplicate of this bug. ***
Quick code review: 1. + my $child_types = Koha::ItemTypes->search( parent_type => $type ); search take a hashef! 2. + $parent_maxissueqty_rule = undef if $parent_maxissueqty_rule && $parent_maxissueqty_rule->itemtype eq '*'; Is that possible? IIRC we had NULL in DB to tell "all", not "*" 3. + if (C4::Context->preference('item-level_itypes')) { + $count_query .= q|SELECT COALESCE( SUM( IF(items.itype = '| .$type . q|',1,0) ), 0) as type_total, COUNT(*) AS total, COALESCE(SUM(onsite_checkout), 0) AS onsite_checkouts|; + } else{ + $count_query .= q|SELECT COALESCE(SUM( IF(biblioitems.itype = '| .$type . q|',1,0) ), 0) as type_total, COUNT(*) AS total, COALESCE(SUM(onsite_checkout), 0) AS onsite_checkouts| + } Please do not repeat, the only change is items vs biblioitems, so put it into a var then generate the query. 4. + while ( my $child = $children->next ){ + push @types, $child->itemtype; + } You need $children->get_column 5. - <h3>Add item type</h3> + ]<h3>Add item type</h3> 6. The indentation fix should not be here, it will add unnecessary conflict with patches already in the queue (bug 15522 and deps for instance!). smart-rules.tt particularly 7. I am really not sure the comparisons with '*' in smart-rules.tt are correct.
Created attachment 89167 [details] [review] Bug 21946: Database updates Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Created attachment 89168 [details] [review] Bug 21946: SCHEMA UPDATES DO NOT PUSH Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Created attachment 89169 [details] [review] Bug 21946: Update C4::Circulation->TooMany to check parent itemtypes To test: 1 - prove -v t/db_dependent/Circulation/TooMany.t Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Created attachment 89170 [details] [review] Bug 21946: Update Administration->Item types to allow for defining parent types To test: 1 - Browse to Administration -> Item types 2 - Not a new colum for prent type 3 - Edit an existing type and not you can select any type (except the current) as a parent 4 - Select one 5 - Edit the type you selected as parent 6 - Note it cannot have a parent defined 7 - Edit a new type, note you can select a parent Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Created attachment 89171 [details] [review] Bug 21946: Fix display of parent types Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Created attachment 89172 [details] [review] Bug 21946: Use objects for displaying circulation rules Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Created attachment 89173 [details] [review] Bug 21946: Display parent-child relationship on smart-rules.pl To test: 1 - Set some itemtypes to have a parent 2 - Browse to Administration -> Circulation and fines rules 3 - Note new description of parent/child relationships at top of page 4 - Note that itemtype dropdown for circ rules shows child types under parents 5 - Set a rule for a child type 6 - Note it displays as Parent->Child 7 - Have three child types under a parent 8 - Set the parent 'Current checkouts allowed' to 3 9 - Set the children 'Current checkouts allowed' to: type1 = 2 type2 = 1 type3 = 1 10 - Create some items of the type above 11 - Note you can checkout 2 of type 1, and not 3 12 - Note you can checkout 1 of type 2, but not 2 13 - Note that you now cannot checkout any of type3 14 - Note you cannot checkout any of the parent type 15 - Return one of the other items and note you can now checkout an item of type3 16 - Return another item and note you can checkout an item of the parent type 17 - Return all 18 - Set the parent type to 1 19 - Now note you can only checkout 1 of any of the children 20 - Set the parent to 0 21 - Note you cannot checkout any of the child types Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Created attachment 89174 [details] [review] Bug 21946: (follow-up) Correct dropdown when editing rule Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Created attachment 89175 [details] [review] Bug 21946: (QA follow-up) - remove console.log To make QA tools happy Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Created attachment 89176 [details] [review] Bug 21946: (follow-up) Address QA concerns 1 - Correct hashref for searching 2 - Correct undef for all (this only simplifies later logic so didn't affect outcomes) 3 - remove duplicated query 4 - Use get_column instead of loops 5 - Fix typo 6 - Whitespace fixes removed from earlier patches
(In reply to Jonathan Druart from comment #29) > 7. I am really not sure the comparisons with '*' in smart-rules.tt are > correct. Apologies, I don't see an issue with this, can you explain a bit more?
Forget that, I had the new version in mind (NULL in DB instead of '*'). At some point we really will need it (bug 15522) in I think, we will need to stop building stuffs that will conflict with it. Why not implement on top of it instead?
(In reply to Jonathan Druart from comment #41) > Forget that, I had the new version in mind (NULL in DB instead of '*'). > At some point we really will need it (bug 15522) in I think, we will need to > stop building stuffs that will conflict with it. > Why not implement on top of it instead? I think that will need work at this point. I believe the idea was to take care of the architecture, make sure it comes out as we planned without any great changes in structure during QA etc. Then work on the interface.
(In reply to Nick Clemens from comment #42) > (In reply to Jonathan Druart from comment #41) > > Forget that, I had the new version in mind (NULL in DB instead of '*'). > > At some point we really will need it (bug 15522) in I think, we will need to > > stop building stuffs that will conflict with it. > > Why not implement on top of it instead? > > I think that will need work at this point. > > I believe the idea was to take care of the architecture, make sure it comes > out as we planned without any great changes in structure during QA etc. Then > work on the interface. I understand Joubu's point of view. We started moving from issuingrules to circulation rules following what I think BWS has started. But at the same time we keep adding to issuingrules... that's not good. I think we need to decide which way to go, otherwise we will create conflicts and waste developer time. The patch set is quite heavy, only have one comment so far: Could we use parent_itemtype or parent_itype instead of parent_type? I think it could get confusing fast especially if we want to introduce similar in other spots.
Hm, I realized now that while this contains quite some changes to issuingrules, it doesn't add a new column.
Created attachment 93075 [details] [review] Bug 21946: Database updates Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Created attachment 93076 [details] [review] Bug 21946: Update C4::Circulation->TooMany to check parent itemtypes To test: 1 - prove -v t/db_dependent/Circulation/TooMany.t Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Created attachment 93077 [details] [review] Bug 21946: Update Administration->Item types to allow for defining parent types To test: 1 - Browse to Administration -> Item types 2 - Not a new colum for prent type 3 - Edit an existing type and not you can select any type (except the current) as a parent 4 - Select one 5 - Edit the type you selected as parent 6 - Note it cannot have a parent defined 7 - Edit a new type, note you can select a parent Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Created attachment 93078 [details] [review] Bug 21946: Fix display of parent types Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Created attachment 93079 [details] [review] Bug 21946: Use objects for displaying circulation rules Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Created attachment 93080 [details] [review] Bug 21946: Display parent-child relationship on smart-rules.pl To test: 1 - Set some itemtypes to have a parent 2 - Browse to Administration -> Circulation and fines rules 3 - Note new description of parent/child relationships at top of page 4 - Note that itemtype dropdown for circ rules shows child types under parents 5 - Set a rule for a child type 6 - Note it displays as Parent->Child 7 - Have three child types under a parent 8 - Set the parent 'Current checkouts allowed' to 3 9 - Set the children 'Current checkouts allowed' to: type1 = 2 type2 = 1 type3 = 1 10 - Create some items of the type above 11 - Note you can checkout 2 of type 1, and not 3 12 - Note you can checkout 1 of type 2, but not 2 13 - Note that you now cannot checkout any of type3 14 - Note you cannot checkout any of the parent type 15 - Return one of the other items and note you can now checkout an item of type3 16 - Return another item and note you can checkout an item of the parent type 17 - Return all 18 - Set the parent type to 1 19 - Now note you can only checkout 1 of any of the children 20 - Set the parent to 0 21 - Note you cannot checkout any of the child types Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Created attachment 93081 [details] [review] Bug 21946: (follow-up) Correct dropdown when editing rule Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Created attachment 93082 [details] [review] Bug 21946: (QA follow-up) - remove console.log To make QA tools happy Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Created attachment 93083 [details] [review] Bug 21946: (follow-up) Address QA concerns 1 - Correct hashref for searching 2 - Correct undef for all (this only simplifies later logic so didn't affect outcomes) 3 - remove duplicated query 4 - Use get_column instead of loops 5 - Fix typo 6 - Whitespace fixes removed from earlier patches
Created attachment 93084 [details] [review] Bug 21946: DO NOT PUSH - Schema updates
Accidentally doing this myself right off the bat, I am going to fail this, simply because you can set an itemtype to be a parent of itself. I don't know if it will cause any problems or not, but this probably should not be allowed to happen. Just in case. Unless you can guarentee it will not cause a problem it is done by mistake.
Okay, I lied. I thought I set it to itself, but I must not have. It must have been something I misread. Sorry. Setting back to signed off. Glad I rechecked.
Cosmetics: * Step 4, there is a blank line between the parent and the children. Can we get rid of this? * I would suggest a '-' before the children. This seems to be universal in showing the children in a list. * When the child is picked, only the child shows in the dropdown, but when saved, it shows the parent->child. Maybe it is a limitation of the dropdown object, but it would be nice to seee it all while editing. Maybe if the '-' is added, it would help. I think there is an issue with understanding how this works. Right now it looks like the parent, with its own set of rules would overrule ALL the children rules. But that is not the case. In this case, only the number of checkouts overrides the children. I think it needs to be clearer, somehow, how items behave under the parent. Having an itemtype act as both an itemtype and parent may be confusing, especially since not all of the rules of the parent apply to the child. Otherwise, it works as described.
Created attachment 93086 [details] [review] Bug 21946: Database updates Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Created attachment 93087 [details] [review] Bug 21946: Update C4::Circulation->TooMany to check parent itemtypes To test: 1 - prove -v t/db_dependent/Circulation/TooMany.t Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Created attachment 93088 [details] [review] Bug 21946: Update Administration->Item types to allow for defining parent types To test: 1 - Browse to Administration -> Item types 2 - Not a new colum for prent type 3 - Edit an existing type and not you can select any type (except the current) as a parent 4 - Select one 5 - Edit the type you selected as parent 6 - Note it cannot have a parent defined 7 - Edit a new type, note you can select a parent Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Created attachment 93089 [details] [review] Bug 21946: Fix display of parent types Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Created attachment 93090 [details] [review] Bug 21946: Use objects for displaying circulation rules Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Created attachment 93091 [details] [review] Bug 21946: Display parent-child relationship on smart-rules.pl To test: 1 - Set some itemtypes to have a parent 2 - Browse to Administration -> Circulation and fines rules 3 - Note new description of parent/child relationships at top of page 4 - Note that itemtype dropdown for circ rules shows child types under parents 5 - Set a rule for a child type 6 - Note it displays as Parent->Child 7 - Have three child types under a parent 8 - Set the parent 'Current checkouts allowed' to 3 9 - Set the children 'Current checkouts allowed' to: type1 = 2 type2 = 1 type3 = 1 10 - Create some items of the type above 11 - Note you can checkout 2 of type 1, and not 3 12 - Note you can checkout 1 of type 2, but not 2 13 - Note that you now cannot checkout any of type3 14 - Note you cannot checkout any of the parent type 15 - Return one of the other items and note you can now checkout an item of type3 16 - Return another item and note you can checkout an item of the parent type 17 - Return all 18 - Set the parent type to 1 19 - Now note you can only checkout 1 of any of the children 20 - Set the parent to 0 21 - Note you cannot checkout any of the child types Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Created attachment 93092 [details] [review] Bug 21946: (follow-up) Correct dropdown when editing rule Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Created attachment 93093 [details] [review] Bug 21946: (QA follow-up) - remove console.log To make QA tools happy Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Created attachment 93094 [details] [review] Bug 21946: (follow-up) Address QA concerns 1 - Correct hashref for searching 2 - Correct undef for all (this only simplifies later logic so didn't affect outcomes) 3 - remove duplicated query 4 - Use get_column instead of loops 5 - Fix typo 6 - Whitespace fixes removed from earlier patches Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Created attachment 93095 [details] [review] Bug 21946: DO NOT PUSH - Schema updates Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Created attachment 93096 [details] [review] Bug 21946: Database updates Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>
Created attachment 93097 [details] [review] Bug 21946: Update C4::Circulation->TooMany to check parent itemtypes To test: 1 - prove -v t/db_dependent/Circulation/TooMany.t Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>
Created attachment 93098 [details] [review] Bug 21946: Update Administration->Item types to allow for defining parent types To test: 1 - Browse to Administration -> Item types 2 - Not a new colum for prent type 3 - Edit an existing type and not you can select any type (except the current) as a parent 4 - Select one 5 - Edit the type you selected as parent 6 - Note it cannot have a parent defined 7 - Edit a new type, note you can select a parent Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>
Created attachment 93099 [details] [review] Bug 21946: Fix display of parent types Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>
Created attachment 93100 [details] [review] Bug 21946: Use objects for displaying circulation rules Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>
Created attachment 93101 [details] [review] Bug 21946: Display parent-child relationship on smart-rules.pl To test: 1 - Set some itemtypes to have a parent 2 - Browse to Administration -> Circulation and fines rules 3 - Note new description of parent/child relationships at top of page 4 - Note that itemtype dropdown for circ rules shows child types under parents 5 - Set a rule for a child type 6 - Note it displays as Parent->Child 7 - Have three child types under a parent 8 - Set the parent 'Current checkouts allowed' to 3 9 - Set the children 'Current checkouts allowed' to: type1 = 2 type2 = 1 type3 = 1 10 - Create some items of the type above 11 - Note you can checkout 2 of type 1, and not 3 12 - Note you can checkout 1 of type 2, but not 2 13 - Note that you now cannot checkout any of type3 14 - Note you cannot checkout any of the parent type 15 - Return one of the other items and note you can now checkout an item of type3 16 - Return another item and note you can checkout an item of the parent type 17 - Return all 18 - Set the parent type to 1 19 - Now note you can only checkout 1 of any of the children 20 - Set the parent to 0 21 - Note you cannot checkout any of the child types Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>
Created attachment 93102 [details] [review] Bug 21946: (follow-up) Correct dropdown when editing rule Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>
Created attachment 93103 [details] [review] Bug 21946: (QA follow-up) - remove console.log To make QA tools happy Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>
Created attachment 93104 [details] [review] Bug 21946: (follow-up) Address QA concerns 1 - Correct hashref for searching 2 - Correct undef for all (this only simplifies later logic so didn't affect outcomes) 3 - remove duplicated query 4 - Use get_column instead of loops 5 - Fix typo 6 - Whitespace fixes removed from earlier patches Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>
Created attachment 93105 [details] [review] Bug 21946: DO NOT PUSH - Schema updates Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>
Hi Nick, can you please rebase? ... Bit scared of the datatables code.
Created attachment 96977 [details] [review] Bug 21946: Database updates Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>
Created attachment 96978 [details] [review] Bug 21946: Update C4::Circulation->TooMany to check parent itemtypes To test: 1 - prove -v t/db_dependent/Circulation/TooMany.t Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>
Created attachment 96979 [details] [review] Bug 21946: Update Administration->Item types to allow for defining parent types To test: 1 - Browse to Administration -> Item types 2 - Not a new colum for prent type 3 - Edit an existing type and not you can select any type (except the current) as a parent 4 - Select one 5 - Edit the type you selected as parent 6 - Note it cannot have a parent defined 7 - Edit a new type, note you can select a parent Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>
Created attachment 96980 [details] [review] Bug 21946: Fix display of parent types Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>
Created attachment 96981 [details] [review] Bug 21946: Use objects for displaying circulation rules Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>
Created attachment 96982 [details] [review] Bug 21946: Display parent-child relationship on smart-rules.pl To test: 1 - Set some itemtypes to have a parent 2 - Browse to Administration -> Circulation and fines rules 3 - Note new description of parent/child relationships at top of page 4 - Note that itemtype dropdown for circ rules shows child types under parents 5 - Set a rule for a child type 6 - Note it displays as Parent->Child 7 - Have three child types under a parent 8 - Set the parent 'Current checkouts allowed' to 3 9 - Set the children 'Current checkouts allowed' to: type1 = 2 type2 = 1 type3 = 1 10 - Create some items of the type above 11 - Note you can checkout 2 of type 1, and not 3 12 - Note you can checkout 1 of type 2, but not 2 13 - Note that you now cannot checkout any of type3 14 - Note you cannot checkout any of the parent type 15 - Return one of the other items and note you can now checkout an item of type3 16 - Return another item and note you can checkout an item of the parent type 17 - Return all 18 - Set the parent type to 1 19 - Now note you can only checkout 1 of any of the children 20 - Set the parent to 0 21 - Note you cannot checkout any of the child types Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>
Created attachment 96983 [details] [review] Bug 21946: (follow-up) Correct dropdown when editing rule Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>
Created attachment 96984 [details] [review] Bug 21946: (QA follow-up) - remove console.log To make QA tools happy Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>
Created attachment 96985 [details] [review] Bug 21946: (follow-up) Address QA concerns 1 - Correct hashref for searching 2 - Correct undef for all (this only simplifies later logic so didn't affect outcomes) 3 - remove duplicated query 4 - Use get_column instead of loops 5 - Fix typo 6 - Whitespace fixes removed from earlier patches Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>
Created attachment 96986 [details] [review] Bug 21946: DO NOT PUSH - Schema updates Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>
Created attachment 96987 [details] [review] Bug 21946: Update unit tests to pass under ES
Looks like this dev will conflict with bug 15522.
(In reply to Jonathan Druart from comment #90) > Looks like this dev will conflict with bug 15522. Agreed, but I don't think that should be a blocker here. 15522 needs some work, we are gathering notes to post on that bug. This is a much desired feature and should be allowed to proceed
It seems this only has the ability to have a single parent set. What about when you have a set limit for a smaller group, like video media, but you also have a bigger group that this group is a part of that has a limit? Can a group have a parent?
(In reply to Christopher Brannon from comment #92) > It seems this only has the ability to have a single parent set. What about > when you have a set limit for a smaller group, like video media, but you > also have a bigger group that this group is a part of that has a limit? Can > a group have a parent? No, this development only allows for a single level groups, they cannot be nested
Applying: Bug 21946: Database updates Applying: Bug 21946: Update C4::Circulation->TooMany to check parent itemtypes .git/rebase-apply/patch:95: trailing whitespace. } else { .git/rebase-apply/patch:96: trailing whitespace. $count_query .= " JOIN biblioitems USING (biblionumber) warning: 2 lines add whitespace errors. Using index info to reconstruct a base tree... M C4/Circulation.pm M t/db_dependent/Circulation/TooMany.t .git/rebase-apply/patch:95: trailing whitespace. } else { .git/rebase-apply/patch:96: trailing whitespace. $count_query .= " JOIN biblioitems USING (biblionumber) warning: 2 lines add whitespace errors. Falling back to patching base and 3-way merge... Auto-merging t/db_dependent/Circulation/TooMany.t Auto-merging C4/Circulation.pm CONFLICT (content): Merge conflict in C4/Circulation.pm error: Failed to merge in the changes. Patch failed at 0001 Bug 21946: Update C4::Circulation->TooMany to check parent itemtypes 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". Patch left in /tmp/Bug-21946-Update-C4Circulation-TooMany-to-check-pa-mluqOe.patch
Created attachment 99419 [details] [review] Bug 21946: Database updates Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>
Created attachment 99420 [details] [review] Bug 21946: Update C4::Circulation->TooMany to check parent itemtypes To test: 1 - prove -v t/db_dependent/Circulation/TooMany.t Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>
Created attachment 99421 [details] [review] Bug 21946: Update Administration->Item types to allow for defining parent types To test: 1 - Browse to Administration -> Item types 2 - Not a new colum for prent type 3 - Edit an existing type and not you can select any type (except the current) as a parent 4 - Select one 5 - Edit the type you selected as parent 6 - Note it cannot have a parent defined 7 - Edit a new type, note you can select a parent Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>
Created attachment 99422 [details] [review] Bug 21946: Display parent-child relationship on smart-rules.pl To test: 1 - Set some itemtypes to have a parent 2 - Browse to Administration -> Circulation and fines rules 3 - Note new description of parent/child relationships at top of page 4 - Note that itemtype dropdown for circ rules shows child types under parents 5 - Set a rule for a child type 6 - Note it displays as Parent->Child 7 - Have three child types under a parent 8 - Set the parent 'Current checkouts allowed' to 3 9 - Set the children 'Current checkouts allowed' to: type1 = 2 type2 = 1 type3 = 1 10 - Create some items of the type above 11 - Note you can checkout 2 of type 1, and not 3 12 - Note you can checkout 1 of type 2, but not 2 13 - Note that you now cannot checkout any of type3 14 - Note you cannot checkout any of the parent type 15 - Return one of the other items and note you can now checkout an item of type3 16 - Return another item and note you can checkout an item of the parent type 17 - Return all 18 - Set the parent type to 1 19 - Now note you can only checkout 1 of any of the children 20 - Set the parent to 0 21 - Note you cannot checkout any of the child types Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>
Created attachment 99423 [details] [review] Bug 21946: Update unit tests to pass under ES
Created attachment 99424 [details] [review] Bug 21946: DO NOT PUSH - Schema updates
(In reply to Nick Clemens from comment #91) > (In reply to Jonathan Druart from comment #90) > > Looks like this dev will conflict with bug 15522. > > Agreed, but I don't think that should be a blocker here. 15522 needs some > work, we are gathering notes to post on that bug. > > This is a much desired feature and should be allowed to proceed Rebased and squashed some patches. In terms of 15522 this only affects dropdowns to select itemtype and ordering of display, otherwise rules are set per itemtype as they have always been, TooMany handles the logic of which to apply when. I don't think this will require large changes there
Comment on attachment 99419 [details] [review] Bug 21946: Database updates Review of attachment 99419 [details] [review]: ----------------------------------------------------------------- ::: installer/data/mysql/kohastructure.sql @@ +892,4 @@ > hideinopac tinyint(1) NOT NULL DEFAULT 0, -- Hide the item type from the search options in OPAC > searchcategory varchar(80) default NULL, -- Group this item type with others with the same value on OPAC search options > PRIMARY KEY (`itemtype`), > + FOREIGN KEY (parent_type) REFERENCES itemtypes(itemtype) ON UPDATE CASCADE ON DELETE CASCADE, Can you use CONSTRAINT as in the atomic update. So that we can have the constraint name that is defined in.
Comment on attachment 99421 [details] [review] Bug 21946: Update Administration->Item types to allow for defining parent types Review of attachment 99421 [details] [review]: ----------------------------------------------------------------- ::: Koha/ItemType.pm @@ +151,5 @@ > + return unless $parent_rs; > + return Koha::ItemType->_new_from_dbic( $parent_rs ); > + > +} > + I can't see any unit test for the parent method. Right ?
Also, a rebase is needed
Created attachment 107035 [details] [review] Bug 21946: Database updates Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>
Created attachment 107036 [details] [review] Bug 21946: Update C4::Circulation->TooMany to check parent itemtypes To test: 1 - prove -v t/db_dependent/Circulation/TooMany.t Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>
Created attachment 107037 [details] [review] Bug 21946: Update Administration->Item types to allow for defining parent types To test: 1 - Browse to Administration -> Item types 2 - Not a new colum for prent type 3 - Edit an existing type and not you can select any type (except the current) as a parent 4 - Select one 5 - Edit the type you selected as parent 6 - Note it cannot have a parent defined 7 - Edit a new type, note you can select a parent Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>
Created attachment 107038 [details] [review] Bug 21946: Display parent-child relationship on smart-rules.pl To test: 1 - Set some itemtypes to have a parent 2 - Browse to Administration -> Circulation and fines rules 3 - Note new description of parent/child relationships at top of page 4 - Note that itemtype dropdown for circ rules shows child types under parents 5 - Set a rule for a child type 6 - Note it displays as Parent->Child 7 - Have three child types under a parent 8 - Set the parent 'Current checkouts allowed' to 3 9 - Set the children 'Current checkouts allowed' to: type1 = 2 type2 = 1 type3 = 1 10 - Create some items of the type above 11 - Note you can checkout 2 of type 1, and not 3 12 - Note you can checkout 1 of type 2, but not 2 13 - Note that you now cannot checkout any of type3 14 - Note you cannot checkout any of the parent type 15 - Return one of the other items and note you can now checkout an item of type3 16 - Return another item and note you can checkout an item of the parent type 17 - Return all 18 - Set the parent type to 1 19 - Now note you can only checkout 1 of any of the children 20 - Set the parent to 0 21 - Note you cannot checkout any of the child types Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>
Created attachment 107039 [details] [review] Bug 21946: Update unit tests to pass under ES
Created attachment 107040 [details] [review] Bug 21946: Add test for parent method
Created attachment 107041 [details] [review] Bug 21946: DO NOT PUSH - Schema changes
Created attachment 107278 [details] [review] Bug 21946: Database updates Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com> Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 107279 [details] [review] Bug 21946: Update C4::Circulation->TooMany to check parent itemtypes To test: 1 - prove -v t/db_dependent/Circulation/TooMany.t Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com> Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 107280 [details] [review] Bug 21946: Update Administration->Item types to allow for defining parent types To test: 1 - Browse to Administration -> Item types 2 - Not a new colum for prent type 3 - Edit an existing type and not you can select any type (except the current) as a parent 4 - Select one 5 - Edit the type you selected as parent 6 - Note it cannot have a parent defined 7 - Edit a new type, note you can select a parent Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com> Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 107281 [details] [review] Bug 21946: Display parent-child relationship on smart-rules.pl To test: 1 - Set some itemtypes to have a parent 2 - Browse to Administration -> Circulation and fines rules 3 - Note new description of parent/child relationships at top of page 4 - Note that itemtype dropdown for circ rules shows child types under parents 5 - Set a rule for a child type 6 - Note it displays as Parent->Child 7 - Have three child types under a parent 8 - Set the parent 'Current checkouts allowed' to 3 9 - Set the children 'Current checkouts allowed' to: type1 = 2 type2 = 1 type3 = 1 10 - Create some items of the type above 11 - Note you can checkout 2 of type 1, and not 3 12 - Note you can checkout 1 of type 2, but not 2 13 - Note that you now cannot checkout any of type3 14 - Note you cannot checkout any of the parent type 15 - Return one of the other items and note you can now checkout an item of type3 16 - Return another item and note you can checkout an item of the parent type 17 - Return all 18 - Set the parent type to 1 19 - Now note you can only checkout 1 of any of the children 20 - Set the parent to 0 21 - Note you cannot checkout any of the child types Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com> Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 107282 [details] [review] Bug 21946: Update unit tests to pass under ES Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 107283 [details] [review] Bug 21946: Add test for parent method Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 107284 [details] [review] Bug 21946: DO NOT PUSH - Schema changes
Passed QA Nice feature!
1. There is an ergonomic issue in the circ rules page: https://snipboard.io/ARHowm.jpg There is a space for the label of the optgroup. I don't think we should use an optgroup, maybe more a margin? 2. We should have a "hint" on the item type form to explain what it this "parent" thing 3. This is only affecting checkouts, why? Is there a plan to extend it to holds for instance? 4. Reading the code it seems that it does not affect on site checkouts. I think it should be done in this patchset
Created attachment 107467 [details] [review] Bug 21946: (follow-up) Add optgroup label in circ rules This patch adds a label to the optgroup showing child itemtypes in the circulation and fine rules interface. The parent itemtype description is the optgroup label, and then the parent itemtype is repeated as an <option> with the additional label "All"
Created attachment 107489 [details] [review] Bug 21946: (follow-up) Add a hint
(In reply to Jonathan Druart from comment #120) > 1. There is an ergonomic issue in the circ rules page: Owen fixed this - Thanks Owen! > 2. We should have a "hint" on the item type form to explain what it this > "parent" thing Added a hint - there is a description on circ rules page, so I think referring to that should be enough > 3. This is only affecting checkouts, why? Is there a plan to extend it to > holds for instance? No current development but it does open that possibility > 4. Reading the code it seems that it does not affect on site checkouts. I > think it should be done in this patchset I think this will require some refactoring and I have some questions about how the rules for on-site are applied - I would like to take this one on as a separate bug
There is a failing test: # Failed test 'No tests run for subtest "CanBookBeIssued | item-level_itypes=biblio"' # at t/db_dependent//Circulation.t line 3487. C4::Circulation::TooMany(): DBI Exception: DBD::mysql::db selectrow_array failed: Unknown column 'biblioitems.itemtype' in 'field list' [for Statement "SELECT COALESCE(SUM( IF(biblioitems.itemtype = 'UKARawKT4',1,0) ), 0) as type_total, COUNT(*) AS total, COALESCE(SUM(onsite_checkout), 0) AS onsite_checkouts
Created attachment 107686 [details] [review] Bug 21946: Database updates Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com> Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 107687 [details] [review] Bug 21946: Update C4::Circulation->TooMany to check parent itemtypes To test: 1 - prove -v t/db_dependent/Circulation/TooMany.t Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com> Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> JD amended patch: tidy the subtest
Created attachment 107688 [details] [review] Bug 21946: Update Administration->Item types to allow for defining parent types To test: 1 - Browse to Administration -> Item types 2 - Not a new colum for prent type 3 - Edit an existing type and not you can select any type (except the current) as a parent 4 - Select one 5 - Edit the type you selected as parent 6 - Note it cannot have a parent defined 7 - Edit a new type, note you can select a parent Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com> Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 107689 [details] [review] Bug 21946: Display parent-child relationship on smart-rules.pl To test: 1 - Set some itemtypes to have a parent 2 - Browse to Administration -> Circulation and fines rules 3 - Note new description of parent/child relationships at top of page 4 - Note that itemtype dropdown for circ rules shows child types under parents 5 - Set a rule for a child type 6 - Note it displays as Parent->Child 7 - Have three child types under a parent 8 - Set the parent 'Current checkouts allowed' to 3 9 - Set the children 'Current checkouts allowed' to: type1 = 2 type2 = 1 type3 = 1 10 - Create some items of the type above 11 - Note you can checkout 2 of type 1, and not 3 12 - Note you can checkout 1 of type 2, but not 2 13 - Note that you now cannot checkout any of type3 14 - Note you cannot checkout any of the parent type 15 - Return one of the other items and note you can now checkout an item of type3 16 - Return another item and note you can checkout an item of the parent type 17 - Return all 18 - Set the parent type to 1 19 - Now note you can only checkout 1 of any of the children 20 - Set the parent to 0 21 - Note you cannot checkout any of the child types Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com> Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 107690 [details] [review] Bug 21946: Update unit tests to pass under ES Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 107691 [details] [review] Bug 21946: Add test for parent method Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 107692 [details] [review] Bug 21946: (follow-up) Add optgroup label in circ rules This patch adds a label to the optgroup showing child itemtypes in the circulation and fine rules interface. The parent itemtype description is the optgroup label, and then the parent itemtype is repeated as an <option> with the additional label "All"
Created attachment 107693 [details] [review] Bug 21946: (follow-up) Add a hint
Created attachment 107694 [details] [review] Bug 21946: DBIC schema changes
Created attachment 107701 [details] [review] Bug 21946: (follow-up) Restore join to biblioitems
Created attachment 107733 [details] [review] Bug 21946: Restore defined vs "" The tests still pass. Was the original change really useful?
Nick, I did not find a reason for this change (see last patch), what do you think?
I've written some follow-up patches that I attached to bug 26132. Could you have a look?
(In reply to Jonathan Druart from comment #137) > I've written some follow-up patches that I attached to bug 26132. > Could you have a look? Those look good, signed off!
Pushed to master for 20.11, thanks to everybody involved!
New feature will not be backported to 20.05.x