Bug 26935 - Incorrect basketid sent for claimacquisition and claimissues
Summary: Incorrect basketid sent for claimacquisition and claimissues
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on: 21898
Blocks: 28824
  Show dependency treegraph
 
Reported: 2020-11-05 08:11 UTC by Jonathan Druart
Modified: 2021-08-06 13:50 UTC (History)
2 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 26935: Don't use basket for claimacquisition and claimissues (2.89 KB, patch)
2020-11-05 08:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26935: Don't use basket for claimacquisition and claimissues (2.96 KB, patch)
2020-11-05 09:21 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26935: Don't use basket for claimacquisition and claimissues (3.01 KB, patch)
2020-11-05 14:22 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2020-11-05 08:11:11 UTC
The basket info are not used in the default template of these notices, but if it is used, a wrong basket may be passed to generate the notice.

This has been caught by a test failure after bug 26745.

#   Failed test 'SendAlerts - claimissue'
#   at t/db_dependent/Letters.t line 642.
C4::Letters::_substitute_tables(): DBI Exception: DBD::mysql::st execute failed: called with 2 bind variables when 1 are needed [for Statement "SELECT * FROM aqbasket WHERE       basketno = ?" with ParamValues: 0=53] at /kohadevbox/koha/C4/Letters.pm line 679
Comment 1 Jonathan Druart 2020-11-05 08:15:33 UTC
Created attachment 113033 [details] [review]
Bug 26935: Don't use basket for claimacquisition and claimissues

Incorrect basketid is sent for claimacquisition and claimissues.

The basket info are not used in the default template of these notices, but if it
is used, a wrong basket may be passed to generate the notice.

This has been caught by a test failure after bug 26745.

 #   Failed test 'SendAlerts - claimissue'
 #   at t/db_dependent/Letters.t line 642.
 C4::Letters::_substitute_tables(): DBI Exception: DBD::mysql::st execute failed: called with 2 bind variables when 1 are needed [for Statement "SELECT * FROM aqbasket WHERE       basketno = ?" with ParamValues: 0=53] at /kohadevbox/koha/C4/Letters.pm line 679

This is a quick patch to fix the failing test, bug more work would needed.

We are removing the basket info from those 2 notices and keeping it only
for orderacquisition.

Technically: the $externalid is coming from the parameter list of
SendAlerts, but it is a basketno only for orderacquisition. We must not
pass it for other notices.

Test plan:
Confirm that the failing test is now passing and that the changes make
sense.
Comment 2 Jonathan Druart 2020-11-05 08:25:06 UTC
This patch has a high priority, Jenkins is not happy without it.
Comment 3 Martin Renvoize 2020-11-05 09:21:51 UTC
Created attachment 113047 [details] [review]
Bug 26935: Don't use basket for claimacquisition and claimissues

Incorrect basketid is sent for claimacquisition and claimissues.

The basket info are not used in the default template of these notices, but if it
is used, a wrong basket may be passed to generate the notice.

This has been caught by a test failure after bug 26745.

 #   Failed test 'SendAlerts - claimissue'
 #   at t/db_dependent/Letters.t line 642.
 C4::Letters::_substitute_tables(): DBI Exception: DBD::mysql::st execute failed: called with 2 bind variables when 1 are needed [for Statement "SELECT * FROM aqbasket WHERE       basketno = ?" with ParamValues: 0=53] at /kohadevbox/koha/C4/Letters.pm line 679

This is a quick patch to fix the failing test, bug more work would needed.

We are removing the basket info from those 2 notices and keeping it only
for orderacquisition.

Technically: the $externalid is coming from the parameter list of
SendAlerts, but it is a basketno only for orderacquisition. We must not
pass it for other notices.

Test plan:
Confirm that the failing test is now passing and that the changes make
sense.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 4 Martin Renvoize 2020-11-05 09:22:48 UTC
Yikes, that sub is kinda horrible.. but that's cleanup for another day.

I agree with your changes here and the tests now pass consistently.

Signing off
Comment 5 Tomás Cohen Arazi 2020-11-05 14:22:13 UTC
Created attachment 113118 [details] [review]
Bug 26935: Don't use basket for claimacquisition and claimissues

Incorrect basketid is sent for claimacquisition and claimissues.

The basket info are not used in the default template of these notices, but if it
is used, a wrong basket may be passed to generate the notice.

This has been caught by a test failure after bug 26745.

 #   Failed test 'SendAlerts - claimissue'
 #   at t/db_dependent/Letters.t line 642.
 C4::Letters::_substitute_tables(): DBI Exception: DBD::mysql::st execute failed: called with 2 bind variables when 1 are needed [for Statement "SELECT * FROM aqbasket WHERE       basketno = ?" with ParamValues: 0=53] at /kohadevbox/koha/C4/Letters.pm line 679

This is a quick patch to fix the failing test, but more work would needed.

We are removing the basket info from those 2 notices and keeping it only
for orderacquisition.

Technically: the $externalid is coming from the parameter list of
SendAlerts, but it is a basketno only for orderacquisition. We must not
pass it for other notices.

Test plan:
Confirm that the failing test is now passing and that the changes make
sense.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 6 Jonathan Druart 2020-11-05 14:25:48 UTC
Pushed to master for 20.11, thanks to everybody involved!