Bug 21620 - Errors when using email from stockrotation.pl cronjob
Summary: Errors when using email from stockrotation.pl cronjob
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Martin Renvoize
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 21656
Blocks:
  Show dependency treegraph
 
Reported: 2018-10-19 13:31 UTC by Martin Renvoize
Modified: 2019-10-14 19:58 UTC (History)
6 users (show)

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


Attachments
Bug 21620: Prevent stockrotation cronjob failures (3.49 KB, patch)
2018-10-19 13:42 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21620: Prevent stockrotation cronjob failures (3.54 KB, patch)
2018-10-19 14:00 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 21620: Prevent stockrotation cronjob failures (3.54 KB, patch)
2018-10-24 10:15 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21620: (follow-up) Fix for bad TT syntax in default notice. (3.11 KB, patch)
2018-10-24 10:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21620: (follow-up) Fix for bad TT syntax in default notice. (4.57 KB, patch)
2018-10-25 07:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21620: Prevent stockrotation cronjob failures (3.54 KB, patch)
2018-10-25 08:12 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21620: Prevent stockrotation cronjob failures (3.54 KB, patch)
2018-10-25 08:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21620: Prevent stockrotation cronjob failures (3.63 KB, patch)
2018-11-02 09:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2018-10-19 13:31:22 UTC
There is superfluous use of sprintf within the stockrotation cronjob and it looks like at least one use got rebased incorrectly and was not caught by testing or qa.
Comment 1 Martin Renvoize 2018-10-19 13:42:19 UTC
Created attachment 80934 [details] [review]
Bug 21620: Prevent stockrotation cronjob failures

Superflous use of sprintf combined with rebasing issue left the
stockrotation cronjob such that a 'No action taken' email report would
catastrophically fail with the error:

Redundant argument in sprintf at ./stockrotation.pl line 316

This patch both resolves the error and cleans up some additional
unrequired sprintf usage.

Test Plan
1) Create a rotation plan
2) Add some items to the rotation plan
3) Run the cronjob using `stockrotation.pl --report=mail --send-email
--execute`
4) Note the lack of errors
Comment 2 Mark Tompsett 2018-10-19 14:00:47 UTC
Created attachment 80935 [details] [review]
Bug 21620: Prevent stockrotation cronjob failures

Superflous use of sprintf combined with rebasing issue left the
stockrotation cronjob such that a 'No action taken' email report would
catastrophically fail with the error:

Redundant argument in sprintf at ./stockrotation.pl line 316

This patch both resolves the error and cleans up some additional
unrequired sprintf usage.

Test Plan
1) Create a rotation plan
2) Add some items to the rotation plan
3) Run the cronjob using `stockrotation.pl --report=mail --send-email
--execute`
4) Note the lack of errors

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 3 Jonathan Druart 2018-10-22 18:50:15 UTC
I cannot test this because of bug 11897 comment 366:

% perl misc/cronjobs/stockrotation.pl --report=email --send-email --execute

ERROR PROCESSING TEMPLATE: file error - parse error - input text line 5: unexpected token (ne)
  [% IF item.reason ne 'in-demand' %] at misc/cronjobs/stockrotation.pl line 343.

Also note that the test plan is wrong, the correct value for --report is 'email', not 'mail'
Comment 4 Martin Renvoize 2018-10-24 10:15:59 UTC
Created attachment 81055 [details] [review]
Bug 21620: Prevent stockrotation cronjob failures

Superflous use of sprintf combined with rebasing issue left the
stockrotation cronjob such that a 'No action taken' email report would
catastrophically fail with the error:

Redundant argument in sprintf at ./stockrotation.pl line 316

This patch both resolves the error and cleans up some additional
unrequired sprintf usage.

Test Plan
1) Create a rotation plan
2) Add some items to the rotation plan
3) Run the cronjob using `stockrotation.pl --report=mail --send-email
--execute`
4) Note the lack of errors

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 5 Martin Renvoize 2018-10-24 10:16:03 UTC
Created attachment 81056 [details] [review]
Bug 21620: (follow-up) Fix for bad TT syntax in default notice.
Comment 6 Martin Renvoize 2018-10-24 10:27:31 UTC
Grr... silly template toolkit syntax error.. I think that followup should resolve it.
Comment 7 Jonathan Druart 2018-10-24 12:29:07 UTC
(In reply to Martin Renvoize from comment #5)
> Created attachment 81056 [details] [review] [review]
> Bug 21620: (follow-up) Fix for bad TT syntax in default notice.

Nope, you should fix other languages as well. Maybe doing it on a separate bug report would be better.
Comment 8 Martin Renvoize 2018-10-25 07:08:46 UTC
Created attachment 81108 [details] [review]
Bug 21620: (follow-up) Fix for bad TT syntax in default notice.
Comment 9 Martin Renvoize 2018-10-25 08:12:58 UTC
Created attachment 81110 [details] [review]
Bug 21620: Prevent stockrotation cronjob failures

Superflous use of sprintf combined with rebasing issue left the
stockrotation cronjob such that a 'No action taken' email report would
catastrophically fail with the error:

Redundant argument in sprintf at ./stockrotation.pl line 316

This patch both resolves the error and cleans up some additional
unrequired sprintf usage.

Test Plan
1) Create a rotation plan
2) Add some items to the rotation plan
3) Run the cronjob using `stockrotation.pl --report=mail --send-email
--execute`
4) Note the lack of errors

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 10 Martin Renvoize 2018-10-25 08:14:18 UTC
Created attachment 81111 [details] [review]
Bug 21620: Prevent stockrotation cronjob failures

Superflous use of sprintf combined with rebasing issue left the
stockrotation cronjob such that a 'No action taken' email report would
catastrophically fail with the error:

Redundant argument in sprintf at ./stockrotation.pl line 316

This patch both resolves the error and cleans up some additional
unrequired sprintf usage.

Test Plan
1) Create a rotation plan
2) Add some items to the rotation plan
3) Run the cronjob using `stockrotation.pl --report=email --send-email
--execute`
4) Note the lack of errors

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 11 Martin Renvoize 2018-10-25 08:15:33 UTC
As requested I've moved the followup patch to a second bug and committed the missing changes to it there.
Comment 12 Marcel de Rooy 2018-11-02 09:02:18 UTC
Created attachment 81872 [details] [review]
Bug 21620: Prevent stockrotation cronjob failures

Superflous use of sprintf combined with rebasing issue left the
stockrotation cronjob such that a 'No action taken' email report would
catastrophically fail with the error:

Redundant argument in sprintf at ./stockrotation.pl line 316

This patch both resolves the error and cleans up some additional
unrequired sprintf usage.

Test Plan
1) Create a rotation plan
2) Add some items to the rotation plan
3) Run the cronjob using `stockrotation.pl --report=email --send-email
--execute`
4) Note the lack of errors

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Nick Clemens 2018-11-02 10:42:38 UTC
Awesome work all!

Pushed to master for 18.11
Comment 14 Martin Renvoize 2018-11-09 16:35:40 UTC
Bug 21656 not in 18.05.x series.