|
Description
Kyle M Hall (khall)
2020-03-12 15:45:49 UTC
Created attachment 100644 [details] [review] Bug 24860: Add reserves.volume_id This feature builds upon bug 24857 and allows placing holds that target a specific volume of a record. It is patterned after the feature that allows limiting hold selection by itemtype ( AllowHoldItemTypeSelection ). Test Plan: 1) Apply bug 24857 and this bug's patches 2) Run updatedatabase.pl 3) Restart all the things! 4) Enable the sysprefs EnableVolumes and EnableVolumeHolds 5) Create a record, items and volumes, with each item being assigned to a volume 6) Place a hold for a patron 7) Note the new selector to choose a volume in addition to record and item level holds 8) Place a volume level hold 9) Check in an item from the record that is not part of that volume 10) Note the hold is not trapped for that item 11) Check in an item from the record that *is* part of that volume 12) Note the hold is trapped for that item 13) Place a volume level hold for another item 14) Run the holds queue builder 15) Note the holds queue targets only items from that volume 16) Check out all the items of that volume to other patrosn 17) Re-run the holds queue builder 18) Note the holds queue no longer has a line for that hold, as all items that could fill that hold are now unavailable Created attachment 100645 [details] [review] Bug 24860: Update existing Schema files [DO NOT PUSH] Created attachment 100646 [details] [review] Bug 24860: Add new syspref EnableVolumeHolds Created attachment 100647 [details] [review] Bug 24860: Implement reserves.volume_id Created attachment 100648 [details] [review] Bug 24860: Add ability to select a volume when placing a hold TODO: Add 'Volume' column to holds table on circulation.pl and moremember.pl Requires embedding volume in hold API Created attachment 100649 [details] [review] Bug 24860: Add ability to select a volume when placing a hold - OPAC Created attachment 100650 [details] [review] Bug 24860: Skip non-matching volume holds in CheckReserves Created attachment 100651 [details] [review] Bug 24860: Skip non-matching volume holds in HoldsQueue Created attachment 101744 [details] [review] Bug 24860: Add reserves.volume_id This feature builds upon bug 24857 and allows placing holds that target a specific volume of a record. It is patterned after the feature that allows limiting hold selection by itemtype ( AllowHoldItemTypeSelection ). Test Plan: 1) Apply bug 24857 and this bug's patches 2) Run updatedatabase.pl 3) Restart all the things! 4) Enable the sysprefs EnableVolumes and EnableVolumeHolds 5) Create a record, items and volumes, with each item being assigned to a volume 6) Place a hold for a patron 7) Note the new selector to choose a volume in addition to record and item level holds 8) Place a volume level hold 9) Check in an item from the record that is not part of that volume 10) Note the hold is not trapped for that item 11) Check in an item from the record that *is* part of that volume 12) Note the hold is trapped for that item 13) Place a volume level hold for another item 14) Run the holds queue builder 15) Note the holds queue targets only items from that volume 16) Check out all the items of that volume to other patrosn 17) Re-run the holds queue builder 18) Note the holds queue no longer has a line for that hold, as all items that could fill that hold are now unavailable Created attachment 101745 [details] [review] Bug 24860: Add new syspref EnableVolumeHolds Created attachment 101746 [details] [review] Bug 24860: Implement reserves.volume_id Created attachment 101747 [details] [review] Bug 24860: Add ability to select a volume when placing a hold TODO: Add 'Volume' column to holds table on circulation.pl and moremember.pl Requires embedding volume in hold API Created attachment 101748 [details] [review] Bug 24860: Add ability to select a volume when placing a hold - OPAC Created attachment 101749 [details] [review] Bug 24860: Skip non-matching volume holds in CheckReserves Created attachment 101750 [details] [review] Bug 24860: Skip non-matching volume holds in HoldsQueue Created attachment 101751 [details] [review] Bug 24860: Update existing Schema files [DO NOT PUSH] Created attachment 101827 [details] [review] Bug 24860: Add reserves.volume_id This feature builds upon bug 24857 and allows placing holds that target a specific volume of a record. It is patterned after the feature that allows limiting hold selection by itemtype ( AllowHoldItemTypeSelection ). Test Plan: 1) Apply bug 24857 and this bug's patches 2) Run updatedatabase.pl 3) Restart all the things! 4) Enable the sysprefs EnableVolumes and EnableVolumeHolds 5) Create a record, items and volumes, with each item being assigned to a volume 6) Place a hold for a patron 7) Note the new selector to choose a volume in addition to record and item level holds 8) Place a volume level hold 9) Check in an item from the record that is not part of that volume 10) Note the hold is not trapped for that item 11) Check in an item from the record that *is* part of that volume 12) Note the hold is trapped for that item 13) Place a volume level hold for another item 14) Run the holds queue builder 15) Note the holds queue targets only items from that volume 16) Check out all the items of that volume to other patrosn 17) Re-run the holds queue builder 18) Note the holds queue no longer has a line for that hold, as all items that could fill that hold are now unavailable Created attachment 101828 [details] [review] Bug 24860: Add new syspref EnableVolumeHolds Created attachment 101829 [details] [review] Bug 24860: Implement reserves.volume_id Created attachment 101830 [details] [review] Bug 24860: Add ability to select a volume when placing a hold Created attachment 101831 [details] [review] Bug 24860: Add ability to select a volume when placing a hold - OPAC Created attachment 101832 [details] [review] Bug 24860: Skip non-matching volume holds in CheckReserves Created attachment 101833 [details] [review] Bug 24860: Skip non-matching volume holds in HoldsQueue Created attachment 101834 [details] [review] Bug 24860: Update existing Schema files [DO NOT PUSH] Created attachment 101835 [details] [review] Bug 24860: Add hold's volume to holds table on circulation.pl and moremember.pl Receiving an ISE: - find/create bib with 3 items - put item_1 and item_2 in volume - place hold on volume - check in item_3 - get error: Can't call method "id" on an undefined value at /kohadevbox/koha/C4/Reserves.pm line 846 Created attachment 102230 [details] [review] Bug 24860: Add reserves.volume_id This feature builds upon bug 24857 and allows placing holds that target a specific volume of a record. It is patterned after the feature that allows limiting hold selection by itemtype ( AllowHoldItemTypeSelection ). Test Plan: 1) Apply bug 24857 and this bug's patches 2) Run updatedatabase.pl 3) Restart all the things! 4) Enable the sysprefs EnableVolumes and EnableVolumeHolds 5) Create a record, items and volumes, with each item being assigned to a volume 6) Place a hold for a patron 7) Note the new selector to choose a volume in addition to record and item level holds 8) Place a volume level hold 9) Check in an item from the record that is not part of that volume 10) Note the hold is not trapped for that item 11) Check in an item from the record that *is* part of that volume 12) Note the hold is trapped for that item 13) Place a volume level hold for another item 14) Run the holds queue builder 15) Note the holds queue targets only items from that volume 16) Check out all the items of that volume to other patrosn 17) Re-run the holds queue builder 18) Note the holds queue no longer has a line for that hold, as all items that could fill that hold are now unavailable Created attachment 102231 [details] [review] Bug 24860: Add new syspref EnableVolumeHolds Created attachment 102232 [details] [review] Bug 24860: Implement reserves.volume_id Created attachment 102233 [details] [review] Bug 24860: Add ability to select a volume when placing a hold Created attachment 102234 [details] [review] Bug 24860: Add ability to select a volume when placing a hold - OPAC Created attachment 102235 [details] [review] Bug 24860: Skip non-matching volume holds in CheckReserves Created attachment 102236 [details] [review] Bug 24860: Skip non-matching volume holds in HoldsQueue Created attachment 102237 [details] [review] Bug 24860: Update existing Schema files [DO NOT PUSH] Created attachment 102238 [details] [review] Bug 24860: Add hold's volume to holds table on circulation.pl and moremember.pl Created attachment 102239 [details] [review] Bug 24860: Add reserves.volume_id This feature builds upon bug 24857 and allows placing holds that target a specific volume of a record. It is patterned after the feature that allows limiting hold selection by itemtype ( AllowHoldItemTypeSelection ). Test Plan: 1) Apply bug 24857 and this bug's patches 2) Run updatedatabase.pl 3) Restart all the things! 4) Enable the sysprefs EnableVolumes and EnableVolumeHolds 5) Create a record, items and volumes, with each item being assigned to a volume 6) Place a hold for a patron 7) Note the new selector to choose a volume in addition to record and item level holds 8) Place a volume level hold 9) Check in an item from the record that is not part of that volume 10) Note the hold is not trapped for that item 11) Check in an item from the record that *is* part of that volume 12) Note the hold is trapped for that item 13) Place a volume level hold for another item 14) Run the holds queue builder 15) Note the holds queue targets only items from that volume 16) Check out all the items of that volume to other patrosn 17) Re-run the holds queue builder 18) Note the holds queue no longer has a line for that hold, as all items that could fill that hold are now unavailable Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Created attachment 102240 [details] [review] Bug 24860: Add new syspref EnableVolumeHolds Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Created attachment 102241 [details] [review] Bug 24860: Implement reserves.volume_id Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Created attachment 102242 [details] [review] Bug 24860: Add ability to select a volume when placing a hold Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Created attachment 102243 [details] [review] Bug 24860: Add ability to select a volume when placing a hold - OPAC Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Created attachment 102244 [details] [review] Bug 24860: Skip non-matching volume holds in CheckReserves Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Created attachment 102245 [details] [review] Bug 24860: Skip non-matching volume holds in HoldsQueue Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Created attachment 102246 [details] [review] Bug 24860: Update existing Schema files [DO NOT PUSH] Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Created attachment 102247 [details] [review] Bug 24860: Add hold's volume to holds table on circulation.pl and moremember.pl Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Created attachment 106589 [details] [review] Bug 24860: Add reserves.volume_id This feature builds upon bug 24857 and allows placing holds that target a specific volume of a record. It is patterned after the feature that allows limiting hold selection by itemtype ( AllowHoldItemTypeSelection ). Test Plan: 1) Apply bug 24857 and this bug's patches 2) Run updatedatabase.pl 3) Restart all the things! 4) Enable the sysprefs EnableVolumes and EnableVolumeHolds 5) Create a record, items and volumes, with each item being assigned to a volume 6) Place a hold for a patron 7) Note the new selector to choose a volume in addition to record and item level holds 8) Place a volume level hold 9) Check in an item from the record that is not part of that volume 10) Note the hold is not trapped for that item 11) Check in an item from the record that *is* part of that volume 12) Note the hold is trapped for that item 13) Place a volume level hold for another item 14) Run the holds queue builder 15) Note the holds queue targets only items from that volume 16) Check out all the items of that volume to other patrosn 17) Re-run the holds queue builder 18) Note the holds queue no longer has a line for that hold, as all items that could fill that hold are now unavailable Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Created attachment 106590 [details] [review] Bug 24860: Add new syspref EnableVolumeHolds Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Created attachment 106591 [details] [review] Bug 24860: Implement reserves.volume_id Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Created attachment 106592 [details] [review] Bug 24860: Add ability to select a volume when placing a hold Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Created attachment 106593 [details] [review] Bug 24860: Add ability to select a volume when placing a hold - OPAC Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Created attachment 106594 [details] [review] Bug 24860: Skip non-matching volume holds in CheckReserves Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Created attachment 106595 [details] [review] Bug 24860: Skip non-matching volume holds in HoldsQueue Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Created attachment 106596 [details] [review] Bug 24860: Add hold's volume to holds table on circulation.pl and moremember.pl Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Created attachment 106597 [details] [review] Bug 24860: Update existing Schema files [DO NOT PUSH] Created attachment 106682 [details] [review] Bug 24860: Add reserves.volume_id This feature builds upon bug 24857 and allows placing holds that target a specific volume of a record. It is patterned after the feature that allows limiting hold selection by itemtype ( AllowHoldItemTypeSelection ). Test Plan: 1) Apply bug 24857 and this bug's patches 2) Run updatedatabase.pl 3) Restart all the things! 4) Enable the sysprefs EnableVolumes and EnableVolumeHolds 5) Create a record, items and volumes, with each item being assigned to a volume 6) Place a hold for a patron 7) Note the new selector to choose a volume in addition to record and item level holds 8) Place a volume level hold 9) Check in an item from the record that is not part of that volume 10) Note the hold is not trapped for that item 11) Check in an item from the record that *is* part of that volume 12) Note the hold is trapped for that item 13) Place a volume level hold for another item 14) Run the holds queue builder 15) Note the holds queue targets only items from that volume 16) Check out all the items of that volume to other patrosn 17) Re-run the holds queue builder 18) Note the holds queue no longer has a line for that hold, as all items that could fill that hold are now unavailable Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Hugo Agud (hagud@orex.es Current status: Signed Off Current depends: 24857 Created attachment 106683 [details] [review] Bug 24860: Add new syspref EnableVolumeHolds Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Your Full Name <your_email> Created attachment 106684 [details] [review] Bug 24860: Implement reserves.volume_id Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Your Full Name <your_email> Created attachment 106685 [details] [review] Bug 24860: Add ability to select a volume when placing a hold Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Your Full Name <your_email> Created attachment 106686 [details] [review] Bug 24860: Add ability to select a volume when placing a hold - OPAC Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Your Full Name <your_email> Created attachment 106687 [details] [review] Bug 24860: Skip non-matching volume holds in CheckReserves Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Your Full Name <your_email> Created attachment 106688 [details] [review] Bug 24860: Skip non-matching volume holds in HoldsQueue Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Your Full Name <your_email> Created attachment 106689 [details] [review] Bug 24860: Add hold's volume to holds table on circulation.pl and moremember.pl Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Your Full Name <your_email> Created attachment 106690 [details] [review] Bug 24860: Update existing Schema files [DO NOT PUSH] Signed-off-by: Your Full Name <your_email> I have already signed this patch, the patch does what it promises, but I wish to share some doubts. When you talk about volume, you're talking about Bindings items? We only can add a volume to an intem if the items is not created, there is no way of assign volumes to already created items? Volume is shown in a new colum, but in fact the value is also copied in enumchrom, that it can manage this and other values what happens if there is already another value at enumchrom? At opac-detail at staff we are seeing twice de value (Volume and enumchrom), however at edit items page, we are not seeing volume column, just enumchron that it can be modified by a librarian without any problem. There is no info on volume on opac-detail.tt page, then the user can't know there is a volume, although it can place a hold at volume level, without knowing anything kindest regards Created attachment 106742 [details] [review] Bug 24860: Add reserves.volume_id This feature builds upon bug 24857 and allows placing holds that target a specific volume of a record. It is patterned after the feature that allows limiting hold selection by itemtype ( AllowHoldItemTypeSelection ). Test Plan: 1) Apply bug 24857 and this bug's patches 2) Run updatedatabase.pl 3) Restart all the things! 4) Enable the sysprefs EnableVolumes and EnableVolumeHolds 5) Create a record, items and volumes, with each item being assigned to a volume 6) Place a hold for a patron 7) Note the new selector to choose a volume in addition to record and item level holds 8) Place a volume level hold 9) Check in an item from the record that is not part of that volume 10) Note the hold is not trapped for that item 11) Check in an item from the record that *is* part of that volume 12) Note the hold is trapped for that item 13) Place a volume level hold for another item 14) Run the holds queue builder 15) Note the holds queue targets only items from that volume 16) Check out all the items of that volume to other patrosn 17) Re-run the holds queue builder 18) Note the holds queue no longer has a line for that hold, as all items that could fill that hold are now unavailable Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Your Full Name <your_email> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 106743 [details] [review] Bug 24860: Add new syspref EnableVolumeHolds Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Your Full Name <your_email> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 106744 [details] [review] Bug 24860: Implement reserves.volume_id Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Your Full Name <your_email> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 106745 [details] [review] Bug 24860: Add ability to select a volume when placing a hold Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Your Full Name <your_email> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 106746 [details] [review] Bug 24860: Add ability to select a volume when placing a hold - OPAC Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Your Full Name <your_email> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 106747 [details] [review] Bug 24860: Skip non-matching volume holds in CheckReserves Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Your Full Name <your_email> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 106748 [details] [review] Bug 24860: Skip non-matching volume holds in HoldsQueue Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Your Full Name <your_email> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 106749 [details] [review] Bug 24860: Add hold's volume to holds table on circulation.pl and moremember.pl Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Your Full Name <your_email> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 106750 [details] [review] Bug 24860: Update existing Schema files [DO NOT PUSH] Signed-off-by: Your Full Name <your_email> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Currently blocked by depending bug 24857 not applying. Created attachment 108458 [details] [review] Bug 24860: Add reserves.volume_id This feature builds upon bug 24857 and allows placing holds that target a specific volume of a record. It is patterned after the feature that allows limiting hold selection by itemtype ( AllowHoldItemTypeSelection ). Test Plan: 1) Apply bug 24857 and this bug's patches 2) Run updatedatabase.pl 3) Restart all the things! 4) Enable the sysprefs EnableVolumes and EnableVolumeHolds 5) Create a record, items and volumes, with each item being assigned to a volume 6) Place a hold for a patron 7) Note the new selector to choose a volume in addition to record and item level holds 8) Place a volume level hold 9) Check in an item from the record that is not part of that volume 10) Note the hold is not trapped for that item 11) Check in an item from the record that *is* part of that volume 12) Note the hold is trapped for that item 13) Place a volume level hold for another item 14) Run the holds queue builder 15) Note the holds queue targets only items from that volume 16) Check out all the items of that volume to other patrosn 17) Re-run the holds queue builder 18) Note the holds queue no longer has a line for that hold, as all items that could fill that hold are now unavailable Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 108459 [details] [review] Bug 24860: Add new syspref EnableVolumeHolds Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 108460 [details] [review] Bug 24860: Implement reserves.volume_id Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 108461 [details] [review] Bug 24860: Add ability to select a volume when placing a hold Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 108462 [details] [review] Bug 24860: Add ability to select a volume when placing a hold - OPAC Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 108463 [details] [review] Bug 24860: Skip non-matching volume holds in CheckReserves Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 108464 [details] [review] Bug 24860: Skip non-matching volume holds in HoldsQueue Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 108465 [details] [review] Bug 24860: Add hold's volume to holds table on circulation.pl and moremember.pl Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 108466 [details] [review] Bug 24860: Update existing Schema files [DO NOT PUSH] Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 109097 [details] [review] Bug 24860: Add reserves.volume_id This feature builds upon bug 24857 and allows placing holds that target a specific volume of a record. It is patterned after the feature that allows limiting hold selection by itemtype ( AllowHoldItemTypeSelection ). Test Plan: 1) Apply bug 24857 and this bug's patches 2) Run updatedatabase.pl 3) Restart all the things! 4) Enable the sysprefs EnableVolumes and EnableVolumeHolds 5) Create a record, items and volumes, with each item being assigned to a volume 6) Place a hold for a patron 7) Note the new selector to choose a volume in addition to record and item level holds 8) Place a volume level hold 9) Check in an item from the record that is not part of that volume 10) Note the hold is not trapped for that item 11) Check in an item from the record that *is* part of that volume 12) Note the hold is trapped for that item 13) Place a volume level hold for another item 14) Run the holds queue builder 15) Note the holds queue targets only items from that volume 16) Check out all the items of that volume to other patrosn 17) Re-run the holds queue builder 18) Note the holds queue no longer has a line for that hold, as all items that could fill that hold are now unavailable Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 109098 [details] [review] Bug 24860: Add new syspref EnableVolumeHolds Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 109099 [details] [review] Bug 24860: Implement reserves.volume_id Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 109100 [details] [review] Bug 24860: Add ability to select a volume when placing a hold Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 109101 [details] [review] Bug 24860: Add ability to select a volume when placing a hold - OPAC Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 109102 [details] [review] Bug 24860: Skip non-matching volume holds in CheckReserves Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 109103 [details] [review] Bug 24860: Skip non-matching volume holds in HoldsQueue Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 109104 [details] [review] Bug 24860: Add hold's volume to holds table on circulation.pl and moremember.pl Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 109105 [details] [review] Bug 24860: Update existing Schema files [DO NOT PUSH] Created attachment 109171 [details] [review] Bug 24860: Add reserves.volume_id This feature builds upon bug 24857 and allows placing holds that target a specific volume of a record. It is patterned after the feature that allows limiting hold selection by itemtype ( AllowHoldItemTypeSelection ). Test Plan: 1) Apply bug 24857 and this bug's patches 2) Run updatedatabase.pl 3) Restart all the things! 4) Enable the sysprefs EnableVolumes and EnableVolumeHolds 5) Create a record, items and volumes, with each item being assigned to a volume 6) Place a hold for a patron 7) Note the new selector to choose a volume in addition to record and item level holds 8) Place a volume level hold 9) Check in an item from the record that is not part of that volume 10) Note the hold is not trapped for that item 11) Check in an item from the record that *is* part of that volume 12) Note the hold is trapped for that item 13) Place a volume level hold for another item 14) Run the holds queue builder 15) Note the holds queue targets only items from that volume 16) Check out all the items of that volume to other patrosn 17) Re-run the holds queue builder 18) Note the holds queue no longer has a line for that hold, as all items that could fill that hold are now unavailable Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 109172 [details] [review] Bug 24860: Add new syspref EnableVolumeHolds Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 109173 [details] [review] Bug 24860: Implement reserves.volume_id Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 109174 [details] [review] Bug 24860: Add ability to select a volume when placing a hold Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 109175 [details] [review] Bug 24860: Add ability to select a volume when placing a hold - OPAC Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 109176 [details] [review] Bug 24860: Skip non-matching volume holds in CheckReserves Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 109177 [details] [review] Bug 24860: Skip non-matching volume holds in HoldsQueue Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 109178 [details] [review] Bug 24860: Add hold's volume to holds table on circulation.pl and moremember.pl Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 109179 [details] [review] Bug 24860: Update existing Schema files [DO NOT PUSH] Created attachment 113616 [details] [review] Bug 24860: Add reserves.volume_id This feature builds upon bug 24857 and allows placing holds that target a specific volume of a record. It is patterned after the feature that allows limiting hold selection by itemtype ( AllowHoldItemTypeSelection ). Test Plan: 1) Apply bug 24857 and this bug's patches 2) Run updatedatabase.pl 3) Restart all the things! 4) Enable the sysprefs EnableVolumes and EnableVolumeHolds 5) Create a record, items and volumes, with each item being assigned to a volume 6) Place a hold for a patron 7) Note the new selector to choose a volume in addition to record and item level holds 8) Place a volume level hold 9) Check in an item from the record that is not part of that volume 10) Note the hold is not trapped for that item 11) Check in an item from the record that *is* part of that volume 12) Note the hold is trapped for that item 13) Place a volume level hold for another item 14) Run the holds queue builder 15) Note the holds queue targets only items from that volume 16) Check out all the items of that volume to other patrosn 17) Re-run the holds queue builder 18) Note the holds queue no longer has a line for that hold, as all items that could fill that hold are now unavailable Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 113617 [details] [review] Bug 24860: Add new syspref EnableVolumeHolds Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 113618 [details] [review] Bug 24860: Implement reserves.volume_id Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 113619 [details] [review] Bug 24860: Add ability to select a volume when placing a hold Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 113620 [details] [review] Bug 24860: Add ability to select a volume when placing a hold - OPAC Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 113621 [details] [review] Bug 24860: Skip non-matching volume holds in CheckReserves Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 113622 [details] [review] Bug 24860: Skip non-matching volume holds in HoldsQueue Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 113623 [details] [review] Bug 24860: Add hold's volume to holds table on circulation.pl and moremember.pl Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 113624 [details] [review] Bug 24860: Update existing Schema files [DO NOT PUSH] Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 24860: Add reserves.volume_id error: sha1 information is lacking or useless (installer/data/mysql/atomicupdate/volumes.perl). error: could not build fake ancestor Patch failed at 0001 Bug 24860: Add reserves.volume_id The copy of the patch that failed is found in: .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". Created attachment 135922 [details] [review] Bug 24860: Add reserves.item_group_it This feature builds upon bug 24857 and allows placing holds that target a specific item group of a record. It is patterned after the feature that allows limiting hold selection by itemtype ( AllowHoldItemTypeSelection ). Test Plan: 1) Apply bug 24857 and this bug's patches 2) Run updatedatabase.pl 3) Restart all the things! 4) Enable the sysprefs EnableVolumes and EnableVolumeHolds 5) Create a record, items and item groups, with each item being assigned to an item group 6) Place a hold for a patron 7) Note the new selector to choose an item group in addition to record and item level holds 8) Place an item group level hold 9) Check in an item from the record that is not part of that item group 10) Note the hold is not trapped for that item 11) Check in an item from the record that *is* part of that item group 12) Note the hold is trapped for that item 13) Place an item group level hold for another item 14) Run the holds queue builder 15) Note the holds queue targets only items from that item group 16) Check out all the items of that item group to other patrosn 17) Re-run the holds queue builder 18) Note the holds queue no longer has a line for that hold, as all items that could fill that hold are now unavailable Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 135923 [details] [review] Bug 24860: Add new syspref EnableItemGroupHolds Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 135924 [details] [review] Bug 24860: Implement reserves.item_group_id Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 135925 [details] [review] Bug 24860: Update existing Schema files [DO NOT PUSH] Created attachment 135926 [details] [review] Bug 24860: Add ability to select an item group when placing a hold Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 135927 [details] [review] Bug 24860: Add ability to select an item group when placing a hold - OPAC Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 135928 [details] [review] Bug 24860: Skip non-matching item group holds in CheckReserves Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 135929 [details] [review] Bug 24860: Skip non-matching item group holds in HoldsQueue Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 135930 [details] [review] Bug 24860: Add hold's item group to holds table on circulation.pl and moremember.pl Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Sorry Kyle.. I'm not seeing tests added or adopted for the C4 module changes.. could you add these please? Failing QA for now... but this is close :) I've been playing with this a little whilst continuing QA. Should there be a way from the OPAC to see what items are in a group when placing a group hold. I suppose, the main thoughts behind this is 'the patron doesn't need to know' and that the group name should be descriptive enough for them to make an informed choice at hold time?.. or maybe I've missed a bug where exposing the groups more on the OPAC is done? Created attachment 138221 [details] [review] Bug 24860: Add reserves.item_group_it This feature builds upon bug 24857 and allows placing holds that target a specific item group of a record. It is patterned after the feature that allows limiting hold selection by itemtype ( AllowHoldItemTypeSelection ). Test Plan: 1) Apply bug 24857 and this bug's patches 2) Run updatedatabase.pl 3) Restart all the things! 4) Enable the sysprefs EnableVolumes and EnableVolumeHolds 5) Create a record, items and item groups, with each item being assigned to an item group 6) Place a hold for a patron 7) Note the new selector to choose an item group in addition to record and item level holds 8) Place an item group level hold 9) Check in an item from the record that is not part of that item group 10) Note the hold is not trapped for that item 11) Check in an item from the record that *is* part of that item group 12) Note the hold is trapped for that item 13) Place an item group level hold for another item 14) Run the holds queue builder 15) Note the holds queue targets only items from that item group 16) Check out all the items of that item group to other patrosn 17) Re-run the holds queue builder 18) Note the holds queue no longer has a line for that hold, as all items that could fill that hold are now unavailable Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 138222 [details] [review] Bug 24860: Add new syspref EnableItemGroupHolds Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 138224 [details] [review] Bug 24860: Implement reserves.item_group_id Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 138225 [details] [review] Bug 24860: Add ability to select an item group when placing a hold Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 138226 [details] [review] Bug 24860: Add ability to select an item group when placing a hold - OPAC Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 138227 [details] [review] Bug 24860: Skip non-matching item group holds in CheckReserves Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 138228 [details] [review] Bug 24860: Skip non-matching item group holds in HoldsQueue Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 138229 [details] [review] Bug 24860: Add hold's item group to holds table on circulation.pl and moremember.pl Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 138230 [details] [review] Bug 24860: Update existing Schema files [DO NOT PUSH] (In reply to Martin Renvoize from comment #121) > I've been playing with this a little whilst continuing QA. Should there be > a way from the OPAC to see what items are in a group when placing a group > hold. I suppose, the main thoughts behind this is 'the patron doesn't need > to know' and that the group name should be descriptive enough for them to > make an informed choice at hold time?.. or maybe I've missed a bug where > exposing the groups more on the OPAC is done? You've got it. This is being used in production by one of our partners, and everything is centered around the item group description. Since items in a group are supposed to be identical, knowing which items are in a group does not provide any benefit outside the group name itself. (In reply to Martin Renvoize from comment #120) > Sorry Kyle.. I'm not seeing tests added or adopted for the C4 module > changes.. could you add these please? > > Failing QA for now... but this is close :) I've not seeing what module is missing tests. I've added tests for AddReserve already! Is there one I missed? 1) Unit tests
> I've not seeing what module is missing tests. I've added tests for
> AddReserve already! Is there one I missed?
Doing some code review and... there are some more *hides*
Maybe discuss with Martin which ones should be included in tests:
* C4/HoldsQueue.pm
* GetHoldsQueueItems
* GetPendingHoldRequestsForBib
* MapItemsToHoldRequests
C4/Reserves.pm
* AddReserve (covered)
* CheckReserves
* _Findgroupreserve
Koha/Hold.pm
* item_group
* forced_hold_level
Koha/REST/V1/Holds.pm
* add
2) Fixes some smaller stuff in 2 follow-ups, please have a look!
3) Holds to pull is off
First my test case:
* Added 9 items to a record, 3 each for vol. 1-3 in enumchron
* Grouped them into 3 item groups
* In the OPAC, I placed a hold onto the volume 2 group
In /circ/pendingreserves.pl it shows wrong information:
* 39999000005776 or any available.
* Available enumchron: vol. 1, vol. 2, vol. 3
* There is no information that only volume 2 is wanted.
4) Holds queue
Explodes:
C4::HoldsQueue::GetHoldsQueueItems(): DBI Exception: DBD::mysql::st execute failed: Unknown column 'item_group_items.item_group_id' in 'on clause' at /kohadevbox/koha/circ/view_holdsqueue.pl line 51
at /usr/share/perl5/DBIx/Class/Exception.pm line 77
I've run a dbic and a restart_all, but the problem remains. The column exists in my database:
describe item_group_items;
+---------------------+---------+------+-----+---------+----------------+
| Field | Type | Null | Key | Default | Extra |
+---------------------+---------+------+-----+---------+----------------+
| item_group_items_id | int(11) | NO | PRI | NULL | auto_increment |
| item_group_id | int(11) | NO | MUL | 0 | |
| item_id | int(11) | NO | UNI | 0 | |
+---------------------+---------+------+-----+---------+----------------+
Created attachment 141214 [details] [review] Bug 24860: (QA follow-up) Fix number of tests in t/db_dependent/Koha/Holds.t Created attachment 141215 [details] [review] Bug 24860: (QA follow-up) Terminology and translation fixes * Fixes a single to a double underscore for translation in holds.js * Fixes item_group to item group in API description * Fixes volume hold to item group hold in syspref db descrpitions Created attachment 141735 [details] [review] Bug 24860: Add reserves.item_group_it This feature builds upon bug 24857 and allows placing holds that target a specific item group of a record. It is patterned after the feature that allows limiting hold selection by itemtype ( AllowHoldItemTypeSelection ). Test Plan: 1) Apply bug 24857 and this bug's patches 2) Run updatedatabase.pl 3) Restart all the things! 4) Enable the sysprefs EnableVolumes and EnableVolumeHolds 5) Create a record, items and item groups, with each item being assigned to an item group 6) Place a hold for a patron 7) Note the new selector to choose an item group in addition to record and item level holds 8) Place an item group level hold 9) Check in an item from the record that is not part of that item group 10) Note the hold is not trapped for that item 11) Check in an item from the record that *is* part of that item group 12) Note the hold is trapped for that item 13) Place an item group level hold for another item 14) Run the holds queue builder 15) Note the holds queue targets only items from that item group 16) Check out all the items of that item group to other patrosn 17) Re-run the holds queue builder 18) Note the holds queue no longer has a line for that hold, as all items that could fill that hold are now unavailable Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 141736 [details] [review] Bug 24860: Add new syspref EnableItemGroupHolds Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 141737 [details] [review] Bug 24860: Implement reserves.item_group_id Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 141738 [details] [review] Bug 24860: Add ability to select an item group when placing a hold Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 141739 [details] [review] Bug 24860: Add ability to select an item group when placing a hold - OPAC Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 141740 [details] [review] Bug 24860: Skip non-matching item group holds in CheckReserves Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 141741 [details] [review] Bug 24860: Skip non-matching item group holds in HoldsQueue Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 141742 [details] [review] Bug 24860: Add hold's item group to holds table on circulation.pl and moremember.pl Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 141743 [details] [review] Bug 24860: Update existing Schema files [DO NOT PUSH] Created attachment 141744 [details] [review] Bug 24860: (QA follow-up) Fix number of tests in t/db_dependent/Koha/Holds.t Created attachment 141745 [details] [review] Bug 24860: (QA follow-up) Terminology and translation fixes * Fixes a single to a double underscore for translation in holds.js * Fixes item_group to item group in API description * Fixes volume hold to item group hold in syspref db descrpitions Created attachment 141746 [details] [review] Bug 24860: (QA follow-up) Add unit tests to cover changes to HoldsQueue.pm Created attachment 141747 [details] [review] Bug 24860: (QA follow-up) Add unit tests to cover changes to Reserves.pm Created attachment 141748 [details] [review] Bug 24860: (QA follow-up) Add unit tests to cover Koha::Hold::item_group Created attachment 141749 [details] [review] Bug 24860: (QA follow-up) Add unit tests to cover Koha::Holds::forced_hold_level Created attachment 141750 [details] [review] Bug 24860: (QA follow-up) Add unit tests to cover Koha::REST::V1::Holds::add (In reply to Katrin Fischer from comment #133) > 1) Unit tests I've added unit tests for everything you listed! > 2) Fixes some smaller stuff in 2 follow-ups, please have a look! Thank you! > 3) Holds to pull is off > > First my test case: > * Added 9 items to a record, 3 each for vol. 1-3 in enumchron > * Grouped them into 3 item groups > * In the OPAC, I placed a hold onto the volume 2 group > > In /circ/pendingreserves.pl it shows wrong information: > * 39999000005776 or any available. > * Available enumchron: vol. 1, vol. 2, vol. 3 > * There is no information that only volume 2 is wanted. I don't think Holds to pull is the tool a library should be using if they are using item group holds. This same is true for the holds itemtype filter, which has been in Koha since 16.05. I'm finding the holds to pull interface doesn't display data well. For instance, if I place one item level hold, and one biblio level hold for the same record, holds to pull doesn't really indicate that. If you can can tell me what you'd like to see, I'll see what I can do! > 4) Holds queue One line of the query got removed in a rebase. Fixed! Having another look here... 1) All the new and old tests pass.
But: I did a reset_all to have a clean starting page before applying these patches. And my branches are all mangled after running the tests:
I end up with 36 branches on the sample database, a lot of those generated ones like: brW5iS.
There are also a ton of generated patrons...
I am not completely sure it's the new tests here, but could you please check your clean-up?
3) Holds queue is looking good now:
a1 or any item from item_group A
Just please remove the _ :)
2) Pending holds / Holds to pull
> I don't think Holds to pull is the tool a library should be using if they
> are using item group holds. This same is true for the holds itemtype filter,
> which has been in Koha since 16.05. I'm finding the holds to pull interface
> doesn't display data well. For instance, if I place one item level hold, and
> one biblio level hold for the same record, holds to pull doesn't really
> indicate that. If you can can tell me what you'd like to see, I'll see what
> I can do!
I don't think we can argue that, unless we hid the report once the item group holds are activated... and that might create quite some complaints. I know single branch libraries like to use the report, also the less busy ones. Maybe it would be worth adding a note to the system preference? "Might not go well with..." or so? In general the page needs work, I agree with you on that!
I did some more testing:
a) If there is a record level hold only: Only 39999000011418
b) If there is an item level hold only: 39999000005097 or any available.
c) If there is a group level hold only: 39999000005097 or any available.
For a start, could we catch the third case and have a correct message display? Similar to the one on holds queue?
I'd be ok with that as a solution for this patch set.
For the future 2 ideas (just to get your opinion, will file later):
* If there are different types of holds we could display this as a note: "Different types of holds, please check holds page"?
* We could repeat the information for every hold, maybe deduplicated. At the moment it looks like we only get the info for the first hold placed. Maye something like:
* Only 39999000011418 (1 hold)
* 39999000005097 or any available (2 holds)
To summarize:
* Verify/make sure tests clean up their created data
* Remove _ from message on holds queue page
* Adapt message when only a item group hold exists on holds to pull
OK?
Created attachment 142413 [details] [review] Bug 24860: Add reserves.item_group_it This feature builds upon bug 24857 and allows placing holds that target a specific item group of a record. It is patterned after the feature that allows limiting hold selection by itemtype ( AllowHoldItemTypeSelection ). Test Plan: 1) Apply bug 24857 and this bug's patches 2) Run updatedatabase.pl 3) Restart all the things! 4) Enable the sysprefs EnableVolumes and EnableVolumeHolds 5) Create a record, items and item groups, with each item being assigned to an item group 6) Place a hold for a patron 7) Note the new selector to choose an item group in addition to record and item level holds 8) Place an item group level hold 9) Check in an item from the record that is not part of that item group 10) Note the hold is not trapped for that item 11) Check in an item from the record that *is* part of that item group 12) Note the hold is trapped for that item 13) Place an item group level hold for another item 14) Run the holds queue builder 15) Note the holds queue targets only items from that item group 16) Check out all the items of that item group to other patrosn 17) Re-run the holds queue builder 18) Note the holds queue no longer has a line for that hold, as all items that could fill that hold are now unavailable Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 142414 [details] [review] Bug 24860: Add new syspref EnableItemGroupHolds Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 142415 [details] [review] Bug 24860: Implement reserves.item_group_id Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 142416 [details] [review] Bug 24860: Add ability to select an item group when placing a hold Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 142417 [details] [review] Bug 24860: Add ability to select an item group when placing a hold - OPAC Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 142418 [details] [review] Bug 24860: Skip non-matching item group holds in CheckReserves Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 142419 [details] [review] Bug 24860: Skip non-matching item group holds in HoldsQueue Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 142420 [details] [review] Bug 24860: Add hold's item group to holds table on circulation.pl and moremember.pl Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 142421 [details] [review] Bug 24860: Update existing Schema files [DO NOT PUSH] Created attachment 142422 [details] [review] Bug 24860: (QA follow-up) Fix number of tests in t/db_dependent/Koha/Holds.t Created attachment 142423 [details] [review] Bug 24860: (QA follow-up) Terminology and translation fixes * Fixes a single to a double underscore for translation in holds.js * Fixes item_group to item group in API description * Fixes volume hold to item group hold in syspref db descrpitions Created attachment 142424 [details] [review] Bug 24860: (QA follow-up) Add unit tests to cover changes to HoldsQueue.pm Created attachment 142425 [details] [review] Bug 24860: (QA follow-up) Add unit tests to cover changes to Reserves.pm Created attachment 142426 [details] [review] Bug 24860: (QA follow-up) Add unit tests to cover Koha::Hold::item_group Created attachment 142427 [details] [review] Bug 24860: (QA follow-up) Add unit tests to cover Koha::Holds::forced_hold_level Created attachment 142428 [details] [review] Bug 24860: (QA follow-up) Add unit tests to cover Koha::REST::V1::Holds::add Created attachment 142429 [details] [review] Bug 24860: (QA follow-up) Fix typo in holds queue viewer Created attachment 142430 [details] [review] Bug 24860: (QA follow-up) Show group holds on Holds to pull > To summarize: > * Verify/make sure tests clean up their created data Fixed on Bug 31920 > * Remove _ from message on holds queue page Fixed with followup! > * Adapt message when only a item group hold exists on holds to pull Also fixed with a followup! Back here. Sorry Kyle, I tried to understand what's going wrong here, but failed:
When opening 'Holds to pull':
Can't call method "id" on an undefined value at /kohadevbox/koha/circ/pendingreserves.pl line 288
287: if ( $res_info->item_group ) {
288: $hold_info->{barcodes} = [ uniq map { defined $_->barcode && $_->item_group->id == $res_info->item_group_id ? $_->barcode : () } @$items ];
289: }
If you can fix quickly, I'll retest.
(In reply to Katrin Fischer from comment #175) > Sorry Kyle, I tried to understand what's going wrong here, but failed: > > When opening 'Holds to pull': > > Can't call method "id" on an undefined value at > /kohadevbox/koha/circ/pendingreserves.pl line 288 > > 287: if ( $res_info->item_group ) { > 288: $hold_info->{barcodes} = [ uniq map { defined $_->barcode && > $_->item_group->id == $res_info->item_group_id ? $_->barcode : () } @$items > ]; > 289: } > > If you can fix quickly, I'll retest. *ping*? Created attachment 142827 [details] [review] Bug 24860: (QA follow-up): Don't assume item group is set for all items Back here again. I am sorry Kyle, the latest follow-up fixed the error, but the new text in pendingreserves never displays. I have tried to figure out what's going on there, but got stuck on this line:
+ if ( $res_info->item_group ) {
+ $hold_info->{barcodes} = [ uniq map { defined $_->barcode && $_->item_group && ( $_->item_group->id == $res_info->item_group_id ) ? $_->barcode : () } @$items ];
+ }
+
I believe that [% IF ( hold_info.barcodes.size ) %] is not set for a hold group hold.
Created attachment 142955 [details] [review] Bug 24860: Add reserves.item_group_it This feature builds upon bug 24857 and allows placing holds that target a specific item group of a record. It is patterned after the feature that allows limiting hold selection by itemtype ( AllowHoldItemTypeSelection ). Test Plan: 1) Apply bug 24857 and this bug's patches 2) Run updatedatabase.pl 3) Restart all the things! 4) Enable the sysprefs EnableVolumes and EnableVolumeHolds 5) Create a record, items and item groups, with each item being assigned to an item group 6) Place a hold for a patron 7) Note the new selector to choose an item group in addition to record and item level holds 8) Place an item group level hold 9) Check in an item from the record that is not part of that item group 10) Note the hold is not trapped for that item 11) Check in an item from the record that *is* part of that item group 12) Note the hold is trapped for that item 13) Place an item group level hold for another item 14) Run the holds queue builder 15) Note the holds queue targets only items from that item group 16) Check out all the items of that item group to other patrosn 17) Re-run the holds queue builder 18) Note the holds queue no longer has a line for that hold, as all items that could fill that hold are now unavailable Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 142956 [details] [review] Bug 24860: Add new syspref EnableItemGroupHolds Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 142957 [details] [review] Bug 24860: Implement reserves.item_group_id Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 142958 [details] [review] Bug 24860: Add ability to select an item group when placing a hold Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 142959 [details] [review] Bug 24860: Add ability to select an item group when placing a hold - OPAC Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 142960 [details] [review] Bug 24860: Skip non-matching item group holds in CheckReserves Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 142961 [details] [review] Bug 24860: Skip non-matching item group holds in HoldsQueue Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 142962 [details] [review] Bug 24860: Add hold's item group to holds table on circulation.pl and moremember.pl Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Created attachment 142963 [details] [review] Bug 24860: Update existing Schema files [DO NOT PUSH] Created attachment 142964 [details] [review] Bug 24860: (QA follow-up) Fix number of tests in t/db_dependent/Koha/Holds.t Created attachment 142965 [details] [review] Bug 24860: (QA follow-up) Terminology and translation fixes * Fixes a single to a double underscore for translation in holds.js * Fixes item_group to item group in API description * Fixes volume hold to item group hold in syspref db descrpitions Created attachment 142966 [details] [review] Bug 24860: (QA follow-up) Add unit tests to cover changes to HoldsQueue.pm Created attachment 142967 [details] [review] Bug 24860: (QA follow-up) Add unit tests to cover changes to Reserves.pm Created attachment 142968 [details] [review] Bug 24860: (QA follow-up) Add unit tests to cover Koha::Hold::item_group Created attachment 142969 [details] [review] Bug 24860: (QA follow-up) Add unit tests to cover Koha::Holds::forced_hold_level Created attachment 142970 [details] [review] Bug 24860: (QA follow-up) Add unit tests to cover Koha::REST::V1::Holds::add Created attachment 142971 [details] [review] Bug 24860: (QA follow-up) Fix typo in holds queue viewer Created attachment 142972 [details] [review] Bug 24860: (QA follow-up) Show group holds on Holds to pull Created attachment 142973 [details] [review] Bug 24860: (QA follow-up): Don't assume item group is set for all items Created attachment 142974 [details] [review] Bug 24860: (QA follow-up) Ask for any available item of matching group Created attachment 142975 [details] [review] Bug 24860: (QA follow-up) Don't show holds where a matching item group item is not available (In reply to Katrin Fischer from comment #179) > I am sorry Kyle, the latest follow-up fixed the error, but the new text in > pendingreserves never displays. I have tried to figure out what's going on > there, but got stuck on this line: > > + if ( $res_info->item_group ) { > + $hold_info->{barcodes} = [ uniq map { defined $_->barcode && > $_->item_group && ( $_->item_group->id == $res_info->item_group_id ) ? > $_->barcode : () } @$items ]; > + } > + > > I believe that [% IF ( hold_info.barcodes.size ) %] is not set for a hold > group hold. Yes, the problem here is that there are not items with the selected hold group currently available. We have two choices where, either we hide that hold until there is an item from that item group available, or we just display an "Any item from item group $item_group" message. "Bug 24860: (QA follow-up) Don't show holds where a matching item group item is not available" does the former, "Bug 24860: (QA follow-up) Ask for any available item of matching group" does the latter. Please choose which behavior you would prefer and deprecate the other patch! (In reply to Kyle M Hall from comment #201) > (In reply to Katrin Fischer from comment #179) > > I am sorry Kyle, the latest follow-up fixed the error, but the new text in > > pendingreserves never displays. I have tried to figure out what's going on > > there, but got stuck on this line: > > > > + if ( $res_info->item_group ) { > > + $hold_info->{barcodes} = [ uniq map { defined $_->barcode && > > $_->item_group && ( $_->item_group->id == $res_info->item_group_id ) ? > > $_->barcode : () } @$items ]; > > + } > > + > > > > I believe that [% IF ( hold_info.barcodes.size ) %] is not set for a hold > > group hold. > > Yes, the problem here is that there are not items with the selected hold > group currently available. We have two choices where, either we hide that > hold until there is an item from that item group available, or we just > display an "Any item from item group $item_group" message. > > "Bug 24860: (QA follow-up) Don't show holds where a matching item group item > is not available" does the former, "Bug 24860: (QA follow-up) Ask for any > available item of matching group" does the latter. Please choose which > behavior you would prefer and deprecate the other patch! But it's not true... in my test case I had 2 available items within this group. I can retest, but I am pretty sure. I had created new items in order to group and place a hold on them. Ah! I was testing a slightly different scenario. You are right, the last patch is no good. I've obsoleted it and now everything should be good!
I'm not sure what scenario you are
> But it's not true... in my test case I had 2 available items within this
> group. I can retest, but I am pretty sure. I had created new items in order
> to group and place a hold on them.
Created attachment 142992 [details] [review] Bug 24860: Add reserves.item_group_it This feature builds upon bug 24857 and allows placing holds that target a specific item group of a record. It is patterned after the feature that allows limiting hold selection by itemtype ( AllowHoldItemTypeSelection ). Test Plan: 1) Apply bug 24857 and this bug's patches 2) Run updatedatabase.pl 3) Restart all the things! 4) Enable the sysprefs EnableVolumes and EnableVolumeHolds 5) Create a record, items and item groups, with each item being assigned to an item group 6) Place a hold for a patron 7) Note the new selector to choose an item group in addition to record and item level holds 8) Place an item group level hold 9) Check in an item from the record that is not part of that item group 10) Note the hold is not trapped for that item 11) Check in an item from the record that *is* part of that item group 12) Note the hold is trapped for that item 13) Place an item group level hold for another item 14) Run the holds queue builder 15) Note the holds queue targets only items from that item group 16) Check out all the items of that item group to other patrosn 17) Re-run the holds queue builder 18) Note the holds queue no longer has a line for that hold, as all items that could fill that hold are now unavailable Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 142993 [details] [review] Bug 24860: Add new syspref EnableItemGroupHolds Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 142994 [details] [review] Bug 24860: Implement reserves.item_group_id Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 142995 [details] [review] Bug 24860: Add ability to select an item group when placing a hold Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 142996 [details] [review] Bug 24860: Add ability to select an item group when placing a hold - OPAC Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 142997 [details] [review] Bug 24860: Skip non-matching item group holds in CheckReserves Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 142998 [details] [review] Bug 24860: Skip non-matching item group holds in HoldsQueue Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 142999 [details] [review] Bug 24860: Add hold's item group to holds table on circulation.pl and moremember.pl Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 143000 [details] [review] Bug 24860: Update existing Schema files [DO NOT PUSH] Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 143001 [details] [review] Bug 24860: (QA follow-up) Fix number of tests in t/db_dependent/Koha/Holds.t Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 143002 [details] [review] Bug 24860: (QA follow-up) Terminology and translation fixes * Fixes a single to a double underscore for translation in holds.js * Fixes item_group to item group in API description * Fixes volume hold to item group hold in syspref db descrpitions Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 143003 [details] [review] Bug 24860: (QA follow-up) Add unit tests to cover changes to HoldsQueue.pm Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 143004 [details] [review] Bug 24860: (QA follow-up) Add unit tests to cover changes to Reserves.pm Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 143005 [details] [review] Bug 24860: (QA follow-up) Add unit tests to cover Koha::Hold::item_group Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 143006 [details] [review] Bug 24860: (QA follow-up) Add unit tests to cover Koha::Holds::forced_hold_level Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 143007 [details] [review] Bug 24860: (QA follow-up) Add unit tests to cover Koha::REST::V1::Holds::add Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 143008 [details] [review] Bug 24860: (QA follow-up) Fix typo in holds queue viewer Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 143009 [details] [review] Bug 24860: (QA follow-up) Show group holds on Holds to pull Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 143010 [details] [review] Bug 24860: (QA follow-up): Don't assume item group is set for all items Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 143011 [details] [review] Bug 24860: (QA follow-up) Ask for any available item of matching group Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Pushed to master for 22.11. Nice work everyone, thanks! `biblio_id` int(11) NOT NULL DEFAULT 0 COMMENT 'id for the bibliographic record the group belongs to', DEFAULT 0, is this correct? I think we are missing from_local_hold_group in reserve/request.tt
1581 switch (override_items[itemnumber].holdallowed) {
(In reply to Jonathan Druart from comment #225) > `biblio_id` int(11) NOT NULL DEFAULT 0 COMMENT 'id for the bibliographic > record the group belongs to', > > DEFAULT 0, is this correct? As 0 cannot be used, it's harmless but you are correct and we should update the table schema. (In reply to Kyle M Hall from comment #227) > (In reply to Jonathan Druart from comment #225) > > `biblio_id` int(11) NOT NULL DEFAULT 0 COMMENT 'id for the bibliographic > > record the group belongs to', > > > > DEFAULT 0, is this correct? > > As 0 cannot be used, it's harmless but you are correct and we should update > the table schema. Out of scope for this bug, but the recalls table has the same issue. (In reply to Kyle M Hall from comment #228) > (In reply to Kyle M Hall from comment #227) > > (In reply to Jonathan Druart from comment #225) > > > `biblio_id` int(11) NOT NULL DEFAULT 0 COMMENT 'id for the bibliographic > > > record the group belongs to', > > > > > > DEFAULT 0, is this correct? > > > > As 0 cannot be used, it's harmless but you are correct and we should update > > the table schema. > > Out of scope for this bug, but the recalls table has the same issue. Bug 33053 - Tables item_groups and recalls have a biblio_id column with a default of 0 (In reply to Jonathan Druart from comment #226) > I think we are missing from_local_hold_group in reserve/request.tt > > 1581 switch (override_items[itemnumber].holdallowed) { Bug 33052 - Add a message for from_local_hold_group override See bug 34155, OPAC item level holds="force" is broken in opac-reserve.pl: 495 $biblioLoopIter{force_hold} = 1 if $opac_hold_policy eq 'F'; But we are ignoring this with the change in "24860: Add ability to select an item group when placing a hold - OPAC" |