Obvious test case to check for fails does.
Make root
Make sub group
Add some libraries off both.
Attempt to delete root.
"Software error:

DBIx::Class::Storage::DBI::_dbh_execute(): Cannot delete or update a parent row: a foreign key constraint fails (`koha_library`.`library_groups`, CONSTRAINT `library_groups_ibfk_1` FOREIGN KEY (`parent_id`) REFERENCES `library_groups` (`id`)) at /home/mtompset/kohaclone/Koha/Object.pm line 155
For help, please send mail to the webmaster (webmaster@debian), giving this error message and the time and date of the error."

Treegrid library does add.
Updates both fresh and update work.
Given that the Unit Tests work, I believe the new modules and schema files function.
There is a nice about page entry.
So, this is purely an "Add editor" patch issue.

Deletion from bottom up does work, however. :)

(In reply to M. Tompsett from comment #15)
> Deletion from bottom up does work, however. :)

Thanks for testing! Deleting a group with children should have worked, I'm guessing my ON DELETE isn't quite right. I'll look into it!

I've been unable to reproduce your error. Here's is a video of my testing: http://screencast.com/t/cM1Duz9y

Am I not taking the same steps as you were? Can you reproduce the issue repeatedly?

(In reply to Kyle M Hall from comment #17)
> I've been unable to reproduce your error. Here's is a video of my testing:
> http://screencast.com/t/cM1Duz9y
> 
> Am I not taking the same steps as you were? Can you reproduce the issue
> repeatedly?

One question.
Were you running from user account or system account?
Where you running from an upgrade or a fresh install?

My scenario, I drop'd the db, created an empty one, did the web install and was using the system account.

root -> group 1 -> subgroup 1 & subgroup 2
add libraries to subgroup 1 & subgroup 2
attempt to delete root.

I'll reattempt after lunch.

(In reply to M. Tompsett from comment #18)

For the record, I was using a user account and tested via upgrade.

Preliminary setup: http://screencast.com/t/AtYoC3pJY
Actual test explosion: http://screencast.com/t/SgusNdiT

I noticed in the replay that Koha Admin is Midway, and Midway was one of the libraries I added. I don't know if that has anything to do with it, but I thought I'd mention that.

(In reply to M. Tompsett from comment #20)
> Preliminary setup: http://screencast.com/t/AtYoC3pJY
> Actual test explosion: http://screencast.com/t/SgusNdiT
> 
> I noticed in the replay that Koha Admin is Midway, and Midway was one of the
> libraries I added. I don't know if that has anything to do with it, but I
> thought I'd mention that.

Thanks! I was able to pinpoint the issue thanks to your screencasts. There was missing ON DELETE CASCADE for the parent id column. The cascade existed in the update sql so that's why I wasn't getting the error but you were! This issue should be fixed with this new patch set!

Failed test case is corrected.
Prove works fine.
About is there.
Applies nicely.

Kyle,
1/ Could you provide a test plan?
2/ What's next for these library groups? What's the difference with our existing groups? Is there a plan to move the existing ones to this new table?
3/ We already have a treetable plugin (compatible with DataTables), is there a reason to add another one?

Marked as Failed QA for 1.

(In reply to Jonathan Druart from comment #37)
> Kyle,
> 1/ Could you provide a test plan?

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Browse to Administration/Library groups
4) Create a new group via the add group button
5) Create one or more subgroups in this group
6) Add libraries to the group and subgroups
7) Add another top level group via the add group button
8) Try adding groups and libraries to this group
9) Test editing a group
10) Test deleting a group
11) Test removing a library from a group

> 2/ What's next for these library groups? What's the difference with our
> existing groups? Is there a plan to move the existing ones to this new table?
> 3/ We already have a treetable plugin (compatible with DataTables), is there
> a reason to add another one?

I had thought the treetable plugin wouldn't work well in this instance, but I gave treetables a shot and it looks almost identical. I've removed the patch that adds the treegrid library and have added a follow switching to treetables.

(In reply to Jonathan Druart from comment #37)
> 2/ What's next for these library groups? What's the difference with our
> existing groups? Is there a plan to move the existing ones to this new table?

Bug 15708 is my first enhancement that will take advantage of this. However, I think there will be many other possible uses for this system in the future. The existing groups function is not actually used anywhere in Koha and should probably be removed. The search groups could go either way, but are fine the way they are.

What makes this far more powerful is the ability to define hierarchical group structures. This will be a cornerstone for making Koha much more usable in consortial library settings.

(In reply to Kyle M Hall from comment #49)
> (In reply to Jonathan Druart from comment #37)
> > 2/ What's next for these library groups? What's the difference with our
> > existing groups? Is there a plan to move the existing ones to this new table?
> 
> Bug 15708 is my first enhancement that will take advantage of this. However,
> I think there will be many other possible uses for this system in the
> future. The existing groups function is not actually used anywhere in Koha
> and should probably be removed. The search groups could go either way, but
> are fine the way they are.
> 
> What makes this far more powerful is the ability to define hierarchical
> group structures. This will be a cornerstone for making Koha much more
> usable in consortial library settings.

It is weird to have 2 "library groups" features and I think the first step should be to replace the existing one. The existing feature is used in the advanced search form and should work.
Moreover I'd like to get Owen opinion on the modal you use to create/update a new entry. I am going to ping him on IRC.
There are also some inconsistencies on styles, the confirmation of add/update is a warning (orange)

Keeping it in the Signed Off queue to get other opinions.

Also it might be odd to have a new feature not doing anything (if pushed as it) :)

The following patch could be obsoleted: Bug 15707 - Add TreeGrid library to about page

> (In reply to Jonathan Druart from comment #59)
> It is weird to have 2 "library groups" features and I think the first step
> should be to replace the existing one.

I agree. It is confusing to have two menu items on the administration page, "Libraries and groups" and "Library groups." I think we should think of a way to manage all this functionality in one section.

Some other comments:

- Nowhere else do we use the "fa-plus-circle" icon for "add." We always use "fa-plus."

- Nowhere else in Koha do we use a modal to display an add form. We either display a form on a separate page or we use JS to show/hide the edit form on the page to mimic the display of a separate page. See admin/items_search_fields.pl for example.

- Nowhere else in Koha do we use the "fa-edit" icon for "edit." We always use "fa-pencil."

- When displaying confirmation messages we use '<div class="dialog message">' not '<div class="dialog alert">'

- There is no reason to display an icon ("<i class="fa fa-object-group"></i>") next to each group. It doesn't serve any function.

- There is no reason to display an icon ("<i class="fa fa-home"></i>") next to each library in a group. It doesn't serve any function.

- Nowhere else in Koha do we use "fa-minus-circle" to mean "remove." It should be "fa-trash."

- Nowhere else in Koha do we add an icon to the "actions" menu.

- Nowhere else in Koha do we use a modal to display a delete confirmation message. I think I could be convinced that we should, but I'd like to hear the argument. If we do, we should use the space given to display more information about the deletion than this implementation does, e.g. what exactly am I deleting?

- Instead of a "home" icon for "Add library" I think it should be the "plus" icon which is associated with adding or creating something.

- The list of libraries in the "Add library" dropdown should be alphabetized by library name, not library code.

- This patch adds five new instances of "onclick." I've been working very hard to eliminate the use of "onclick" and I don't think new patches should add them.

> I agree. It is confusing to have two menu items on the administration page,
> "Libraries and groups" and "Library groups." I think we should think of a
> way to manage all this functionality in one section.

These two sub-systems are completely unrelated. They really don't belong together. I'd be more than happy to rename it "Library hierarchies" if you find that acceptable, or to rename the existing groups to "search groups" which is what they are.
 
> Some other comments:
> 
> - Nowhere else do we use the "fa-plus-circle" icon for "add." We always use
> "fa-plus."

Changed with followup.

> 
> - Nowhere else in Koha do we use a modal to display an add form. We either
> display a form on a separate page or we use JS to show/hide the edit form on
> the page to mimic the display of a separate page. See
> admin/items_search_fields.pl for example.

You are correct, but I believe the modal may be a better paradigm. Would you disagree? It means we aren't shifting elements on the page around when we hide/display a form. I know it's without precedent at the moment but I'd like you to consider the advantages. Think about how every other site does this stuff. It's typically with modals. Koha is generally working against the standard web ux by *not* using modals! Would you agree?

> 
> - Nowhere else in Koha do we use the "fa-edit" icon for "edit." We always
> use "fa-pencil."

Changed with followup!
> 
> - When displaying confirmation messages we use '<div class="dialog
> message">' not '<div class="dialog alert">'

Can do!

> 
> - There is no reason to display an icon ("<i class="fa
> fa-object-group"></i>") next to each group. It doesn't serve any function.

Changed in followup!

> 
> - There is no reason to display an icon ("<i class="fa fa-home"></i>") next
> to each library in a group. It doesn't serve any function.

Changed in followup!

> 
> - Nowhere else in Koha do we use "fa-minus-circle" to mean "remove." It
> should be "fa-trash."

Fixed in followup!

> 
> - Nowhere else in Koha do we add an icon to the "actions" menu.

Will do!

> - Nowhere else in Koha do we use a modal to display a delete confirmation
> message. I think I could be convinced that we should, but I'd like to hear
> the argument. If we do, we should use the space given to display more
> information about the deletion than this implementation does, e.g. what
> exactly am I deleting?

We use plenty of js confirm dialogs for delete confirmation messages. Over the years they've gotten more and more jarring in my opinion. They cannot be styled to match the look of Koha. Even more importantly, we tend to abuse confirm dialogs by adding far to much text to them which we attempt to lay out with only spaces and newlines. With the modal this is a non-issue because we can simply use html. The modal seems much more natural and integrated, while presenting the exact same options to the user.

> 
> - Instead of a "home" icon for "Add library" I think it should be the "plus"
> icon which is associated with adding or creating something.

Fixed!

> 
> - The list of libraries in the "Add library" dropdown should be alphabetized
> by library name, not library code.

Fixed!

> - This patch adds five new instances of "onclick." I've been working very
> hard to eliminate the use of "onclick" and I don't think new patches should
> add them.

Fixed! I wrote this patch before the move away from onclick and I'm happy to see them go ; )

(In reply to Kyle M Hall from comment #63)

> These two sub-systems are completely unrelated. They really don't belong
> together.

If this is a feature without any real application in Koha yet, perhaps we shouldn't add it to the menu system at all?

> > - Nowhere else in Koha do we use a modal to display an add form.
>
> You are correct, but I believe the modal may be a better paradigm.

Perhaps. However, I think introducing a new interface pattern requires a certain amount of commitment to making that change consistently throughout the interface. For example, using jEditable for the Quote of the Day feature was an interesting proof of concept, but it went nowhere after that. Introducing the use of Font Awesome was a good improvement, but it took a lot of buy-in and work to make it consistent.

> > - Nowhere else in Koha do we use a modal to display a delete confirmation
> > message.
>
> The modal seems much more natural and
> integrated, while presenting the exact same options to the user.

I think that's fine, especially if we take advantage of the opportunity to display thorough information to the user. As I said above, I think making the change requires /some/ commitment to implementing the same change throughout the staff client.

> I've removed the
> patch that adds the treegrid library and have added a follow switching to
> treetables.

Treegrid is still getting added to the about page.

(In reply to Owen Leonard from comment #65)
> (In reply to Kyle M Hall from comment #63)
> 
> > These two sub-systems are completely unrelated. They really don't belong
> > together.
> 
> If this is a feature without any real application in Koha yet, perhaps we
> shouldn't add it to the menu system at all?

I don't expect this patch set to be pushed to master until Bug 15708 is ready to be pushed along with it. I would like to get this to Passed QA in the meantime.

> 
> > > - Nowhere else in Koha do we use a modal to display an add form.
> >
> > You are correct, but I believe the modal may be a better paradigm.
> 
> Perhaps. However, I think introducing a new interface pattern requires a
> certain amount of commitment to making that change consistently throughout
> the interface. For example, using jEditable for the Quote of the Day feature
> was an interesting proof of concept, but it went nowhere after that.
> Introducing the use of Font Awesome was a good improvement, but it took a
> lot of buy-in and work to make it consistent.

I agree. I'm willing to put in some work if you can identify some areas where you'd like to see modals. I think the low-hanging fruit would be any of the hide/show forms we have. The audio alerts editor is the first one that comes to mind, but I'm sure you can point out some others!

> 
> > > - Nowhere else in Koha do we use a modal to display a delete confirmation
> > > message.
> >
> > The modal seems much more natural and
> > integrated, while presenting the exact same options to the user.
> 
> I think that's fine, especially if we take advantage of the opportunity to
> display thorough information to the user. As I said above, I think making
> the change requires /some/ commitment to implementing the same change
> throughout the staff client.

Agreed, this latest followup takes advantage of that a bit be showing the group to be deleted in one case, in the case of removing a library from a group it shows the library name and the group it will be removed from with some minor styling. Nothing amazing but definitely easier on the eyes and no unnecessary use of quotes around the names of things because the confirm dialogs are only plaintext. If you think my styling could be improved just let me know! Basically it's just styled with the group and library names italicized and bold. I think we need a standard pattern for how to style the names of things in this fashion, and I see no reason not to start here.

(In reply to Kyle M Hall from comment #63)
> > I agree. It is confusing to have two menu items on the administration page,
> > "Libraries and groups" and "Library groups." I think we should think of a
> > way to manage all this functionality in one section.
> 
> These two sub-systems are completely unrelated.

Do you not plan to replace the existing library groups functionality with this?

The discussion about the modal should be brought at the next dev meeting I think:
Is everybody (i.e. the majority) agrees on this change, who is going to implement it to the other pages, etc.

(In reply to Owen Leonard from comment #69)
> (In reply to Kyle M Hall from comment #63)
> > > I agree. It is confusing to have two menu items on the administration page,
> > > "Libraries and groups" and "Library groups." I think we should think of a
> > > way to manage all this functionality in one section.
> > 
> > These two sub-systems are completely unrelated.
> 
> Do you not plan to replace the existing library groups functionality with
> this?

I had not thought about it, but it would make a log of sense! That should definitely be a separate bug to limit scope creep, but I think it's an excellent idea. After that we could do away with the existing groups functionality altogether.

(In reply to Jonathan Druart from comment #70)
> The discussion about the modal should be brought at the next dev meeting I
> think:
> Is everybody (i.e. the majority) agrees on this change, who is going to
> implement it to the other pages, etc.

That sounds good to me. I think we should vote to add this behavior as the preferred "one page" viewer/editor style, deprecating the older "hide/show" style in preference to it. Once that is voted on, we can identify pages that use the "hide/show" style and file bugs to replace each one. I'd be happy to work on those bugs. I think using the modal is great ux improvement and it would be worthwhile to make that change throughout Koha.

>Basically it's just styled with the group and library names italicized and >bold. I think we need a standard pattern for how to style the names of things >in this fashion, and I see no reason not to start here.

Please don't do this! It breaks translation by splitting the sentence into multiple parts in the po files. Until that is fixed, using in-sentence mark-up is a really bad thing for translators. Using TT variables works fine and they get replaced by %s, but <b> and <i> mid-sentence are bad.

Talking about these for an example:

+    <div class="dialog alert group-added">
+        [% IF added.branchcode %]
+            <i>[% added.library.branchname %]</i> added to group.
+        [% ELSE %]
+            Group <i>[% added.title %]</i> created.
+        [% END %]
+    </div>

Second example in the po file is:
- Group
- created
Instead of: Group %s created.

(In reply to Katrin Fischer from comment #74)
> Talking about these for an example:
> 
> +    <div class="dialog alert group-added">
> +        [% IF added.branchcode %]
> +            <i>[% added.library.branchname %]</i> added to group.
> +        [% ELSE %]
> +            Group <i>[% added.title %]</i> created.
> +        [% END %]
> +    </div>
> 
> Second example in the po file is:
> - Group
> - created
> Instead of: Group %s created.

What if we were to take advantage of TT to "hide" the markup for translations?
Instead of:
Group <i>[% added.title %]</i> created.
we could do this:
Group [% "<i>" _ added.title _ "</i>" %] created.

That way, we can still emphasize names, but the markup will not show. We could even create a filter if we standardize this so we could do something like this:

Group [% added.title | $KohaSpan id => 'myId' class => 'name'  %] created.

That solves the problem of translations while still allowing us to style particular elements in a sentence!

What do you think?

If that works with the po files - it's totally ok for me. I can see how highlighting words or pre-formatting things can be useful.

(In reply to Kyle M Hall from comment #75)
> (In reply to Katrin Fischer from comment #74)
> > Talking about these for an example:
> > 
> > +    <div class="dialog alert group-added">
> > +        [% IF added.branchcode %]
> > +            <i>[% added.library.branchname %]</i> added to group.
> > +        [% ELSE %]
> > +            Group <i>[% added.title %]</i> created.
> > +        [% END %]
> > +    </div>
> > 
> > Second example in the po file is:
> > - Group
> > - created
> > Instead of: Group %s created.
> 
> What if we were to take advantage of TT to "hide" the markup for
> translations?
> Instead of:
> Group <i>[% added.title %]</i> created.
> we could do this:
> Group [% "<i>" _ added.title _ "</i>" %] created.
> 
> That way, we can still emphasize names, but the markup will not show. We
> could even create a filter if we standardize this so we could do something
> like this:
> 
> Group [% added.title | $KohaSpan id => 'myId' class => 'name'  %] created.
> 
> That solves the problem of translations while still allowing us to style
> particular elements in a sentence!
> 
> What do you think?

That sounds interesting, see: 
http://template-toolkit.org/docs/manual/Directives.html

---start snip---

You can concatenate strings together using the ' _ ' operator. In Perl 5, the . is used for string concatenation, but in Perl 6, as in the Template Toolkit, the . will be used as the method calling operator and ' _ ' will be used for string concatenation. Note that the operator must be specified with surrounding whitespace which, as Larry says, is construed as a feature:

[% copyright = '(C) Copyright' _ year _ ' ' _ author %]

You can, of course, achieve a similar effect with double quoted string interpolation.

[% copyright = "(C) Copyright $year $author" %]

---end snip---

(In reply to Kyle M Hall from comment #75)
(...)
> That way, we can still emphasize names, but the markup will not show. We
> could even create a filter if we standardize this so we could do something
> like this:
>Group [% added.title | $KohaSpan id => 'myId' class => 'name'  %] created.
(...)

Hi Kyle,
I tried the follwing:

Group [% '<i>' _ added.title | $KohaSpan id => 'myId' class => 'name' _ '</i>'  %] created.
...but it results in a template error. 

A workaround would be:

[% myvalue = added.title | $KohaSpan id => 'myId' class => 'name' %]
Group [% '<i>' _ myvalue _ '</i>'  %] created.

Marc, did you create a KohaSpan filter? If not, that would cause your error. I will write that and add it to this bug!

Created attachment 52563 [details] [review]
Bug 15707 - Update DB

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Created attachment 52564 [details] [review]
Bug 15707 - Add new modules

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Created attachment 52565 [details] [review]
Bug 15707 - Add editor

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Created attachment 52566 [details] [review]
Bug 15707 - Add and update schema files

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Created attachment 52567 [details] [review]
Bug 15707 - Add TreeGrid library to about page

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Created attachment 52568 [details] [review]
Bug 15707 - Unit Tests

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Created attachment 52569 [details] [review]
Bug 15707: Fix conflict with bug 15446 (type vs _type)

Created attachment 52570 [details] [review]
Bug 15707 [QA Followup] - Switch to treetable which Koha already uses

Created attachment 52571 [details] [review]
Bug 15707: QA follow-up - Drop table if exists

1/ DROP table if exists
2/ FAIL   spelling
    decendents  ==> descendants

Created attachment 52572 [details] [review]
Bug 15707 [QA Followup] - Fix UI issues

Created attachment 52573 [details] [review]
Bug 15707 [QA Followup] - Remove class for treegrid

Created attachment 52575 [details] [review]
Bug 15707 [QA Followup] - Allow object names to be styled without impeding translation

What is the status of this?

IMO we should wait for bug 16735 before pushing this one. It won't make sense to have 15707 without 16735.

Is this still moving.. looks interesting

Created attachment 57080 [details] [review]
Bug 15707 - Update DB

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Created attachment 57081 [details] [review]
Bug 15707 - Update DB

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Created attachment 57082 [details] [review]
Bug 15707 - Update DB

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Created attachment 57083 [details] [review]
Bug 15707 - Add new modules

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Created attachment 57084 [details] [review]
Bug 15707 - Add editor

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Created attachment 57085 [details] [review]
Bug 15707 - Add TreeGrid library to about page

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Created attachment 57086 [details] [review]
Bug 15707 - Unit Tests

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Created attachment 57087 [details] [review]
Bug 15707: Fix conflict with bug 15446 (type vs _type)

Created attachment 57088 [details] [review]
Bug 15707 [QA Followup] - Switch to treetable which Koha already uses

Created attachment 57089 [details] [review]
Bug 15707: QA follow-up - Drop table if exists

1/ DROP table if exists
2/ FAIL   spelling
    decendents  ==> descendants

Created attachment 57090 [details] [review]
Bug 15707 [QA Followup] - Fix UI issues

Created attachment 57091 [details] [review]
Bug 15707 [QA Followup] - Remove class for treegrid

Created attachment 57092 [details] [review]
Bug 15707 [QA Followup] - Allow object names to be styled without impeding translation

Created attachment 57093 [details] [review]
Bug 15707 - Update Schema Files

Created attachment 58086 [details] [review]
Bug 15707: Followup - use cat-search instead of cities-search in header

When testing bug 16735, I found, that in header is used cities search form, I fixed it in followup as it is an easy fix.

QA comments (with bug 16735):

1/ treegrid plugin patches should be removed
2/ updatedb: I think we decided not to use Koha code from the updatedb process
3/ I have added a library to  __SEARCH_GROUPS__, on the adv search form, the dropdown list contains an empty line, if I select it, the result list is empty
4/ There are 2 occurrences of Koha::Library->get_categories in C4/Circulation.pm
5/ There is a lack of tests, some new subroutines are not tested
6/ DB structure
- updated_on could be "timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP". That would avoid the trick in ->store
- Don't we want a constraint a the library_groups table? A unique key on title for instance.

(In reply to Owen Leonard from comment #62)

> - Nowhere else in Koha do we use a modal to display an add form. We either
> display a form on a separate page or we use JS to show/hide the edit form on
> the page to mimic the display of a separate page. See
> admin/items_search_fields.pl for example.

> - Nowhere else in Koha do we add an icon to the "actions" menu.

> - Nowhere else in Koha do we use a modal to display a delete confirmation
> message. I think I could be convinced that we should, but I'd like to hear
> the argument. If we do, we should use the space given to display more
> information about the deletion than this implementation does, e.g. what
> exactly am I deleting?


This is still valid.

(In reply to Kyle M Hall from comment #142)
> Created attachment 60586 [details] [review] [review]
> Bug 15707 - Add UNIQUE constraint to library groups table

This works but explodes if happens (DBD::mysql::st execute failed: Duplicate entry 'xxx' for key 'title')

- Empty groups (without libraries) are displayed in the adv search form, I do not think they should.

- If Search groups contain several levels of subgroups, only the first one will be used. Not sure it's the expected behaviour.

Example:
__SEARCH_GROUP__
  Group 1
    Library 1
    Subgroup
      Library 2

Search on "Group 1" will search in library 1.

Comment 144 in discussion

Failing QA for comments 145 and 146.

Also note that last DB changes have not been propagated to schema files.

Created attachment 60755 [details] [review]
Bug 15707 - Update DB

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Created attachment 60756 [details] [review]
Bug 15707 - Update DB

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Created attachment 60757 [details] [review]
Bug 15707 - Add new modules

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Created attachment 60758 [details] [review]
Bug 15707 - Add editor

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Created attachment 60759 [details] [review]
Bug 15707 - Unit Tests

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Created attachment 60760 [details] [review]
Bug 15707: Fix conflict with bug 15446 (type vs _type)

Created attachment 60761 [details] [review]
Bug 15707 [QA Followup] - Switch to treetable which Koha already uses

Created attachment 60762 [details] [review]
Bug 15707: QA follow-up - Drop table if exists

1/ DROP table if exists
2/ FAIL   spelling
    decendents  ==> descendants

Created attachment 60763 [details] [review]
Bug 15707 [QA Followup] - Fix UI issues

Created attachment 60764 [details] [review]
Bug 15707 [QA Followup] - Remove class for treegrid

Created attachment 60765 [details] [review]
Bug 15707 [QA Followup] - Allow object names to be styled without impeding translation

Created attachment 60766 [details] [review]
Bug 15707 - Update Schema Files

Created attachment 60767 [details] [review]
Bug 15707: Followup - use cat-search instead of cities-search in header

Created attachment 60768 [details] [review]
Bug 15707 - Switch datetimes to timestamps

Created attachment 60769 [details] [review]
Bug 15707 - Add UNIQUE constraint to library groups table

Created attachment 60770 [details] [review]
Bug 15707 - Updates for Bootstrap 3

Created attachment 60771 [details] [review]
Bug 15707 - Display error if group title is already used

(In reply to Jonathan Druart from comment #146)
> - Empty groups (without libraries) are displayed in the adv search form, I
> do not think they should.

Fixed!
 
> - If Search groups contain several levels of subgroups, only the first one
> will be used. Not sure it's the expected behaviour.
> 
> Example:
> __SEARCH_GROUP__
>   Group 1
>     Library 1
>     Subgroup
>       Library 2
> 
> Search on "Group 1" will search in library 1.

That is expected behavior and will be documented in the manual. There is no additional utility for this particular feature to sub-groups.

(In reply to Kyle M Hall from comment #166)
> > - If Search groups contain several levels of subgroups, only the first one
> > will be used. Not sure it's the expected behaviour.
> > 
> > Example:
> > __SEARCH_GROUP__
> >   Group 1
> >     Library 1
> >     Subgroup
> >       Library 2
> > 
> > Search on "Group 1" will search in library 1.
> 
> That is expected behavior and will be documented in the manual. There is no
> additional utility for this particular feature to sub-groups.

If L1 is in a subgroup that is in a group, L1 is not considered part of the group?
This behaviour sounds very weird.
Why did you choose this behaviour?

(In reply to Jonathan Druart from comment #167)
> (In reply to Kyle M Hall from comment #166)
> > > - If Search groups contain several levels of subgroups, only the first one
> > > will be used. Not sure it's the expected behaviour.
> > > 
> > > Example:
> > > __SEARCH_GROUP__
> > >   Group 1
> > >     Library 1
> > >     Subgroup
> > >       Library 2
> > > 
> > > Search on "Group 1" will search in library 1.
> > 
> > That is expected behavior and will be documented in the manual. There is no
> > additional utility for this particular feature to sub-groups.
> 
> If L1 is in a subgroup that is in a group, L1 is not considered part of the
> group?
> This behaviour sounds very weird.
> Why did you choose this behaviour?

For efficiency. If you think it makes more sense to have all libraries in all subgroups selected as well I'd be happy to change it.

(In reply to Kyle M Hall from comment #168)
> For efficiency. If you think it makes more sense to have all libraries in
> all subgroups selected as well I'd be happy to change it.

To me it does not make sense not to go all over the whole tree, I think it would be good to get other opinions.

I've not really been following this one too closely yet, though it is interesting.

I agree that we need a clear inheritance model defined, and personally would agree with the above comments.

As a further note, I see you've chosen to use Adjacency List, and you've coded the queries yourself.  Did you investigate any of the other options for storing hierarchies in a database.. Nested Set or Materialized Path perhaps, and did you not consider leaning on the shoulders of giants and using one of the DBIx::Class modules to construct the queries for you in the most optimal way for the data you require?

It's important to consider future use cases, each hierarchical data model has it's strengths and it's weaknesses in terms of performance and space requirements.  In general, I think Materialized Path is the defacto recommended way of doing hierarchies in relational databases now.

Is there a 'functional requirements' document anywhere for all this.. the more I read the code and linked bugs the more confused I get about what it's actually trying to achieve :(

(In reply to Martin Renvoize from comment #171)
> Is there a 'functional requirements' document anywhere for all this.. the
> more I read the code and linked bugs the more confused I get about what it's
> actually trying to achieve :(

Bug 10276 - RIP.

Created attachment 65666 [details] [review]
Bug 15707 - Update DB

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Created attachment 65667 [details] [review]
Bug 15707 - Add new modules

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Created attachment 65668 [details] [review]
Bug 15707 - Add editor

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Created attachment 65669 [details] [review]
Bug 15707 - Unit Tests

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Created attachment 65670 [details] [review]
Bug 15707: Fix conflict with bug 15446 (type vs _type)

Created attachment 65671 [details] [review]
Bug 15707 [QA Followup] - Switch to treetable which Koha already uses

Created attachment 65672 [details] [review]
Bug 15707: QA follow-up - Drop table if exists

1/ DROP table if exists
2/ FAIL   spelling
    decendents  ==> descendants

Created attachment 65673 [details] [review]
Bug 15707 [QA Followup] - Fix UI issues

Created attachment 65674 [details] [review]
Bug 15707 [QA Followup] - Remove class for treegrid

Created attachment 65675 [details] [review]
Bug 15707 [QA Followup] - Allow object names to be styled without impeding translation

Created attachment 65676 [details] [review]
Bug 15707 - Update Schema Files

Created attachment 65677 [details] [review]
Bug 15707: Followup - use cat-search instead of cities-search in header

Created attachment 65678 [details] [review]
Bug 15707 - Switch datetimes to timestamps

Created attachment 65679 [details] [review]
Bug 15707 - Add UNIQUE constraint to library groups table

Created attachment 65680 [details] [review]
Bug 15707 - Updates for Bootstrap 3

Created attachment 65681 [details] [review]
Bug 15707 - Display error if group title is already used

Created attachment 69514 [details] [review]
Bug 15707 - Update DB

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Created attachment 69515 [details] [review]
Bug 15707 - Add new modules

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Created attachment 69516 [details] [review]
Bug 15707 - Add editor

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Created attachment 69517 [details] [review]
Bug 15707 - Unit Tests

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Created attachment 69518 [details] [review]
Bug 15707: Fix conflict with bug 15446 (type vs _type)

Created attachment 69519 [details] [review]
Bug 15707 [QA Followup] - Switch to treetable which Koha already uses

Created attachment 69520 [details] [review]
Bug 15707: QA follow-up - Drop table if exists

1/ DROP table if exists
2/ FAIL   spelling
    decendents  ==> descendants

Created attachment 69521 [details] [review]
Bug 15707 [QA Followup] - Fix UI issues

Created attachment 69522 [details] [review]
Bug 15707 [QA Followup] - Remove class for treegrid

Created attachment 69523 [details] [review]
Bug 15707 [QA Followup] - Allow object names to be styled without impeding translation

Created attachment 69524 [details] [review]
Bug 15707: Followup - use cat-search instead of cities-search in header

Created attachment 69525 [details] [review]
Bug 15707 - Switch datetimes to timestamps

Created attachment 69526 [details] [review]
Bug 15707 - Add UNIQUE constraint to library groups table

Created attachment 69527 [details] [review]
Bug 15707 - Updates for Bootstrap 3

Created attachment 69528 [details] [review]
Bug 15707 - Display error if group title is already used

Created attachment 69529 [details] [review]
Bug 15707: DBIC Schema files

Patches have been rebased.

Created attachment 69573 [details] [review]
Bug 15707: Update DB

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 69574 [details] [review]
Bug 15707: Add Koha::Library::Group(s) modules

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 69575 [details] [review]
Bug 15707: Add library groups editor

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 69576 [details] [review]
Bug 15707: Unit Tests

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 69577 [details] [review]
Bug 15707: Fix conflict with bug 15446 (type vs _type)

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 69578 [details] [review]
Bug 15707: (QA followup) Switch to treetable which Koha already uses

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 69579 [details] [review]
Bug 15707: (QA followup) Drop table if exists

1/ DROP table if exists
2/ FAIL   spelling
    decendents  ==> descendants

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 69580 [details] [review]
Bug 15707: (QA followup) Fix UI issues

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 69581 [details] [review]
Bug 15707: (QA followup) Remove class for treegrid

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 69582 [details] [review]
Bug 15707: (QA followup) Allow object names to be styled without impeding translation

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 69583 [details] [review]
Bug 15707: (followup) use cat-search instead of cities-search in header

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 69584 [details] [review]
Bug 15707: Switch datetimes to timestamps

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 69585 [details] [review]
Bug 15707: Add UNIQUE constraint to library groups table

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 69586 [details] [review]
Bug 15707: Updates for Bootstrap 3

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 69587 [details] [review]
Bug 15707: Display error if group title is already used

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Edit: I added !$branchcode && to the checked condition so we can add multiple
libraries back.

Created attachment 69588 [details] [review]
Bug 15707: DBIC Schema files

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 69589 [details] [review]
Bug 15707: (QA followup) Style buttons correctly

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

library_groups.created_on and updated_on are DATETIME in kohastructure but timestamp in atomic update.

Patches do not apply but rebased on https://github.com/joubu/Koha/commits/bug_18403

(In reply to Jonathan Druart from comment #223)
> library_groups.created_on and updated_on are DATETIME in kohastructure but
> timestamp in atomic update.

I kept TIMESTAMP

Pushed to master for 18.05, thanks to everybody involved!

Awesome work all! New feature, not backported to stable.