Bug 2969 - Report Name should be mandatory for saved reports
Summary: Report Name should be mandatory for saved reports
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Reports (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Owen Leonard
QA Contact: Bugs List
URL:
Keywords:
: 12213 (view as bug list)
Depends on:
Blocks:
 
Reported: 2009-02-19 08:04 UTC by Owen Leonard
Modified: 2015-12-03 22:11 UTC (History)
7 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:


Attachments
Fixes Bug 2969: Report Name should be mandatory for saved reports (3.08 KB, patch)
2010-07-15 11:30 UTC, Amit Gupta
Details | Diff | Splinter Review
Bug 2969 - Report Name should be mandatory for saved reports (5.34 KB, patch)
2013-07-19 14:04 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 2969 - Report Name should be mandatory for saved reports (5.42 KB, patch)
2013-07-19 23:02 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 2969 - Report Name should be mandatory for saved reports (5.88 KB, patch)
2013-07-20 21:25 UTC, Chris Cormack
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 2969 - Report Name should be mandatory for saved reports (5.91 KB, patch)
2014-09-17 13:01 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 2969 - Report Name should be mandatory for saved reports (5.94 KB, patch)
2014-09-18 14:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
edit form (58.42 KB, image/png)
2014-10-29 16:42 UTC, Nicole C. Engard
Details

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


---- Reported by oleonard@myacpl.org 2009-02-19 08:04:36 ----

I was able to save a Guided Report using the Create Report From SQL option with no report name. This results in a blank in the list of saved reports.



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

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

Actual time not defined. Setting to 0.0

Comment 1 Amit Gupta 2010-07-15 11:30:18 UTC Comment hidden (obsolete)
Comment 2 Owen Leonard 2010-08-17 13:46:54 UTC
I see a couple of problems with this patch: It makes a change to the database structure, but doesn't include a change to updatedatabase.pl. It also misses one case: It's missing form validation when creating a new report from SQL.
Comment 3 Owen Leonard 2013-07-19 14:04:27 UTC Comment hidden (obsolete)
Comment 4 Liz Rea 2013-07-19 23:02:32 UTC Comment hidden (obsolete)
Comment 5 Chris Cormack 2013-07-20 21:25:28 UTC Comment hidden (obsolete)
Comment 6 Galen Charlton 2013-07-22 15:37:34 UTC
(In reply to Chris Cormack from comment #5)
> However we are not checking this server side .. if this was the public
> interface that would be a fail. I will leave this up to the RM to
> decided if client side checks are ok for this. Bearing in mind we
> weren't handling any sql errors properly server side before this.

Yes, I'm willing to push it, in particular because the column is changing to not null default '', not just a plain 'not null', and because the consequences of somebody actively trying to save a report with a blank name are ... they now have a report with a blank name.

I suppose that raises one question: would it be better to tighten the screws a bit more and have it just be plain 'not null'?

Another question: any objections to have the database update set the report name to something like concat('report ', id) for reports where the name is blank or null?

Setting to in discussion, but I've no particular objection if Owen's response is simply to set it back to passed QA.
Comment 7 Owen Leonard 2013-07-23 12:51:03 UTC
I picked up this patch because it looked useful and wasn't getting a follow-up from the original author. I think Galen's suggestions would make this better, but someone else would have to take it from here.
Comment 8 Katrin Fischer 2013-09-26 13:03:26 UTC
I think client side checking is a good first step and we could open a new bug for the server side to also fix that. I think 'not null' would make sense, I am not so fond of the automatic 'report X' as this is another possible translation issue.
Comment 9 Owen Leonard 2014-09-17 13:01:55 UTC Comment hidden (obsolete)
Comment 10 Owen Leonard 2014-09-17 13:03:35 UTC
This was "in discussion" for a long time without much discussion. I have rebased the patch against current master and resubmitted it in it's signed-off state.

I think that while this patch is an incomplete solution it is nevertheless an improvement.
Comment 11 Jonathan Druart 2014-09-18 14:23:33 UTC
Created attachment 31717 [details] [review]
Bug 2969 - Report Name should be mandatory for saved reports

This patch re-implements the database changes from the original patch
and adds an update for existing reports. It also reimplements the
client-side validation using the new built-in validation plugin and adds
coverage for SQL reports.

I have also added the "required" attribute to the SQL report textarea
since saving a report with no SQL triggers an error.

To test:

- Create a new guided report. Confirm that you cannot save your guided
  report without a report name.
- Create or edit a saved SQL report. Confirm that you cannot save your
  report without a name or without SQL.
- Confirm that the saved_sql table structure has been updated.

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Functional tests pass, database tables look correct to me.

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Even with JS disabled, the form can not be submitted without the
report name filled in.
However we are not checking this server side .. if this was the public
interface that would be a fail. I will leave this up to the RM to
decided if client side checks are ok for this. Bearing in mind we
weren't handling any sql errors properly server side before this.

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Amended patch: description of the updatedb entry
Comment 12 Jonathan Druart 2014-09-18 14:25:06 UTC
(In reply to Owen Leonard from comment #10)
> I think that while this patch is an incomplete solution it is nevertheless
> an improvement.

I completely agree.
Marked as Passed QA.
Comment 13 Tomás Cohen Arazi 2014-10-24 16:24:34 UTC
Patch pushed to master.

Thanks Owen!
Comment 14 Nicole C. Engard 2014-10-29 16:42:51 UTC
Created attachment 32927 [details]
edit form

This works great, but when you edit a report the required labels go away.
Comment 15 Pablo AB 2015-05-06 15:46:18 UTC
*** Bug 12213 has been marked as a duplicate of this bug. ***