---- 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
I don't think this bug is valid for recent versions of Koha. Changing to resolved-invalid.
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.
Created attachment 52775 [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
Created attachment 52776 [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
Created attachment 52777 [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
Created attachment 52778 [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
Created attachment 52779 [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
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>
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>
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?
(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.
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.
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.
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
(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?
(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.
(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.
(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?
(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.
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
Created attachment 62541 [details] [review] Bug 3311: Followup clean up HTML on page.
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
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
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.
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>
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>
Any good reasons to have the javascript code in the middle of the page?
(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.
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.
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.
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>
Thanks for the follow up, Owen! Comment #25 is the test plan. This should meet your standards now, JD.
(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.
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.
(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.
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>
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>
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>
Created attachment 62796 [details] [review] Bug 3311: Create common Row Columns Require javascript
Created attachment 62797 [details] [review] Bug 3311: catalogue stats use js version
Created attachment 62798 [details] [review] Bug 3311: Borrowers Stats warnings added
Created attachment 62799 [details] [review] Bug 3311: Borrower stats HTML clean up.
Created attachment 62800 [details] [review] Bug 3311: borrower_stats indentation issues
Created attachment 62801 [details] [review] Bug 3311: Issues stat report row and column required
Created attachment 62802 [details] [review] Bug 3311: Issues stat report HTML clean up.
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
While testing this I noticed that the Acquisitions and Holds statistics have column and row preselected - could this be a more elegant way of solving this problem? What I don't like about this patch is the changes to the pull downs. Instead of empty for 'non-filtered' it shows a text now. Quickly scanning the form, this makes it much harder to see, what was actually filtered on (lots of text). This is not part of the problem description and should be dealt with and discussed separately. - <option value=""> </option> + <option value="">Not filtered by holding library</option> Sorry, not signing off. Not sure which status to set - in Discussion?
(In reply to Katrin Fischer from comment #47) > While testing this I noticed that the Acquisitions and Holds statistics have > column and row preselected - could this be a more elegant way of solving > this problem? It is a "less is more" type solution. Though, someone would need to specify what row and columns are picked by default. And since the original report says, "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." It says nothing of defaults, hence this solution. > What I don't like about this patch is the changes to the pull downs. Instead > of empty for 'non-filtered' it shows a text now. Quickly scanning the form, > this makes it much harder to see, what was actually filtered on (lots of > text). This is not part of the problem description and should be dealt with > and discussed separately. True, I creeped the scope to include fixing the HTML. The HTML as it is triggered validation errors. Owen had suggested his preference would be some text describing what the actual blank value means. I can provide another patch to reset those parts. > Sorry, not signing off. Not sure which status to set - in Discussion? I suppose.
I'd give it another try with the empty default for the pull downs. But not sure what others think about this.
*** This bug has been marked as a duplicate of bug 21005 ***