Bug 15533

Summary: Allow patrons and librarians to select itemtype when placing hold
Product: Koha Reporter: Kyle M Hall <kyle>
Component: Hold requestsAssignee: Kyle M Hall <kyle>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: andreas.hedstrom.mace, cbrannon, f.demians, gmcharlt, hector.hecaxmmx, julian.maurice, katrin.fischer, lisettepalouse+koha, patrick.robitaille
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 15532    
Bug Blocks: 16213    
Attachments: Bug 15533 - Allow patrons and librarians to select itemtype when placing hold
Bug 15533 - Allow patrons and librarians to select itemtype when placing hold
Bug 15533 - Allow patrons and librarians to select itemtype when placing hold
Bug 15533 - Allow patrons and librarians to select itemtype when placing hold
Bug 15533 - Allow patrons and librarians to select itemtype when placing hold
Bug 15533: Fix 'ambiguous' SQL query in CanItemBeReserved
Bug 15533 - Allow patrons and librarians to select itemtype when placing hold
Bug 15533: Fix 'ambiguous' SQL query in CanItemBeReserved
Bug 15533 - Allow patrons and librarians to select itemtype when placing hold
Bug 15533: Fix 'ambiguous' SQL query in CanItemBeReserved
Bug 15533 - Allow patrons and librarians to select itemtype when placing hold
Bug 15533: Fix 'ambiguous' SQL query in CanItemBeReserved
Bug 15533 [QA Followup] - Add a system preference
Bug 15533 [QA Followup] - Move tags for translatability
Bug 15533 [QA Followup] - Itemtype limit missing from tables
Bug 15533 [QA Followup] - All itemtypes for all items showing in OPAC multi-hold
Bug 15533 [QA Followup] - All itemtypes for all items showing in OPAC multi-hold
[PASSED QA] Bug 15533 - Allow patrons and librarians to select itemtype when placing hold
[PASSED QA] Bug 15533: Fix 'ambiguous' SQL query in CanItemBeReserved
[PASSED QA] Bug 15533 [QA Followup] - Add a system preference
[PASSED QA] Bug 15533 [QA Followup] - Move tags for translatability
[PASSED QA] Bug 15533 [QA Followup] - Itemtype limit missing from tables
Bug 15533: QA follow-up - Formatting and improving translatability
[PASSED QA] Bug 15533 [QA Followup] - All itemtypes for all items showing in OPAC multi-hold

Description Kyle M Hall 2016-01-09 11:10:29 UTC
Some libraries would like the ability to select the itemtype to request when placing holds. For example, if a record has 3 copies of BookA and 3 copies of BookA in large print, this feature would allow a person to place a hold on the record, but still be able to target only the Large Print edition so that the first Large Print copy that becomes available is targeted, rather than forcing the patron to select a particular copy to hold.
Comment 1 Kyle M Hall 2016-01-09 11:14:26 UTC Comment hidden (obsolete)
Comment 2 Frédéric Demians 2016-01-15 17:32:32 UTC
I can't apply this patch on master. I get:

fatal: sha1 information is lacking or useless (C4/HoldsQueue.pm).
error: could not build fake ancestor
Comment 3 Héctor Eduardo Castro Avalos 2016-01-16 00:25:47 UTC
(In reply to Frédéric Demians from comment #2)
> I can't apply this patch on master. I get:
> 
> fatal: sha1 information is lacking or useless (C4/HoldsQueue.pm).
> error: could not build fake ancestor

Applying: Bug 15533 - Allow patrons and librarians to select itemtype when placing hold
fatal: sha1 information is lacking or useless (C4/HoldsQueue.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 15533 - Allow patrons and librarians to select itemtype when placing hold
The copy of the patch that failed is found in:
   /var/koha/koha/.git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-15533---Allow-patrons-and-librarians-to-select-UnO2iN.patch
Comment 4 Kyle M Hall 2016-01-16 12:39:52 UTC
I'm having no trouble applying the patch. Did you apply it's dependency ( Bug 15532 ) first?
Comment 5 Héctor Eduardo Castro Avalos 2016-01-17 01:55:38 UTC
(In reply to Kyle M Hall from comment #4)
> I'm having no trouble applying the patch. Did you apply it's dependency (
> Bug 15532 ) first?

You're right, patch apply without trouble
Comment 6 Kyle M Hall 2016-02-21 12:18:25 UTC Comment hidden (obsolete)
Comment 7 Kyle M Hall 2016-03-16 13:59:48 UTC Comment hidden (obsolete)
Comment 8 Andreas Hedström Mace 2016-03-23 21:44:33 UTC Comment hidden (obsolete)
Comment 9 Benjamin Rokseth 2016-04-07 09:53:36 UTC Comment hidden (obsolete)
Comment 10 Julian Maurice 2016-04-12 13:23:02 UTC Comment hidden (obsolete)
Comment 11 Kyle M Hall 2016-04-28 14:56:56 UTC Comment hidden (obsolete)
Comment 12 Kyle M Hall 2016-04-28 14:57:03 UTC Comment hidden (obsolete)
Comment 13 Kyle M Hall 2016-04-28 15:00:18 UTC Comment hidden (obsolete)
Comment 14 Kyle M Hall 2016-04-28 15:00:32 UTC Comment hidden (obsolete)
Comment 15 Katrin Fischer 2016-04-28 15:29:02 UTC
Ok, starting with code review and found a few things:

1) Please make this feature optional! I think not all libraries will want to use it, while it's very useful for others. I'd suggest to add a preference here, so it can't be circumvented like CSS would allow to.

2) Some smallish translation issues, easy to fix:
<i>Next available <b>[% ItemTypes.GetDescription( reserveloo.itemtype ) %]</b> item</i> 
Pending for next available <i>[% ItemTypes.GetDescription( RESERVE.itemtype ) %]</i> item

Tests and QA script pass.

I will continue testing a bit more!
Comment 16 Kyle M Hall 2016-04-28 15:35:20 UTC Comment hidden (obsolete)
Comment 17 Kyle M Hall 2016-04-28 15:35:27 UTC Comment hidden (obsolete)
Comment 18 Kyle M Hall 2016-04-28 15:35:30 UTC Comment hidden (obsolete)
Comment 19 Kyle M Hall 2016-04-28 15:35:33 UTC Comment hidden (obsolete)
Comment 20 Katrin Fischer 2016-04-28 17:00:38 UTC
Found a problem:

When placing multiple holds at once from the result list, the itemtype list is not correct - It seems to show the itemtypes from the first record. This allows you to place 'unfillable' holds - as there are no items of this itemtype on the other records necessarily.
Comment 21 Katrin Fischer 2016-04-28 17:19:25 UTC
Another thing: The requested itemtype is perfectly visible in the patron account in the OPAC, but not on the holds summary page of the record nor in the patron account in staff (Checkouts and detail tabs)
Comment 22 Katrin Fischer 2016-04-28 17:24:57 UTC
On a positive note: holds are not triggered for the wrong itemtype, but are triggered for the correct one. Setting holds with the pref off still works as expected.

Kyle, can you please take a look at the last 2 comments?
Comment 23 Kyle M Hall 2016-04-28 18:20:41 UTC Comment hidden (obsolete)
Comment 24 Kyle M Hall 2016-04-28 18:55:26 UTC Comment hidden (obsolete)
Comment 25 Kyle M Hall 2016-04-28 18:57:15 UTC Comment hidden (obsolete)
Comment 26 Katrin Fischer 2016-04-28 19:02:59 UTC
Created attachment 50943 [details] [review]
[PASSED QA] Bug 15533 - Allow patrons and librarians to select itemtype when placing hold

Some libraries would like the ability to select the itemtype to request
when placing holds. For example, if a record has 3 copies of BookA and 3
copies of BookA in large print, this feature would allow a person to
place a hold on the record, but still be able to target only the Large
Print edition so that the first Large Print copy that becomes available
is targeted, rather than forcing the patron to select a particular copy
to hold.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Create a record with items of two or more itemtypes
4) Place a record level hold on the record while choosing one particular
   itemtype
5) Check in an item from the record that is not of that itemtype
6) Notee it is not trapped for the hold
7) Check in an item from the record that does match the selected itemtype
8) Note the item is trapped for the hold

Signed-off-by: Andreas Hedström Mace <andreas.hedstrom.mace@sub.su.se>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 27 Katrin Fischer 2016-04-28 19:03:03 UTC
Created attachment 50944 [details] [review]
[PASSED QA] Bug 15533: Fix 'ambiguous' SQL query in CanItemBeReserved

Adding reserves.itemtype made a query to fail in CanItemBeReserved.
This patch prefixes the column names with the corresponding table names
to prevent that.

You can trigger the error by running
  prove t/db_dependent/Holds.t

You should see something like this:
  Column 'itemtype' in where clause is ambiguous

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 28 Katrin Fischer 2016-04-28 19:03:06 UTC
Created attachment 50945 [details] [review]
[PASSED QA] Bug 15533 [QA Followup] - Add a system preference

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 29 Katrin Fischer 2016-04-28 19:03:10 UTC
Created attachment 50946 [details] [review]
[PASSED QA] Bug 15533 [QA Followup] - Move tags for translatability

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 30 Katrin Fischer 2016-04-28 19:03:13 UTC
Created attachment 50947 [details] [review]
[PASSED QA] Bug 15533 [QA Followup] - Itemtype limit missing from tables

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 31 Katrin Fischer 2016-04-28 19:03:17 UTC
Created attachment 50948 [details] [review]
Bug 15533: QA follow-up - Formatting and improving translatability

- Makes new message in the holds tables in the patron account
  translatable
- Unbolds the note in the holds table for the record
Comment 32 Katrin Fischer 2016-04-28 19:03:20 UTC
Created attachment 50949 [details] [review]
[PASSED QA] Bug 15533 [QA Followup] - All itemtypes for all items showing in OPAC multi-hold

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 33 Kyle M Hall 2016-04-29 10:34:13 UTC
Pushed to Master - should be in the May 2016 - Release. Thanks Katrin!
Comment 34 Josef Moravec 2016-11-13 09:51:49 UTC
*** Bug 17619 has been marked as a duplicate of this bug. ***
Comment 35 Christopher Brannon 2016-11-14 15:29:06 UTC
What version is this pushed to?
Comment 36 Katrin Fischer 2016-11-15 06:13:01 UTC
(In reply to Kyle M Hall from comment #33)
> Pushed to Master - should be in the May 2016 - Release. Thanks Katrin!

16.05