Bug 24488 - Holds to Pull sometimes shows the wrong 'first patron' details
Summary: Holds to Pull sometimes shows the wrong 'first patron' details
Status: ASSIGNED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Aleisha Amohia
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-01-23 03:58 UTC by Aleisha Amohia
Modified: 2020-02-16 20:07 UTC (History)
6 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 24488: Show correct first patron details on Holds to pull (19.79 KB, patch)
2020-01-23 04:03 UTC, Aleisha Amohia
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Aleisha Amohia 2020-01-23 03:58:29 UTC
The query that pulls the information for the Circ -> Holds to pull table is incredibly complex and doesn't always pull the correct information. It has a bunch of commands in it that repeatedly reorders the results (group_concats and counts). This means the order of the results cannot be guaranteed to be the same with every execution, and sometimes we see the wrong 'first patron' for the given date selection.

It's hard to test because it only happens sometimes, but I have been able to reproduce it this way:
- Place two holds for two different patrons on the same biblio
- Check the Holds to pull, it'll show the details of the first patron
- Change the reservedate for the second hold in the database to be yesterday
- Check the Holds to pull, it shows the details of the wrong hold under 'first patron' but shows the correct details 'under earliest hold date' 
- Change the date selection, it doesn't fix itself
Comment 1 Aleisha Amohia 2020-01-23 04:03:01 UTC
Created attachment 97773 [details] [review]
Bug 24488: Show correct first patron details on Holds to pull

This patch rewrites the complex search query to reduce the risk of
reordering the results multiple times.

This patch includes a change to schema files so may need to upgrade
schema and/or restart memcached

To test:
1) Reproduce problem following test plan in Description
2) Apply patch and refresh page
3) Notice the correct patron is now shown
4) Play with date selection, confirm correct results are still shown
5) Test cancelling holds
6) Test filtering table results

Sponsored by: IHC New Zealand
Comment 2 Kelly McElligott 2020-01-24 14:34:25 UTC
Aleisha- something odd- nothing is showing in the Holds To Pull report.  I have tried this multiple times and unable to get the holds to populate. Both biblio and Item level holds.
Comment 3 Aleisha Amohia 2020-01-26 20:42:37 UTC
(In reply to Kelly McElligott from comment #2)
> Aleisha- something odd- nothing is showing in the Holds To Pull report.  I
> have tried this multiple times and unable to get the holds to populate. Both
> biblio and Item level holds.

Hi Kelly, is this before or after you apply the patch? If after, did you try upgrading the schema files and restarting memcached?
Comment 4 Kelly McElligott 2020-01-27 15:20:12 UTC
Hi Aleisha,
I have done what you have recommended.  I am now getting an error when I try to go to the Holds to Pull report 

DBIx::Class::Storage::DBI::_dbh_execute(): 'koha_hold.reserve.reserve_id' isn't in GROUP BY at /kohadevbox/koha/Koha/Objects.pm line 130
 at /usr/share/perl5/DBIx/Class/Exception.pm line 77
Comment 5 Aleisha Amohia 2020-01-27 22:06:34 UTC
(In reply to Kelly McElligott from comment #4)
> Hi Aleisha,
> I have done what you have recommended.  I am now getting an error when I try
> to go to the Holds to Pull report 
> 
> DBIx::Class::Storage::DBI::_dbh_execute(): 'koha_hold.reserve.reserve_id'
> isn't in GROUP BY at /kohadevbox/koha/Koha/Objects.pm line 130
>  at /usr/share/perl5/DBIx/Class/Exception.pm line 77

Hi Kelly, I can't reproduce this error. There might be a problem in your database? Sorry!
Comment 6 Katrin Fischer 2020-01-27 22:34:21 UTC
Hi Aleisha, I think this is related to bug 21941/bug 17258 - a DBMS version problem. Can you please check?
Comment 7 Aleisha Amohia 2020-02-13 01:03:44 UTC
(In reply to Katrin Fischer from comment #6)
> Hi Aleisha, I think this is related to bug 21941/bug 17258 - a DBMS version
> problem. Can you please check?

Hi Katrin, sorry I'm not really sure how I'm meant to check? It could be related, but also that query is pretty horrible and I think this fix is nicer code to work with in the long term.

Setting back to Needs signoff because I'm not getting the aforementioned error, and wonder if it happened when the reservedate was manually changed in the database? Regardless, patch still works for me.
Comment 8 Katrin Fischer 2020-02-14 15:11:08 UTC
> DBIx::Class::Storage::DBI::_dbh_execute(): 'koha_hold.reserve.reserve_id'
> isn't in GROUP BY at /kohadevbox/koha/Koha/Objects.pm line 130
>  at /usr/share/perl5/DBIx/Class/Exception.pm line 77

Issues with GROUP BY usually point to problems with stricter DB settings - that's what I meant. It's possible the tester had a different DBMS version / different settings which made it explode - trying to help because you said you cannot reproduce.
Comment 9 Martin Renvoize 2020-02-14 15:21:51 UTC
To summarise.. try setting `<strict_sql_modes>1</strict_sql_modes>` in your koha-conf.xml Aleisha.. it's on by default in sandboxes and dev environments to weed out queries that would fail we strict features enabled at the SQL level.

Looking at the error Kelly is reporting it sounds like strict SQL is enabled and she's catching an error in your reworked SQL query that you're not seeing with sql in 'forgiving' mode ;).

I've not looked at the code yet, but this usually arrises from a group by on a query with a join where you're trying to group by an id field in on table.. the fix, as horrible as it is, is to add all fields in that table to the group by.
Comment 10 Aleisha Amohia 2020-02-16 20:07:50 UTC
I've misunderstood here - I thought that Katrin's comment was talking about the potential cause of the original bug reported, not in relation to reproducing Kelly's error.

Also I'm more than happy to write tests. I had simply forgotten in the first instance.

Will take another look when I get a chance.