Bug 19812 - Holds count in "Already received" table has confusing and unexpected values
Summary: Holds count in "Already received" table has confusing and unexpected values
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Kyle M Hall
QA Contact: Marcel de Rooy
URL:
Keywords:
: 19741 19846 (view as bug list)
Depends on:
Blocks: 23188
  Show dependency treegraph
 
Reported: 2017-12-14 12:03 UTC by Kyle M Hall
Modified: 2019-06-29 15:29 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 19812 - Holds count in "Already received" table has confusing and unexpected values (3.24 KB, patch)
2017-12-14 12:10 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19812 - Holds count in "Already received" table has confusing and unexpected values (3.35 KB, patch)
2017-12-15 18:52 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19812 - Holds count in "Already received" table has confusing and unexpected values (3.45 KB, patch)
2017-12-22 11:03 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19812 [QA Followup] - Swap sides for total and item holds (2.05 KB, patch)
2018-04-04 10:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19812 [QA Followup] - If 0 holds show '0' not '0 / 0' (1.62 KB, patch)
2018-04-04 11:03 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19812 [QA Followup] - Remove unnecessary line (783 bytes, patch)
2018-04-04 11:03 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2017-12-14 12:03:18 UTC
In acqui/parcel.pl both the "Pending orders" and "Already received" tables show how many holds there are for the given record. However, the count of holds in the "Pending orders" table confuses librarians because it only lists holds for the particular items in the orderline. Due to that, the holds column may show 0 holds even if there are a dozen record level holds for that bib! This is not what librarians seem to expect, instead it seems that the same total holds in the "Pending orders" table would be preferred.
Comment 1 Kyle M Hall 2017-12-14 12:10:45 UTC
Created attachment 69799 [details] [review]
Bug 19812 - Holds count in "Already received" table has confusing and unexpected values

In acqui/parcel.pl both the "Pending orders" and "Already received" tables show how many holds there are for the given record. However, the count of holds in the "Pending orders" table confuses librarians because it only lists holds for the particular items in the orderline. Due to that, the holds column may show 0 holds even if there are a dozen record level holds for that bib! This is not what librarians seem to expect, instead it seems that the same total holds in the "Pending orders" table would be preferred.

Test Plan:
1) Find an invoice with an item in the "Already received" table
2) Add one or more record level holds to the record
3) Note the holds column does not count those holds
4) Apply this patch
5) Note the holds column now shows total holds and holds for just those ordered items
Comment 2 Nick Clemens 2017-12-15 17:19:07 UTC
*** Bug 19741 has been marked as a duplicate of this bug. ***
Comment 3 Kyle M Hall 2017-12-15 18:52:06 UTC
Created attachment 69829 [details] [review]
Bug 19812 - Holds count in "Already received" table has confusing and unexpected values

In acqui/parcel.pl both the "Pending orders" and "Already received" tables show how many holds there are for the given record. However, the count of holds in the "Pending orders" table confuses librarians because it only lists holds for the particular items in the orderline. Due to that, the holds column may show 0 holds even if there are a dozen record level holds for that bib! This is not what librarians seem to expect, instead it seems that the same total holds in the "Pending orders" table would be preferred.

Test Plan:
1) Find an invoice with an item in the "Already received" table
2) Add one or more record level holds to the record
3) Note the holds column does not count those holds
4) Apply this patch
5) Note the holds column now shows total holds and holds for just those ordered items

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Nancy Keener <nkeener@washoecounty.us>
Comment 4 Barton Chittenden 2017-12-19 21:55:11 UTC
*** Bug 19846 has been marked as a duplicate of this bug. ***
Comment 5 Marcel de Rooy 2017-12-22 11:03:53 UTC
Created attachment 70115 [details] [review]
Bug 19812 - Holds count in "Already received" table has confusing and unexpected values

In acqui/parcel.pl both the "Pending orders" and "Already received" tables show how many holds there are for the given record. However, the count of holds in the "Pending orders" table confuses librarians because it only lists holds for the particular items in the orderline. Due to that, the holds column may show 0 holds even if there are a dozen record level holds for that bib! This is not what librarians seem to expect, instead it seems that the same total holds in the "Pending orders" table would be preferred.

Test Plan:
1) Find an invoice with an item in the "Already received" table
2) Add one or more record level holds to the record
3) Note the holds column does not count those holds
4) Apply this patch
5) Note the holds column now shows total holds and holds for just those ordered items

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Nancy Keener <nkeener@washoecounty.us>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 6 Jonathan Druart 2017-12-22 14:21:36 UTC
If there are no holds, should not we display "0" instead of "0 / 0"?
Comment 7 Jonathan Druart 2017-12-22 16:03:01 UTC
"Holds count in "Already received" table has confusing and unexpected values"
Does not describe what the patch does.
Comment 8 Jonathan Druart 2017-12-22 16:08:48 UTC
There is an access to the removed variable "holds" in the template loop_order.holds

That means we now see "Can't delete catalog record, see constraints below", but without anything below...
Comment 9 Kyle M Hall 2018-03-23 11:50:13 UTC
(In reply to Jonathan Druart from comment #8)
> There is an access to the removed variable "holds" in the template
> loop_order.holds
> 
> That means we now see "Can't delete catalog record, see constraints below",
> but without anything below...

The code you are referencing is in loop_orders, but this patch only alters loop_received which does not have that message. I'm not sure how this patch could be causing that. If you can give me some line numbers I'd be happy to take another look!
Comment 10 Jonathan Druart 2018-04-02 20:11:32 UTC
(In reply to Kyle M Hall from comment #9)
> (In reply to Jonathan Druart from comment #8)
> > There is an access to the removed variable "holds" in the template
> > loop_order.holds
> > 
> > That means we now see "Can't delete catalog record, see constraints below",
> > but without anything below...
> 
> The code you are referencing is in loop_orders, but this patch only alters
> loop_received which does not have that message. I'm not sure how this patch
> could be causing that. If you can give me some line numbers I'd be happy to
> take another look!

Indeed, sounds like I mixed the 2 tables reading the code.

Something else:
1. What about comment 6?

2. "1 / 0" reads wrong, but I have no idea how it could be better.

3. line 141     $line{holds} = 0;
should be removed, right?
Comment 11 Katrin Fischer 2018-04-02 21:15:17 UTC
I've tested it and I think it's still a little confusing.

Right now it is: total of holds / holds on on order items

Which can lead to the 1 / 0 that Jonathan pointed out. Maybe it would make more sense to switch the values? Or just show the total number of holds = record level + item level on on order items?
Comment 12 Kyle M Hall 2018-04-04 10:54:54 UTC
(In reply to Katrin Fischer from comment #11)
> I've tested it and I think it's still a little confusing.
> 
> Right now it is: total of holds / holds on on order items
> 
> Which can lead to the 1 / 0 that Jonathan pointed out. Maybe it would make
> more sense to switch the values? Or just show the total number of holds =
> record level + item level on on order items?

I like the idea of switching the values around. I'll get a followup done asap!
Comment 13 Kyle M Hall 2018-04-04 10:56:56 UTC
Created attachment 73606 [details] [review]
Bug 19812 [QA Followup] - Swap sides for total and item holds
Comment 14 Kyle M Hall 2018-04-04 11:03:06 UTC
Created attachment 73607 [details] [review]
Bug 19812 [QA Followup] - If 0 holds show '0' not '0 / 0'
Comment 15 Kyle M Hall 2018-04-04 11:03:13 UTC
Created attachment 73608 [details] [review]
Bug 19812 [QA Followup] - Remove unnecessary line
Comment 16 Jonathan Druart 2018-04-04 19:04:27 UTC
Patches will be squashed for readability.
Comment 17 Jonathan Druart 2018-04-04 19:07:48 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 18 Nick Clemens 2018-05-14 21:14:46 UTC
Pushed to stable for 17.11.06

Awesome work all!
Comment 19 Fridolin Somers 2018-05-22 10:45:46 UTC
I prefer not to push to 17.05.x, its minor