Bug 21608 - Arranging holds priority with dropdowns is faulty when there are waiting/intransit holds
Summary: Arranging holds priority with dropdowns is faulty when there are waiting/intr...
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Target Milestone: ---
Assignee: Nick Clemens
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
: 21752 (view as bug list)
Depends on: 19636 21611
Blocks: 21729
  Show dependency treegraph
 
Reported: 2018-10-18 15:57 UTC by Nick Clemens
Modified: 2019-01-09 19:10 UTC (History)
12 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 21608: WIP - attempt to fix priority dropdowns (18.05 based) (5.53 KB, patch)
2018-10-19 12:54 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21608: Disable dropdown for found holds - add button to revert (2.81 KB, patch)
2018-10-25 11:47 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21608: Disable dropdown for found holds - add button to revert (5.63 KB, patch)
2018-10-25 12:20 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21608: Disable dropdown for found holds - add button to revert (5.69 KB, patch)
2018-10-26 10:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21608: (follow-up) Use RevertWaitingStatus and do not alter _FixPriority (3.86 KB, patch)
2018-11-01 18:56 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21608: Disable dropdown for found holds - add button to revert (7.03 KB, patch)
2018-11-09 11:17 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21608: Add ID and fix filters (1.80 KB, patch)
2018-11-13 19:26 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21608: Add value for 'rank-request' when dropdwn disabled (1.47 KB, patch)
2018-11-26 18:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21608: Disable dropdown for found holds - add button to revert (7.03 KB, patch)
2018-12-07 10:12 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21608: Add ID and fix filters (1.89 KB, patch)
2018-12-07 10:12 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21608: Add value for 'rank-request' when dropdwn disabled (1.56 KB, patch)
2018-12-07 10:12 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21608: (QA follow-up) Rename revert button (2.25 KB, patch)
2018-12-07 10:12 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21608: [18.05.X] disable dropdown and add revert button (4.13 KB, patch)
2018-12-28 17:02 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2018-10-18 15:57:16 UTC
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
Comment 1 Nick Clemens 2018-10-18 16:02:44 UTC
This seems to be the changes on 19636 that doesn't account for waiting and transit holds when renumbering
Comment 2 Nick Clemens 2018-10-19 12:54:51 UTC Comment hidden (obsolete)
Comment 3 Nick Clemens 2018-10-25 11:47:56 UTC Comment hidden (obsolete)
Comment 4 Nick Clemens 2018-10-25 11:49:20 UTC
While this patch depends on 21611 we will need an 18.05 version which does not. Can provide once this one is through QA
Comment 5 Nick Clemens 2018-10-25 12:20:39 UTC
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
Comment 6 Martin Renvoize 2018-10-26 10:33:15 UTC
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>
Comment 7 Martin Renvoize 2018-10-26 10:33:49 UTC
Works as expected, Signing Off
Comment 8 Katrin Fischer 2018-10-27 23:48:06 UTC
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)
Comment 9 Katrin Fischer 2018-10-28 00:01:26 UTC
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.
Comment 10 Katrin Fischer 2018-10-28 00:16:59 UTC
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.
Comment 11 Nick Clemens 2018-10-29 17:48:22 UTC
(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.
Comment 12 Katrin Fischer 2018-10-29 18:49:42 UTC
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.
Comment 13 Nick Clemens 2018-10-30 19:11:07 UTC
(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
Comment 14 Jonathan Druart 2018-10-31 20:31:05 UTC
Tests missing.
Comment 15 Christopher Brannon 2018-11-01 17:57:31 UTC
*** Bug 21752 has been marked as a duplicate of this bug. ***
Comment 16 Nick Clemens 2018-11-01 18:56:44 UTC
Created attachment 81835 [details] [review]
Bug 21608: (follow-up) Use RevertWaitingStatus and do not alter _FixPriority
Comment 17 Marcel de Rooy 2018-11-02 11:16:38 UTC
Time too short, not trivial
Comment 18 Marcel de Rooy 2018-11-09 10:11:07 UTC
 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&amp;where=down&amp;first_priority=[% first_priority | html %]&amp;last_priority=[% last_priority | html %]&amp;prev_priority=0&amp;next_priority=1&amp;borrowernumber=[% hold.borrowernumber | html %]&amp;biblionumber=[% hold.biblionumber | html %]&amp;itemnumber=[% hold.itemnumber %]&amp;reserve_id=[% hold.reserve_id | html %]&amp;date=[% hold.date | html %]'">)
Comment 19 Marcel de Rooy 2018-11-09 10:11:39 UTC
QA: Having another try
Comment 20 Marcel de Rooy 2018-11-09 10:19:39 UTC
(In reply to Jonathan Druart from comment #14)
> Tests missing.

Definitely! Interventions in Reserves need tests.
Comment 21 Marcel de Rooy 2018-11-09 10:26:19 UTC
(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.
Comment 22 Marcel de Rooy 2018-11-09 11:13:52 UTC
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.
Comment 23 Marcel de Rooy 2018-11-09 11:17:35 UTC
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>
Comment 24 Marcel de Rooy 2018-11-09 11:21:42 UTC
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.
Comment 25 Marcel de Rooy 2018-11-09 11:27:59 UTC
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 !
Comment 26 Nick Clemens 2018-11-13 19:26:09 UTC
(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 !

:-)
Comment 27 Nick Clemens 2018-11-13 19:26:50 UTC
Created attachment 82322 [details] [review]
Bug 21608: Add ID and fix filters
Comment 28 Nick Clemens 2018-11-13 19:28:09 UTC
If no adjustments are made and 'Update holds' is clicked then any waiting holds are reverted
Comment 29 Nick Clemens 2018-11-26 18:22:30 UTC
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
Comment 30 Marcel de Rooy 2018-12-07 10:12:07 UTC
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>
Comment 31 Marcel de Rooy 2018-12-07 10:12:13 UTC
Created attachment 82957 [details] [review]
Bug 21608: Add ID and fix filters

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 32 Marcel de Rooy 2018-12-07 10:12:18 UTC
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>
Comment 33 Marcel de Rooy 2018-12-07 10:12:24 UTC
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>
Comment 34 Nick Clemens 2018-12-11 20:56:04 UTC
Awesome work all!

Pushed to master for 19.05
Comment 35 Martin Renvoize 2018-12-12 10:48:39 UTC
Pushed to 18.11.x for 18.11.01
Comment 36 Jesse Maseto 2018-12-18 19:12:43 UTC
Will not be backported to 18.05. Merge conflicts.
Comment 37 Nick Clemens 2018-12-28 17:02:11 UTC
Created attachment 83541 [details] [review]
Bug 21608: [18.05.X] disable dropdown and add revert button
Comment 38 Nick Clemens 2018-12-28 17:03:27 UTC
(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