Koha has item level holds (only a specific item was requested) and record level holds (any next available item). There can be a case when a librarian wishes to change an item level hold to a record level hold. This is currently not (easily) possible, as they would have to cancel the hold and then set another type of hold and place it to the previous point in hold queue. This Bug aims to add an easy way to change hold types. Only one type of hold is allowed simultaneously for a record and a patron. The two types cannot be mixed. In other words a patron can't have both item level and record level holds on a specific record at the same time. This must be kept in mind when designing such functionality.
Created attachment 141388 [details] [review] Bug 31692: Add Koha::Hold::change_type and unit tests To test: 1. prove t/db_dependent/Koha/Hold.t
Created attachment 141390 [details] [review] Bug 31692: Allow librarians to change hold type in staff client To test: 1. Apply patch 2. Add item level hold to a record/item, make sure patron has no other holds on that record 3. Go to /cgi-bin/koha/reserve/request.pl?biblionumber=xxx where xxx is the record you placed the hold for 4. Under "Existing holds" table, in "Details" column you should see "Only item <barcode> X". Click the red X and click Update hold(s) 5. Observe "Only item <barcode>" has changed to "Next available" 6. Cancel the hold and add two item level holds 7. Under "Existing holds" table, in "Details" column you should see "Only item <barcode>", but no red X for either holds.
I think the use of another red X on the page to change the item level hold into a bib level one is confusing. There are already a few red X's on the page and that particular one offers no explanation to the user as to what it will do. Maybe we need a dropdown? I'm not sure if that is the proper solution either.
(In reply to Lucas Gass from comment #3) > I think the use of another red X on the page to change the item level hold > into a bib level one is confusing. There are already a few red X's on the > page and that particular one offers no explanation to the user as to what it > will do. > > Maybe we need a dropdown? I'm not sure if that is the proper solution either. Lucas is making a good point, could you please have a look Lari? Maybe some other icon could be used? I have to admit my first idea was some kind of upwards arrow for (level upwards) but... there are also a lot of arrows already. A tooltip for sure makes sense.
Created attachment 142102 [details] [review] Bug 31692: Add Koha::Hold::change_type and unit tests To test: 1. prove t/db_dependent/Koha/Hold.t
Created attachment 142103 [details] [review] Bug 31692: Allow librarians to change hold type in staff client To test: 1. Apply patch 2. Add item level hold to a record/item, make sure patron has no other holds on that record 3. Go to /cgi-bin/koha/reserve/request.pl?biblionumber=xxx where xxx is the record you placed the hold for 4. Under "Existing holds" table, in "Details" column you should see "Only item <barcode> X". Click the red X and click Update hold(s) 5. Observe "Only item <barcode>" has changed to "Next available" 6. Cancel the hold and add two item level holds 7. Under "Existing holds" table, in "Details" column you should see "Only item <barcode>", but no red X for either holds.
Created attachment 142104 [details] [review] Bug 31692: (follow-up) Add tooltip for changing hold type
(In reply to Lucas Gass from comment #3) > I think the use of another red X on the page to change the item level hold > into a bib level one is confusing. There are already a few red X's on the > page and that particular one offers no explanation to the user as to what it > will do. > > Maybe we need a dropdown? I'm not sure if that is the proper solution either. (In reply to Katrin Fischer from comment #4) > Lucas is making a good point, could you please have a look Lari? Maybe some > other icon could be used? I have to admit my first idea was some kind of > upwards arrow for (level upwards) but... there are also a lot of arrows > already. A tooltip for sure makes sense. Thanks for the feedback, Lucas and Katrin. I've added a tooltip in the form of "title" attribute - if Koha has another way of showing tooltips then please correct me. Great suggestion! About the X, (by the way, it just became green with the new theme) my reasoning for choosing it was to represent something to "remove" the item from the hold. Perhaps an "unlinking" icon could work too. https://fontawesome.com/v4/icon/chain-broken Dropdown is also a nice idea in the sense that it could be extended to show other items as well and let librarians change the item if necessary. But that is unfortunately out of scope for what I am trying to achieve here. I have also rebased to resolve some git conflicts and patches can now be tested again.
Created attachment 142106 [details] [review] Bug 31692: Allow librarians to change hold type in staff client To test: 1. Apply patch 2. Add item level hold to a record/item, make sure patron has no other holds on that record 3. Go to /cgi-bin/koha/reserve/request.pl?biblionumber=xxx where xxx is the record you placed the hold for 4. Under "Existing holds" table, in "Details" column you should see "Only item <barcode> X". Click the red X and click Update hold(s) 5. Observe "Only item <barcode>" has changed to "Next available" 6. Cancel the hold and add two item level holds 7. Under "Existing holds" table, in "Details" column you should see "Only item <barcode>", but no red X for either holds.
Created attachment 142107 [details] [review] Bug 31692: (follow-up) Add tooltip for changing hold type
Created attachment 142108 [details] [review] Bug 31692: (follow-up) Use fa-chain and fa-chain-broken icons
Created attachment 142109 [details] [review] Bug 31692: (follow-up) Rewording tooltip Using "change to next available item" would require changing title and aria-label dynamically to "change to only item ..." when clicked. While possible, simply using "toggle hold type" covers both cases.
Created attachment 142114 [details] [review] Bug 31692: Allow librarians to change hold type in staff client To test: 1. Apply patch 2. Add item level hold to a record/item, make sure patron has no other holds on that record 3. Go to /cgi-bin/koha/reserve/request.pl?biblionumber=xxx where xxx is the record you placed the hold for 4. Under "Existing holds" table, in "Details" column you should see "Only item <barcode> X". Click the red X and click Update hold(s) 5. Observe "Only item <barcode>" has changed to "Next available" 6. Cancel the hold and add two item level holds 7. Under "Existing holds" table, in "Details" column you should see "Only item <barcode>", but no red X for either holds.
Created attachment 142115 [details] [review] Bug 31692: (follow-up) Add tooltip for changing hold type
Created attachment 142116 [details] [review] Bug 31692: (follow-up) Use fa-chain and fa-chain-broken icons
Created attachment 142117 [details] [review] Bug 31692: (follow-up) Rewording tooltip Using "change to next available item" would require changing title and aria-label dynamically to "change to only item ..." when clicked. While possible, simply using "toggle hold type" covers both cases.
Created attachment 142118 [details] [review] Bug 31692: (alternative look) Use select dropdown
In reply to Lucas Gass from comment #3) > Maybe we need a dropdown? I'm not sure if that is the proper solution either. I actually wanted to see what it looks like so I implemented patch: Bug 31692: (alternative look) Use select dropdown I think it looks quite okay. It also allows me to get rid of all extra JavaScript and tooltip issues. Great idea Lucas. Thoughts?
(In reply to Lari Taskula from comment #18) > In reply to Lucas Gass from comment #3) > > Maybe we need a dropdown? I'm not sure if that is the proper solution either. > > I actually wanted to see what it looks like so I implemented patch: > > Bug 31692: (alternative look) Use select dropdown > > I think it looks quite okay. It also allows me to get rid of all extra > JavaScript and tooltip issues. Great idea Lucas. Thoughts? Lari, I do like that much better! Everything works great. I do wonder if there is still going to be some confusion on what the dropdown does. Do we need a hint or a tip to help describe what the dropdown is going to do?
Created attachment 142586 [details] [review] Bug 31692: Add Koha::Hold::change_type and unit tests To test: 1. prove t/db_dependent/Koha/Hold.t
Created attachment 142587 [details] [review] Bug 31692: Allow librarians to change hold type in staff client To test: 1. Apply patch 2. Add item level hold to a record/item, make sure patron has no other holds on that record 3. Go to /cgi-bin/koha/reserve/request.pl?biblionumber=xxx where xxx is the record you placed the hold for 4. Under "Existing holds" table, in "Details" column you should see "Only item <barcode>" dropdown 5. Select "Next available" from the dropdown 6. Click Update hold(s) 7. Observe dropdown value has changed from "Only item <barcode>" to "Next available" 8. Cancel the hold and add two item level holds 9. Under "Existing holds" table, in "Details" column you should see "Only item <barcode>", but no select dropdown
(In reply to Lucas Gass from comment #19) > I do wonder if there is still going to be some confusion on what the > dropdown does. Do we need a hint or a tip to help describe what the dropdown > is going to do? I think the dropdown values themselves explain what it does. If you think more detailed description is needed, feel free to make a suggestion. On another note I've squashed the follow-ups into the main patch and updated test plan.
Created attachment 142595 [details] [review] Bug 31692: Add Koha::Hold::change_type and unit tests To test: 1. prove t/db_dependent/Koha/Hold.t Signed-off-by: David Nind <david@davidnind.com>
Created attachment 142596 [details] [review] Bug 31692: Allow librarians to change hold type in staff client To test: 1. Apply patch 2. Add item level hold to a record/item, make sure patron has no other holds on that record 3. Go to /cgi-bin/koha/reserve/request.pl?biblionumber=xxx where xxx is the record you placed the hold for 4. Under "Existing holds" table, in "Details" column you should see "Only item <barcode>" dropdown 5. Select "Next available" from the dropdown 6. Click Update hold(s) 7. Observe dropdown value has changed from "Only item <barcode>" to "Next available" 8. Cancel the hold and add two item level holds 9. Under "Existing holds" table, in "Details" column you should see "Only item <barcode>", but no select dropdown Signed-off-by: David Nind <david@davidnind.com>
I've signed off, as everything works as per the test plan. For step 8, I added two item level holds for the same patron - I hope that is what was required. My only suggestion would be to change the "Details" column label to "Hold type".
(In reply to David Nind from comment #25) > For step 8, I added two item level holds for the same patron - I hope that > is what was required. Yes that's correct. > My only suggestion would be to change the "Details" column label to "Hold > type". It looks like the details column can also contain a "Non priority hold" text. If we were to rename the column, can we consider "non priority hold" as a "hold type", or does hold type just mean an item/biblio hold? https://github.com/Koha-Community/Koha/blob/48bf9b1d91dbbc336f859ac1a83d7d25b0e6a3c1/koha-tmpl/intranet-tmpl/prog/en/includes/holds_table.inc#L189-L191
(In reply to Lari Taskula from comment #26) > > My only suggestion would be to change the "Details" column label to "Hold > > type". > > It looks like the details column can also contain a "Non priority hold" > text. If we were to rename the column, can we consider "non priority hold" > as a "hold type", or does hold type just mean an item/biblio hold? > https://github.com/Koha-Community/Koha/blob/ > 48bf9b1d91dbbc336f859ac1a83d7d25b0e6a3c1/koha-tmpl/intranet-tmpl/prog/en/ > includes/holds_table.inc#L189-L191 I wasn't aware of that :D Also noticed that the details column already exists, so happy to leave the same without any changes.
Created attachment 142818 [details] [review] Bug 31692: Add Koha::Hold::change_type and unit tests To test: 1. prove t/db_dependent/Koha/Hold.t Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 142819 [details] [review] Bug 31692: Allow librarians to change hold type in staff client To test: 1. Apply patch 2. Add item level hold to a record/item, make sure patron has no other holds on that record 3. Go to /cgi-bin/koha/reserve/request.pl?biblionumber=xxx where xxx is the record you placed the hold for 4. Under "Existing holds" table, in "Details" column you should see "Only item <barcode>" dropdown 5. Select "Next available" from the dropdown 6. Click Update hold(s) 7. Observe dropdown value has changed from "Only item <barcode>" to "Next available" 8. Cancel the hold and add two item level holds 9. Under "Existing holds" table, in "Details" column you should see "Only item <barcode>", but no select dropdown Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
I was starting to doubt myself, but this patch broke my evil "FQA" streak. Thanks Lari, I think the discussion resulted in a nice clean GUI here!
Oh no... I found an issue :( You remove the itemnumber, which would have been enough on old versions, but now we also need to change reserves.item_level_hold Should be an easy fix - please follow-up.
(In reply to Katrin Fischer from comment #31) > Oh no... I found an issue :( > > You remove the itemnumber, which would have been enough on old versions, but > now we also need to change reserves.item_level_hold > > Should be an easy fix - please follow-up. Good catch. Although it has been there for 3 years I didn't know of this "new" column :) Fix will follow.
Created attachment 143151 [details] [review] Bug 31692: (follow-up) Adjust reserves.item_level_hold Original patch missed handling reserves.item_level_hold column. To test: 1. prove t/db_dependent/Koha/Hold.t
The patches still apply, but something seems changed: 1. Step 7 - There is now no dropdown list (before there was a drop down list with Only item <barcode> and Next available); selecting next available and updating removes the drop down list). 2. Step 9 - There is still a dropdown list with the two options (Only item <barcode> and Next available). Happy to sign off again if that is what is expected. Tests pass both before and after patches are applied.
(In reply to David Nind from comment #34) > The patches still apply, but something seems changed: > > 1. Step 7 - There is now no dropdown list (before there was a drop down list > with Only item <barcode> and Next available); selecting next available and > updating removes the drop down list). The wording in this step is a bit silly, it appears I didn't properly rewrite it when I squashed the new dropdown menu with the original patch. What you describe seems expected behavior to me. After changing from item level hold to record level hold there should no longer be a dropdown. So at step 7 if you see just the text "Next available" then that is to be expected. This is because the patches here do not add GUI support for going from record level to item level hold, but only the other way around. > 2. Step 9 - There is still a dropdown list with the two options (Only item > <barcode> and Next available). This is not expected, did you add the two item level holds for the same patron? I noticed the test plan does not clarify that this is required.
(In reply to Lari Taskula from comment #35) > (In reply to David Nind from comment #34) > > The patches still apply, but something seems changed: > > > > 1. Step 7 - There is now no dropdown list (before there was a drop down list > > with Only item <barcode> and Next available); selecting next available and > > updating removes the drop down list). > > The wording in this step is a bit silly, it appears I didn't properly > rewrite it when I squashed the new dropdown menu with the original patch. > > What you describe seems expected behavior to me. After changing from item > level hold to record level hold there should no longer be a dropdown. So at > step 7 if you see just the text "Next available" then that is to be expected. > > This is because the patches here do not add GUI support for going from > record level to item level hold, but only the other way around. Thanks for clarifying! > > 2. Step 9 - There is still a dropdown list with the two options (Only item > > <barcode> and Next available). > > This is not expected, did you add the two item level holds for the same > patron? I noticed the test plan does not clarify that this is required. I added for different patrons, so will use the same patron. Just re-testing, so should be signed off shortly!
Created attachment 143160 [details] [review] Bug 31692: Add Koha::Hold::change_type and unit tests To test: 1. prove t/db_dependent/Koha/Hold.t Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 143161 [details] [review] Bug 31692: Allow librarians to change hold type in staff client To test: 1. Apply patch 2. Add item level hold to a record/item, make sure patron has no other holds on that record 3. Go to /cgi-bin/koha/reserve/request.pl?biblionumber=xxx where xxx is the record you placed the hold for 4. Under "Existing holds" table, in "Details" column you should see "Only item <barcode>" dropdown 5. Select "Next available" from the dropdown 6. Click Update hold(s) 7. Observe dropdown value has changed from "Only item <barcode>" to "Next available" 8. Cancel the hold and add two item level holds 9. Under "Existing holds" table, in "Details" column you should see "Only item <barcode>", but no select dropdown Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 143162 [details] [review] Bug 31692: (follow-up) Adjust reserves.item_level_hold Original patch missed handling reserves.item_level_hold column. To test: 1. prove t/db_dependent/Koha/Hold.t Signed-off-by: David Nind <david@davidnind.com>
I've ended up adding sign off lines for the first two patches - not sure how to remove those!
There is a conflict in the .t, can you please fix?
Created attachment 143614 [details] [review] Bug 31692: Add Koha::Hold::change_type and unit tests To test: 1. prove t/db_dependent/Koha/Hold.t
Created attachment 143615 [details] [review] Bug 31692: Allow librarians to change hold type in staff client To test: 1. Apply patch 2. Add item level hold to a record/item, make sure patron has no other holds on that record 3. Go to /cgi-bin/koha/reserve/request.pl?biblionumber=xxx where xxx is the record you placed the hold for 4. Under "Existing holds" table, in "Details" column you should see "Only item <barcode>" dropdown 5. Select "Next available" from the dropdown 6. Click Update hold(s) 7. Observe dropdown value has changed from "Only item <barcode>" to "Next available" 8. Cancel the hold and add two item level holds 9. Under "Existing holds" table, in "Details" column you should see "Only item <barcode>", but no select dropdown
Created attachment 143616 [details] [review] Bug 31692: (follow-up) Adjust reserves.item_level_hold Original patch missed handling reserves.item_level_hold column. To test: 1. prove t/db_dependent/Koha/Hold.t
I accidentally removed the signed-off lines from patches, fixing...
Created attachment 143617 [details] [review] Bug 31692: Add Koha::Hold::change_type and unit tests To test: 1. prove t/db_dependent/Koha/Hold.t Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 143618 [details] [review] Bug 31692: Allow librarians to change hold type in staff client To test: 1. Apply patch 2. Add item level hold to a record/item, make sure patron has no other holds on that record 3. Go to /cgi-bin/koha/reserve/request.pl?biblionumber=xxx where xxx is the record you placed the hold for 4. Under "Existing holds" table, in "Details" column you should see "Only item <barcode>" dropdown 5. Select "Next available" from the dropdown 6. Click Update hold(s) 7. Observe dropdown is gone and cell value has changed from "Only item <barcode>" to "Next available" 8. Cancel the hold and add two item level holds for the same patron 9. Under "Existing holds" table, in "Details" column you should see "Only item <barcode>", but no select dropdown Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 143619 [details] [review] Bug 31692: (follow-up) Adjust reserves.item_level_hold Original patch missed handling reserves.item_level_hold column. To test: 1. prove t/db_dependent/Koha/Hold.t Signed-off-by: David Nind <david@davidnind.com>
(In reply to David Nind from comment #40) > I've ended up adding sign off lines for the first two patches - not sure how > to remove those! I've removed those for you. (In reply to Katrin Fischer from comment #41) > There is a conflict in the .t, can you please fix? Fixed!
Created attachment 143627 [details] [review] Bug 31692: Add Koha::Hold::change_type and unit tests To test: 1. prove t/db_dependent/Koha/Hold.t Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 143628 [details] [review] Bug 31692: Allow librarians to change hold type in staff client To test: 1. Apply patch 2. Add item level hold to a record/item, make sure patron has no other holds on that record 3. Go to /cgi-bin/koha/reserve/request.pl?biblionumber=xxx where xxx is the record you placed the hold for 4. Under "Existing holds" table, in "Details" column you should see "Only item <barcode>" dropdown 5. Select "Next available" from the dropdown 6. Click Update hold(s) 7. Observe dropdown is gone and cell value has changed from "Only item <barcode>" to "Next available" 8. Cancel the hold and add two item level holds for the same patron 9. Under "Existing holds" table, in "Details" column you should see "Only item <barcode>", but no select dropdown Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 143629 [details] [review] Bug 31692: (follow-up) Adjust reserves.item_level_hold Original patch missed handling reserves.item_level_hold column. To test: 1. prove t/db_dependent/Koha/Hold.t Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Thx for the quick fix, Lari. This has some string changes, so not sure if we can still make it.
Can we please change the method signature? I'd expect $hold->change_type( { type => [biblio|item], [ item_id => $item->id, ] } ); Or even split it in two methods: $hold->make_biblio_level $hold->make_item_level( $item->id ) each of them checking the right things and throwing the required exceptions.
(In reply to Tomás Cohen Arazi from comment #54) > Can we please change the method signature? I'd expect > > $hold->change_type( > { > type => [biblio|item], > [ item_id => $item->id, ] > } > ); > > Or even split it in two methods: > > $hold->make_biblio_level > $hold->make_item_level( $item->id ) > > each of them checking the right things and throwing the required exceptions. Maybe use switch? switch_to_record_level (we want to avoid biblio) and switch_to_item_level
Lari, are you still willing to work on this? Thanks
Pushed to master for 23.11. Nice work everyone, thanks!
Enhancement not pushed to 23.05.x
Testing with a superlibrarian, but I don't get a pull down in the details column on my item level holds. Any idea what could go wrong there?
(In reply to Katrin Fischer from comment #59) > Testing with a superlibrarian, but I don't get a pull down in the details > column on my item level holds. Any idea what could go wrong there? I'm not seeing it either...
(In reply to Katrin Fischer from comment #59) > Testing with a superlibrarian, but I don't get a pull down in the details > column on my item level holds. Any idea what could go wrong there? Looks like the change introduced to koha-tmpl/intranet-tmpl/prog/en/includes/holds_table.inc in "Allow librarians to change hold type in staff client" patch was reverted by Bug 33575. So this feature is no longer working as of now.
See Bug 35483 for restoring the missing feature. (In reply to Tomás Cohen Arazi from comment #56) > Lari, are you still willing to work on this? Thanks Sorry I had either missed or forgot to answer your comment! (In reply to Tomás Cohen Arazi from comment #54) > Can we please change the method signature? I'd expect > > $hold->change_type( > { > type => [biblio|item], > [ item_id => $item->id, ] > } > ); > > Or even split it in two methods: > > $hold->make_biblio_level > $hold->make_item_level( $item->id ) Well, why not, if you feel it's still important even after pushing the Bug (sorry again for the late answer). I'm not extremely attached or strongly opinionated about the choice I made - I suppose I was thinking of it more as a toggle hence not introducing two methods. The itemnumber param in my sense specifies the type already, if given => item, if not given => biblio.