Bug 22173 - Search Groups broken by change in expected structure
Summary: Search Groups broken by change in expected structure
Status: CLOSED WONTFIX
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Kyle M Hall (khall)
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-01-18 18:17 UTC by Kyle M Hall (khall)
Modified: 2021-12-13 21:10 UTC (History)
3 users (show)

See Also:
GIT URL:
Initiative type: ---
Sponsorship status: ---
Comma delimited list of Sponsors:
Crowdfunding goal: 0
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 22173: Search Groups broken by change in expected structure (6.89 KB, patch)
2019-01-18 18:21 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 22173: Search Groups broken by change in expected structure (6.94 KB, patch)
2019-01-18 19:17 UTC, Maryse Simard
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall (khall) 2019-01-18 18:17:33 UTC
The original logic of Search Groups using the heirarchical library groups was to enable search groups on a root group and have each sub group be a search group listed in the pulldown:

RootGroup:
   SearchGroup1:
        LibraryA
        LibraryB
   SearchGroup2:
        LibraryC
        LibraryD
   SearchGroup3:
        LibraryE
        LibraryF

At some point, the code was changed to expect the each search group to be the root group:

RootSearchGroup1:
    LibraryA
    LibraryB

RootSearchGroup2:
    LibraryC
    LibraryD

RootSearchGroup3:
    LibraryE
    LibraryF

It's likely that both styles are being used in production now with different versions. If a library upgrades, their search groups will be broken. If we revert the change, currently up to date libraries will have search groups broken.

Instead, we should accommodate both configurations.

So, if a root group has libraries as direct children, it should be displayed as a search group. If it does not, we should display the child search groups instead.
Comment 1 Kyle M Hall (khall) 2019-01-18 18:21:21 UTC
Created attachment 84209 [details] [review]
Bug 22173: Search Groups broken by change in expected structure

The original logic of Search Groups using the heirarchical library groups was to enable search groups on a root group and have each sub group be a search group listed in the pulldown:

RootGroup:
   SearchGroup1:
        LibraryA
        LibraryB
   SearchGroup2:
        LibraryC
        LibraryD
   SearchGroup3:
        LibraryE
        LibraryF

At some point, the code was changed to expect the each search group to be the root group:

RootSearchGroup1:
    LibraryA
    LibraryB

RootSearchGroup2:
    LibraryC
    LibraryD

RootSearchGroup3:
    LibraryE
    LibraryF

It's likely that both styles are being used in production now with different versions. If a library upgrades, their search groups will be broken. If we revert the change, currently up to date libraries will have search groups broken.

Instead, we should accommodate both configurations.

So, if a root group has libraries as direct children, it should be displayed as a search group. If it does not, we should display the child search groups instead.

Test Plan:
1) Set up a root group with multiple subgroups each with libraries,
   enable staff and opac search groups for that root group
2) Note the root group is displayed in all the search pulldowns instead
   of the subgroups
3) Apply this patch
4) Restart all the things!
5) Note the search pulldowns now contain the subgroups instead
6) Create another root group, add only libraries to it.
   Set it as a search group for staff and opac.
7) Note the search pulldowns now contain this group as well!
Comment 2 Maryse Simard 2019-01-18 19:17:35 UTC
Created attachment 84211 [details] [review]
Bug 22173: Search Groups broken by change in expected structure

The original logic of Search Groups using the heirarchical library groups was to enable search groups on a root group and have each sub group be a search group listed in the pulldown:

RootGroup:
   SearchGroup1:
        LibraryA
        LibraryB
   SearchGroup2:
        LibraryC
        LibraryD
   SearchGroup3:
        LibraryE
        LibraryF

At some point, the code was changed to expect the each search group to be the root group:

RootSearchGroup1:
    LibraryA
    LibraryB

RootSearchGroup2:
    LibraryC
    LibraryD

RootSearchGroup3:
    LibraryE
    LibraryF

It's likely that both styles are being used in production now with different versions. If a library upgrades, their search groups will be broken. If we revert the change, currently up to date libraries will have search groups broken.

Instead, we should accommodate both configurations.

So, if a root group has libraries as direct children, it should be displayed as a search group. If it does not, we should display the child search groups instead.

Test Plan:
1) Set up a root group with multiple subgroups each with libraries,
   enable staff and opac search groups for that root group
2) Note the root group is displayed in all the search pulldowns instead
   of the subgroups
3) Apply this patch
4) Restart all the things!
5) Note the search pulldowns now contain the subgroups instead
6) Create another root group, add only libraries to it.
   Set it as a search group for staff and opac.
7) Note the search pulldowns now contain this group as well!

Signed-off-by: Maryse Simard <maryse.simard@inlibro.com>
Comment 3 Katrin Fischer 2019-02-09 21:26:33 UTC
Hi Kyle, 

I have seen the issue on updating, but I am not able to create the first setup from the GUI anymore, I only get the checkbox options when I am at root level. I am not sure what the best solution is, changing the GUI to allow for both or improving the update scripts?

RootGroup
  Searchgroup 

Can you take a look?
Comment 4 Katrin Fischer 2019-02-09 21:27:49 UTC
Hm, I think I misread the test plan. 
So the 'attributes' are set top level and then inherited?
Comment 5 Katrin Fischer 2019-02-09 21:32:37 UTC
I am still not sure if the fix is right :(

My reasoning is that people might have added sub groups for organizing relying on the fact that they will not take effect. So changing it one way or the other now will make someone unhappy.

Allowing only the root group settings seems the more logical path to me.

Also: An empty root group won't show up (good), but an empty sub group will.
Comment 6 Lisette Scheer 2019-05-17 16:11:59 UTC
If the decision is to allow root groups only, is there a way to give an option on subgroups for people to convert subgroups to root groups? 

If so, that would help people who have it set up in the original logic switch over without much trouble. 

Lisette
Comment 7 Kyle M Hall (khall) 2019-05-17 17:27:49 UTC
(In reply to Katrin Fischer from comment #3)
> Hi Kyle, 
> 
> I have seen the issue on updating, but I am not able to create the first
> setup from the GUI anymore, I only get the checkbox options when I am at
> root level. I am not sure what the best solution is, changing the GUI to
> allow for both or improving the update scripts?
> 
> RootGroup
>   Searchgroup 
> 
> Can you take a look?

Right, you only enable this at the root level. The idea is to have a single group per-feature. We definitely wouldn't want a given feature to be turned on or off in different subgroups of a root group.
Comment 8 Kyle M Hall (khall) 2019-05-17 17:31:00 UTC
(In reply to Katrin Fischer from comment #5)
> I am still not sure if the fix is right :(
> 
> My reasoning is that people might have added sub groups for organizing
> relying on the fact that they will not take effect. So changing it one way
> or the other now will make someone unhappy.

I think we have the best compromise here at this time.

> Allowing only the root group settings seems the more logical path to me.

I'm not sure I understand. Are you saying you want to codify this regression?

> 
> Also: An empty root group won't show up (good), but an empty sub group will.

I'd be happy to look at that after we get this cleared up.
Comment 9 Kyle M Hall (khall) 2021-01-13 19:18:18 UTC
Closing this bug out. At this point we've had the new behavior longer than we had the old behavior and I expect it's simpler for everyone to change their groups hierarchy rather than push this patch.

For posterity, the fix is to make new root groups for each of your first tier sub-groups, and enable the search group feature for each of them.

That means changing something like this:

RootGroup:
   SearchGroup1:
        LibraryA
        LibraryB
   SearchGroup2:
        LibraryC
        LibraryD
   SearchGroup3:
        LibraryE
        LibraryF

to this:

RootSearchGroup1:
    LibraryA
    LibraryB

RootSearchGroup2:
    LibraryC
    LibraryD

RootSearchGroup3:
    LibraryE
    LibraryF