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)' );
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>
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>
snake case: single_library_mode?
(In reply to Jonathan Druart from comment #3) > snake case: single_library_mode? Do we prefer that now?
Coding guidelines mention snakecase for subroutines ;)
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 :)
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>
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>
(In reply to Marcel de Rooy from comment #5) > Coding guidelines mention snakecase for subroutines ;) Het kwartje is gevallen :)
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>
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.
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>
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>
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>
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>
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>
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>
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.
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.
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.
(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.
(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.
I am abandoning this development. This will be consuming too much time.