Bug 27012 - Merging records with holds causes SQL error
Summary: Merging records with holds causes SQL error
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Joonas Kylmälä
QA Contact: Josef Moravec
URL:
Keywords:
Depends on: 12556
Blocks:
  Show dependency treegraph
 
Reported: 2020-11-13 10:30 UTC by Joonas Kylmälä
Modified: 2021-12-13 21:09 UTC (History)
5 users (show)

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


Attachments
Bug 27012: Fix incorrect SQL syntax in hold merging (2.00 KB, patch)
2020-11-13 10:36 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 27012: Add test for C4::Reserves::MergeHolds (1.99 KB, patch)
2020-11-13 11:51 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 27012: Fix incorrect SQL syntax in hold merging (1.77 KB, patch)
2020-11-13 11:51 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 27012: Add test for C4::Reserves::MergeHolds (2.04 KB, patch)
2020-11-13 20:19 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 27012: Fix incorrect SQL syntax in hold merging (1.83 KB, patch)
2020-11-13 20:19 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 27012: Add test for C4::Reserves::MergeHolds (2.10 KB, patch)
2020-11-16 09:34 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 27012: Fix incorrect SQL syntax in hold merging (1.88 KB, patch)
2020-11-16 09:34 UTC, Josef Moravec
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Joonas Kylmälä 2020-11-13 10:30:28 UTC
If one merges records A and B and A has holds in it then we get following error in /var/log/koha/kohadev/plack-intranet-error.log:

> [WARN] DBD::mysql::st execute failed: called with 4 bind variables when 3 are needed [for Statement "SELECT * FROM reserves WHERE biblionumber = ? AND (found <> ? AND found <> ? OR found is NULL) ORDER BY reservedate ASC" with ParamValues: 0=Null!, 1=Null!, 2=Null!] at /kohadevbox/koha/C4/Reserves.pm line 2002.
Comment 1 Joonas Kylmälä 2020-11-13 10:36:13 UTC
Created attachment 113592 [details] [review]
Bug 27012: Fix incorrect SQL syntax in hold merging

If you merge two records with holds in them following error happens
without this patch:

[WARN] DBD::mysql::st execute failed: called with 4 bind variables when 3 are needed [for Statement "SELECT * FROM reserves WHERE biblionumber = ? AND (found <> ? AND found <> ? OR found is NULL) ORDER BY reservedate ASC" with ParamValues: 0=Null!, 1=Null!, 2=Null!] at /kohadevbox/koha/C4/Reserves.pm line 2002.

To test:
 1) Create a hold in record A
 2) Use cataloging search to search for something that brings up
    record A and another record B.
 3) Select A and B records in the search results and click Edit ->
    Merge records
 4) Merge the records and notice that the above error is printed to
    plack-intranet-error.log
 5) Apply patch and repeat steps 1-4 but notice the error is gone
Comment 2 Jonathan Druart 2020-11-13 11:22:47 UTC
Can you provide a test please?
Comment 3 Joonas Kylmälä 2020-11-13 11:51:29 UTC
Created attachment 113593 [details] [review]
Bug 27012: Add test for C4::Reserves::MergeHolds

This does a basic check whether a hold gets moved to another biblio
when MergeHolds() is called.
Comment 4 Joonas Kylmälä 2020-11-13 11:51:32 UTC
Created attachment 113594 [details] [review]
Bug 27012: Fix incorrect SQL syntax in hold merging

If you merge two records with holds in them following error happens
without this patch:

[WARN] DBD::mysql::st execute failed: called with 4 bind variables when 3 are needed [for Statement "SELECT * FROM reserves WHERE biblionumber = ? AND (found <> ? AND found <> ? OR found is NULL) ORDER BY reservedate ASC" with ParamValues: 0=Null!, 1=Null!, 2=Null!] at /kohadevbox/koha/C4/Reserves.pm line 2002.

To test:
   1) Notice prove t/db_dependent/Reserves.t fails with above error
   2) Apply patch
   3) Notice prove t/db_dependent/Reserves.t passes
Comment 5 Joonas Kylmälä 2020-11-13 11:59:40 UTC
(In reply to Jonathan Druart from comment #2)
> Can you provide a test please?

I provided but I'm still against having those for old untested subroutines because those are mostly very long and hard to provide tests for due to having multiple functions combined in them and will have to go another round of changes when moving to Koha namespace. Also I think it as a separate matter that can be handled at another time and does not relate to the bug itself. This would allow the testers and QA team to work on smaller tasks therefore allowing higher chance of finding the time to review the code and pay more attention to every detail in the patch compared to long patch set with multiple goals (in this case: adding tests, fixing bug).
Comment 6 Victor Grousset/tuxayo 2020-11-13 20:19:22 UTC
Created attachment 113632 [details] [review]
Bug 27012: Add test for C4::Reserves::MergeHolds

This does a basic check whether a hold gets moved to another biblio
when MergeHolds() is called.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 7 Victor Grousset/tuxayo 2020-11-13 20:19:24 UTC
Created attachment 113633 [details] [review]
Bug 27012: Fix incorrect SQL syntax in hold merging

If you merge two records with holds in them following error happens
without this patch:

[WARN] DBD::mysql::st execute failed: called with 4 bind variables when 3 are needed [for Statement "SELECT * FROM reserves WHERE biblionumber = ? AND (found <> ? AND found <> ? OR found is NULL) ORDER BY reservedate ASC" with ParamValues: 0=Null!, 1=Null!, 2=Null!] at /kohadevbox/koha/C4/Reserves.pm line 2002.

To test:
   1) Notice prove t/db_dependent/Reserves.t fails with above error
   2) Apply patch
   3) Notice prove t/db_dependent/Reserves.t passes

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 8 Victor Grousset/tuxayo 2020-11-13 20:19:58 UTC
Manual test and automated test ok! :)
Comment 9 Josef Moravec 2020-11-16 09:34:53 UTC
Created attachment 113651 [details] [review]
Bug 27012: Add test for C4::Reserves::MergeHolds

This does a basic check whether a hold gets moved to another biblio
when MergeHolds() is called.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 10 Josef Moravec 2020-11-16 09:34:58 UTC
Created attachment 113652 [details] [review]
Bug 27012: Fix incorrect SQL syntax in hold merging

If you merge two records with holds in them following error happens
without this patch:

[WARN] DBD::mysql::st execute failed: called with 4 bind variables when 3 are needed [for Statement "SELECT * FROM reserves WHERE biblionumber = ? AND (found <> ? AND found <> ? OR found is NULL) ORDER BY reservedate ASC" with ParamValues: 0=Null!, 1=Null!, 2=Null!] at /kohadevbox/koha/C4/Reserves.pm line 2002.

To test:
   1) Notice prove t/db_dependent/Reserves.t fails with above error
   2) Apply patch
   3) Notice prove t/db_dependent/Reserves.t passes

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 11 Jonathan Druart 2020-11-16 12:49:57 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 12 Andrew Fuerste-Henry 2020-12-08 17:35:41 UTC
Missing dependencies, no backport for 20.05.x