Description
Chris Cormack
2010-05-21 01:04:52 UTC
Created attachment 2366 [details] [review] Fixes Bug 2969: Report Name should be mandatory for saved reports 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. Created attachment 19795 [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. Created attachment 19814 [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. Created attachment 19821 [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> (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. 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. 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. Created attachment 31675 [details] [review] [SIGNED-OFF] 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> 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. 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 (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. Patch pushed to master. Thanks Owen! Created attachment 32927 [details]
edit form
This works great, but when you edit a report the required labels go away.
*** Bug 12213 has been marked as a duplicate of this bug. *** |