Bug 3311 - Statistical reports should error if row and column aren't selected
Summary: Statistical reports should error if row and column aren't selected
Status: Needs Signoff
Alias: None
Product: Koha
Classification: Unclassified
Component: Reports (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: M. Tompsett
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-06-08 05:33 UTC by Chris Cormack
Modified: 2017-04-27 18:48 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 3311: Testing Cases (4.48 KB, patch)
2016-06-24 03:41 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 3311: Testing Cases (4.48 KB, patch)
2016-06-24 03:41 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 3311: Statistical reports should error if row and column aren't selected (6.92 KB, patch)
2016-06-24 03:41 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 3311: Testing Cases (4.56 KB, patch)
2016-06-24 03:46 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 3311: Statistical reports should error if row and column aren't selected (6.92 KB, patch)
2016-06-24 03:46 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 3311: Testing Cases (4.62 KB, patch)
2016-07-07 19:06 UTC, Nicole C. Engard
Details | Diff | Splinter Review
Bug 3311: Statistical reports should error if row and column aren't selected (6.98 KB, patch)
2016-07-07 19:06 UTC, Nicole C. Engard
Details | Diff | Splinter Review
Bug 3311: Follow-up to add alerts and colour corrections (3.06 KB, patch)
2016-07-11 23:50 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 3311: Form validation feedback (2.58 KB, patch)
2017-04-21 14:55 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 3311: Followup clean up HTML on page. (3.70 KB, patch)
2017-04-21 16:44 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 3311: Form validation feedback (2.58 KB, patch)
2017-04-24 17:34 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 3311: Followup clean up HTML on page. (6.09 KB, patch)
2017-04-24 17:34 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 3311: Followup clean up HTML on page. (6.21 KB, patch)
2017-04-24 18:44 UTC, M. Tompsett
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 3311: Form validation feedback (2.64 KB, patch)
2017-04-25 12:10 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 3311: Followup clean up HTML on page. (6.27 KB, patch)
2017-04-25 12:10 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 3311 [Follow-up] Statistical reports should error if row and column aren't selected (2.74 KB, patch)
2017-04-27 13:27 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED OFF] Bug 3311 [Follow-up] Statistical reports should error if row and column aren't selected (2.80 KB, patch)
2017-04-27 14:11 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 3311: Form validation feedback (2.63 KB, patch)
2017-04-27 18:38 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 3311: Followup clean up HTML on page. (6.26 KB, patch)
2017-04-27 18:38 UTC, M. Tompsett
Details | Diff | Splinter Review
[SIGNED OFF] Bug 3311 [Follow-up] Statistical reports should error if row and column aren't selected (2.80 KB, patch)
2017-04-27 18:39 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 3311: Create common Row Columns Require javascript (1.11 KB, patch)
2017-04-27 18:39 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 3311: catalogue stats use js version (1.41 KB, patch)
2017-04-27 18:39 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 3311: Borrowers Stats warnings added (2.22 KB, patch)
2017-04-27 18:39 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 3311: Borrower stats HTML clean up. (5.75 KB, patch)
2017-04-27 18:39 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 3311: borrower_stats indentation issues (4.28 KB, patch)
2017-04-27 18:39 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 3311: Issues stat report row and column required (2.47 KB, patch)
2017-04-27 18:39 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 3311: Issues stat report HTML clean up. (7.17 KB, patch)
2017-04-27 18:39 UTC, M. Tompsett
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Cormack 2010-05-21 01:09:00 UTC


---- Reported by maria.laude@liblime.com 2009-06-08 17:33:35 ----

When running Statistical Reports,  interface does not clearly specify that both a column and a row must be selected.  User is able to generate a report with selection of just row.  Koha should either error out, or add instructions on-screen indicating both column and row are required.



--- Bug imported by chris@bigballofwax.co.nz 2010-05-21 01:09 UTC  ---

This bug was previously known as _bug_ 3311 at http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=3311

Actual time not defined. Setting to 0.0
The original reporter of this bug does not have
   an account here. Reassigning to the person who moved
   it here: chris@bigballofwax.co.nz.
   Previous reporter was maria.laude@liblime.com.
CC member maria.laude@liblime.com does not have an account here

Comment 1 Mirko Tietgen 2012-12-16 13:24:23 UTC
I don't think this bug is valid for recent versions of Koha. Changing to resolved-invalid.
Comment 2 M. Tompsett 2016-06-23 19:20:40 UTC
Home -> Reports -> Catalog

If you immediately click 'Submit' without entering anything, it blows up.
If you select only a row or a column, it blows up.

SAMPLE EXPLOSION
----------------

Software error:

DBD::mysql::st execute failed: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'FROM items
                    LEFT JOIN biblioitems USING (biblioitemnumber)
  ' at line 1 [for Statement "SELECT DISTINCTROW  FROM items
                    LEFT JOIN biblioitems USING (biblioitemnumber)
                    LEFT JOIN biblio ON (biblioitems.biblionumber = biblio.biblionumber)
                  WHERE 1  ORDER BY "] at /home/mtompset/kohaclone/reports/catalogue_stats.pl line 325.
For help, please send mail to the webmaster (webmaster@ubuntu), giving this error message and the time and date of the error.
Comment 3 M. Tompsett 2016-06-24 03:41:06 UTC Comment hidden (obsolete)
Comment 4 M. Tompsett 2016-06-24 03:41:34 UTC Comment hidden (obsolete)
Comment 5 M. Tompsett 2016-06-24 03:41:40 UTC Comment hidden (obsolete)
Comment 6 M. Tompsett 2016-06-24 03:46:25 UTC Comment hidden (obsolete)
Comment 7 M. Tompsett 2016-06-24 03:46:34 UTC Comment hidden (obsolete)
Comment 8 Nicole C. Engard 2016-07-07 19:06:49 UTC
Created attachment 53194 [details] [review]
Bug 3311: Testing Cases

There was no form of testing for the page. In order to test
the perl-side of the changes, I just went to a variety of
URLs to see if it will blow up.

TEST PLAN
---------
1) Apply this patch
2) At the bash prompt, export KOHA_INTRANET_URL=http://localhost:8080/
   -- Or whatever yours is. With or without the trailing /
3) prove -v t/db_dependent/www/catalogue_stats.t
   -- before applying the other patch, there should be failures.
4) Apply both patches
5) prove -v t/db_dependent/www/catalogue_stats.t
   -- there should be no failures now.
6) run koha qa test tools

Signed-off-by: Nicole C Engard <nengard@bywatersolutions.com>
Comment 9 Nicole C. Engard 2016-07-07 19:06:54 UTC
Created attachment 53195 [details] [review]
Bug 3311: Statistical reports should error if row and column aren't selected

There are three parameters to play with which do not always
generate friendly results. A couple of them seriously crash now.

TEST PLAN
---------
1) Staff -> Reports -> Catalog
   -- should be at 'Catalog statistics' area.
2) Run through combinations:
      no row, no column
      no row, column
      row, no column
      row, column
   After selecting a combination to test,
   click submit, see what happens.
   Pre-patch apply, nasty crashes sometimes.
   Step 1 may require repeating, since you
   can't select a radio button and then
   unselect it.
3) Apply the patch
4) Repeat the first two steps again, and
   this time, a pretty informational or
   warning message should be given.
5) Run koha qa test tools

Signed-off-by: Nicole C Engard <nengard@bywatersolutions.com>
Comment 10 Jonathan Druart 2016-07-08 15:37:49 UTC
1/ I am not sure that it is useful to display the warning when we arrive on the page ("Information: Both row and column values must be selected.").
2/ How can be useful the changes done in the perl script?
3/ If you are submitting the form and one column is missing, the message is displayed on the top of the page, which is not necessarily visible (on relatively small screens).

Why don't you just display a JS alert if at least one of the 2 infos is missing?
Comment 11 M. Tompsett 2016-07-09 14:31:00 UTC
(In reply to Jonathan Druart from comment #10)
> 1/ I am not sure that it is useful to display the warning when we arrive on
> the page ("Information: Both row and column values must be selected.").

It seemed to make sense to put a pre-emptive instruction, so the actual problem cases would be generally avoided.


> 2/ How can be useful the changes done in the perl script?

The value of $do_it is wrong without those changes, and causes nasty crashes. That is why I provided test coverage.


> 3/ If you are submitting the form and one column is missing, the message is
> displayed on the top of the page, which is not necessarily visible (on
> relatively small screens).

The whole screen looks bad on small screens. I just tried it on my phone.
But the yellow at the top made me read it.


> Why don't you just display a JS alert if at least one of the 2 infos is
> missing?

Because this is prettier? I'd prefer bootstrap in the staff client too, but that hasn't happened yet.

Would this + alerts be okay? After all, it's really easy to click okay, and forget the message.
Comment 12 Katrin Fischer 2016-07-11 21:56:39 UTC
Hm, ading another opinion after some testing:

A hint is ok, but I feel this looks too much like a warning/an error message - and it takes up a lot of space. I'd prefer the light grey font color hint style we use on other pages. If you really want it to stand out more, maybe the note style from the authorised value administration page would do (I'd ask Owen for advice) 

Also: +1 for alerts. Why? Because on my screen when I scroll to submit the form, I don't see the message change on top and so it seems like nothing happens on pressing the button - especially since there already was a yellow message before.
Comment 13 M. Tompsett 2016-07-11 23:50:44 UTC
Created attachment 53291 [details] [review]
Bug 3311: Follow-up to add alerts and colour corrections

This should deal with comment #12.

I agree the coloring was poor. Used 'message' instead
of 'alert' for information one. The whole width thing
is a CSS thing that I'd rather not touch.

And having tested on my phone, I totally understand
the request for alerts. Those have been added.

NOTE: Wasn't sure what status to put it to.
Comment 14 Owen Leonard 2016-07-12 12:31:26 UTC
We don't have any established pattern for adding icons to message or alert dialog, and my instinct is to say that they are unnecessary. 

Also, the new JavaScript has HTML comments in it:

<!-- Column is not defined -->

One-line comments in JavaScript should look like:

// Column is not defined
Comment 15 Jonathan Druart 2016-07-12 17:14:19 UTC
(In reply to M. Tompsett from comment #11)
> (In reply to Jonathan Druart from comment #10)
> > 2/ How can be useful the changes done in the perl script?
> 
> The value of $do_it is wrong without those changes, and causes nasty
> crashes. That is why I provided test coverage.

How can I get the crash with the JS alert blocking the form submission?
Comment 16 M. Tompsett 2016-07-13 02:24:03 UTC
(In reply to Jonathan Druart from comment #15)
> (In reply to M. Tompsett from comment #11)
> > (In reply to Jonathan Druart from comment #10)
> > > 2/ How can be useful the changes done in the perl script?
> > 
> > The value of $do_it is wrong without those changes, and causes nasty
> > crashes. That is why I provided test coverage.
> 
> How can I get the crash with the JS alert blocking the form submission?

Hard coded URLs.
Comment 17 Jonathan Druart 2016-07-15 12:43:04 UTC
(In reply to M. Tompsett from comment #16)
> (In reply to Jonathan Druart from comment #15)
> > (In reply to M. Tompsett from comment #11)
> > > (In reply to Jonathan Druart from comment #10)
> > > > 2/ How can be useful the changes done in the perl script?
> > > 
> > > The value of $do_it is wrong without those changes, and causes nasty
> > > crashes. That is why I provided test coverage.
> > 
> > How can I get the crash with the JS alert blocking the form submission?
> 
> Hard coded URLs.

If it cannot be recreate from the interface, I don't think it's useful to catch these errors. IMO this kind of errors should not be managed, especially because it adds relatively lot of code (which means much maintenance). A simple JS alert should do the trick.

Waiting for more QA opinions.
Comment 18 M. Tompsett 2016-11-21 17:58:53 UTC
(In reply to Jonathan Druart from comment #17)
> Waiting for more QA opinions.

We never got them. What should we do to get this moving again?
Comment 19 Jonathan Druart 2016-11-22 07:48:45 UTC
(In reply to M. Tompsett from comment #18)
> (In reply to Jonathan Druart from comment #17)
> > Waiting for more QA opinions.
> 
> We never got them. What should we do to get this moving again?

Either you fix what I raised or you find someone to agree with your points.
Comment 20 M. Tompsett 2017-04-21 14:55:43 UTC
Created attachment 62533 [details] [review]
Bug 3311: Form validation feedback

By using the validated class, make use of jquery form submission
validation to inform user of required fields.

TEST PLAN
---------
1) run Home -> Reports -> Catalog -> Submit
   -- no meaningful output and no error messages.
2) apply patch
3) repeat 1
   -- meaningful error messages appear.
   -- headings are in red, indicative of required values.
4) try other combinations of row or column selected/unselected.
   -- meaningful error messages.
5) run koha qa test tools
Comment 21 M. Tompsett 2017-04-21 16:44:06 UTC
Created attachment 62541 [details] [review]
Bug 3311: Followup clean up HTML on page.
Comment 22 M. Tompsett 2017-04-24 17:34:14 UTC
Created attachment 62623 [details] [review]
Bug 3311: Form validation feedback

By using the validated class, make use of jquery form submission
validation to inform user of required fields.

TEST PLAN
---------
1) run Home -> Reports -> Catalog -> Submit
   -- no meaningful output and no error messages.
2) apply patch
3) repeat 1
   -- meaningful error messages appear.
   -- headings are in red, indicative of required values.
4) try other combinations of row or column selected/unselected.
   -- meaningful error messages.
5) run koha qa test tools
Comment 23 M. Tompsett 2017-04-24 17:34:43 UTC
Created attachment 62624 [details] [review]
Bug 3311: Followup clean up HTML on page.

Should be no validation errors if you paste source code to validator.w3.org
Comment 24 M. Tompsett 2017-04-24 18:44:48 UTC
Created attachment 62630 [details] [review]
Bug 3311: Followup clean up HTML on page.

There should be no validator.w3.org validation issues
for this page's source.
Comment 25 Owen Leonard 2017-04-25 12:10:10 UTC
Created attachment 62658 [details] [review]
[SIGNED-OFF] Bug 3311: Form validation feedback

By using the validated class, make use of jquery form submission
validation to inform user of required fields.

TEST PLAN
---------
1) run Home -> Reports -> Catalog -> Submit
   -- no meaningful output and no error messages.
2) apply patch
3) repeat 1
   -- meaningful error messages appear.
   -- headings are in red, indicative of required values.
4) try other combinations of row or column selected/unselected.
   -- meaningful error messages.
5) run koha qa test tools

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 26 Owen Leonard 2017-04-25 12:10:13 UTC
Created attachment 62659 [details] [review]
[SIGNED-OFF] Bug 3311: Followup clean up HTML on page.

There should be no validator.w3.org validation issues
for this page's source.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 27 Jonathan Druart 2017-04-26 19:03:26 UTC
Any good reasons to have the javascript code in the middle of the page?
Comment 28 M. Tompsett 2017-04-26 22:58:09 UTC
(In reply to Jonathan Druart from comment #27)
> Any good reasons to have the javascript code in the middle of the page?

This is my understanding: the script needs to be after #configure_report, #LineError, #ColumnError in order for the error messages to appear in the correct place: the column headers. I put it there, so as to ensure that it would function.

I did try in the document ready with the other javascript, but it failed to work properly. Its current position works. I have also tested with it placed at the end (above the last include line), if you would prefer that.
Comment 29 Owen Leonard 2017-04-27 13:27:07 UTC
Created attachment 62776 [details] [review]
Bug 3311 [Follow-up] Statistical reports should error if row and column aren't selected

This patch moves the validation JavaScript from the body of the page to
the <head>. The "validated" class is removed from the form so that there
is not a conflict between default error handling and the custom
validation added to the page.

To test, apply the patch and verify that form validation still enforces
a selection of both row and column and that the error messages display
in the table header.
Comment 30 Owen Leonard 2017-04-27 13:31:34 UTC
M. Tompsett: The script wasn't working at the top of the page because it was being overridden by the default validation. The position of your JS was fixing that problem by executing later.

By removing the "validated" class from the form your JS is able to be responsible for all the validation and still be placed correctly.
Comment 31 M. Tompsett 2017-04-27 14:11:04 UTC
Created attachment 62777 [details] [review]
[SIGNED OFF] Bug 3311 [Follow-up] Statistical reports should error if row and column aren't selected

This patch moves the validation JavaScript from the body of the page to
the <head>. The "validated" class is removed from the form so that there
is not a conflict between default error handling and the custom
validation added to the page.

To test, apply the patch and verify that form validation still enforces
a selection of both row and column and that the error messages display
in the table header.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 32 M. Tompsett 2017-04-27 14:12:52 UTC
Thanks for the follow up, Owen!
Comment #25 is the test plan.
This should meet your standards now, JD.
Comment 33 Jonathan Druart 2017-04-27 14:22:25 UTC
(In reply to M. Tompsett from comment #32)
> This should meet your standards now, JD.

s/your/our

I will give it another try today or tomorrow.
Comment 34 Jonathan Druart 2017-04-27 16:12:10 UTC
ok, let's assume it's the way to go.
What about other pages (eg. reports/borrowers_stats.pl, reports/issues_stats.pl)?
The title of the bug report says "stat reports", I guess we are expecting to see all of them fixed.
Comment 35 M. Tompsett 2017-04-27 16:40:40 UTC
(In reply to Jonathan Druart from comment #34)
> What about other pages (eg. reports/borrowers_stats.pl,
> reports/issues_stats.pl)?
> The title of the bug report says "stat reports", I guess we are expecting to
> see all of them fixed.

*facepalm* You're right.
borrower_stats.pl
issues_stats.pl
The others with Row and Column have default selections.
Changing to assigned.
Comment 36 M. Tompsett 2017-04-27 18:38:48 UTC
Created attachment 62793 [details] [review]
Bug 3311: Form validation feedback

By using the validated class, make use of jquery form submission
validation to inform user of required fields.

TEST PLAN
---------
1) run Home -> Reports -> Catalog -> Submit
   -- no meaningful output and no error messages.
2) apply patch
3) repeat 1
   -- meaningful error messages appear.
   -- headings are in red, indicative of required values.
4) try other combinations of row or column selected/unselected.
   -- meaningful error messages.
5) run koha qa test tools

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 37 M. Tompsett 2017-04-27 18:38:55 UTC
Created attachment 62794 [details] [review]
Bug 3311: Followup clean up HTML on page.

There should be no validator.w3.org validation issues
for this page's source.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 38 M. Tompsett 2017-04-27 18:39:02 UTC
Created attachment 62795 [details] [review]
[SIGNED OFF] Bug 3311 [Follow-up] Statistical reports should error if row and column aren't selected

This patch moves the validation JavaScript from the body of the page to
the <head>. The "validated" class is removed from the form so that there
is not a conflict between default error handling and the custom
validation added to the page.

To test, apply the patch and verify that form validation still enforces
a selection of both row and column and that the error messages display
in the table header.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 39 M. Tompsett 2017-04-27 18:39:08 UTC
Created attachment 62796 [details] [review]
Bug 3311: Create common Row Columns Require javascript
Comment 40 M. Tompsett 2017-04-27 18:39:14 UTC
Created attachment 62797 [details] [review]
Bug 3311: catalogue stats use js version
Comment 41 M. Tompsett 2017-04-27 18:39:19 UTC
Created attachment 62798 [details] [review]
Bug 3311: Borrowers Stats warnings added
Comment 42 M. Tompsett 2017-04-27 18:39:24 UTC
Created attachment 62799 [details] [review]
Bug 3311: Borrower stats HTML clean up.
Comment 43 M. Tompsett 2017-04-27 18:39:32 UTC
Created attachment 62800 [details] [review]
Bug 3311: borrower_stats indentation issues
Comment 44 M. Tompsett 2017-04-27 18:39:37 UTC
Created attachment 62801 [details] [review]
Bug 3311: Issues stat report row and column required
Comment 45 M. Tompsett 2017-04-27 18:39:43 UTC
Created attachment 62802 [details] [review]
Bug 3311: Issues stat report HTML clean up.
Comment 46 M. Tompsett 2017-04-27 18:48:11 UTC
TEST PLAN
---------
1) run Home -> Reports -> Catalog -> Submit
   -- no meaningful output and no error messages.
2) apply patch
3) repeat 1
   -- meaningful error messages appear.
   -- headings are in red, indicative of required values.
4) try other combinations of row or column selected/unselected.
   -- meaningful error messages.
5) repeat steps 1-4, except for Home -> Reports -> Patrons -> Submit
6) repeat steps 1-4, except for Home -> Reports -> Circulation -> Submit
7) run koha qa test tools