Bug 9188 - Remove 'debug' information from patron statistics
Summary: Remove 'debug' information from patron statistics
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Reports (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Andreas Roussos
QA Contact:
URL: /cgi-bin/koha/reports/borrowers_stats.pl
Keywords:
Depends on:
Blocks:
 
Reported: 2012-12-02 22:22 UTC by Katrin Fischer
Modified: 2019-10-14 19:55 UTC (History)
5 users (show)

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


Attachments
screenshot of patron statistics (76.37 KB, image/png)
2012-12-02 22:22 UTC, Katrin Fischer
Details
Bug 9188 - Remove 'debug' information from patron statistics (3.44 KB, patch)
2016-09-13 10:29 UTC, Andreas Roussos
Details | Diff | Splinter Review
Bug 9188 - Remove 'debug' information from patron statistics (3.61 KB, patch)
2016-09-16 16:45 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 9188: counter patch -- just meaningful filter information (3.42 KB, patch)
2017-06-07 19:31 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 9188: counter patch -- just meaningful filter information (3.55 KB, patch)
2018-09-30 17:22 UTC, Andreas Roussos
Details | Diff | Splinter Review
Bug 9188: Remove 'debug' information from patron statistics (3.62 KB, patch)
2018-09-30 23:15 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2012-12-02 22:22:53 UTC
Created attachment 13828 [details]
screenshot of patron statistics

Patron statistics show a lot of information that is confusing to a normal user and looks more like debug information than information that is intended for the end user.

I think this information should be removed or hidden by default.
Comment 1 Andreas Roussos 2016-09-13 09:56:05 UTC
Apparently this is still relevant as of the current master (8a74bb2).
It's easy to fix, so I'll submit a patch soon.
Comment 2 Andreas Roussos 2016-09-13 10:29:23 UTC
Created attachment 55526 [details] [review]
Bug 9188 - Remove 'debug' information from patron statistics

In the staff client, if you submit a report under Reports ->
Statistics wizards -> Patrons you see a lot of debug info
with the heading "Filtered on:".

This patch fixes that by adding an extra template parameter
('filtered_on_debug') in reports/borrowers_stats.pl and setting
it to 0. The relevant template file is also updated to include
a conditional directive which checks for the 'filtered_on_debug'
variable. Debug info will only be shown if the variable is set
to something other than 0. Finally, the associated help file
template is updated to reflect the changes made.

Test plan:
1) In the staff client, submit a report under Reports ->
   Statistics wizards -> Patrons. Observe that you see a
   lot of debug information.
2) Apply the patch.
3) Repeat step 1. Confirm that the patch works, i.e. you
   no longer see the debug information.
Comment 3 Marc Véron 2016-09-16 16:45:26 UTC
Created attachment 55625 [details] [review]
Bug 9188 - Remove 'debug' information from patron statistics

In the staff client, if you submit a report under Reports ->
Statistics wizards -> Patrons you see a lot of debug info
with the heading "Filtered on:".

This patch fixes that by adding an extra template parameter
('filtered_on_debug') in reports/borrowers_stats.pl and setting
it to 0. The relevant template file is also updated to include
a conditional directive which checks for the 'filtered_on_debug'
variable. Debug info will only be shown if the variable is set
to something other than 0. Finally, the associated help file
template is updated to reflect the changes made.

Test plan:
1) In the staff client, submit a report under Reports ->
   Statistics wizards -> Patrons. Observe that you see a
   lot of debug information.
2) Apply the patch.
3) Repeat step 1. Confirm that the patch works, i.e. you
   no longer see the debug information.

Followed test plan, works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>
Comment 4 Katrin Fischer 2016-09-18 09:22:03 UTC
Checking the other statistical reports I notice that they all show a 'Filtered on' - maybe instead of hiding this totally, we should try to clean up the bits that are not easy to understand? Or should we try and clean up the other reports too? 

What do you think?
Comment 5 Andreas Roussos 2016-09-20 20:06:13 UTC
(In reply to Katrin Fischer from comment #4)
> Checking the other statistical reports I notice that they all show a
> 'Filtered on' - maybe instead of hiding this totally, we should try to clean
> up the bits that are not easy to understand? Or should we try and clean up
> the other reports too? 
> 
> What do you think?
Well, the "Query =" bits are probably the ones that can confuse a non-tech savvy
user the most and IMHO should not be displayed at all. As for the "Filtered on:"
info, I think it clutters the display i.e. you have asked for a report, so you
should get a report (and just that), not a repeat of what filters you chose when
designing the report. Besides, when outputting to a file, you never get to see
this "debug" info which (again, IMHO) is yet another reason to not display it at
all unless a flag is set.

Perhaps we've missed something, though. Usually when more people express their
views a better decision can be made, so I was wondering if this bug is worth
adding to the agenda of the next dev meeting on 28/09?

May I also suggest changing the bug title to "Remove 'debug' information from
report results" or something similar to reflect its wider scope?
Comment 6 Katrin Fischer 2017-06-05 10:37:20 UTC
Sorry Andreas, this got lost in my bug mails. I am resetting to 'Needs sign-off' - maybe this will get us another point of view!
Comment 7 Mark Tompsett 2017-06-07 19:01:44 UTC
I agree the top part is messy. However, I believe the things which are not filtered expressly don't need to be listed.

For example, I chose a particular category code and branch code, and there would have been no way to know the branch code without the mess at the top. And yes, it was just a single number.

I agree the Query parts are rather pointless.

I think if you choose a row and column, there is no need to display a value for either the row or column unless it is filtered to that value. This means there should only be two lines at most at the top.

As such, I don't think this patch is the correct way to do it.
Comment 8 Mark Tompsett 2017-06-07 19:31:37 UTC
Created attachment 64087 [details] [review]
Bug 9188: counter patch -- just meaningful filter information

TEST PLAN
---------
run patron statistics without filtering
-- notice lots of text at top.
run patron statistics with filtering
-- notice lots of text at top.
apply patch
run it without and without filtering
-- amount of text should be small or none and be related to the filtering selected.
run koha qa test tools
Comment 9 Mark Tompsett 2017-06-07 19:33:00 UTC
If you believe the counter patch is good, obsolete the first patch and sign off the second. If you believe there is need for more discussion, discuss away. Do not apply both patches, just one or the other.
Comment 10 Mark Tompsett 2017-06-07 19:33:42 UTC
P.S. This does NOT fix the row/column not fully selected problem, as that is beyond scope.
Comment 11 Andreas Roussos 2018-09-30 17:12:22 UTC
(In reply to M. Tompsett from comment #9)
> If you believe the counter patch is good, obsolete the first patch and sign
> off the second. If you believe there is need for more discussion, discuss
> away. Do not apply both patches, just one or the other.
Please accept my apologies for taking so long to reply.

Thank you for providing a counter patch, I've had a look at it and it's good,
definitely a better approach than mine ;-) Interestingly, it still applies
to the current master after all this time without any need for rebasing.

I'm going to sign it off and obsolete mine.

Also, as per the patch submitting guidelines in the wiki, I'm raising the
patch complexity from trivial to small.
Comment 12 Andreas Roussos 2018-09-30 17:22:05 UTC
Created attachment 79659 [details] [review]
Bug 9188: counter patch -- just meaningful filter information

TEST PLAN
---------
run patron statistics without filtering
-- notice lots of text at top.
run patron statistics with filtering
-- notice lots of text at top.
apply patch
run it without and without filtering
-- amount of text should be small or none and be related to the filtering selected.
run koha qa test tools

Signed-off-by: Andreas Roussos <arouss1980@gmail.com>
Works as expected, i.e. only filtering information is displayed.
Comment 13 Andreas Roussos 2018-09-30 17:41:00 UTC
Sorry, I've set the wrong status after signing off Mark's patch.
Comment 14 Jonathan Druart 2018-09-30 23:15:26 UTC
Created attachment 79666 [details] [review]
Bug 9188: Remove 'debug' information from patron statistics

TEST PLAN
---------
run patron statistics without filtering
-- notice lots of text at top.
run patron statistics with filtering
-- notice lots of text at top.
apply patch
run it without and without filtering
-- amount of text should be small or none and be related to the filtering selected.
run koha qa test tools

Signed-off-by: Andreas Roussos <arouss1980@gmail.com>
Works as expected, i.e. only filtering information is displayed.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 15 Nick Clemens 2018-10-01 16:46:48 UTC
Awesome work all!

Pushed to master for 18.11
Comment 16 Martin Renvoize 2018-10-08 07:58:44 UTC
Enhancement, will not be backported.