Bug 27432 - Add report run to action logs
Summary: Add report run to action logs
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: Reports (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Fridolin Somers
QA Contact: Testopia
URL:
Keywords:
Depends on: 14024
Blocks:
  Show dependency treegraph
 
Reported: 2021-01-14 14:33 UTC by Fridolin Somers
Modified: 2023-01-26 07:19 UTC (History)
4 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:
Reports run are now logged in Tools > Logs.


Attachments
Bug 27432: Add report run to action logs (1.60 KB, patch)
2021-01-14 14:45 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 27432: Add report run to action logs (1.66 KB, patch)
2021-01-14 21:13 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 27432: (QA follow-up) fix if report_id (855 bytes, patch)
2021-01-28 09:43 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 27432: Update ReportsLog system preference description (1019 bytes, patch)
2021-01-28 09:49 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 27432: (follow-up) Correct grammar of preference description (1.00 KB, patch)
2021-02-25 17:45 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 27432: Add report run to action logs (1.66 KB, patch)
2021-02-25 17:54 UTC, Kelly McElligott
Details | Diff | Splinter Review
Bug 27432: (QA follow-up) fix if report_id (855 bytes, patch)
2021-02-25 17:54 UTC, Kelly McElligott
Details | Diff | Splinter Review
Bug 27432: Update ReportsLog system preference description (1023 bytes, patch)
2021-02-25 17:54 UTC, Kelly McElligott
Details | Diff | Splinter Review
Bug 27432: Add report run to action logs (1.66 KB, patch)
2021-03-29 12:20 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 27432: (QA follow-up) fix if report_id (855 bytes, patch)
2021-03-29 12:20 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 27432: Update ReportsLog system preference description (1014 bytes, patch)
2021-03-29 12:20 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 27432: Add report run to action logs (1.72 KB, patch)
2021-04-18 09:27 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 27432: (QA follow-up) fix if report_id (912 bytes, patch)
2021-04-18 09:27 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 27432: Update ReportsLog system preference description (1.05 KB, patch)
2021-04-18 09:27 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Fridolin Somers 2021-01-14 14:33:21 UTC
Reports run should be logged in action logs.
Comment 1 Fridolin Somers 2021-01-14 14:45:51 UTC Comment hidden (obsolete)
Comment 2 Fridolin Somers 2021-01-14 14:47:25 UTC
If accepted I'll create a patch to edit ReportsLog syspref description
Comment 3 PTFS Europe Sandboxes 2021-01-14 21:13:47 UTC
Created attachment 115184 [details] [review]
Bug 27432: Add report run to action logs

Reports run should be logged in action logs.

This uses the exisiting action "Run" from cronjobs.
Like for other actions, infos contains the report name ("Unknown report" is
no report id provided) and executed SQL query.

1) Apply patch
2) Enable system preference "ReportsLog"
3) Run a report
4) Go to Tools > Log viewer
5) Select module "Reports" and submit
6) Check you see a log with report name and SQL query
7) Change SQL of report, re-run
8) Go to Tools > Log viewer
9) Select module "Reports" and submit
10) Select the 2 logs "Compare" and click "View comparison"

Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov>
Comment 4 Katrin Fischer 2021-01-17 16:48:04 UTC
Hi Frido,

we have a fail in the QA tools here:

 FAIL	C4/Reports/Guided.pm
   FAIL	  critic
		# Variables::ProhibitConditionalDeclarations: Got 1 violation(s). 

Also, can you please update the system preference description?
Comment 5 Fridolin Somers 2021-01-28 09:43:19 UTC
Created attachment 115926 [details] [review]
Bug 27432: (QA follow-up) fix if report_id
Comment 6 Fridolin Somers 2021-01-28 09:49:48 UTC
Created attachment 115927 [details] [review]
Bug 27432: Update ReportsLog system preference description
Comment 7 Fridolin Somers 2021-01-28 09:51:23 UTC
(In reply to Katrin Fischer from comment #4)
> Hi Frido,
> 
> we have a fail in the QA tools here:
> 
>  FAIL	C4/Reports/Guided.pm
>    FAIL	  critic
> 		# Variables::ProhibitConditionalDeclarations: Got 1 violation(s). 
Done.
I will be more careful in the future ;)

> 
> Also, can you please update the system preference description?
Done
Comment 8 Owen Leonard 2021-02-25 17:45:43 UTC
Created attachment 117355 [details] [review]
Bug 27432: (follow-up) Correct grammar of preference description
Comment 9 Kelly McElligott 2021-02-25 17:54:29 UTC
Created attachment 117356 [details] [review]
Bug 27432: Add report run to action logs

Reports run should be logged in action logs.

This uses the exisiting action "Run" from cronjobs.
Like for other actions, infos contains the report name ("Unknown report" is
no report id provided) and executed SQL query.

1) Apply patch
2) Enable system preference "ReportsLog"
3) Run a report
4) Go to Tools > Log viewer
5) Select module "Reports" and submit
6) Check you see a log with report name and SQL query
7) Change SQL of report, re-run
8) Go to Tools > Log viewer
9) Select module "Reports" and submit
10) Select the 2 logs "Compare" and click "View comparison"

Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov>
Comment 10 Kelly McElligott 2021-02-25 17:54:32 UTC
Created attachment 117357 [details] [review]
Bug 27432: (QA follow-up) fix if report_id
Comment 11 Kelly McElligott 2021-02-25 17:54:36 UTC
Created attachment 117358 [details] [review]
Bug 27432: Update ReportsLog system preference description
Comment 12 Katrin Fischer 2021-02-28 17:00:43 UTC
I feel something is out of order here - patches don't apply in the current sequence.

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 27432: (follow-up) Correct grammar of preference description
error: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/logs.pref).
error: could not build fake ancestor
Patch failed at 0001 Bug 27432: (follow-up) Correct grammar of preference description
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-27432-follow-up-Correct-grammar-of-preference--qT_FEt.patch
Comment 13 Fridolin Somers 2021-03-01 09:48:03 UTC
(In reply to Katrin Fischer from comment #12)
> I feel something is out of order here - patches don't apply in the current
> sequence.
> 
> Apply? [(y)es, (n)o, (i)nteractive] y
> Applying: Bug 27432: (follow-up) Correct grammar of preference description
> error: sha1 information is lacking or useless
> (koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/logs.pref).
> error: could not build fake ancestor
> Patch failed at 0001 Bug 27432: (follow-up) Correct grammar of preference
> description
> The copy of the patch that failed is found in: .git/rebase-apply/patch
> When you have resolved this problem run "git bz apply --continue".
> If you would prefer to skip this patch, instead run "git bz apply --skip".
> To restore the original branch and stop patching run "git bz apply --abort".
> Patch left in
> /tmp/Bug-27432-follow-up-Correct-grammar-of-preference--qT_FEt.patch

Ah indeed, patch :
  Bug 27432: (follow-up) Correct grammar of preference description
Is from 17:45 and other patches are from 17:54.
I think this patch must be moved after main patch :
  Bug 27432: Add report run to action logs
Comment 14 Katrin Fischer 2021-03-21 12:37:56 UTC
I've tried to apply patches 2 3 4 as attached, but I still have a conflict that needs to be solved by the dev:

Apply? [(y)es, (n)o, (i)nteractive] i
Applying: Bug 27432: Add report run to action logs
Applying: Bug 27432: (QA follow-up) fix if report_id
Applying: Bug 27432: Update ReportsLog system preference description
error: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/logs.pref).
error: could not build fake ancestor
Patch failed at 0001 Bug 27432: Update ReportsLog system preference description
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-27432-Update-ReportsLog-system-preference-desc-Ygc9OB.patch
Comment 15 Fridolin Somers 2021-03-29 12:19:18 UTC
Mhh strange rebase does not work.

I simply fix my commit :
 Bug 27432: Update ReportsLog system preference description
With patch (manually set to obsolete) :
 Bug 27432: (follow-up) Correct grammar of preference description
Comment 16 Fridolin Somers 2021-03-29 12:20:21 UTC
Created attachment 118953 [details] [review]
Bug 27432: Add report run to action logs

Reports run should be logged in action logs.

This uses the exisiting action "Run" from cronjobs.
Like for other actions, infos contains the report name ("Unknown report" is
no report id provided) and executed SQL query.

1) Apply patch
2) Enable system preference "ReportsLog"
3) Run a report
4) Go to Tools > Log viewer
5) Select module "Reports" and submit
6) Check you see a log with report name and SQL query
7) Change SQL of report, re-run
8) Go to Tools > Log viewer
9) Select module "Reports" and submit
10) Select the 2 logs "Compare" and click "View comparison"

Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov>
Comment 17 Fridolin Somers 2021-03-29 12:20:37 UTC
Created attachment 118954 [details] [review]
Bug 27432: (QA follow-up) fix if report_id
Comment 18 Fridolin Somers 2021-03-29 12:20:53 UTC
Created attachment 118955 [details] [review]
Bug 27432: Update ReportsLog system preference description
Comment 19 Katrin Fischer 2021-04-18 09:27:47 UTC
Created attachment 119817 [details] [review]
Bug 27432: Add report run to action logs

Reports run should be logged in action logs.

This uses the exisiting action "Run" from cronjobs.
Like for other actions, infos contains the report name ("Unknown report" is
no report id provided) and executed SQL query.

1) Apply patch
2) Enable system preference "ReportsLog"
3) Run a report
4) Go to Tools > Log viewer
5) Select module "Reports" and submit
6) Check you see a log with report name and SQL query
7) Change SQL of report, re-run
8) Go to Tools > Log viewer
9) Select module "Reports" and submit
10) Select the 2 logs "Compare" and click "View comparison"

Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 20 Katrin Fischer 2021-04-18 09:27:51 UTC
Created attachment 119818 [details] [review]
Bug 27432: (QA follow-up) fix if report_id

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 21 Katrin Fischer 2021-04-18 09:27:56 UTC
Created attachment 119819 [details] [review]
Bug 27432: Update ReportsLog system preference description

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 22 Katrin Fischer 2021-04-18 09:28:13 UTC
Also checked that these can be found with the log viewer tool.
Comment 23 Jonathan Druart 2021-04-21 12:59:35 UTC
Missing test coverage.
Comment 24 David Cook 2021-09-13 00:35:30 UTC
I would love to see this get through. What do we need to do to push this along?
Comment 25 Fridolin Somers 2021-09-14 00:29:15 UTC
We need a unit test.
Maybe in t/db_dependent/Log.t.

Actually there are no test for other reports actions right ?
Comment 26 David Cook 2021-09-14 01:56:30 UTC
(In reply to Fridolin Somers from comment #25)
> We need a unit test.
> Maybe in t/db_dependent/Log.t.
> 
> Actually there are no test for other reports actions right ?

That's what I was wondering too.
Comment 27 Katrin Fischer 2023-01-23 23:55:27 UTC
Could we get some help here with the unit tests? Would make a real nice addition.
Comment 28 Jonathan Druart 2023-01-24 12:29:40 UTC
There is no difficulty in writing the require tests. Let me know if you need guidance however.
Comment 29 Jonathan Druart 2023-01-24 12:30:09 UTC
Note that we should not fetch the report if ReportsLog is off.
Comment 30 Fridolin Somers 2023-01-26 07:19:27 UTC
(In reply to Jonathan Druart from comment #29)
> Note that we should not fetch the report if ReportsLog is off.
Correct.

I see in execute_query() :
    my ( $is_sql_valid, $errors ) = Koha::Report->new({ savedsql => $sql })->is_sql_valid;
    return (undef, @{$errors}[0]) unless $is_sql_valid;

Should we replace with :
my $report = Koha::Reports->find($report_id) if $report_id;
...
$report->is_sql_valid

Plus :
======
I see an issue with this feature.
Some public reports maybe run very often via JSON, like webservices.
Maybe we should log only runs via the staff interface ?