Bug 21946

Summary: Group circulation by item type
Product: Koha Reporter: Jesse Maseto <jesse>
Component: CirculationAssignee: Nick Clemens <nick>
Status: CLOSED FIXED QA Contact: Alex Arnaud <alex.arnaud>
Severity: new feature    
Priority: P5 - low CC: alex.arnaud, cbrannon, fridolin.somers, gmcharlt, janet.mcgowan, jonathan.druart, jzairo, katrin.fischer, kyle.m.hall, lisettepalouse+koha, lucas, m.de.rooy, marjorie.barry-vila, nengard, nick, sally.healey, wizzyrea
Version: master   
Hardware: All   
OS: All   
Change sponsored?: Sponsored Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
This feature was sponsored by VOKAL and adds the ability to define some itemtypes as 'parent' to other item types for defining circulation limits. This allows to create 'groups' of related items types. E.g. - a library has both Blu-ray and DVD itemtypes - these can be grouped under the 'Media' itemtype. The checkout limit for Media will then apply to both Blu-ray and DVD. So if a library says a patron can have 4 dvds - 4 blu-ray - and sets Media to have a limit of 4 then patrons can have up to 4 items of either type (e.g. 1 DVD, 3 blu-rays) but will be prevented from checking out more of either type by the limit on the parent. Parent rules only apply to checkout limits. Sponsored by: Vermont Organization of Koha Automated Libraries (VOKAL), USA
Version(s) released in:
20.11.00
Bug Depends on: 22759    
Bug Blocks: 30324, 26132, 26782, 28190    
Attachments: Bug 21946: Database updates
Bug 21946: SCHEMA UPDATES DO NOT PUSH
Bug 21946: Update C4::Circulation->TooMany to check parent itemtypes
Bug 21946: Update Administration->Item types to llow for defining parent types
Bug 21946: Fix display of parent types
Bug 21946: Use objects for displaying circulation rules
Bug 21946: Display parent-child relationship on smart-rules.pl
Bug 21946: (follow-up) Correct dropdown when editing rule
Bug 21946: Database updates
Bug 21946: SCHEMA UPDATES DO NOT PUSH
Bug 21946: Update C4::Circulation->TooMany to check parent itemtypes
Bug 21946: Update Administration->Item types to allow for defining parent types
Bug 21946: Fix display of parent types
Bug 21946: Use objects for displaying circulation rules
Bug 21946: Display parent-child relationship on smart-rules.pl
Bug 21946: (follow-up) Correct dropdown when editing rule
Bug 21946: Database updates
Bug 21946: SCHEMA UPDATES DO NOT PUSH
Bug 21946: Update C4::Circulation->TooMany to check parent itemtypes
Bug 21946: Update Administration->Item types to allow for defining parent types
Bug 21946: Fix display of parent types
Bug 21946: Use objects for displaying circulation rules
Bug 21946: Display parent-child relationship on smart-rules.pl
Bug 21946: (follow-up) Correct dropdown when editing rule
Bug 21946: (QA follow-up) - remove console.log
Bug 21946: Database updates
Bug 21946: SCHEMA UPDATES DO NOT PUSH
Bug 21946: Update C4::Circulation->TooMany to check parent itemtypes
Bug 21946: Update Administration->Item types to allow for defining parent types
Bug 21946: Fix display of parent types
Bug 21946: Use objects for displaying circulation rules
Bug 21946: Display parent-child relationship on smart-rules.pl
Bug 21946: (follow-up) Correct dropdown when editing rule
Bug 21946: (QA follow-up) - remove console.log
Bug 21946: (follow-up) Address QA concerns
Bug 21946: Database updates
Bug 21946: Update C4::Circulation->TooMany to check parent itemtypes
Bug 21946: Update Administration->Item types to allow for defining parent types
Bug 21946: Fix display of parent types
Bug 21946: Use objects for displaying circulation rules
Bug 21946: Display parent-child relationship on smart-rules.pl
Bug 21946: (follow-up) Correct dropdown when editing rule
Bug 21946: (QA follow-up) - remove console.log
Bug 21946: (follow-up) Address QA concerns
Bug 21946: DO NOT PUSH - Schema updates
Bug 21946: Database updates
Bug 21946: Update C4::Circulation->TooMany to check parent itemtypes
Bug 21946: Update Administration->Item types to allow for defining parent types
Bug 21946: Fix display of parent types
Bug 21946: Use objects for displaying circulation rules
Bug 21946: Display parent-child relationship on smart-rules.pl
Bug 21946: (follow-up) Correct dropdown when editing rule
Bug 21946: (QA follow-up) - remove console.log
Bug 21946: (follow-up) Address QA concerns
Bug 21946: DO NOT PUSH - Schema updates
Bug 21946: Database updates
Bug 21946: Update C4::Circulation->TooMany to check parent itemtypes
Bug 21946: Update Administration->Item types to allow for defining parent types
Bug 21946: Fix display of parent types
Bug 21946: Use objects for displaying circulation rules
Bug 21946: Display parent-child relationship on smart-rules.pl
Bug 21946: (follow-up) Correct dropdown when editing rule
Bug 21946: (QA follow-up) - remove console.log
Bug 21946: (follow-up) Address QA concerns
Bug 21946: DO NOT PUSH - Schema updates
Bug 21946: Database updates
Bug 21946: Update C4::Circulation->TooMany to check parent itemtypes
Bug 21946: Update Administration->Item types to allow for defining parent types
Bug 21946: Fix display of parent types
Bug 21946: Use objects for displaying circulation rules
Bug 21946: Display parent-child relationship on smart-rules.pl
Bug 21946: (follow-up) Correct dropdown when editing rule
Bug 21946: (QA follow-up) - remove console.log
Bug 21946: (follow-up) Address QA concerns
Bug 21946: DO NOT PUSH - Schema updates
Bug 21946: Update unit tests to pass under ES
Bug 21946: Database updates
Bug 21946: Update C4::Circulation->TooMany to check parent itemtypes
Bug 21946: Update Administration->Item types to allow for defining parent types
Bug 21946: Display parent-child relationship on smart-rules.pl
Bug 21946: Update unit tests to pass under ES
Bug 21946: DO NOT PUSH - Schema updates
Bug 21946: Database updates
Bug 21946: Update C4::Circulation->TooMany to check parent itemtypes
Bug 21946: Update Administration->Item types to allow for defining parent types
Bug 21946: Display parent-child relationship on smart-rules.pl
Bug 21946: Update unit tests to pass under ES
Bug 21946: Add test for parent method
Bug 21946: DO NOT PUSH - Schema changes
Bug 21946: Database updates
Bug 21946: Update C4::Circulation->TooMany to check parent itemtypes
Bug 21946: Update Administration->Item types to allow for defining parent types
Bug 21946: Display parent-child relationship on smart-rules.pl
Bug 21946: Update unit tests to pass under ES
Bug 21946: Add test for parent method
Bug 21946: DO NOT PUSH - Schema changes
Bug 21946: (follow-up) Add optgroup label in circ rules
Bug 21946: (follow-up) Add a hint
Bug 21946: Database updates
Bug 21946: Update C4::Circulation->TooMany to check parent itemtypes
Bug 21946: Update Administration->Item types to allow for defining parent types
Bug 21946: Display parent-child relationship on smart-rules.pl
Bug 21946: Update unit tests to pass under ES
Bug 21946: Add test for parent method
Bug 21946: (follow-up) Add optgroup label in circ rules
Bug 21946: (follow-up) Add a hint
Bug 21946: DBIC schema changes
Bug 21946: (follow-up) Restore join to biblioitems
Bug 21946: Restore defined vs ""

Description Jesse Maseto 2018-12-04 16:58:24 UTC
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
Comment 1 Nick Clemens 2019-04-10 13:36:27 UTC
Created attachment 87713 [details] [review]
Bug 21946: Database updates
Comment 2 Nick Clemens 2019-04-10 13:36:29 UTC
Created attachment 87714 [details] [review]
Bug 21946: SCHEMA UPDATES DO NOT PUSH
Comment 3 Nick Clemens 2019-04-10 13:36:31 UTC
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
Comment 4 Nick Clemens 2019-04-10 13:36:34 UTC
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
Comment 5 Nick Clemens 2019-04-10 13:36:36 UTC
Created attachment 87717 [details] [review]
Bug 21946: Fix display of parent types
Comment 6 Nick Clemens 2019-04-10 13:36:39 UTC
Created attachment 87718 [details] [review]
Bug 21946: Use objects for displaying circulation rules
Comment 7 Nick Clemens 2019-04-10 13:36:41 UTC
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
Comment 8 Liz Rea 2019-04-11 16:19:51 UTC
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
Comment 9 Nick Clemens 2019-04-22 19:03:47 UTC
Created attachment 88392 [details] [review]
Bug 21946: (follow-up) Correct dropdown when editing rule
Comment 10 Liz Rea 2019-04-23 14:42:58 UTC
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
Comment 11 Nick Clemens 2019-04-24 00:22:57 UTC
Created attachment 88574 [details] [review]
Bug 21946: Database updates
Comment 12 Nick Clemens 2019-04-24 00:22:59 UTC
Created attachment 88575 [details] [review]
Bug 21946: SCHEMA UPDATES DO NOT PUSH
Comment 13 Nick Clemens 2019-04-24 00:23:02 UTC
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
Comment 14 Nick Clemens 2019-04-24 00:23:05 UTC
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
Comment 15 Nick Clemens 2019-04-24 00:23:08 UTC
Created attachment 88578 [details] [review]
Bug 21946: Fix display of parent types
Comment 16 Nick Clemens 2019-04-24 00:23:11 UTC
Created attachment 88579 [details] [review]
Bug 21946: Use objects for displaying circulation rules
Comment 17 Nick Clemens 2019-04-24 00:23:14 UTC
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
Comment 18 Nick Clemens 2019-04-24 00:23:17 UTC
Created attachment 88581 [details] [review]
Bug 21946: (follow-up) Correct dropdown when editing rule
Comment 19 Liz Rea 2019-04-26 14:52:44 UTC
Created attachment 88926 [details] [review]
Bug 21946: Database updates

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 20 Liz Rea 2019-04-26 14:53:46 UTC
Created attachment 88927 [details] [review]
Bug 21946: SCHEMA UPDATES DO NOT PUSH

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 21 Liz Rea 2019-04-26 14:53:49 UTC
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>
Comment 22 Liz Rea 2019-04-26 14:53:53 UTC
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>
Comment 23 Liz Rea 2019-04-26 14:53:56 UTC
Created attachment 88930 [details] [review]
Bug 21946: Fix display of parent types

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 24 Liz Rea 2019-04-26 14:53:59 UTC
Created attachment 88931 [details] [review]
Bug 21946: Use objects for displaying circulation rules

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 25 Liz Rea 2019-04-26 14:54:02 UTC
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>
Comment 26 Liz Rea 2019-04-26 14:54:05 UTC
Created attachment 88933 [details] [review]
Bug 21946: (follow-up) Correct dropdown when editing rule

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 27 Liz Rea 2019-04-26 14:54:08 UTC
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>
Comment 28 Katrin Fischer 2019-04-28 23:21:06 UTC
*** Bug 6685 has been marked as a duplicate of this bug. ***
Comment 29 Jonathan Druart 2019-04-30 19:22:35 UTC
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.
Comment 30 Nick Clemens 2019-05-01 11:02:23 UTC
Created attachment 89167 [details] [review]
Bug 21946: Database updates

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 31 Nick Clemens 2019-05-01 11:02:26 UTC
Created attachment 89168 [details] [review]
Bug 21946: SCHEMA UPDATES DO NOT PUSH

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 32 Nick Clemens 2019-05-01 11:02:29 UTC
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>
Comment 33 Nick Clemens 2019-05-01 11:02:32 UTC
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>
Comment 34 Nick Clemens 2019-05-01 11:02:35 UTC
Created attachment 89171 [details] [review]
Bug 21946: Fix display of parent types

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 35 Nick Clemens 2019-05-01 11:02:39 UTC
Created attachment 89172 [details] [review]
Bug 21946: Use objects for displaying circulation rules

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 36 Nick Clemens 2019-05-01 11:02:42 UTC
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>
Comment 37 Nick Clemens 2019-05-01 11:02:46 UTC
Created attachment 89174 [details] [review]
Bug 21946: (follow-up) Correct dropdown when editing rule

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 38 Nick Clemens 2019-05-01 11:02:49 UTC
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>
Comment 39 Nick Clemens 2019-05-01 11:02:52 UTC
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
Comment 40 Nick Clemens 2019-05-01 11:28:12 UTC
(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?
Comment 41 Jonathan Druart 2019-05-01 13:47:22 UTC
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?
Comment 42 Nick Clemens 2019-05-01 13:52:05 UTC
(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.
Comment 43 Katrin Fischer 2019-05-11 17:39:57 UTC
(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.
Comment 44 Katrin Fischer 2019-05-11 17:43:02 UTC
Hm, I realized now that while this contains quite some changes to issuingrules, it doesn't add a new column.
Comment 45 Nick Clemens 2019-09-21 03:16:32 UTC
Created attachment 93075 [details] [review]
Bug 21946: Database updates

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 46 Nick Clemens 2019-09-21 03:16:37 UTC
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>
Comment 47 Nick Clemens 2019-09-21 03:16:42 UTC
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>
Comment 48 Nick Clemens 2019-09-21 03:16:46 UTC
Created attachment 93078 [details] [review]
Bug 21946: Fix display of parent types

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 49 Nick Clemens 2019-09-21 03:16:49 UTC
Created attachment 93079 [details] [review]
Bug 21946: Use objects for displaying circulation rules

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 50 Nick Clemens 2019-09-21 03:16:54 UTC
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>
Comment 51 Nick Clemens 2019-09-21 03:16:58 UTC
Created attachment 93081 [details] [review]
Bug 21946: (follow-up) Correct dropdown when editing rule

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 52 Nick Clemens 2019-09-21 03:17:04 UTC
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>
Comment 53 Nick Clemens 2019-09-21 03:17:12 UTC
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
Comment 54 Nick Clemens 2019-09-21 03:17:16 UTC
Created attachment 93084 [details] [review]
Bug 21946: DO NOT PUSH - Schema updates
Comment 55 Christopher Brannon 2019-09-21 15:17:01 UTC
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.
Comment 56 Christopher Brannon 2019-09-21 15:22:12 UTC
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.
Comment 57 Christopher Brannon 2019-09-21 16:31:37 UTC
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.
Comment 58 ByWater Sandboxes 2019-09-21 16:35:01 UTC
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>
Comment 59 ByWater Sandboxes 2019-09-21 16:35:10 UTC
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>
Comment 60 ByWater Sandboxes 2019-09-21 16:35:20 UTC
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>
Comment 61 ByWater Sandboxes 2019-09-21 16:35:25 UTC
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>
Comment 62 ByWater Sandboxes 2019-09-21 16:35:29 UTC
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>
Comment 63 ByWater Sandboxes 2019-09-21 16:35:35 UTC
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>
Comment 64 ByWater Sandboxes 2019-09-21 16:35:43 UTC
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>
Comment 65 ByWater Sandboxes 2019-09-21 16:35:48 UTC
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>
Comment 66 ByWater Sandboxes 2019-09-21 16:35:56 UTC
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>
Comment 67 ByWater Sandboxes 2019-09-21 16:36:04 UTC
Created attachment 93095 [details] [review]
Bug 21946: DO NOT PUSH - Schema updates

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Comment 68 ByWater Sandboxes 2019-09-21 16:53:39 UTC
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>
Comment 69 ByWater Sandboxes 2019-09-21 16:53:45 UTC
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>
Comment 70 ByWater Sandboxes 2019-09-21 16:53:54 UTC
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>
Comment 71 ByWater Sandboxes 2019-09-21 16:53:58 UTC
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>
Comment 72 ByWater Sandboxes 2019-09-21 16:54:05 UTC
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>
Comment 73 ByWater Sandboxes 2019-09-21 16:54:12 UTC
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>
Comment 74 ByWater Sandboxes 2019-09-21 16:54:23 UTC
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>
Comment 75 ByWater Sandboxes 2019-09-21 16:54:35 UTC
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>
Comment 76 ByWater Sandboxes 2019-09-21 16:54:41 UTC
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>
Comment 77 ByWater Sandboxes 2019-09-21 16:54:48 UTC
Created attachment 93105 [details] [review]
Bug 21946: DO NOT PUSH - Schema updates

Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>
Comment 78 Katrin Fischer 2019-10-12 12:43:57 UTC
Hi Nick, can you please rebase? ... Bit scared of the datatables code.
Comment 79 Nick Clemens 2020-01-08 12:31:19 UTC
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>
Comment 80 Nick Clemens 2020-01-08 12:31:23 UTC
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>
Comment 81 Nick Clemens 2020-01-08 12:31:27 UTC
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>
Comment 82 Nick Clemens 2020-01-08 12:31:30 UTC
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>
Comment 83 Nick Clemens 2020-01-08 12:31:34 UTC
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>
Comment 84 Nick Clemens 2020-01-08 12:31:37 UTC
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>
Comment 85 Nick Clemens 2020-01-08 12:31:41 UTC
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>
Comment 86 Nick Clemens 2020-01-08 12:31:44 UTC
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>
Comment 87 Nick Clemens 2020-01-08 12:31:48 UTC
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>
Comment 88 Nick Clemens 2020-01-08 12:31:51 UTC
Created attachment 96986 [details] [review]
Bug 21946: DO NOT PUSH - Schema updates

Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>
Comment 89 Nick Clemens 2020-01-08 12:31:55 UTC
Created attachment 96987 [details] [review]
Bug 21946: Update unit tests to pass under ES
Comment 90 Jonathan Druart 2020-01-14 10:09:53 UTC
Looks like this dev will conflict with bug 15522.
Comment 91 Nick Clemens 2020-02-14 15:21:26 UTC
(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
Comment 92 Christopher Brannon 2020-02-14 15:25:25 UTC
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?
Comment 93 Nick Clemens 2020-02-14 17:03:38 UTC
(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
Comment 94 Lisette Scheer 2020-02-20 22:54:25 UTC
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
Comment 95 Nick Clemens 2020-02-21 17:14:48 UTC
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>
Comment 96 Nick Clemens 2020-02-21 17:14:52 UTC
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>
Comment 97 Nick Clemens 2020-02-21 17:14:56 UTC
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>
Comment 98 Nick Clemens 2020-02-21 17:15:00 UTC
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>
Comment 99 Nick Clemens 2020-02-21 17:15:04 UTC
Created attachment 99423 [details] [review]
Bug 21946: Update unit tests to pass under ES
Comment 100 Nick Clemens 2020-02-21 17:15:08 UTC
Created attachment 99424 [details] [review]
Bug 21946: DO NOT PUSH - Schema updates
Comment 101 Nick Clemens 2020-02-21 17:18:30 UTC
(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 102 Alex Arnaud 2020-06-26 13:13:04 UTC
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 103 Alex Arnaud 2020-06-26 13:14:16 UTC
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 ?
Comment 104 Alex Arnaud 2020-06-26 13:14:46 UTC
Also, a rebase is needed
Comment 105 Nick Clemens 2020-07-17 15:22:23 UTC
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>
Comment 106 Nick Clemens 2020-07-17 15:22:28 UTC
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>
Comment 107 Nick Clemens 2020-07-17 15:22:32 UTC
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>
Comment 108 Nick Clemens 2020-07-17 15:22:36 UTC
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>
Comment 109 Nick Clemens 2020-07-17 15:22:40 UTC
Created attachment 107039 [details] [review]
Bug 21946: Update unit tests to pass under ES
Comment 110 Nick Clemens 2020-07-17 15:22:45 UTC
Created attachment 107040 [details] [review]
Bug 21946: Add test for parent method
Comment 111 Nick Clemens 2020-07-17 15:22:49 UTC
Created attachment 107041 [details] [review]
Bug 21946: DO NOT PUSH - Schema changes
Comment 112 Alex Arnaud 2020-07-24 08:31:41 UTC
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>
Comment 113 Alex Arnaud 2020-07-24 08:31:55 UTC
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>
Comment 114 Alex Arnaud 2020-07-24 08:32:08 UTC
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>
Comment 115 Alex Arnaud 2020-07-24 08:32:22 UTC
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>
Comment 116 Alex Arnaud 2020-07-24 08:32:33 UTC
Created attachment 107282 [details] [review]
Bug 21946: Update unit tests to pass under ES

Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 117 Alex Arnaud 2020-07-24 08:32:41 UTC
Created attachment 107283 [details] [review]
Bug 21946: Add test for parent method

Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 118 Alex Arnaud 2020-07-24 08:32:51 UTC
Created attachment 107284 [details] [review]
Bug 21946: DO NOT PUSH - Schema changes
Comment 119 Alex Arnaud 2020-07-24 08:34:47 UTC
Passed QA

Nice feature!
Comment 120 Jonathan Druart 2020-07-24 14:19:57 UTC
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
Comment 121 Owen Leonard 2020-07-28 13:22:58 UTC
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"
Comment 122 Nick Clemens 2020-07-29 12:08:53 UTC
Created attachment 107489 [details] [review]
Bug 21946: (follow-up) Add a hint
Comment 123 Nick Clemens 2020-07-29 12:11:35 UTC
(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
Comment 124 Jonathan Druart 2020-08-03 10:27:01 UTC
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
Comment 125 Jonathan Druart 2020-08-03 10:32:20 UTC
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>
Comment 126 Jonathan Druart 2020-08-03 10:32:25 UTC
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
Comment 127 Jonathan Druart 2020-08-03 10:32:30 UTC
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>
Comment 128 Jonathan Druart 2020-08-03 10:32:36 UTC
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>
Comment 129 Jonathan Druart 2020-08-03 10:32:41 UTC
Created attachment 107690 [details] [review]
Bug 21946: Update unit tests to pass under ES

Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 130 Jonathan Druart 2020-08-03 10:32:46 UTC
Created attachment 107691 [details] [review]
Bug 21946: Add test for parent method

Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 131 Jonathan Druart 2020-08-03 10:32:52 UTC
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"
Comment 132 Jonathan Druart 2020-08-03 10:32:58 UTC
Created attachment 107693 [details] [review]
Bug 21946: (follow-up) Add a hint
Comment 133 Jonathan Druart 2020-08-03 10:33:03 UTC
Created attachment 107694 [details] [review]
Bug 21946: DBIC schema changes
Comment 134 Nick Clemens 2020-08-03 12:27:41 UTC
Created attachment 107701 [details] [review]
Bug 21946: (follow-up) Restore join to biblioitems
Comment 135 Jonathan Druart 2020-08-04 10:33:25 UTC
Created attachment 107733 [details] [review]
Bug 21946: Restore defined vs ""

The tests still pass. Was the original change really useful?
Comment 136 Jonathan Druart 2020-08-04 10:34:13 UTC
Nick, I did not find a reason for this change (see last patch), what do you think?
Comment 137 Jonathan Druart 2020-08-04 10:35:23 UTC
I've written some follow-up patches that I attached to bug 26132.
Could you have a look?
Comment 138 Nick Clemens 2020-08-11 14:15:32 UTC
(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!
Comment 139 Jonathan Druart 2020-08-13 08:19:13 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 140 Lucas Gass 2020-08-17 15:33:13 UTC
New feature will not be backported to 20.05.x