To recreate: 1 - Find a record with multiple items 2 - Place 4 holds (or more) 3 - Capture one ohld as waiting, one as in transit 4 - View the holds on the record - switch the last to priority one 5 - Waiting and transit statuses get confused
This seems to be the changes on 19636 that doesn't account for waiting and transit holds when renumbering
Created attachment 80933 [details] [review] Bug 21608: WIP - attempt to fix priority dropdowns (18.05 based)
Created attachment 81131 [details] [review] Bug 21608: Disable dropdown for found holds - add button to revert To test: 1 - Find a record with multiple items 2 - Place 4 holds (or more) 3 - Capture one ohld as waiting, one as in transit 4 - View the holds on the record - switch the last to priority one 5 - Waiting and transit statuses get confused 6 - Apply patch 7 - Observe dropdown is now disabled for waiting holds 8 - Confirm other holds operate as expected 9 - Confirm 'Revert found status' resets hold
While this patch depends on 21611 we will need an 18.05 version which does not. Can provide once this one is through QA
Created attachment 81134 [details] [review] Bug 21608: Disable dropdown for found holds - add button to revert This patch disables the dropdown for found holds, and adds a new button to revert the waiting status, setting the hold to priority 1 Additionally we remove some changes from 19469 and update the JS to skip found holds when updating priority To test: 1 - Find a record with multiple items 2 - Place 4 holds (or more) 3 - Capture one ohld as waiting, one as in transit 4 - View the holds on the record - switch the last to priority one 5 - Waiting and transit statuses get confused 6 - Apply patch 7 - Observe dropdown is now disabled for waiting holds 8 - Confirm other holds operate as expected 9 - Confirm 'Revert found status' resets hold
Created attachment 81272 [details] [review] Bug 21608: Disable dropdown for found holds - add button to revert This patch disables the dropdown for found holds, and adds a new button to revert the waiting status, setting the hold to priority 1 Additionally we remove some changes from 19469 and update the JS to skip found holds when updating priority To test: 1 - Find a record with multiple items 2 - Place 4 holds (or more) 3 - Capture one ohld as waiting, one as in transit 4 - View the holds on the record - switch the last to priority one 5 - Waiting and transit statuses get confused 6 - Apply patch 7 - Observe dropdown is now disabled for waiting holds 8 - Confirm other holds operate as expected 9 - Confirm 'Revert found status' resets hold Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Works as expected, Signing Off
Only thing I am not sure about is the label: Revert found status I am not sure if that is easy to understand/translate, but haven't found a better description yet. (not blocker)
The revert "feature to found" doesn't work correctly for me. I had 2 items in transit and one waiting. I reverted 1 transit and 1 waiting. On the holds page, everything looks like expected. BUT: the items are all wrong on normal view. All items are still in transit or waiting. As this is also marked for backporting, maybe we should have a patch without the revert to found button? I'd also suggest to add at least an id to it, so libraries can hide it if necessary.
Sorry, might have missed a reload, display is now correct for the items. I see some problems with reverting: If the patron has chosen an expiration date on placing the hold, it will be overwritten by the pickup date when the hold is found. So all we can do now is revert to "empty" as there is no way to tell the old expiration date. These 2 things should be 2 different fields in db. If the hold was formerly a record level hold, it's a item level hold after reverting. The only indicator if we have an item level or record level hold is the itemnumber field being set or not. As it's always set after the hold has been found, there is no clean way to revert from this either. Without this patch you can only change the hold from "Waiting" to "del" - which means you can only cancel it. I am not sure about the the changed behaviour, especially as this is marked for backporting to 18.05. Maybe we should fix the display here but leave the new button for a separate bug report? We could still lock the pull down maybe - the hold can still be deleted using the red X.
(In reply to Katrin Fischer from comment #10) > Sorry, might have missed a reload, display is now correct for the items. > > I see some problems with reverting: > > If the patron has chosen an expiration date on placing the hold, it will be > overwritten by the pickup date when the hold is found. So all we can do now > is revert to "empty" as there is no way to tell the old expiration date. > These 2 things should be 2 different fields in db. > > If the hold was formerly a record level hold, it's a item level hold after > reverting. The only indicator if we have an item level or record level hold > is the itemnumber field being set or not. As it's always set after the hold > has been found, there is no clean way to revert from this either. This has been existing behaviour through 17.11 - with bug 19636 we saw the dropdowns begin to work incorrectly > > Without this patch you can only change the hold from "Waiting" to "del" - > which means you can only cancel it. I think this was accidental change of behaviour in 21611 > > I am not sure about the the changed behaviour, especially as this is marked > for backporting to 18.05. Maybe we should fix the display here but leave the > new button for a separate bug report? > I am happy to split this on a bug report, but if we remove this feature/don't repair it I expect there will be complaints from the user Use case example: 1 - Item arrives at library for a next available hold 2 - Hold is triggered, item is set to 'waiting' 3 - Patron calls, "Can you please send the book to library X/" 4 - We revert hold to priority 1, change pickup location, check in and route to correct library I can split the revert to its own bug, but I would be inclined to file it as a bug and consider for backport > We could still lock the pull down maybe - the hold can still be deleted > using the red X.
Hm, I wasn't aware it has ever worked (changing priority once waiting). So only one fix needed then: When you revert, you need to clear the expiration date, or the reverted hold will expire way too soon.
(In reply to Katrin Fischer from comment #12) > Hm, I wasn't aware it has ever worked (changing priority once waiting). > > So only one fix needed then: When you revert, you need to clear the > expiration date, or the reverted hold will expire way too soon. Katrin and I discussed this today - while the expiration date reset may be desirable this bug restores existing behaviour from 17.11 New bug 21729
Tests missing.
*** Bug 21752 has been marked as a duplicate of this bug. ***
Created attachment 81835 [details] [review] Bug 21608: (follow-up) Use RevertWaitingStatus and do not alter _FixPriority
Time too short, not trivial
FAIL koha-tmpl/intranet-tmpl/prog/en/includes/holds_table.inc FAIL filters missing_filter at line 194 ( <input type="button" value="Revert found status" onclick="window.location.href='request.pl?action=move&where=down&first_priority=[% first_priority | html %]&last_priority=[% last_priority | html %]&prev_priority=0&next_priority=1&borrowernumber=[% hold.borrowernumber | html %]&biblionumber=[% hold.biblionumber | html %]&itemnumber=[% hold.itemnumber %]&reserve_id=[% hold.reserve_id | html %]&date=[% hold.date | html %]'">)
QA: Having another try
(In reply to Jonathan Druart from comment #14) > Tests missing. Definitely! Interventions in Reserves need tests.
(In reply to Marcel de Rooy from comment #20) > (In reply to Jonathan Druart from comment #14) > > Tests missing. > > Definitely! Interventions in Reserves need tests. And reverting change A in patch B is not my favorite either. Squash them.
Hmm. If I have one waiting and one on pos 1, I can select pos 2 in the priority box and it automatically selects pos 1 again. This is sort of OK but still confusing. Pos 2 should not be there.
Created attachment 82157 [details] [review] Bug 21608: Disable dropdown for found holds - add button to revert This patch disables the dropdown for found holds, and adds a new button to revert the waiting status, setting the hold to priority 1 Additionally we remove some changes from 19469 and update the JS to skip found holds when updating priority To test: 1 - Find a record with multiple items 2 - Place 4 holds (or more) 3 - Capture one ohld as waiting, one as in transit 4 - View the holds on the record - switch the last to priority one 5 - Waiting and transit statuses get confused 6 - Apply patch 7 - Observe dropdown is now disabled for waiting holds 8 - Confirm other holds operate as expected 9 - Confirm 'Revert found status' resets hold Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Bug 21608: (follow-up) Use RevertWaitingStatus and do not alter _FixPriority [EDIT] Squashed. Completely removed the changes to Reserves.pm by adding module prefix in the request.pl script. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
This is a Passed QA with some reservation (as is appropriate for holds patches indeed :) I do not feel completely comfortable with the patch somehow. Intuitively. But I cant say that it is wrong. And the region is a bit buggy. So I pass it on to the RM. He will be able to view it as objectively as possible ;) Leaning a bit here on the tests by Martin and Katrin too.
Oops forgot this suggestion: Rename: Revert found status => Revert waiting status And two functional questions: Should someone be informed about this revert? Or should we make the button preference controlled ? Have fun !
(In reply to Marcel de Rooy from comment #25) > Oops forgot this suggestion: > Rename: Revert found status => Revert waiting status This affects 'In-transit' holds too, I will add an id to the button > And two functional questions: > Should someone be informed about this revert? > Or should we make the button preference controlled ? I think yes, on both of these, however, we are restoring behvaiour (buggy, but existing) so it should be a new report > Have fun ! :-)
Created attachment 82322 [details] [review] Bug 21608: Add ID and fix filters
If no adjustments are made and 'Update holds' is clicked then any waiting holds are reverted
Created attachment 82652 [details] [review] Bug 21608: Add value for 'rank-request' when dropdwn disabled When a field is disabled no value is submitted in the form - this was causing found holds to be reverted To test: 1 - Apply all patches except this one 2 - PLace several holds on one bib 3 - Set at least one waiting or intransit 4 - Making no adjustments hit "Update holds" 5 - Waiting/intransit holds are all reset 6 - Mark them waiting/intransit again 7 - Apply this patch 8 - Click 'update holds' 9 - Found status is not accidentally reverted
Created attachment 82956 [details] [review] Bug 21608: Disable dropdown for found holds - add button to revert This patch disables the dropdown for found holds, and adds a new button to revert the waiting status, setting the hold to priority 1 Additionally we remove some changes from 19469 and update the JS to skip found holds when updating priority To test: 1 - Find a record with multiple items 2 - Place 4 holds (or more) 3 - Capture one ohld as waiting, one as in transit 4 - View the holds on the record - switch the last to priority one 5 - Waiting and transit statuses get confused 6 - Apply patch 7 - Observe dropdown is now disabled for waiting holds 8 - Confirm other holds operate as expected 9 - Confirm 'Revert found status' resets hold Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Bug 21608: (follow-up) Use RevertWaitingStatus and do not alter _FixPriority [EDIT] Completely removed the changes to Reserves.pm by adding module prefix in the request.pl script. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 82957 [details] [review] Bug 21608: Add ID and fix filters Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 82958 [details] [review] Bug 21608: Add value for 'rank-request' when dropdwn disabled When a field is disabled no value is submitted in the form - this was causing found holds to be reverted To test: 1 - Apply all patches except this one 2 - PLace several holds on one bib 3 - Set at least one waiting or intransit 4 - Making no adjustments hit "Update holds" 5 - Waiting/intransit holds are all reset 6 - Mark them waiting/intransit again 7 - Apply this patch 8 - Click 'update holds' 9 - Found status is not accidentally reverted Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 82959 [details] [review] Bug 21608: (QA follow-up) Rename revert button The label 'Revert found status' is replaced by 'Revert waiting status' or 'Revert transit status' depending on hold.found/hold.intransit. Note that hold comes from reserveloop in request.pl. The columns/keys found and intransit are just booleans. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Awesome work all! Pushed to master for 19.05
Pushed to 18.11.x for 18.11.01
Will not be backported to 18.05. Merge conflicts.
Created attachment 83541 [details] [review] Bug 21608: [18.05.X] disable dropdown and add revert button
(In reply to Nick Clemens from comment #37) > Created attachment 83541 [details] [review] [review] > Bug 21608: [18.05.X] disable dropdown and add revert button Please test thoroughly - dropdown counts are higher that they should be, but I think this exists in master too
_FixPriority seems horrifically broken. I was really hoping that someone else had already fixed it... heh