Bug 13022 - Hardcoded limit causes records with more than 20 items to show inaccurate statuses
Summary: Hardcoded limit causes records with more than 20 items to show inaccurate sta...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 15929
  Show dependency treegraph
 
Reported: 2014-10-02 13:51 UTC by Kyle M Hall
Modified: 2017-12-07 22:16 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
If a record has more than 20 items, all the items over 20 will show as available on results even if they are not! This is a hard coded limit in the Search module. This is made configurable with the new system preference MaxSearchResultsItemsPerRecordStatu
Version(s) released in:


Attachments
Bug 13022 - Hardcoded limit causes records with more than 20 items to show inaccurate statuses (4.42 KB, patch)
2014-10-02 13:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13022 - Hardcoded limit causes records with more than 20 items to show inaccurate statuses (4.17 KB, patch)
2015-04-16 15:48 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13022 - Hardcoded limit causes records with more than 20 items to show inaccurate statuses (4.32 KB, patch)
2015-04-16 15:52 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13022 - Hardcoded limit causes records with more than 20 items to show inaccurate statuses (4.54 KB, patch)
2015-04-17 13:36 UTC, Andreas Hedström Mace
Details | Diff | Splinter Review
Bug 13022 [QA Followup] - Allow the pref to be unset for unlimited item status checks (2.13 KB, patch)
2015-04-17 13:51 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13022 [QA Followup] - Allow the pref to be unset for unlimited item status checks (2.34 KB, patch)
2015-04-17 14:08 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13022 [QA Followup] - Highlight the results with inaccurate status, add explanation when mouse hovers over. (4.04 KB, patch)
2015-04-17 14:42 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13022 [QA Followup] - Highlight the results with inaccurate status, add explanation when mouse hovers over. (4.00 KB, patch)
2015-04-17 14:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13022 [QA Followup] - Revert changes in whitespace back to original for items count lines (2.17 KB, patch)
2015-04-23 16:52 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13022 [QA Followup] - Revert changes in whitespace back to original for items count lines (2.21 KB, patch)
2015-04-23 16:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13022 - Hardcoded limit causes records with more than 20 items to show inaccurate statuses (4.60 KB, patch)
2015-04-24 08:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13022 [QA Followup] - Allow the pref to be unset for unlimited item status checks (2.43 KB, patch)
2015-04-24 08:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13022 [QA Followup] - Highlight the results with inaccurate status, add explanation when mouse hovers over. (3.69 KB, patch)
2015-04-24 08:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13022 - Hardcoded limit causes records with more than 20 items to show inaccurate statuses (4.59 KB, patch)
2015-12-23 18:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13022 [QA Followup] - Allow the pref to be unset for unlimited item status checks (2.43 KB, patch)
2015-12-23 19:00 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13022 [QA Followup] - Highlight the results with inaccurate status, add explanation when mouse hovers over. (3.69 KB, patch)
2015-12-23 19:00 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 2014-10-02 13:51:47 UTC
If a record has more than 20 items, all the items over 20 will show as available on the search results even if they are not!

This is a hard coded limit in the Search module. This number should be configurable.
Comment 1 Kyle M Hall 2014-10-02 13:59:48 UTC Comment hidden (obsolete)
Comment 2 Owen Leonard 2014-10-10 16:27:47 UTC
This is an interesting bug... I had no idea this arbitrary limitation was in the code. Note the associated FIXME:

    # A couple heuristics to limit how many times
    # we query the database for item transfer information, sacrificing
    # accuracy in some cases for speed;
    #
    # 1. don't query if item has one of the other statuses
    # 2. don't check transit status if the bib has
    #    more than 20 items
    #
    # FIXME: to avoid having the query the database like this, and to make
    #        the in transit status count as unavailable for search limiting,
    #        should map transit status to record indexed in Zebra.

The problem with both the old way and the patched version of things is that something is happening which the user doesn't know anything about. You can pick an arbitrarily high number for MaxSearchResultsItemsPerRecordStatusCheck (that's a mouthful!) and rest assured that no inaccurate information will be displayed or you can set it to something else and take your chances.

How does the librarian administrator know what value of MaxSearchResultsItemsPerRecordStatusCheck is reasonable?

I'm interested in hearing why the solution proposed in the FIXME isn't the one which has been implemented.

If the Zebra solution is not a good one, I think a better solution to this might just be to add a note which tells the user to click through to the detail page for accurate status information.
Comment 3 Kyle M Hall 2015-04-16 15:43:46 UTC
(In reply to Owen Leonard from comment #2)
> This is an interesting bug... I had no idea this arbitrary limitation was in
> the code. Note the associated FIXME:
> 
>     # A couple heuristics to limit how many times
>     # we query the database for item transfer information, sacrificing
>     # accuracy in some cases for speed;
>     #
>     # 1. don't query if item has one of the other statuses
>     # 2. don't check transit status if the bib has
>     #    more than 20 items
>     #
>     # FIXME: to avoid having the query the database like this, and to make
>     #        the in transit status count as unavailable for search limiting,
>     #        should map transit status to record indexed in Zebra.
> 
> The problem with both the old way and the patched version of things is that
> something is happening which the user doesn't know anything about. You can
> pick an arbitrarily high number for
> MaxSearchResultsItemsPerRecordStatusCheck (that's a mouthful!) and rest
> assured that no inaccurate information will be displayed or you can set it
> to something else and take your chances.
> 
> How does the librarian administrator know what value of
> MaxSearchResultsItemsPerRecordStatusCheck is reasonable?
> 
> I'm interested in hearing why the solution proposed in the FIXME isn't the
> one which has been implemented.
> 
> If the Zebra solution is not a good one, I think a better solution to this
> might just be to add a note which tells the user to click through to the
> detail page for accurate status information.

I don't think that would be a satisfactory solution for many librarians. The zebra fix is also limited. This problem isn't just inherent in the transit status, but also the lost status ( and probably the not for loan status and who knows what else ). I think the most correct solution would be to remove the limit altogether, but in the interest of those libraries that would prefer speed over accuracy, I made the limit controllable via a system preference instead.
Comment 4 Kyle M Hall 2015-04-16 15:48:45 UTC Comment hidden (obsolete)
Comment 5 Kyle M Hall 2015-04-16 15:52:27 UTC Comment hidden (obsolete)
Comment 6 Andreas Hedström Mace 2015-04-17 13:36:30 UTC Comment hidden (obsolete)
Comment 7 Kyle M Hall 2015-04-17 13:51:52 UTC Comment hidden (obsolete)
Comment 8 Kyle M Hall 2015-04-17 14:08:40 UTC Comment hidden (obsolete)
Comment 9 Kyle M Hall 2015-04-17 14:42:37 UTC Comment hidden (obsolete)
Comment 10 Kyle M Hall 2015-04-17 14:44:30 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2015-04-20 13:36:18 UTC
I tried with and without the patch, I modified the pref value, but always got the same display.

Could I get a more complete test plan?
Comment 12 Jonathan Druart 2015-04-20 13:38:23 UTC
I think that the strings introduced in the last patch won't be easily/correctly translatable.

+  "48 items , 44 available" => There is a space before the comma.
Comment 13 Kyle M Hall 2015-04-23 16:52:02 UTC Comment hidden (obsolete)
Comment 14 Kyle M Hall 2015-04-23 16:53:35 UTC Comment hidden (obsolete)
Comment 15 Kyle M Hall 2015-04-23 17:33:50 UTC
(In reply to Jonathan Druart from comment #11)
> I tried with and without the patch, I modified the pref value, but always
> got the same display.
> 
> Could I get a more complete test plan?

Here's a screencast demo: http://screencast.com/t/dXO8k9MCpaA

This is with the patch set applied. I made it simple. The record in question has two items, the second of which is on hold. With any number greater then 2 in the syspref, the item statuses are accurate. But, if you drop the system preference down to 1, it only checks the status of the first item, and thus you get inaccurate results on the search results page.
Comment 16 Jonathan Druart 2015-04-24 08:32:29 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2015-04-24 08:32:45 UTC Comment hidden (obsolete)
Comment 18 Jonathan Druart 2015-04-24 08:32:51 UTC Comment hidden (obsolete)
Comment 19 Jonathan Druart 2015-04-24 08:34:42 UTC
I have tried with another record and it works now.
I have squashed the last 2 patches for more readability.

Marked as Passed QA.
Comment 20 Tomás Cohen Arazi 2015-07-20 14:36:09 UTC
I think this should be solved on indexing, no calculation on what is the status of the records/items should be done on rendering the results page.

If you take a look at C4::Circulation::{AddIssue|AddReturn} you will notice that they call C4::Items::ModItem, which ultimately calls ModZebra...

So we should always assume the data we retrieve from Zebra is current... and no calculation should be done outside the resultset...

I belive at some point we messed up things A LOT (by adding the <items> thing in the XSLT step... but until someone rewrites all that... I think we should do the search engine-way.
Comment 21 Kyle M Hall 2015-12-23 17:17:41 UTC
(In reply to Tomás Cohen Arazi from comment #20)
> I think this should be solved on indexing, no calculation on what is the
> status of the records/items should be done on rendering the results page.
> 
> If you take a look at C4::Circulation::{AddIssue|AddReturn} you will notice
> that they call C4::Items::ModItem, which ultimately calls ModZebra...
> 
> So we should always assume the data we retrieve from Zebra is current... and
> no calculation should be done outside the resultset...
> 
> I belive at some point we messed up things A LOT (by adding the <items>
> thing in the XSLT step... but until someone rewrites all that... I think we
> should do the search engine-way.

So what steps do we need to take to resolve this? It sounds like we need to create an availability index. This would also mean we need to re-index whenever other tables related to items change ( transfers and reserves at the least ).

I think that this patch could be pushed in the meantime as a band-aid at least, until we can get such a system in place.
Comment 22 Kyle M Hall 2015-12-23 18:59:50 UTC
Created attachment 45957 [details] [review]
Bug 13022 - Hardcoded limit causes records with more than 20 items to show inaccurate statuses

If a record has more than 20 items, all the items over 20 will show as
available on the search results even if they are not!

This is a hard coded limit in the Search module. This number should be
configurable.

Test Plan:
1) Create a record with more than 20 items
2) Set all the items to waiting holds or in transit
3) Search for results that will include that item
4) Note some say they are available even though they are not
5) Apply this patch
6) Run updatedatabase.pl
7) Set the new system preference MaxSearchResultsItemsPerRecordStatusCheck
   to a number larger than the number of items on your record
8) Re-run the search
9) Note that the hold and transit statuses for the items are now correct

Signed-off-by: Andreas Hedström Mace <andreas.hedstrom.mace@sub.su.se>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 23 Kyle M Hall 2015-12-23 19:00:04 UTC
Created attachment 45958 [details] [review]
Bug 13022 [QA Followup] - Allow the pref to be unset for unlimited item status checks

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 24 Kyle M Hall 2015-12-23 19:00:11 UTC
Created attachment 45959 [details] [review]
Bug 13022 [QA Followup] - Highlight the results with inaccurate status, add explanation when mouse hovers over.

Bug 13022 [QA Followup] - Revert changes in whitespace back to original for items count lines

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 25 Jonathan Druart 2015-12-26 11:13:52 UTC
(In reply to Kyle M Hall from comment #21)
> (In reply to Tomás Cohen Arazi from comment #20)
> > I think this should be solved on indexing, no calculation on what is the
> > status of the records/items should be done on rendering the results page.
> > 
> > If you take a look at C4::Circulation::{AddIssue|AddReturn} you will notice
> > that they call C4::Items::ModItem, which ultimately calls ModZebra...
> > 
> > So we should always assume the data we retrieve from Zebra is current... and
> > no calculation should be done outside the resultset...
> > 
> > I belive at some point we messed up things A LOT (by adding the <items>
> > thing in the XSLT step... but until someone rewrites all that... I think we
> > should do the search engine-way.
> 
> So what steps do we need to take to resolve this? It sounds like we need to
> create an availability index. This would also mean we need to re-index
> whenever other tables related to items change ( transfers and reserves at
> the least ).
> 
> I think that this patch could be pushed in the meantime as a band-aid at
> least, until we can get such a system in place.

I would like to see a comment from Tomas before these patches are pushed.
Comment 26 Kyle M Hall 2015-12-30 16:18:09 UTC
> I would like to see a comment from Tomas before these patches are pushed.

I should have noted Tomas gave his blessing to have this pushed to master in a pm. I think we can all agree that the best long term solution is to index these values instead of calculating them for each search, but this is at least a decent stop-gap measure until such changes can be made.

I will ask Tomas to confirm our conversation here before pushing this.
Comment 27 Brendan Gallagher 2015-12-31 16:42:09 UTC
(In reply to Kyle M Hall from comment #26)
> > I would like to see a comment from Tomas before these patches are pushed.
> 
> I should have noted Tomas gave his blessing to have this pushed to master in
> a pm. I think we can all agree that the best long term solution is to index
> these values instead of calculating them for each search, but this is at
> least a decent stop-gap measure until such changes can be made.
> 
> I will ask Tomas to confirm our conversation here before pushing this.

I am ok with this as a stop-gap.  I will push this. But I am really interested to see if we can get this right before the next release is out the door.
Comment 28 Brendan Gallagher 2015-12-31 18:36:39 UTC
Pushed to master - Thanks
Comment 29 Julian Maurice 2016-01-04 07:42:01 UTC
Patches pushed to 3.22.x, will be in 3.22.2
Comment 30 Frédéric Demians 2016-01-04 15:11:03 UTC
This patch has been pushed to 3.20.x, will be in 3.20.8.
Comment 31 Jonathan Druart 2016-06-14 10:33:17 UTC
@RM: the DB entry 3.23.00.012 refers to "Bug 15380 - Move the authority types related code to Koha::Authority::Type[s] - part 1", please correct.