Bug 19812

Summary: Holds count in "Already received" table has confusing and unexpected values
Product: Koha Reporter: Kyle M Hall <kyle>
Component: AcquisitionsAssignee: Kyle M Hall <kyle>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: minor    
Priority: P5 - low CC: 1joynelson, fridolin.somers, jonathan.druart, katrin.fischer, m.de.rooy, nick
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 23188    
Attachments: Bug 19812 - Holds count in "Already received" table has confusing and unexpected values
Bug 19812 - Holds count in "Already received" table has confusing and unexpected values
Bug 19812 - Holds count in "Already received" table has confusing and unexpected values
Bug 19812 [QA Followup] - Swap sides for total and item holds
Bug 19812 [QA Followup] - If 0 holds show '0' not '0 / 0'
Bug 19812 [QA Followup] - Remove unnecessary line

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