|
Description
Joy Nelson
2019-04-26 19:42:18 UTC
This would be very helpful here. Lisette This is a functionality libraries request on a regular basis. Great idea! Created attachment 108703 [details] [review] Bug 22789: Add tests Created attachment 108704 [details] [review] Bug 22789: Add non_priority column to reserves and old_reserves Created attachment 108705 [details] [review] Bug 22789: Set non_priority column as boolean in schema 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 Created attachment 108707 [details] [review] Bug 22789: Add tests Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org> Created attachment 108708 [details] [review] Bug 22789: Add non_priority column to reserves and old_reserves Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org> Created attachment 108709 [details] [review] Bug 22789: Set non_priority column as boolean in schema Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org> 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> 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 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?
Created attachment 109123 [details] [review] Bug 22789: Add tests Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org> 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 Created attachment 109125 [details] [review] Bug 22789: Add tests Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org> Created attachment 109126 [details] [review] Bug 22789: Add non_priority column to reserves and old_reserves Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org> Created attachment 109127 [details] [review] Bug 22789: Set non_priority column as boolean in schema Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org> 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> 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 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> 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> 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> 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> 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> 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> Pushed to master for 20.11, thanks to everybody involved! enhancement will not be backported to 20.05.x |