Bug 31784 - SingleBranchMode ignores public flag
Summary: SingleBranchMode ignores public flag
Status: CLOSED WONTFIX
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Marcel de Rooy
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on:
Blocks: 31817 31816
  Show dependency treegraph
 
Reported: 2022-10-14 06:17 UTC by Marcel de Rooy
Modified: 2023-06-08 22:26 UTC (History)
2 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 31784: Add Libraries->singleBranchMode (2.23 KB, patch)
2022-10-14 07:03 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31784: Replace some singleBranchMode occurrences (4.54 KB, patch)
2022-10-14 07:03 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31784: Add Libraries->singleBranchMode (2.24 KB, patch)
2022-10-14 07:39 UTC, David Nind
Details | Diff | Splinter Review
Bug 31784: Replace some singleBranchMode occurrences (4.54 KB, patch)
2022-10-14 07:39 UTC, David Nind
Details | Diff | Splinter Review
Bug 31784: (QA follow-up) Snake case for subroutine (4.41 KB, patch)
2022-10-14 10:07 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31784: Add Libraries->singleBranchMode (2.30 KB, patch)
2022-10-15 19:23 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 31784: Replace some singleBranchMode occurrences (4.60 KB, patch)
2022-10-15 19:23 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 31784: (QA follow-up) Snake case for subroutine (4.43 KB, patch)
2022-10-15 19:23 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 31784: Add Libraries->singleBranchMode (2.37 KB, patch)
2022-10-17 13:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 31784: Replace some singleBranchMode occurrences (4.67 KB, patch)
2022-10-17 13:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 31784: (QA follow-up) Cleanup (1.27 KB, patch)
2022-10-17 13:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2022-10-14 06:17:26 UTC
The test in C4/Auth.pm:
singleBranchMode   => ( Koha::Libraries->search->count == 1 ),
misses a fundamental condition, i.e. public should be true.

C4/XSLT.pm:    my $singleBranchMode = Koha::Libraries->search->count == 1 ? 1 : 0;

t/Koha.t:    is ( Koha::Libraries->search->count, 1, 'There should be only 1 library (singleBranchMode on)' );
Comment 1 Marcel de Rooy 2022-10-14 07:03:38 UTC
Created attachment 141855 [details] [review]
Bug 31784: Add Libraries->singleBranchMode

Test plan:
Run t/db_dependent/Koha/Libraries.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 2 Marcel de Rooy 2022-10-14 07:03:41 UTC
Created attachment 141856 [details] [review]
Bug 31784: Replace some singleBranchMode occurrences

Speaks for itself.

Test plan:
Run t/Koha.t.
Check if OPAC shows Library or Libraries in masthead, depending
on number of public branches.
Login, place a hold. Verify things work as expected.

Note: MARC21 XSLT does not care about singleBranchMode.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 3 Jonathan Druart 2022-10-14 07:10:09 UTC
snake case: single_library_mode?
Comment 4 Marcel de Rooy 2022-10-14 07:14:00 UTC
(In reply to Jonathan Druart from comment #3)
> snake case: single_library_mode?

Do we prefer that now?
Comment 5 Marcel de Rooy 2022-10-14 07:15:08 UTC
Coding guidelines mention snakecase for subroutines ;)
Comment 6 Marcel de Rooy 2022-10-14 07:15:49 UTC
        LoginFirstname  => ( C4::Context->userenv ? C4::Context->userenv->{"firstname"} : "Bel" ),
        LoginSurname    => C4::Context->userenv ? C4::Context->userenv->{"surname"}      : "Inconnu",
        emailaddress    => C4::Context->userenv ? C4::Context->userenv->{"emailaddress"} : undef,
        TagsEnabled     => C4::Context->preference("TagsEnabled"),
        hide_marc       => C4::Context->preference("hide_marc"),
        item_level_itypes  => C4::Context->preference('item-level_itypes'),
        patronimages       => C4::Context->preference("patronimages"),
        singleBranchMode   => $singleLibraryMode, #TODO one day our templates will only contain Library, and this can be removed
        singleLibraryMode  => $singleLibraryMode,
        noItemTypeImages   => C4::Context->preference("noItemTypeImages"),
        marcflavour        => C4::Context->preference("marcflavour"),
        OPACBaseURL        => C4::Context->preference('OPACBaseURL'),
        minPasswordLength  => $minPasswordLength,

Some work to do :)
Comment 7 David Nind 2022-10-14 07:39:28 UTC
Created attachment 141857 [details] [review]
Bug 31784: Add Libraries->singleBranchMode

Test plan:
Run t/db_dependent/Koha/Libraries.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: David Nind <david@davidnind.com>
Comment 8 David Nind 2022-10-14 07:39:33 UTC
Created attachment 141858 [details] [review]
Bug 31784: Replace some singleBranchMode occurrences

Speaks for itself.

Test plan:
Run t/Koha.t.
Check if OPAC shows Library or Libraries in masthead, depending
on number of public branches.
Login, place a hold. Verify things work as expected.

Note: MARC21 XSLT does not care about singleBranchMode.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: David Nind <david@davidnind.com>
Comment 9 Marcel de Rooy 2022-10-14 09:59:45 UTC
(In reply to Marcel de Rooy from comment #5)
> Coding guidelines mention snakecase for subroutines ;)

Het kwartje is gevallen :)
Comment 10 Marcel de Rooy 2022-10-14 10:07:26 UTC
Created attachment 141870 [details] [review]
Bug 31784: (QA follow-up) Snake case for subroutine

singleLibraryMode => single_library_mode

Test plan:
Run t/db_dependent/Koha/Libraries.t
Run qa tools.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 11 Katrin Fischer 2022-10-15 19:22:33 UTC
It looks like there is still some code in Koha.pm/Search.pm using Koha::Libraries->search->count == 1 to decide about displaying library facets. But since the routine is the same for staff and OPAC a bigger change would be needed. 

I've filed bug 31816 to switch singleBranchMode to singleLibraryMode everywhere.
Comment 12 Katrin Fischer 2022-10-15 19:23:43 UTC
Created attachment 141927 [details] [review]
Bug 31784: Add Libraries->singleBranchMode

Test plan:
Run t/db_dependent/Koha/Libraries.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 13 Katrin Fischer 2022-10-15 19:23:47 UTC
Created attachment 141928 [details] [review]
Bug 31784: Replace some singleBranchMode occurrences

Speaks for itself.

Test plan:
Run t/Koha.t.
Check if OPAC shows Library or Libraries in masthead, depending
on number of public branches.
Login, place a hold. Verify things work as expected.

Note: MARC21 XSLT does not care about singleBranchMode.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 14 Katrin Fischer 2022-10-15 19:23:52 UTC
Created attachment 141929 [details] [review]
Bug 31784: (QA follow-up) Snake case for subroutine

singleLibraryMode => single_library_mode

Test plan:
Run t/db_dependent/Koha/Libraries.t
Run qa tools.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 15 Tomás Cohen Arazi 2022-10-17 13:16:19 UTC
Created attachment 141969 [details] [review]
Bug 31784: Add Libraries->singleBranchMode

Test plan:
Run t/db_dependent/Koha/Libraries.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 16 Tomás Cohen Arazi 2022-10-17 13:16:24 UTC
Created attachment 141970 [details] [review]
Bug 31784: Replace some singleBranchMode occurrences

Speaks for itself.

Test plan:
Run t/Koha.t.
Check if OPAC shows Library or Libraries in masthead, depending
on number of public branches.
Login, place a hold. Verify things work as expected.

Note: MARC21 XSLT does not care about singleBranchMode.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 17 Tomás Cohen Arazi 2022-10-17 13:16:28 UTC
Created attachment 141971 [details] [review]
Bug 31784: (QA follow-up) Cleanup

This patch makes use of a ternary operator to simplify the code, and
removes references to renamed things.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 18 Tomás Cohen Arazi 2022-10-17 13:19:00 UTC
Hi, I like what we are approaching here. But I don't think ->single_branch_mode is a Koha::Libraries (resultset) method. I.e. you're using it in Koha::Libraries->new->single_branch_mode, but devs could mistakenly use it on already calculated resultsets...

If it wasn't ugly, I'd just say this should be a C4::Context method.

I'm sorry to fail it, but don't have the bandwidth to think of an alternative placement right now. Happy to help if you follow-up on this.
Comment 19 Marcel de Rooy 2022-10-17 13:20:53 UTC
Ternary is fine, Tomas.

-    Aka singleBranchMode.

-sub single_library_mode { # formerly called singleBranchMode

Removing those I would not do yet, while we still have lots of old occurrences.
Comment 20 Katrin Fischer 2022-10-17 19:23:00 UTC
Looking at this again today... I also wonder if we are not overstretching the meaning of the public flag. At the moment on the form it appears with the hint:

Set to 'yes' to show this library on the Libraries page in the OPAC.

Libraries might have a reason to not want a library show in the list (it could be stacks, they don't the address to show), but it should still show up in facets, on the detail pages, etc.

We might need to differentiate more.
Comment 21 Marcel de Rooy 2022-10-18 11:46:14 UTC
(In reply to Katrin Fischer from comment #20)
> Looking at this again today... I also wonder if we are not overstretching
> the meaning of the public flag. At the moment on the form it appears with
> the hint:
> 
> Set to 'yes' to show this library on the Libraries page in the OPAC.
> 
> Libraries might have a reason to not want a library show in the list (it
> could be stacks, they don't the address to show), but it should still show
> up in facets, on the detail pages, etc.
> 
> We might need to differentiate more.

Sounds reasonable :)
Main thing here is actually getting this test a bit more consolidated.
Some features are added in a very narrow scope but end up geting used much wider, with all benefits and disadvantages coming for free.
Comment 22 Katrin Fischer 2022-10-18 13:30:37 UTC
(In reply to Marcel de Rooy from comment #21)
> (In reply to Katrin Fischer from comment #20)
> > Looking at this again today... I also wonder if we are not overstretching
> > the meaning of the public flag. At the moment on the form it appears with
> > the hint:
> > 
> > Set to 'yes' to show this library on the Libraries page in the OPAC.
> > 
> > Libraries might have a reason to not want a library show in the list (it
> > could be stacks, they don't the address to show), but it should still show
> > up in facets, on the detail pages, etc.
> > 
> > We might need to differentiate more.
> 
> Sounds reasonable :)
> Main thing here is actually getting this test a bit more consolidated.
> Some features are added in a very narrow scope but end up geting used much
> wider, with all benefits and disadvantages coming for free.

I am not too keen on undocumented features, got way too many of those. At least leaving a trace outside of the code is necessary.

At the moment we have a setting for the libraries page, a setting for pickup-locations and there is one about the library pull downs in search too:

Bug 31654 - Hide non-public libraries from MastheadLibraryPulldown

It might be worth checking the effects of SingleLibraryMode over to decide.
Comment 23 Marcel de Rooy 2022-10-19 11:29:44 UTC
I am abandoning this development. This will be consuming too much time.