Bug 28118 - Fix missing "selected" attribute in "Pickup at" dropdown
Summary: Fix missing "selected" attribute in "Pickup at" dropdown
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords:
Depends on: 27071
Blocks:
  Show dependency treegraph
 
Reported: 2021-04-09 12:00 UTC by Magnus Enger
Modified: 2021-12-13 21:10 UTC (History)
8 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:
21.05.00,20.11.05,20.05.11


Attachments
Bug 28118: Default to current branch when placing hold (1.28 KB, patch)
2021-04-12 19:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28118: Default to current branch when placing hold (1.90 KB, patch)
2021-04-14 11:59 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28118: Default to current branch when placing hold (1.89 KB, patch)
2021-04-15 07:06 UTC, Henry Bolshaw
Details | Diff | Splinter Review
Bug 28118: Default to current branch when placing hold (1.95 KB, patch)
2021-04-15 07:08 UTC, Henry Bolshaw
Details | Diff | Splinter Review
Bug 28118: Default to current branch when placing hold (2.01 KB, patch)
2021-04-15 08:21 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28118: Default to current branch when placing hold (2.04 KB, patch)
2021-04-15 14:09 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28118: Default to current branch when placing hold (2.12 KB, patch)
2021-04-15 14:18 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28118: Default to current branch when placing hold (2.18 KB, patch)
2021-04-15 16:16 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28118: Default to current branch when placing hold (2.25 KB, patch)
2021-04-15 16:23 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Magnus Enger 2021-04-09 12:00:36 UTC
In 20.05.06, when i find a record, click on "Place hold" and search for a patron, I get a form that contains a dropdown labeled "Pickup at". This dropdown has a "selected" attribute on one of the libraries: 

<select name="pickup" size="1" id="pickup">
 <option value="A" selected="selected">A public library</option>
 <option value="B">B public library</option>
</select>

In 20.05.10 and 20.12.00.027 (current master), this "selected" attribute seems to be missing. 

I have not looked deeply at how different settings might affect which library is selected, but I think either way, one library should always be selected?
Comment 1 Jonathan Druart 2021-04-09 12:45:37 UTC
Certainly caused by
  commit aa221e0c8bcf0fa0377d6c2154bad0ef61c97a2b
  Bug 27071: Use GetReservesControlBranch to pick the branch

-my $pickup = $input->param('pickup') || C4::Context->userenv->{'branch'};
+my $pickup = $input->param('pickup');
Comment 2 Tomás Cohen Arazi 2021-04-12 11:58:58 UTC
(In reply to Jonathan Druart from comment #1)
> Certainly caused by
>   commit aa221e0c8bcf0fa0377d6c2154bad0ef61c97a2b
>   Bug 27071: Use GetReservesControlBranch to pick the branch
> 
> -my $pickup = $input->param('pickup') || C4::Context->userenv->{'branch'};
> +my $pickup = $input->param('pickup');

That change slipped in inadvertently!
Comment 3 Tomás Cohen Arazi 2021-04-12 19:23:02 UTC Comment hidden (obsolete)
Comment 4 Tomás Cohen Arazi 2021-04-12 19:25:36 UTC
BTW: Sorry for this, Magnus :-/
Comment 5 Jonathan Druart 2021-04-13 07:15:43 UTC
I don't think your patch is correct, Tomas.

The main point of the commit I cited is to not pick the logged in branchcode.

In the following line we will never pick the expected branchcode:
554                 my $reserves_control_branch = $pickup // C4::Reserves::GetReservesControlBranch( $item, $patron_unblessed );
Comment 6 Tomás Cohen Arazi 2021-04-14 11:59:56 UTC
Created attachment 119565 [details] [review]
Bug 28118: Default to current branch when placing hold

During bug 27071 development, this line got inadvertedly changed. This
patch restores the original behaviour.

To test:
1. Search for a title
2. Go try place a hold
=> FAIL: Look at the HTML, there's no pickup location with
selected="selected"
3. Switch branch and repeat 2
=> FAIL: Still the same
4. Apply this patch
5. Repeat 2 and 3
=> SUCCESS: Branch is selected, chosen current branch is picked
6. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 7 Henry Bolshaw 2021-04-15 07:06:10 UTC
Created attachment 119602 [details] [review]
Bug 28118: Default to current branch when placing hold

During bug 27071 development, this line got inadvertedly changed. This
patch restores the original behaviour.

To test:
1. Search for a title
2. Go try place a hold
=> FAIL: Look at the HTML, there's no pickup location with
selected="selected"
3. Switch branch and repeat 2
=> FAIL: Still the same
4. Apply this patch
5. Repeat 2 and 3
=> SUCCESS: Branch is selected, chosen current branch is picked
6. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 8 Henry Bolshaw 2021-04-15 07:08:59 UTC
Created attachment 119603 [details] [review]
Bug 28118: Default to current branch when placing hold

During bug 27071 development, this line got inadvertedly changed. This
patch restores the original behaviour.

To test:
1. Search for a title
2. Go try place a hold
=> FAIL: Look at the HTML, there's no pickup location with
selected="selected"
3. Switch branch and repeat 2
=> FAIL: Still the same
4. Apply this patch
5. Repeat 2 and 3
=> SUCCESS: Branch is selected, chosen current branch is picked
6. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Henry Bolshaw <bolshawh@parliament.uk>
Comment 9 Henry Bolshaw 2021-04-15 07:18:56 UTC
Signed off.

This patch changes the default pickup branch selected when placing a hold on the staff interface. When testing without the patch, the pickup branch for the hold was simply the branch at the top of the list alphabetically. After the patch, the default pickup branch is the library set by the staff interface. after the patch, this branch also has "selected=selected" in the html code.
Comment 10 Martin Renvoize 2021-04-15 08:21:08 UTC
Created attachment 119605 [details] [review]
Bug 28118: Default to current branch when placing hold

During bug 27071 development, this line got inadvertedly changed. This
patch restores the original behaviour.

To test:
1. Search for a title
2. Go try place a hold
=> FAIL: Look at the HTML, there's no pickup location with
selected="selected"
3. Switch branch and repeat 2
=> FAIL: Still the same
4. Apply this patch
5. Repeat 2 and 3
=> SUCCESS: Branch is selected, chosen current branch is picked
6. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Henry Bolshaw <bolshawh@parliament.uk>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Martin Renvoize 2021-04-15 08:21:39 UTC
Works as described and is a solid improvement. Passing QA
Comment 12 Jonathan Druart 2021-04-15 08:30:21 UTC
We need to keep $pickup for $reserves_control_branch, in case it's passed.
request.pl can be called with a specific branchcode from this code in request.tt:
1079             [% IF Koha.Preference('UseBranchTransferLimits') %]
1080                 $("#pickup").on('change', function(){
1081                     var pickup = $("#pickup").val();
1082                     var url = "?pickup=" + pickup;
1083                     url += "&borrowernumber=" + borrowernumber;
1084                     url += "&biblionumber=" + biblionumber;
1085                     window.location.replace(url);
1086                 });
1087             [% END %]


We need to use Branches.GetLoggedInBranchcode to preselect the option, if pickup is not passed.
Comment 13 Tomás Cohen Arazi 2021-04-15 14:09:29 UTC
Created attachment 119629 [details] [review]
Bug 28118: Default to current branch when placing hold

During bug 27071 development, this line got inadvertedly changed. This
patch restores the original behaviour.

To test:
1. Search for a title
2. Go try place a hold
=> FAIL: Look at the HTML, there's no pickup location with
selected="selected"
3. Switch branch and repeat 2
=> FAIL: Still the same
4. Apply this patch
5. Repeat 2 and 3
=> SUCCESS: Branch is selected, chosen current branch is picked
6. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 14 Tomás Cohen Arazi 2021-04-15 14:18:43 UTC
Created attachment 119630 [details] [review]
Bug 28118: Default to current branch when placing hold

During bug 27071 development, this line got inadvertedly changed. This
patch restores the original behaviour.

To test:
1. Search for a title
2. Go try place a hold
=> FAIL: Look at the HTML, there's no pickup location with
selected="selected"
3. Switch branch and repeat 2
=> FAIL: Still the same
4. Apply this patch
5. Repeat 2 and 3
=> SUCCESS: Branch is selected, chosen current branch is picked
6. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 15 Nick Clemens 2021-04-15 16:16:49 UTC
Created attachment 119673 [details] [review]
Bug 28118: Default to current branch when placing hold

During bug 27071 development, this line got inadvertedly changed. This
patch restores the original behaviour.

To test:
1. Search for a title
2. Go try place a hold
=> FAIL: Look at the HTML, there's no pickup location with
selected="selected"
3. Switch branch and repeat 2
=> FAIL: Still the same
4. Apply this patch
5. Repeat 2 and 3
=> SUCCESS: Branch is selected, chosen current branch is picked
6. Sign off :-D

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

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 16 Kyle M Hall 2021-04-15 16:23:01 UTC
Created attachment 119674 [details] [review]
Bug 28118: Default to current branch when placing hold

During bug 27071 development, this line got inadvertedly changed. This
patch restores the original behaviour.

To test:
1. Search for a title
2. Go try place a hold
=> FAIL: Look at the HTML, there's no pickup location with
selected="selected"
3. Switch branch and repeat 2
=> FAIL: Still the same
4. Apply this patch
5. Repeat 2 and 3
=> SUCCESS: Branch is selected, chosen current branch is picked
6. Sign off :-D

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

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 17 Jonathan Druart 2021-04-16 10:30:13 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 18 Fridolin Somers 2021-04-22 13:58:02 UTC
Pushed to 20.11.x for 20.11.05
Comment 19 Andrew Fuerste-Henry 2021-04-25 16:03:55 UTC
Pushed to 20.05.x for 20.05.11
Comment 20 Victor Grousset/tuxayo 2021-04-25 20:41:31 UTC
Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.