Bug 28622 - Selected branchcode incorrectly passed to adv search
Summary: Selected branchcode incorrectly passed to adv search
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 23271 15758
Blocks:
  Show dependency treegraph
 
Reported: 2021-06-23 14:08 UTC by Jonathan Druart
Modified: 2021-08-03 01:53 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.03


Attachments
Bug 28622: (bug 15758 follow-up) Remove incorrect branch selection in adv search (2.44 KB, patch)
2021-06-23 14:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28622: (bug 15758 follow-up) Remove incorrect branch selection in adv search (2.49 KB, patch)
2021-06-27 21:08 UTC, David Nind
Details | Diff | Splinter Review
Bug 28622: (bug 15758 follow-up) Remove incorrect branch selection in adv search (2.53 KB, patch)
2021-07-06 06:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28622: (bug 15758 follow-up) Remove incorrect branch selection in adv search (2.58 KB, patch)
2021-07-09 13:48 UTC, David Nind
Details | Diff | Splinter Review
Bug 28622: (bug 15758 follow-up) Remove incorrect branch selection in adv search (2.67 KB, patch)
2021-07-16 07:08 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2021-06-23 14:08:23 UTC
From
  commit df97814f3034a3d5015546c4eca21f08ae951e37
  Bug 15758: Koha::Libraries - Remove GetBranches

    selected_branchcode => ( C4::Context->IsSuperLibrarian ? C4::Context->userenv : '' ),

We must pass C4::Context->userenv->{branch}
Comment 1 Jonathan Druart 2021-06-23 14:21:47 UTC
Created attachment 122339 [details] [review]
Bug 28622: (bug 15758 follow-up) Remove incorrect branch selection in adv search

From
  commit df97814f3034a3d5015546c4eca21f08ae951e37
  Bug 15758: Koha::Libraries - Remove GetBranches

+    selected_branchcode => ( C4::Context->IsSuperLibrarian ? C4::Context->userenv : '' ),

And

-        [% PROCESS options_for_libraries libraries => Branches.all() %]
+        [%# FIXME Should not we filter the libraries displayed? %]
+        [% PROCESS options_for_libraries libraries => Branches.all( selected => selected_branchcode, unfiltered => 1 ) %]

The change in the pl is wrong as it should pass C4::Context->userenv->{branch},
but prior to this commit it seems that we didn't select the logged in
library anyway.

Looks like we should remove this code.
Comment 2 David Nind 2021-06-27 21:08:49 UTC
Created attachment 122467 [details] [review]
Bug 28622: (bug 15758 follow-up) Remove incorrect branch selection in adv search

From
  commit df97814f3034a3d5015546c4eca21f08ae951e37
  Bug 15758: Koha::Libraries - Remove GetBranches

+    selected_branchcode => ( C4::Context->IsSuperLibrarian ? C4::Context->userenv : '' ),

And

-        [% PROCESS options_for_libraries libraries => Branches.all() %]
+        [%# FIXME Should not we filter the libraries displayed? %]
+        [% PROCESS options_for_libraries libraries => Branches.all( selected => selected_branchcode, unfiltered => 1 ) %]

The change in the pl is wrong as it should pass C4::Context->userenv->{branch},
but prior to this commit it seems that we didn't select the logged in
library anyway.

Looks like we should remove this code.

Signed-off-by: David Nind <david@davidnind.com>
Comment 3 David Nind 2021-06-27 21:13:30 UTC
Testing notes (koha-testing-docker):
- Before applying patch behavour is:
  . Set library in staff interface
  . Go to the advanced search interface
  . For 'Location and availability', 'All libraries' is the default choice
- After patch is applied, it now defaults to whatever is set as the library
- Changed library (used drop down list > Set library) and then went back to advanced search page: library should reflect the new library set
Comment 4 Jonathan Druart 2021-07-05 14:47:59 UTC
(In reply to David Nind from comment #3)
> - After patch is applied, it now defaults to whatever is set as the library

Heh, I wasn't expecting a behaviour change actually. But my commit message was confusing.

I read the change and code again and, even if I haven't tried, I think that the dropdown list had the logged-in library selected prior to 15758. So yes, this is a bug fix and not simply a code cleanup.

However I am surprised nobody reported the problem before.
Comment 5 Katrin Fischer 2021-07-05 16:57:36 UTC
I don't think the advanced search should limit by default to any library, afaik it was always set to "All libraries". - I just double checked in our 18.11 installations.
Comment 6 Jonathan Druart 2021-07-05 17:39:05 UTC
(In reply to Katrin Fischer from comment #5)
> I don't think the advanced search should limit by default to any library,
> afaik it was always set to "All libraries". - I just double checked in our
> 18.11 installations.

Bug 15758 has been pushed to 16.11.
Comment 7 Katrin Fischer 2021-07-05 17:46:01 UTC
Not sure I understand the last comment, but I am pretty sure Koha did not preselect the logged-in users library since I have been using Koha - 3.2 :) So can we pinpoint when there was a change or could there have been other code in place?
Comment 8 Katrin Fischer 2021-07-05 17:46:41 UTC
Thing is, I think as this wasn't reported - it's very likely it would be an unwanted change, don't you think?
Comment 9 Jonathan Druart 2021-07-06 06:51:41 UTC
Tested on 16.05.x and 'all libraries" was selected.
Comment 10 Jonathan Druart 2021-07-06 06:51:54 UTC
Created attachment 122598 [details] [review]
Bug 28622: (bug 15758 follow-up) Remove incorrect branch selection in adv search

From
  commit df97814f3034a3d5015546c4eca21f08ae951e37
  Bug 15758: Koha::Libraries - Remove GetBranches

+    selected_branchcode => ( C4::Context->IsSuperLibrarian ? C4::Context->userenv : '' ),

And

-        [% PROCESS options_for_libraries libraries => Branches.all() %]
+        [%# FIXME Should not we filter the libraries displayed? %]
+        [% PROCESS options_for_libraries libraries => Branches.all( selected => selected_branchcode, unfiltered => 1 ) %]

The change in the pl is wrong as it should pass C4::Context->userenv->{branch},
but prior to this commit it seems that we didn't select the logged in
library anyway.

Looks like we should remove this code but preserve the current behavior and select "All libraries".
Comment 11 Katrin Fischer 2021-07-06 08:50:18 UTC
Thx for testing and rewriting!
Comment 12 David Nind 2021-07-09 13:48:19 UTC
Created attachment 122750 [details] [review]
Bug 28622: (bug 15758 follow-up) Remove incorrect branch selection in adv search

From
  commit df97814f3034a3d5015546c4eca21f08ae951e37
  Bug 15758: Koha::Libraries - Remove GetBranches

+    selected_branchcode => ( C4::Context->IsSuperLibrarian ? C4::Context->userenv : '' ),

And

-        [% PROCESS options_for_libraries libraries => Branches.all() %]
+        [%# FIXME Should not we filter the libraries displayed? %]
+        [% PROCESS options_for_libraries libraries => Branches.all( selected => selected_branchcode, unfiltered => 1 ) %]

The change in the pl is wrong as it should pass C4::Context->userenv->{branch},
but prior to this commit it seems that we didn't select the logged in
library anyway.

Looks like we should remove this code but preserve the current behavior and select "All libraries".

Signed-off-by: David Nind <david@davidnind.com>
Comment 13 David Nind 2021-07-09 13:53:51 UTC
Testing notes (koha-testing-docker):
- Before and after apply the patch the behavour is:
  . Set library in staff interface
  . Go to the advanced search interface
  . For 'Location and availability', 'All libraries' is the default choice
  . If the library is changed (drop down list > Set library) the default for 'Location and availability' should remain as 'All libraries'
Comment 14 Marcel de Rooy 2021-07-16 07:08:23 UTC
Created attachment 122871 [details] [review]
Bug 28622: (bug 15758 follow-up) Remove incorrect branch selection in adv search

From
  commit df97814f3034a3d5015546c4eca21f08ae951e37
  Bug 15758: Koha::Libraries - Remove GetBranches

+    selected_branchcode => ( C4::Context->IsSuperLibrarian ? C4::Context->userenv : '' ),

And

-        [% PROCESS options_for_libraries libraries => Branches.all() %]
+        [%# FIXME Should not we filter the libraries displayed? %]
+        [% PROCESS options_for_libraries libraries => Branches.all( selected => selected_branchcode, unfiltered => 1 ) %]

The change in the pl is wrong as it should pass C4::Context->userenv->{branch},
but prior to this commit it seems that we didn't select the logged in
library anyway.

Looks like we should remove this code but preserve the current behavior and select "All libraries".

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 15 Jonathan Druart 2021-07-26 09:24:15 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 16 Kyle M Hall 2021-07-30 13:49:18 UTC
Pushed to 21.05.x for 21.05.03
Comment 17 Fridolin Somers 2021-08-03 01:51:34 UTC
Depends on Bug 23271 which added do_not_select_my_library param
Comment 18 Fridolin Somers 2021-08-03 01:53:39 UTC
Depends on Bug 23271 not in 20.11.x