Bug 22566 - Stock rotation cronjob reporting has issues
Summary: Stock rotation cronjob reporting has issues
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Martin Renvoize
QA Contact: Testopia
URL:
Keywords: RM_priority
Depends on: 11897
Blocks: 22569 23088 23432
  Show dependency treegraph
 
Reported: 2019-03-22 17:17 UTC by Nick Clemens
Modified: 2019-08-05 16:12 UTC (History)
5 users (show)

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


Attachments
Bug 22566: Fix some problems (1.04 KB, patch)
2019-03-22 17:24 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 22566: Fix some problems (1.10 KB, patch)
2019-06-07 13:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22566: Fix some more issues (3.92 KB, patch)
2019-06-07 13:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22566: Fix malformed notice for stockrotation (3.55 KB, patch)
2019-06-07 13:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22566: Fix some problems (1.15 KB, patch)
2019-06-07 19:55 UTC, Nadine Pierre
Details | Diff | Splinter Review
Bug 22566: Fix some more issues (3.98 KB, patch)
2019-06-07 19:55 UTC, Nadine Pierre
Details | Diff | Splinter Review
Bug 22566: Fix malformed notice for stockrotation (3.60 KB, patch)
2019-06-07 19:56 UTC, Nadine Pierre
Details | Diff | Splinter Review
Bug 22566: Fix some problems (1.10 KB, patch)
2019-06-18 13:27 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22566: Fix some more issues (3.92 KB, patch)
2019-06-18 13:27 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22566: Fix malformed notice for stockrotation (3.55 KB, patch)
2019-06-18 13:27 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22566: Accept 'all' in the branch params (1.94 KB, patch)
2019-06-18 13:27 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22566: Rename 'report_email' to 'report_by_branch' (1.79 KB, patch)
2019-06-18 13:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22566: Clarify intent of reports and add warnings (2.39 KB, patch)
2019-06-18 13:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22566: Fix some problems (1.10 KB, patch)
2019-06-18 13:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22566: Fix some more issues (3.92 KB, patch)
2019-06-18 13:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22566: Accept 'all' in the branch params (1.94 KB, patch)
2019-06-18 13:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22566: Rename 'report_email' to 'report_by_branch' (1.79 KB, patch)
2019-06-18 13:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22566: Clarify intent of reports and add warnings (2.39 KB, patch)
2019-06-18 13:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22566: Add 'branch' key to GetPreparedLetter (929 bytes, patch)
2019-06-18 13:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22566: Fix some problems (1.15 KB, patch)
2019-06-25 12:41 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 22566: Fix some more issues (3.98 KB, patch)
2019-06-25 12:41 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 22566: Accept 'all' in the branch params (2.00 KB, patch)
2019-06-25 12:41 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 22566: Rename 'report_email' to 'report_by_branch' (1.85 KB, patch)
2019-06-25 12:41 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 22566: Clarify intent of reports and add warnings (2.45 KB, patch)
2019-06-25 12:41 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 22566: Add 'branch' key to GetPreparedLetter (986 bytes, patch)
2019-06-25 12:41 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 22566: (QA follow-up) Fix pod complaint (719 bytes, patch)
2019-06-25 12:41 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2019-03-22 17:17:32 UTC
Trying to test this today we found that some of the reporting features of the cronjob seem to be problematic:

To recreate:
1 - Create some rotas
2 - Add some items
3 - run cronjob with -x
4 - in sql UPDATE branchtransfers SET datearrived=DATE_SUB(CURDATE(), INTERVAL 1 DAY) WHERE comments like '%rota%'
5 - run the cornjob without -x:
perl misc/cronjobs/stockrotation.pl -r full
6 - get an error about undefined emails
7 - run it for branch emails
perl misc/cronjobs/stockrotation.pl -r email
8 - get an error about odd numebr of elements in hash
Comment 1 Nick Clemens 2019-03-22 17:24:49 UTC
Created attachment 86923 [details] [review]
Bug  22566: Fix some problems
Comment 2 Nick Clemens 2019-03-22 17:26:28 UTC
(In reply to Nick Clemens from comment #1)
> Created attachment 86923 [details] [review] [review]
> Bug  22566: Fix some problems

After this patch you shoudl eb able to get some output with -r full, however, -r email says no branches have actionable items - the GetPreparedLetter call seems wrong, we aren't passing substitutions I think, but I am not sure exactly what we must pass here
Comment 3 Jonathan Druart 2019-05-13 01:03:28 UTC
It seems that SR_SLIP has never worked. The template expects branch.items, where does this method could come from?
Comment 4 Martin Renvoize 2019-06-07 13:56:51 UTC
Created attachment 90421 [details] [review]
Bug 22566: Fix some problems

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 5 Martin Renvoize 2019-06-07 13:56:54 UTC
Created attachment 90422 [details] [review]
Bug 22566: Fix some more issues

1) Fix warning for undefined value in sprintf when no items have the
indemand flag set.
2) Corrections to report_full for bad ternary in string construction
3) Perldoc corrections

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize 2019-06-07 13:56:57 UTC
Created attachment 90423 [details] [review]
Bug 22566: Fix malformed notice for stockrotation

The default notice for stockrotation was malformed. This patch replaces
it with a working one.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize 2019-06-07 14:00:06 UTC
This is upsetting.. I think it's a case of the originally working implementation getting destroyed during the QA process and not properly tested after the massive number of QA followups.

I'd appreciate some thorough testing of the cronjob reporting/dry-run functionality with these patches applied.

Test plan as above.. with patches applied the 'full' and 'email' reports should both output meaningful data.
Comment 8 Nadine Pierre 2019-06-07 19:55:52 UTC
Created attachment 90433 [details] [review]
Bug 22566: Fix some problems

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Nadine Pierre <nadine.pierre@inLibro.com>
Comment 9 Nadine Pierre 2019-06-07 19:55:55 UTC
Created attachment 90434 [details] [review]
Bug 22566: Fix some more issues

1) Fix warning for undefined value in sprintf when no items have the
indemand flag set.
2) Corrections to report_full for bad ternary in string construction
3) Perldoc corrections

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Nadine Pierre <nadine.pierre@inLibro.com>
Comment 10 Nadine Pierre 2019-06-07 19:56:00 UTC
Created attachment 90435 [details] [review]
Bug 22566: Fix malformed notice for stockrotation

The default notice for stockrotation was malformed. This patch replaces
it with a working one.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Nadine Pierre <nadine.pierre@inLibro.com>
Comment 11 Martin Renvoize 2019-06-18 13:27:50 UTC
Created attachment 90742 [details] [review]
Bug 22566: Fix some problems

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2019-06-18 13:27:53 UTC
Created attachment 90743 [details] [review]
Bug 22566: Fix some more issues

1) Fix warning for undefined value in sprintf when no items have the
indemand flag set.
2) Corrections to report_full for bad ternary in string construction
3) Perldoc corrections

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2019-06-18 13:27:56 UTC
Created attachment 90744 [details] [review]
Bug 22566: Fix malformed notice for stockrotation

The default notice for stockrotation was malformed. This patch replaces
it with a working one.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2019-06-18 13:27:58 UTC
Created attachment 90745 [details] [review]
Bug 22566: Accept 'all' in the branch params
Comment 15 Martin Renvoize 2019-06-18 13:28:01 UTC
Created attachment 90746 [details] [review]
Bug 22566: Rename 'report_email' to 'report_by_branch'

To clarify the intention of this routine, lets rename it
Comment 16 Martin Renvoize 2019-06-18 13:28:05 UTC
Created attachment 90747 [details] [review]
Bug 22566: Clarify intent of reports and add warnings
Comment 17 Martin Renvoize 2019-06-18 13:31:10 UTC
Created attachment 90748 [details] [review]
Bug 22566: Fix some problems

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Martin Renvoize 2019-06-18 13:31:13 UTC
Created attachment 90749 [details] [review]
Bug 22566: Fix some more issues

1) Fix warning for undefined value in sprintf when no items have the
indemand flag set.
2) Corrections to report_full for bad ternary in string construction
3) Perldoc corrections

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 19 Martin Renvoize 2019-06-18 13:31:16 UTC
Created attachment 90750 [details] [review]
Bug 22566: Accept 'all' in the branch params
Comment 20 Martin Renvoize 2019-06-18 13:31:18 UTC
Created attachment 90751 [details] [review]
Bug 22566: Rename 'report_email' to 'report_by_branch'

To clarify the intention of this routine, lets rename it
Comment 21 Martin Renvoize 2019-06-18 13:31:21 UTC
Created attachment 90752 [details] [review]
Bug 22566: Clarify intent of reports and add warnings
Comment 22 Martin Renvoize 2019-06-18 13:31:24 UTC
Created attachment 90753 [details] [review]
Bug 22566: Add 'branch' key to GetPreparedLetter

The branch key was missing from the subsitution call to
GetPreparedLetter.
Comment 23 Nick Clemens 2019-06-25 12:41:40 UTC
Created attachment 90989 [details] [review]
Bug 22566: Fix some problems

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 24 Nick Clemens 2019-06-25 12:41:43 UTC
Created attachment 90990 [details] [review]
Bug 22566: Fix some more issues

1) Fix warning for undefined value in sprintf when no items have the
indemand flag set.
2) Corrections to report_full for bad ternary in string construction
3) Perldoc corrections

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 25 Nick Clemens 2019-06-25 12:41:46 UTC
Created attachment 90991 [details] [review]
Bug 22566: Accept 'all' in the branch params

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 26 Nick Clemens 2019-06-25 12:41:49 UTC
Created attachment 90992 [details] [review]
Bug 22566: Rename 'report_email' to 'report_by_branch'

To clarify the intention of this routine, lets rename it

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 27 Nick Clemens 2019-06-25 12:41:52 UTC
Created attachment 90993 [details] [review]
Bug 22566: Clarify intent of reports and add warnings

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 28 Nick Clemens 2019-06-25 12:41:55 UTC
Created attachment 90994 [details] [review]
Bug 22566: Add 'branch' key to GetPreparedLetter

The branch key was missing from the subsitution call to
GetPreparedLetter.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 29 Nick Clemens 2019-06-25 12:41:58 UTC
Created attachment 90995 [details] [review]
Bug 22566: (QA follow-up) Fix pod complaint
Comment 30 Nick Clemens 2019-06-25 12:44:00 UTC
I think there is still more work to be done here, but we need guidelines from users to know what this report must show/explain

These patches fix errors and allow us to report out on stock rotation, passing QA
Comment 31 Martin Renvoize 2019-06-25 16:01:58 UTC
Nice work!

Pushed to master for 19.11.00
Comment 32 Fridolin SOMERS 2019-07-25 11:28:15 UTC
Pushed to 19.05.x for 19.05.03
Comment 33 Lucas Gass 2019-07-25 21:08:41 UTC
backported for 18.11.x for 18.11.09