Bug 22789

Summary: Establish non-priority holds
Product: Koha Reporter: Joy Nelson <1joynelson>
Component: Hold requestsAssignee: Agustín Moyano <agustinmoyano>
Status: CLOSED FIXED QA Contact: Katrin Fischer <katrin.fischer>
Severity: enhancement    
Priority: P5 - low CC: bwsdonna, caroline.cyr-la-rose, gmcharlt, jonathan.druart, josef.moravec, lisettepalouse+koha, lucas, tomascohen
Version: MainKeywords: Manual-updated
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.11.00
Bug Depends on:    
Bug Blocks: 25260, 29327    
Attachments: Bug 22789: Add tests
Bug 22789: Add non_priority column to reserves and old_reserves
Bug 22789: Set non_priority column as boolean in schema
Bug 22789: Add non priority feature to C4 classes and staff interface
Bug 22789: Add tests
Bug 22789: Add non_priority column to reserves and old_reserves
Bug 22789: Set non_priority column as boolean in schema
Bug 22789: Add non priority feature to C4 classes and staff interface
Bug 22789: Add tests
Bug 22789: (follow-up) Fix atomic update, GUI and more than one hold
Bug 22789: Add tests
Bug 22789: Add non_priority column to reserves and old_reserves
Bug 22789: Set non_priority column as boolean in schema
Bug 22789: Add non priority feature to C4 classes and staff interface
Bug 22789: (follow-up) Fix atomic update, GUI and more than one hold
Bug 22789: Add tests
Bug 22789: Add non_priority column to reserves and old_reserves
Bug 22789: Set non_priority column as boolean in schema
Bug 22789: Add non priority feature to C4 classes and staff interface
Bug 22789: (follow-up) Fix atomic update, GUI and more than one hold
Bug 22789: (QA follow-up) Some minor formatting changes

Description Joy Nelson 2019-04-26 19:42:18 UTC
This development would allow holds to be placed so the hold wouldn't prevent the current checkout from renewing the hold. 

The goal is a better customer service because certain library departments will place holds to repair or replace a book when it is returned to the library (the internal library need has no rush), they only want the catch the book on it's return.  The library needs to allow the patron who has it out have the flexibility to renew it on their account without a staff override.

This functionality would be specific to the staffside only. 
- Add a Checkbox on the hold form that stipulates that the hold being placed is a 'non-priority' hold. 
- Add a field in the reserves tables to flag the hold as 'non-priority'. 
- The checkreserves module in Koha checks for this flag. If found, the hold request is 'ignored' and item can be renewed.
Comment 1 Lisette Scheer 2019-05-02 18:40:16 UTC
This would be very helpful here. 
Lisette
Comment 2 Donna 2020-03-16 20:02:54 UTC
This is a functionality libraries request on a regular basis. Great idea!
Comment 3 Agustín Moyano 2020-08-19 21:24:40 UTC
Created attachment 108703 [details] [review]
Bug 22789: Add tests
Comment 4 Agustín Moyano 2020-08-19 21:24:44 UTC
Created attachment 108704 [details] [review]
Bug 22789: Add non_priority column to reserves and old_reserves
Comment 5 Agustín Moyano 2020-08-19 21:24:48 UTC
Created attachment 108705 [details] [review]
Bug 22789: Set non_priority column as boolean in schema
Comment 6 Agustín Moyano 2020-08-19 21:24:52 UTC
Created attachment 108706 [details] [review]
Bug 22789: Add non priority feature to C4 classes and staff interface

This patch implements necesary code to implement non priority feature

To test:
1) Apply all patches.
2) Run updatedatabase.
3) Checkout a specific item for patron1.
4) Place a hold on the same item for patron2 (do not check non priority
   hold checkbox).
5) Try to renew the item for patron1.
CHECK => in checkouts table, there is a message that the item could not
be renewed because there was a hold.
6) Cleanup all checkouts and holds.
7) repeat steps 3 to 5, but this time check the non priority checkbox.
SUCCESS => item was renewed
8) prove t/db_dependent/Holds.t
Comment 7 ByWater Sandboxes 2020-08-19 22:10:37 UTC
Created attachment 108707 [details] [review]
Bug 22789: Add tests

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Comment 8 ByWater Sandboxes 2020-08-19 22:10:40 UTC
Created attachment 108708 [details] [review]
Bug 22789: Add non_priority column to reserves and old_reserves

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Comment 9 ByWater Sandboxes 2020-08-19 22:10:42 UTC
Created attachment 108709 [details] [review]
Bug 22789: Set non_priority column as boolean in schema

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Comment 10 ByWater Sandboxes 2020-08-19 22:10:45 UTC
Created attachment 108710 [details] [review]
Bug 22789: Add non priority feature to C4 classes and staff interface

This patch implements necesary code to implement non priority feature

To test:
1) Apply all patches.
2) Run updatedatabase.
3) Checkout a specific item for patron1.
4) Place a hold on the same item for patron2 (do not check non priority
   hold checkbox).
5) Try to renew the item for patron1.
CHECK => in checkouts table, there is a message that the item could not
be renewed because there was a hold.
6) Cleanup all checkouts and holds.
7) repeat steps 3 to 5, but this time check the non priority checkbox.
SUCCESS => item was renewed
8) prove t/db_dependent/Holds.t

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Comment 11 Lisette Scheer 2020-08-19 22:11:31 UTC
Worked as expected and I'm looking forward to this development!
I wasn't able to prove t/db_dependent/Holds.t as I was on the sandbox.
Lisette
Comment 12 Katrin Fischer 2020-08-22 15:23:10 UTC
Starting with a first round of testing and reviewing here:

1) Database update

I feel it would be cleaner to do the same check for the old_reserves table as well:
 if( !column_exists( 'reserves', 'non_priority' ) ) {

2) GUI

request.tt
a) I am not sure of a 'non priority hold' is a standing term, but I haven't seen this feature in another ILS before. I think maybe having an explanation in a tool tip or after the checkbox could be quite helpful here.

I'd also suggest to add an ID to the <li> to help libraries hide this functionality if they don't intend on using it:

+                            <li>
+                                <label for="non_priority">Non priority hold:</label>
+                                <input name="non_priority" id="non_priority" type="checkbox" />
+                            </li>


b) Once the hold is placed, there is no way to visually tell that it's a non-priority hold. I think this could lead to support questions and we will only be able to figure it out looking at the database entry. 

I'd strongly recommend to add a note to request.tt and to the hold list in the patron account on staff side. On request.tt this could go into the Details column.

3) More than one hold

My test case:
- Check out item to Henry
- Place a non-priority hold for admin
- Henry can still renew
- Place another hold for a regular staff patron
- Henry can still renew... 

I feel like it needs to look at all holds and not only the next one?
Comment 13 Agustín Moyano 2020-08-25 18:12:10 UTC
Created attachment 109123 [details] [review]
Bug 22789: Add tests

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Comment 14 Agustín Moyano 2020-08-25 18:12:54 UTC
Created attachment 109124 [details] [review]
Bug 22789: (follow-up) Fix atomic update, GUI and more than one hold

This patch
* sets one check for reserves and another for old_reserves in
atomic update
* Adds a message below the checkbox and adds detail when a hold is non
  priority
* Fixes issue when there are more than one hold, but the first is non
  priority
* Adds test case for this last scenario
Comment 15 Agustín Moyano 2020-08-25 18:14:11 UTC
Created attachment 109125 [details] [review]
Bug 22789: Add tests

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Comment 16 Agustín Moyano 2020-08-25 18:14:16 UTC
Created attachment 109126 [details] [review]
Bug 22789: Add non_priority column to reserves and old_reserves

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Comment 17 Agustín Moyano 2020-08-25 18:14:21 UTC
Created attachment 109127 [details] [review]
Bug 22789: Set non_priority column as boolean in schema

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Comment 18 Agustín Moyano 2020-08-25 18:14:25 UTC
Created attachment 109128 [details] [review]
Bug 22789: Add non priority feature to C4 classes and staff interface

This patch implements necesary code to implement non priority feature

To test:
1) Apply all patches.
2) Run updatedatabase.
3) Checkout a specific item for patron1.
4) Place a hold on the same item for patron2 (do not check non priority
   hold checkbox).
5) Try to renew the item for patron1.
CHECK => in checkouts table, there is a message that the item could not
be renewed because there was a hold.
6) Cleanup all checkouts and holds.
7) repeat steps 3 to 5, but this time check the non priority checkbox.
SUCCESS => item was renewed
8) prove t/db_dependent/Holds.t

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Comment 19 Agustín Moyano 2020-08-25 18:14:30 UTC
Created attachment 109129 [details] [review]
Bug 22789: (follow-up) Fix atomic update, GUI and more than one hold

This patch
* sets one check for reserves and another for old_reserves in
atomic update
* Adds a message below the checkbox and adds detail when a hold is non
  priority
* Fixes issue when there are more than one hold, but the first is non
  priority
* Adds test case for this last scenario
Comment 20 Katrin Fischer 2020-08-26 20:38:30 UTC
Created attachment 109200 [details] [review]
Bug 22789: Add tests

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 21 Katrin Fischer 2020-08-26 20:38:34 UTC
Created attachment 109201 [details] [review]
Bug 22789: Add non_priority column to reserves and old_reserves

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 22 Katrin Fischer 2020-08-26 20:38:38 UTC
Created attachment 109202 [details] [review]
Bug 22789: Set non_priority column as boolean in schema

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 23 Katrin Fischer 2020-08-26 20:38:42 UTC
Created attachment 109203 [details] [review]
Bug 22789: Add non priority feature to C4 classes and staff interface

This patch implements necesary code to implement non priority feature

To test:
1) Apply all patches.
2) Run updatedatabase.
3) Checkout a specific item for patron1.
4) Place a hold on the same item for patron2 (do not check non priority
   hold checkbox).
5) Try to renew the item for patron1.
CHECK => in checkouts table, there is a message that the item could not
be renewed because there was a hold.
6) Cleanup all checkouts and holds.
7) repeat steps 3 to 5, but this time check the non priority checkbox.
SUCCESS => item was renewed
8) prove t/db_dependent/Holds.t

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 24 Katrin Fischer 2020-08-26 20:38:46 UTC
Created attachment 109204 [details] [review]
Bug 22789: (follow-up) Fix atomic update, GUI and more than one hold

This patch
* sets one check for reserves and another for old_reserves in
atomic update
* Adds a message below the checkbox and adds detail when a hold is non
  priority
* Fixes issue when there are more than one hold, but the first is non
  priority
* Adds test case for this last scenario

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 25 Katrin Fischer 2020-08-26 20:38:50 UTC
Created attachment 109205 [details] [review]
Bug 22789: (QA follow-up) Some minor formatting changes

- Remove the unordered list added to the holds list Details column
- Switch div to span to have the hint appear behind the checkbox

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 26 Jonathan Druart 2020-08-31 14:20:38 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 27 Lucas Gass 2020-09-04 20:26:44 UTC
enhancement will not be backported to 20.05.x