Description
Blou
2022-09-19 16:16:02 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? 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 :) Created attachment 140907 [details] [review] Bug 31579: Improve reserve/request.pl to avoid mistakes with pick up locations Reported-by: Géraud Frappier <geraud.frappier@inlibro.com> 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. This patch will resolve this problem. To test: 1) Go in System preferences and find the "UseBranchTransfertLimits". 2) Change the value of "Don't enforce" to "Enforce". 3) Come back on the menu and search a catalog. 4) Click on "Place a hold" and enter the name of a patron. 5) In the sections Hold details change the value of "pickup at" by choosing an other library. 6) Observe that the web page is refreshed, but in the second section the list of "allowed pickup locations" didn't changed. 7) Apply the patch. 8) Redo 3-4-5-6. 9) See that the values of the list of "Allowed pickup locations" has changed to the library you have chosen. Created attachment 140917 [details] [review] Bug 31579: Improve reserve/request.pl to avoid mistakes with pick up locations. Reported-by: Géraud Frappier <geraud.frappier@inlibro.com> 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. This patch will resolve this problem. To test: 1) Go in System preferences and find the "UseBranchTransfertLimits". 2) Change the value of "Don't enforce" to "Enforce". 3) Come back on the menu and search a catalog. 4) Click on "Place a hold" and enter the name of a patron. 5) In the sections Hold details change the value of "pickup at" by choosing an other library. 6) Observe that the web page is refreshed, but in the second section the list of "allowed pickup locations" didn't changed. 7) Apply the patch. 8) Redo 3-4-5-6. 9) See that the values of the list of "Allowed pickup locations" has changed to the library you have chosen. Created attachment 140997 [details] [review] Bug 31579: Improve reserve/request.pl to avoid mistakes with pick up locations 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. This patch will resolve this problem. To test: 1) Go in System preferences and find the "UseBranchTransfertLimits". 2) Change the value of "Don't enforce" to "Enforce". 3) Come back on the menu and search a catalog. 4) Click on "Place a hold" and enter the name of a patron. 5) In the sections Hold details change the value of "pickup at" by choosing an other library. 6) Observe that the web page is refreshed, but in the second section the list of "allowed pickup locations" didn't changed. 7) Apply the patch. 8) Redo 3-4-5-6. 9) See that the values of the list of "Allowed pickup locations" has changed to the library you have chosen. Created attachment 141007 [details] [review] Bug 31579: Improve reserve/request.pl to avoid mistakes with pick up locations 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. This patch will resolve this problem. To test: 1) Go in System preferences and find the "UseBranchTransfertLimits". 2) Change the value of "Don't enforce" to "Enforce". 3) Come back on the menu and search a catalog. 4) Click on "Place a hold" and enter the name of a patron. 5) In the sections Hold details change the value of "pickup at" by choosing an other library. 6) Observe that the web page is refreshed, but in the second section the list of "allowed pickup locations" didn't changed. 7) Apply the patch. 8) Redo 3-4-5-6. 9) See that the values of the list of "Allowed pickup locations" has changed to the library you have chosen. Author: Géraud Frappier <geraud.frappier@inlibro.com> > 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.
Created attachment 141033 [details] [review] Bug 31579: Improve reserve/request.pl to avoid mistakes with pick up locations Remove white space and unnecessary comments. 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 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> (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 :( 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 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 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. > 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. 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. This patch and bz30579 have different logics The bz30579 completely changes the form, with this 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. 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. (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 ? :/ |