Bug 21620

Summary: Errors when using email from stockrotation.pl cronjob
Product: Koha Reporter: Martin Renvoize <martin.renvoize>
Component: CirculationAssignee: Martin Renvoize <martin.renvoize>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: critical    
Priority: P5 - low CC: gmcharlt, jonathan.druart, katrin.fischer, kyle.m.hall, m.de.rooy, nick
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 21656    
Bug Blocks:    
Attachments: Bug 21620: Prevent stockrotation cronjob failures
Bug 21620: Prevent stockrotation cronjob failures
Bug 21620: Prevent stockrotation cronjob failures
Bug 21620: (follow-up) Fix for bad TT syntax in default notice.
Bug 21620: (follow-up) Fix for bad TT syntax in default notice.
Bug 21620: Prevent stockrotation cronjob failures
Bug 21620: Prevent stockrotation cronjob failures
Bug 21620: Prevent stockrotation cronjob failures

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.