Bug 31579

Summary: Improve reserve/request.pl to avoid mistakes with pick up locations
Product: Koha Reporter: Blou <philippe.blouin>
Component: Hold requestsAssignee: Emily-Rose Francoeur <emily-rose.francoeur>
Status: In Discussion --- QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: andrewfh, anneli.osterman, caroline.cyr-la-rose, emily-rose.francoeur, geraud.frappier, gmcharlt, hammat.wele, martin.renvoize, nick, philippe.blouin, victor
Version: MainKeywords: Sandbox
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30579
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 31579: Improve reserve/request.pl to avoid mistakes with pick up locations
Bug 31579: Improve reserve/request.pl to avoid mistakes with pick up locations.
Bug 31579: Improve reserve/request.pl to avoid mistakes with pick up locations
Bug 31579: Improve reserve/request.pl to avoid mistakes with pick up locations
Bug 31579: Improve reserve/request.pl to avoid mistakes with pick up locations
Bug 31579: Improve reserve/request.pl to avoid mistakes with pick up locations
Bug 31579: Improve reserve/request.pl to avoid mistakes with pick up locations
Bug 31579: Improve reserve/request.pl to avoid mistakes with pick up locations
Bug 31579: Improve reserve/request.pl to avoid mistakes with pick up locations

Description Blou 2022-09-19 16:16:02 UTC
In the place hold page reserve/request.pl, there are two sections.

One is the "Hold details" section which allow for record hold.  The other is "Place a hold on a specific item" which speaks for itself.

The pick-up location by default is the signed-in user (the employee).

But it is different from the "Allowed pick-up location" for the item which defaults to the homebranch.
The wording should be "Pick-up location", btw, the "Allowed" states there's a list, not that the user should select it.

A common tread is that the user comes in, select the pick up location in the record section, select an item *and doesn't modify the last column*, which overrule the pickup location in the notice.

We propose a patch where a modification of the first drop down in the record modifies all the items dropdown to the same value by default.
Comment 1 Katrin Fischer 2022-09-19 18:58:39 UTC
Hi Blou,

this bug description looked familiar and I am quite sure that we discussed similar recently. The problem is, that the list of allowed pick-up locations of an item is not the same as the allowed pick-up locations on record level. Actually, the list on record level might allow for much more than a single item does, so we could not change all but maybe only some items.

I think I found the bug I was thinking about:
Bug 30579 - When placing item level hold, some options that are not used are not disabled 

Maybe you'd like to weigh in there?
Comment 2 Blou 2022-09-19 20:27:46 UTC
Would never have guessed from the subject line :)

Ok, that is a redesign as a solution.  I'll have it tested here, see if it works for us (I suppose so) then comment.

This bug here will still go ahead, just in case, and as a training ground for our new rookie :)
Comment 3 geraud.frappier 2022-09-23 13:42:19 UTC Comment hidden (obsolete)
Comment 4 geraud.frappier 2022-09-23 14:18:35 UTC Comment hidden (obsolete)
Comment 5 geraud.frappier 2022-09-26 16:17:10 UTC Comment hidden (obsolete)
Comment 6 geraud.frappier 2022-09-26 19:40:52 UTC Comment hidden (obsolete)
Comment 7 Andrew Fuerste-Henry 2022-09-26 20:06:53 UTC
>     1) Go in System preferences and find the "UseBranchTransfertLimits".
>     2) Change the value of "Don't enforce" to "Enforce".

I don't understand why the behavior you've coded here only applies when UseBranchTransfertLimits is set to Enforce. These two separate dropdowns exist regardless of whether or not one is using Branch Transfer Limits. 

More importantly, this patch breaks existing behavior when I have two items on the same bib with different allowed pickup locations. To recreate:

(apply patch, set UseBranchTransfertLimits to Enforce)
1: Set Hold Pickup Library Match in circ rules to "item's hold group"
2: Create two library groups, both defined as local holds groups. Give each group a couple of libraries, making sure they don't overlap
3: Find or create a biblio record with two items. The items should be held by different libraries, one in each of your hold groups
4: try to place a hold on your bib. on reserve/request.pl, observe that you have 3 pickup location dropdown (one for the biblio, one for each item) and that all 3 show the same value.
5: confirm that changing the biblio-level dropdown forces the page to reload and changes both item dropdowns to match what you selected in the biblio dropdown
6: HOWEVER -- your items each have a distinct set of allowed pickup locations. One of your two items will be showing a branch at which it is not allowed to be picked up.
7: attempt to place the item-level hold with an illegal pickup location that koha has suggested to you. observe that Koha leaves the holds creation page without actually making a hold or giving an error message.
Comment 8 geraud.frappier 2022-09-27 14:55:10 UTC Comment hidden (obsolete)
Comment 9 Hammat wele 2022-12-15 18:40:53 UTC
Created attachment 144634 [details] [review]
Bug 31579: Improve reserve/request.pl to avoid mistakes with pick up locations

This patch improve the behavior of the intranet reservation page cgi-bin/koha/reserve/request.pl
It ensures that when you change the 'pickup at' value in the 'Hold details' section, the 'Allowed pickup locations' values of each item are also changed (in the table)

Prerequisites

     have 3 different branches (libraries)
         library A - logged in library, the one used to log into the intranet by the employee
         library B - item's home library
         library C - item's holding library found in the file of the user who will have the reservation

     have a record with multiple copies
         create one item for each library A, B, C with homebranch = holdingbranch (with barcode)

Test plan

     1- Find a user record
     2- Change the user library (put library C) and Save
     3- Find the notice
     4- Click on 'Place Hold'
     5- Enter user name
     6- Check the 'pickup at' field of the top section and those in the coluumn 'Allowed pickup locations'
     7- Change the library of the top 'pickup at' field
     8- Check the 'Allowed pickup locations' of each item
     9- Notice that the 'Allowed pickup locations' columns are not adjusted
     10- Apply the path
     11- Repeat step 3,4,5,6,7,8
     12- Notice that all the 'Allowed pickup locations' columns are now adjusted and are the same that the selected 'pickup at' field
Comment 10 PTFS Europe Sandboxes 2023-08-17 12:10:49 UTC
Created attachment 154550 [details] [review]
Bug 31579: Improve reserve/request.pl to avoid mistakes with pick up locations

This patch improve the behavior of the intranet reservation page cgi-bin/koha/reserve/request.pl
It ensures that when you change the 'pickup at' value in the 'Hold details' section, the 'Allowed pickup locations' values of each item are also changed (in the table)

Prerequisites

     have 3 different branches (libraries)
         library A - logged in library, the one used to log into the intranet by the employee
         library B - item's home library
         library C - item's holding library found in the file of the user who will have the reservation

     have a record with multiple copies
         create one item for each library A, B, C with homebranch = holdingbranch (with barcode)

Test plan

     1- Find a user record
     2- Change the user library (put library C) and Save
     3- Find the notice
     4- Click on 'Place Hold'
     5- Enter user name
     6- Check the 'pickup at' field of the top section and those in the coluumn 'Allowed pickup locations'
     7- Change the library of the top 'pickup at' field
     8- Check the 'Allowed pickup locations' of each item
     9- Notice that the 'Allowed pickup locations' columns are not adjusted
     10- Apply the path
     11- Repeat step 3,4,5,6,7,8
     12- Notice that all the 'Allowed pickup locations' columns are now adjusted and are the same that the selected 'pickup at' field

Signed-off-by: Anneli Österman <anneli.osterman@koha-suomi.fi>
Comment 11 Katrin Fischer 2023-10-14 20:58:39 UTC
(In reply to Andrew Fuerste-Henry from comment #7)
> >     1) Go in System preferences and find the "UseBranchTransfertLimits".
> >     2) Change the value of "Don't enforce" to "Enforce".

I honestly need a little help here. 

In the patch we still see the dependency on UseBranchTransferLimits, but the test plan in the commit message doesn't mention the preference. 

Could you please revise the test plan?

Please also check below and the results I got there:

> I don't understand why the behavior you've coded here only applies when
> UseBranchTransfertLimits is set to Enforce. These two separate dropdowns
> exist regardless of whether or not one is using Branch Transfer Limits. 
> 
> More importantly, this patch breaks existing behavior when I have two items
> on the same bib with different allowed pickup locations. To recreate:
> 
> (apply patch, set UseBranchTransfertLimits to Enforce)
> 1: Set Hold Pickup Library Match in circ rules to "item's hold group"
> 2: Create two library groups, both defined as local holds groups. Give each
> group a couple of libraries, making sure they don't overlap
> 3: Find or create a biblio record with two items. The items should be held
> by different libraries, one in each of your hold groups
> 4: try to place a hold on your bib. on reserve/request.pl, observe that you
> have 3 pickup location dropdown (one for the biblio, one for each item) and
> that all 3 show the same value.

That's not the case in my test. I got:

Library group F: Fairfield, Fairview
Library group C: Centerville, Liberty

Items: Fairfield, Centerville

Pickup location pull downs:

* Record level: All 4
* Item Fairfield: Fairfield, Fairview
* item Centerville: Centerville, Liberty

> 5: confirm that changing the biblio-level dropdown forces the page to reload
> and changes both item dropdowns to match what you selected in the biblio
> dropdown

When I change the record level pick-up location, nothing happens to the item level ones.

> 6: HOWEVER -- your items each have a distinct set of allowed pickup
> locations. One of your two items will be showing a branch at which it is not
> allowed to be picked up.

Can't replicate this behaviour.

> 7: attempt to place the item-level hold with an illegal pickup location that
> koha has suggested to you. observe that Koha leaves the holds creation page
> without actually making a hold or giving an error message.

Can't replicate this behaviour.

BUT: I see the wrong behaviour if UseBranchTransferLimits is Set to "Don't enforce". 

* Set prefernece to "Don't enforce"
* Change the biblio record pick-up location from Centerville (logged in library) to something else and back to Centerville.
* Items do have pickup location Centerville now. But this is not allowed for the Fairfield item.
* When saved, the hold is not placed.

Then I get the behaviour that invalid pickup locations will get selected for my items, although they are not actually listed in the pull downs.

This only happens with the patch applied :(
Comment 12 Emily-Rose Francoeur 2023-10-30 18:50:17 UTC
Created attachment 158100 [details] [review]
Bug 31579: Improve reserve/request.pl to avoid mistakes with pick up locations

The patch has been updated to ensure it does not break the existing behavior. The test plan have also been updated.

This patch improve the behavior of the intranet reservation page cgi-bin/koha/reserve/request.pl
It ensures that when you change the "pickup at" value in the "Hold details" section, the "Allowed pickup locations" values of each item are also changed (in the table)

System preferences

	ensure that "UseBranchTransferLimits" is set to “Don’t enforce”

Prerequisites

	have 4 different branches (libraries):
		library A - logged in library, the one used to log into the intranet by the employee
		library B - item's home library
		library C - item's holding library found in the file of the user who will have the reservation
		library D – another library

	have 2 library groups (each of them is set as “Is local hold group”):
		group A – contains “library A” and “library B”
		group B – contains “library C” and “library D”

     	have a record with multiple copies
		create an item for each library A, B, C, D with homebranch = holdingbranch (with barcode)

Circulation rules

	in the “Circulation and fine rules”, under “Default holds policy by item type”:
		select the same item type you use for the items you created before
		set “Hold pickup library match” to “item's hold group”
		click on “+ Add”

Test plan

	1- Find a user record
     	2- Change the user library (set it to library C) and save
     	3- Click on “Search to hold”
	4- Find the notice you created
	5- Click on “Place a hold > Place hold for [User]”
     	6- Check the “pickup at” field in the top section and those in the column “Allowed pickup locations”
     	7- Change the library of the top “pickup at” field
     	8- Check the “Allowed pickup locations” of each item
     	9- Notice that the “Allowed pickup locations” columns are not adjusted
	10- Apply the patch
	11- Refresh the page
	12- Repeat steps 6,7 and 8
	13- Notice that the pickup locations change only if they are in the same library group
	14- Go to “Circulation and fine rule” (in Koha administration) and delete the the rule you created under “Default holds policy by item type”
	15- Repeat step 1,3,4,5,6,7 and 8
	16- Notice that all the “Allowed pickup locations” columns are adjusted
Comment 13 Emily-Rose Francoeur 2023-10-30 20:18:17 UTC
Created attachment 158104 [details] [review]
Bug 31579: Improve reserve/request.pl to avoid mistakes with pick up locations

The patch has been updated to ensure it does not break the existing behavior. The test plan have also been updated.

This patch improve the behavior of the intranet reservation page cgi-bin/koha/reserve/request.pl
It ensures that when you change the "pickup at" value in the "Hold details" section, the "Allowed pickup locations" values of each item are also changed (in the table)

System preferences

	ensure that "UseBranchTransferLimits" is set to "Don't enforce"

Prerequisites

	have 4 different branches (libraries):
		library A - logged in library, the one used to log into the intranet by the employee
		library B - item's home library
		library C - item's holding library found in the file of the user who will have the reservation
		library D - another library

	have 2 library groups (each of them is set as "Is local hold group"):
		group A - contains "library A" and "library B"
		group B - contains "library C" and "library D"

     	have a record with multiple copies
		create an item for each library A, B, C, D with homebranch = holdingbranch (with barcode)

Circulation rules

	in the "Circulation and fine rules", under "Default holds policy by item type":
		select the same item type you use for the items you created before
		set "Hold pickup library match" to "item's hold group"
		click on "+ Add"

Test plan

	1- Find a user record
     	2- Change the user library (set it to library C) and save
     	3- Click on "Search to hold"
	4- Find the notice you created
	5- Click on "Place a hold > Place hold for [User]"
     	6- Check the "pickup at" field in the top section and those in the column "Allowed pickup locations"
     	7- Change the library of the top "pickup at" field
     	8- Check the "Allowed pickup locations" of each item
     	9- Notice that the "Allowed pickup locations" columns are not adjusted
	10- Apply the patch
	11- Refresh the page
	12- Repeat steps 6,7 and 8
	13- Notice that the pickup locations change only if they are in the same library group
	14- Go to "Circulation and fine rule" (in Koha administration) and delete the the rule you created under "Default holds policy by item type"
	15- Repeat step 1,3,4,5,6,7 and 8
	16- Notice that all the "Allowed pickup locations" columns are adjusted
Comment 14 Nick Clemens 2023-11-01 13:44:57 UTC
This works as described, but seems to tackle a narrow setup (groups, no transfer limits etc)

Bug 30579 breaks the forms apart, this joins them more together, making a change on one type affect the other

I think there needs to be a decision as to which one is more desirable.
Comment 15 Victor Grousset/tuxayo 2024-01-23 12:44:21 UTC
> Bug 30579 breaks the forms apart, this joins them more together, making a change on one type affect the other

> I think there needs to be a decision as to which one is more desirable.

ok so for now it shouldn't be on need signoff queue.
Comment 16 Blou 2024-03-06 16:31:17 UTC
Well, Bug 30579 is on Signed Off (4 months already).

We just want to avoid user errors.  The solution doesn't matter and we're happy to just build on 30579.  But they need to move ahead.

If there's a discussion to be had "between the two", let's do it for both.  I'll ask our dev to look into 30579, evaluate, compare and comment.
Comment 17 Hammat wele 2024-04-17 04:22:43 UTC Comment hidden (obsolete)
Comment 18 Hammat wele 2024-04-17 04:25:49 UTC
This patch and bz30579 have different logics

The bz30579 completely changes the form, with that patch you can either use “Hold next available item” (containing the “Pickup at” field), or “Place to hold on a specific item” since selecting one disables the other.

However, this patch needs the “Pickup at” and “Place a hold on a specific item” fields to all be active since we configure the “Place a hold on a specific item” fields according to the value chosen in the field “Pickup at” which means that it is impossible to use both patches at the same time, we will have to make a choice between the two.
Comment 19 Victor Grousset/tuxayo 2024-04-18 20:00:44 UTC
(In reply to Blou from comment #16)
> Well, Bug 30579 is on Signed Off (4 months already).
> 
> We just want to avoid user errors.  The solution doesn't matter and we're
> happy to just build on 30579.  But they need to move ahead.

Hmm, indeed :/

If seeking to unblock bug 30579, a second signoff would help because the 1st signoff is from Emily which is in the QA team. Then it frees them to QA if they are comfortable with that patch. And if not, a second signoff and some improvised testing still help building confidence in that there is no issue and will help another QAer to pass it. 

(In reply to Hammat wele from comment #18)
> However, this patch needs the “Pickup at” and “Place a hold on a specific
> item” fields to all be active since we configure the “Place a hold on a
> specific item” fields according to the value chosen in the field “Pickup at”
> which means that it is impossible to use both patches at the same time, we
> will have to make a choice between the two.

So this mean this patch can't be adapted on top of Bug 30579 ? :/