Add saved-reports support, similar to saved-queries

RESOLVED FIXED in Bugzilla 4.4

Status

()

Bugzilla
Reporting/Charting
P1
enhancement
RESOLVED FIXED
12 years ago
5 years ago

People

(Reporter: Albert Ting, Assigned: Julien Heyman)

Tracking

2.21
Bugzilla 4.4
Dependency tree / graph
Bug Flags:
approval +
blocking3.2 -
blocking3.1.3 -
blocking3.0 -
documentation ?
testcase ?

Details

Attachments

(1 attachment, 10 obsolete attachments)

(Reporter)

Description

12 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.12) Gecko/20050915 Firefox/1.0.7
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.12) Gecko/20050915 Firefox/1.0.7

It would be great if Bugzilla could offer the ability to save a report, similar to saved queries.  And I think there's a ticket that discusses shared queries, something that would also be good for reports.  

This may be a dup, but I couldn't find a ticket.  I saw bug 290360, but that addresses streamlining the URL. 

Reproducible: Always

Steps to Reproduce:

Comment 1

12 years ago
I have a client that wants this, so I'm going to work on it.
Assignee: gerv → mkanat
Status: UNCONFIRMED → NEW
Ever confirmed: true
Target Milestone: --- → Bugzilla 3.0
Version: unspecified → 2.21

Updated

12 years ago
Group: webtools-security
Target Milestone: Bugzilla 3.0 → Bugzilla 2.24

Comment 2

12 years ago
Looks like Firefox did some strange selecting of the group radio boxes on the mass-change page when I clicked refresh in my browser before that last mass-change. This is not really a security bug. :-)
Group: webtools-security

Comment 3

11 years ago
Max, any progress on this bug? I'm tired to bookmark reports. ;)

Comment 4

11 years ago
Created attachment 235675 [details] [diff] [review]
Monolithic CNET Patch

Yes, actually! I have copyright release from CNET, who are quite happy to be contributing code to the Bugzilla Project. :-)

This is a monolithic patch that contains all the code that CNET will be contributing to the Bugzilla Project. Somewhere within here is Saved Reports, so I just need to separate out all the code for that part of the patch.

Updated

11 years ago
Whiteboard: Is this patch being reviewed/considered?

Comment 5

11 years ago
could we avoid spamming developers with useless comments? This patch is clearly not a patch "ready to go".
Whiteboard: Is this patch being reviewed/considered?

Comment 6

11 years ago
Created attachment 236445 [details] [diff] [review]
Saved Reports: 2.22 Parts to Port Forward

This is saved and shared reports both, for 2.22, but only the parts that need to be ported forward to 3.0. Some parts were left out, like the addition of namedqueries.id, since that already exists in 3.0.
Attachment #235675 - Attachment is obsolete: true

Comment 7

11 years ago
Created attachment 236464 [details] [diff] [review]
Beginnings of Report.pm for Bugzilla 3.0

And here's the beginnings of what this code will look like. Large parts of it don't work, but new() and create() should work.

The most important part of this patch is the modifications to Bugzilla::Object to allow it to take arbirary WHERE conditions, but *only* from its subclasses. We don't want people getting all fancy inside of CGIs and making a lot of messy code with WHERE conditions in places that should just have simple subroutine calls.

Updated

11 years ago
Depends on: 351098

Comment 8

11 years ago
Created attachment 238056 [details] [diff] [review]
Work In Progress 2

Here's a bit more work done on it. I've realized I need an object for Saved Searches before I can continue.
Attachment #236464 - Attachment is obsolete: true

Updated

11 years ago
Depends on: 352403

Comment 9

11 years ago
We want 3.0 to definitely have this, per our most recent Meeting.
Flags: blocking3.0+

Comment 10

11 years ago
See also bug 171529 ? 
(Is this a duplicate? But, I don't know which should be duplicate from which)

Updated

11 years ago
Depends on: 360028

Comment 11

11 years ago
*** Bug 171529 has been marked as a duplicate of this bug. ***

Comment 12

11 years ago
Created attachment 245618 [details] [diff] [review]
v1

Okay, this seems to work.

Boy, that was a lot of work.

This doesn't include Shared Reports, only Saved Reports. Do we want to have Shared Reports in 3.0, also?
Attachment #236445 - Attachment is obsolete: true
Attachment #238056 - Attachment is obsolete: true
Attachment #245618 - Flags: review?(LpSolit)

Comment 13

11 years ago
Comment on attachment 245618 [details] [diff] [review]
v1

>Index: buglist.cgi

>+            # Do not delete it if it's being used by a saved report
>+            my $reports_in_use = $dbh->selectcol_arrayref(
>+                'SELECT id FROM named_report WHERE named_query_id = ?',
>+                undef, $query_id) || [];
>+            if (@$reports_in_use) {
>+                ThrowUserError('saved_search_used_by_reports',

This doesn't make sense to me. Reports and saved queries should be independent, i.e. I don't want to see queries used by reports in my Saved Searches list in userprefs.cgi.



>Index: report.cgi

>+} elsif ($action eq "namedreport") {

>+    my $report = new Bugzilla::Report($id);
>+    if (!defined ($report)) {
>+        ThrowUserError('report_doesnt_exist');
>+    }

You don't check that the report ID is one of mine. This means I could load any report made by anyone, right? Which is probably not what we want, at least not now (especially because I could get some confidential data this way, especially if boolean charts are in use).


>+} elsif ($action eq "deletenamedreport") {

>+    my $report = new Bugzilla::Report($id);
>+    if (!defined $report) {
>+        ThrowUserError('report_doesnt_exist');
>+    }
>+    $report->remove_from_db;

I suppose I can delete all reports I want, including reports made by other users?



>Index: Bugzilla/Object.pm

>+        elsif (defined $param->{'condition'} && defined $param->{'values'}) {
>+            caller->isa('Bugzilla::Object')
>+                || ThrowCodeError('protection_violation',

I really don't like that. If new() is such complex that it requires a special condition, then you should write your own new() method in the caller. I see no interest in having the condition passed here.


>+sub check_boolean { return $_[1] ? 1 : 0; }
>+sub check_natural { return detaint_natural($_[1]); }
>+sub check_string  { return defined $_[1] ? $_[1] : ''; }

Note sure these methods make sense. Note that detaint_natural doesn't return the detainted number, but true or false.



>Index: Bugzilla/Report.pm

>+sub remove_from_db {
>+    my $self = shift;
>+    Bugzilla->dbh->do("DELETE FROM named_report WHERE id = ?",
>+                      undef, $self->id);
>+}

Deleting a report doesn't remove its query from namedqueries. This makes PostgreSQL to crash the next time you try to add a new report with the same name (after deleting the current one):

DBD::Pg::db do failed: ERREUR:  une clé dupliquée rompt la contrainte unique «namedqueries_userid_idx»
 [for Statement "INSERT INTO namedqueries (userid, query_type, query, name) VALUES (?,?,?,?)"] at Bugzilla/Object.pm line 265
	Bugzilla::Object::insert_create_data('Bugzilla::Search::Saved', 'HASH(0x8c41438)') called at Bugzilla/Search/Saved.pm line 108
	Bugzilla::Search::Saved::create('Bugzilla::Search::Saved', 'HASH(0x8c4130c)') called at Bugzilla/Report.pm line 153
	Bugzilla::Report::create('Bugzilla::Report', 'HASH(0x8c4184c)') called at /var/www/html/bugzilla-pg/report.cgi line 76


>+sub search {
>+    my ($self) = @_;
>+    return $self->{search} if defined $self->{search};
>+    $self->{search} = new Bugzilla::Search::Saved($self->{named_query_id});

Is that the only reason you create one saved search per report?



>Index: Bugzilla/DB/Schema.pm

>+    named_report => {
>+        FIELDS => [
>+            id              => {TYPE => 'MEDIUMSERIAL', NOTNULL => 1,
>+                                PRIMARYKEY => 1},
>+            user_id         => {TYPE => 'INT3', NOTNULL => 1},
>+            name            => {TYPE => 'varchar(64)', NOTNULL => 1},
>+            link_in_footer  => {TYPE => 'BOOLEAN', NOTNULL => 1, 
>+                                DEFAULT => '0'},
>+            x_axis_id       => {TYPE => 'INT3', DEFAULT => 'NULL'},
>+            y_axis_id       => {TYPE => 'INT3', DEFAULT => 'NULL'},
>+            z_axis_id       => {TYPE => 'INT3', DEFAULT => 'NULL'},
>+            format          => {TYPE => 'TINYTEXT', NOTNULL => 1},
>+            action          => {TYPE => 'TINYTEXT', NOTNULL => 1},
>+            named_query_id  => {TYPE => 'INT3', NOTNULL => 1},
>+        ],

Do we really need all these columns? Why not "name, user ID, query, link_in_footer" only? (name, user ID) is unique and query would contain the query itself, including axis and format. I need some explanation here.



>Index: template/en/default/reports/report.html.tmpl

There should be a link to delete a report from here.
Attachment #245618 - Flags: review?(LpSolit) → review-
(Reporter)

Comment 14

11 years ago
(In reply to comment #12)
> This doesn't include Shared Reports, only Saved Reports. Do we want to have
> Shared Reports in 3.0, also?

Perhaps in another ticket, but I'd like to see shared reports supported in 3.0.

Comment 15

11 years ago
(In reply to comment #14)
> Perhaps in another ticket, but I'd like to see shared reports supported in 3.0.

  At this point that's up to justdave. I only marked this bug as a blocker.

  The original customizations I did for CNET had Shared Reports, and I know that they'd also like to see them in 3.0.

Comment 16

11 years ago
(In reply to comment #14)
> Perhaps in another ticket, but I'd like to see shared reports supported in 3.0.

Yeah, do it in a separate bug. But the trunk is frozen already, so shared reports wouldn't go into 3.0. I don't want to delay 2.23.4 and 3.0 RC1 any longer.
Flags: blocking3.0+ → blocking3.0-
Target Milestone: Bugzilla 3.0 → Bugzilla 3.2

Updated

11 years ago
Whiteboard: [roadmap: 3.2]

Comment 17

10 years ago
Max, any chance to have it for 3.2?

Comment 18

10 years ago
Oh, I forgot that we hadn't even done saved reports yet. :-) Yeah, we can get this for 3.2. I'll mark it a blocker so I don't forget.
Flags: blocking3.1.3+
Priority: -- → P1

Comment 19

10 years ago
Okay, this isn't really a 3.1.3 blocker--I'm willing to release 3.1.3 without this.
Flags: blocking3.1.3+ → blocking3.1.3-

Comment 20

10 years ago
Bugzilla 3.2 is now frozen. Only enhancements blocking 3.2 or specifically approved for 3.2 may be checked in to the 3.2 branch. If you would like to nominate your enhancement for Bugzilla 3.2, set the "blocking3.2" flag to "?", and either the target milestone will be changed back, or the blocking3.2 flag will be granted, if we will accept this enhancement for Bugzilla 3.2.
Target Milestone: Bugzilla 3.2 → Bugzilla 4.0

Comment 21

10 years ago
is this not going to be included in 3.2?
Mantis already has this... This should be consider a blocker for 3.2
Flags: blocking3.2?

Comment 22

10 years ago
No, the code is frozen to prepare 3.2, and required changes are too heavy to be taken now.

Updated

10 years ago
Flags: blocking3.2? → blocking3.2-

Comment 23

10 years ago
Frederic - Thanks for update. 
Why do you not consider this a key feature?

Updated

10 years ago
Flags: blocking3.2- → blocking3.2?

Comment 24

10 years ago
(In reply to comment #23)
> Why do you not consider this a key feature?

Please do not overwrite our flag settings. As I said, the code is *already* frozen to prepare 3.2. There has been no traction on this bug recently due to other more important Bugzilla work done by Max. This will be taken for Bugzilla 4.0, but it's not definitely *too late* to be reconsidered for 3.2. So it's a hard blocking-.
Flags: blocking3.2? → blocking3.2-

Comment 25

10 years ago
(In reply to comment #24)
> 4.0, but it's not definitely *too late* to be reconsidered for 3.2.

Argh.... Remove "not" from the sentence: "it's definitely too late"!

Comment 26

10 years ago
(In reply to comment #23)
> Frederic - Thanks for update. 
> Why do you not consider this a key feature?
> 

How to win Bugzilla friends and influence Bugzilla people: Participate in the development, review and QA processes where possible.  We always need testers.  Please be aware that as with any release process, we can only prepare so much at one time to make a timely release.  This feature simply didn't make it in time and it's not worth holding up 3.2 for it.  As LpSolit said, we're unable to consider this for 3.2.  It is likely to land in 4.0, however.

For more information on partiicpating in the development/test/qa/release process, please contact us on irc://irc.mozilla.org#mozwebtools or in the developers mailing list.

As LpSolit also said, changing a flag back to ? after a decision has been made generally isn't a good idea unless you have a *really* good reason to do so and can back it up (like you can demonstrate that it was an oops and not intentional).

Comment 27

9 years ago
Will this also allow to add these reports to the Whining configuration so these reports can be e-mailed on a regular basis?

Comment 28

9 years ago
Created attachment 361517 [details] [diff] [review]
Alternate patch idea (partial)

I've been looking at this, and the saved searches contains a 'query_type' field.  Perhaps by adding a #2 type to reference reports, much of the redundant code and database information can be obsoleted.

Thoughts?  I suspect my method will be more dirty, but quicker.  Without much expertise, I'd like to think there will be no lost functionality.

Please note that this patch only allows 'displaying' saved reports, not maintaining them or separating them from tables.  So in no way is this intended to be an actual patch, just the beginnings of a new direction.

Comment 29

9 years ago
Created attachment 361567 [details] [diff] [review]
Alternate patch idea (complete) based on 3.0

How is this for an alternate idea?  it gets the functionality in the system.

The only piece missing is separating searches from reports in the footer and preferences pages.  please let me know if I should bother adding that and basing it on 3.2.  I've added this particular version to my local Bugzilla for now...
Attachment #361517 - Attachment is obsolete: true
Attachment #361567 - Flags: review?(LpSolit)

Comment 30

9 years ago
one bug, if you edit the report, you cannot save the modified report.

Updated

9 years ago
Whiteboard: [roadmap: 3.2]

Comment 31

9 years ago
Comment on attachment 361567 [details] [diff] [review]
Alternate patch idea (complete) based on 3.0

I'm unable to apply your patch because you made diffs from different directories. All diffs should be made from the bugzilla/ root directory.

Also, in order to have a chance to get a r+ (and based on the limited free time of reviewers), it must be written against Bugzilla 3.5, not 3.0.
Attachment #361567 - Flags: review?(LpSolit) → review-

Updated

8 years ago
Whiteboard: [needs new patch]

Updated

8 years ago
Duplicate of this bug: 290360

Updated

8 years ago
Assignee: mkanat → charting
(Assignee)

Comment 33

6 years ago
Hello,

It's damage that this feature is not yet committed.

What can I do to include it in the next release?

Comment 34

6 years ago
(In reply to Julien Heyman from comment #33)
> 
> What can I do to include it in the next release?

The fastest way to do this is probably to submit a patch.
See https://wiki.mozilla.org/Bugzilla:Developers and http://www.bugzilla.org/docs/developer.html to learn how to contribute.
(Assignee)

Comment 35

6 years ago
I know... but make a new patch, or update this https://bugzilla.mozilla.org/attachment.cgi?id=361567 is sufficient ?

Comment 36

6 years ago
(In reply to Julien Heyman from comment #35)
> I know... but make a new patch, or update this
> https://bugzilla.mozilla.org/attachment.cgi?id=361567 is sufficient ?

I guess updating would be a good start.  But see remarks of comment 31.
Julien, would you like to work on this one ?
(Assignee)

Comment 37

6 years ago
Created attachment 569082 [details] [diff] [review]
Save report patch

:-(, I'm sorry, I just see now your last comment (number 36).

It's my first draft. I haven't write documentation (Report.pm class), because my patch is maybe bad. I would like write documentation only after the patch looks good in the main.

I have create a new table (reports) and a new class.

I see in https://bugzilla.mozilla.org/attachment.cgi?id=361567 you store report in namedqueries table. Why not.

Tell me if I continue on my patch, or if I update this patch https://bugzilla.mozilla.org/attachment.cgi?id=361567

Julien.

Comment 38

6 years ago
Julien,

you'll have to set the review flag on your attachment :
https://wiki.mozilla.org/Bugzilla:Review

Otherwise, probably it will not be noticed by the reviewers.

And once the patch is in your radar, they can answer on your question in comment 37 .
(Assignee)

Updated

6 years ago
Attachment #569082 - Flags: review?(mkanat)
Attachment #569082 - Flags: review?(justdave)
Attachment #569082 - Flags: review?(LpSolit)

Updated

6 years ago
Attachment #361567 - Attachment is obsolete: true

Comment 39

6 years ago
(In reply to Julien Heyman from comment #37)
> It's my first draft. I haven't write documentation (Report.pm class),
> because my patch is maybe bad. I would like write documentation only after
> the patch looks good in the main.

This is reasonable. No problem, you can write the doc later. :)


> I have create a new table (reports) and a new class.
> 
> I see in https://bugzilla.mozilla.org/attachment.cgi?id=361567 you store
> report in namedqueries table. Why not.

Your patch is an update of attachment 245618 [details] [diff] [review], isn't it? It also uses a new class and DB table, which makes more sense than reusing the namedqueries table.



> Tell me if I continue on my patch, or if I update this patch
> https://bugzilla.mozilla.org/attachment.cgi?id=361567

Forget attachment 361567 [details] [diff] [review].


I'm assigning this bug to you as you are working on it. Thanks for taking it! :)
Assignee: charting → jheyman
Status: NEW → ASSIGNED
Whiteboard: [needs new patch]
(Assignee)

Comment 40

6 years ago
In fact, we already had this feature in 3.0 but I am not the author. I only port this patch on 4.0. And in the same time, I think it's better for everyone to include this patch in community.

Could you review it ?

Comment 41

6 years ago
(In reply to Julien Heyman from comment #40)
> Could you review it ?

I will once I have some time (this may take a few days, though). I have some higher priority bugs to fix first. :)

Comment 42

6 years ago
Comment on attachment 569082 [details] [diff] [review]
Save report patch

I'm finally done with all the blockers for Bugzilla 4.2, so I can now look at your patch. :)
I haven't looked at your patch very carefully yet, but I have applied it to my test installation, and it seems to work fine so far. Below are my initial comments.


>=== modified file 'Bugzilla/DB/Schema.pm'

>+    reports => {
>+        FIELDS => [
>+            id           => {TYPE => 'MEDIUMSERIAL', NOTNULL => 1,
>+                             PRIMARYKEY => 1},
>+            userid       => {TYPE => 'INT3', NOTNULL => 1},

Add a foreign key pointing to profiles.userid. Also, use user_id for this table rather than userid. (We have a mix of userid and user_id for historical reasons, but we tend to use user_id now.)


>+            name         => {TYPE => 'varchar(64)', NOTNULL => 1},
>+            query        => {TYPE => 'LONGTEXT', NOTNULL => 1},

No need to add so much indentation. Move => closer to the hash keys.


>+        ],
>+        INDEXES => [
>+            reports_userid_idx => {FIELDS => [qw(userid name)],
>+                                        TYPE => 'UNIQUE'},

The indentation of the last line is incorrect. Align TYPE with FIELDS.



>=== added file 'Bugzilla/Report.pm'

Please add the MPL 2.0 license to this file.
Also, you don't use this module from report.cgi.


>+use Bugzilla::User;

No need to load Bugzilla::User. You don't use it in this module.


>+sub _check_name {
>+    my ($invocant, $name) = @_;
>+    $name = trim($name);

Even better: use clean_text() instead of trim().
Also, you must make sure that the name doesn't already exist when saving a new report.


>+sub _check_userid {
>+    my ($invocant, $userid) = @_;
>+    Bugzilla->login(LOGIN_REQUIRED);

Do not call ->login() here. Simply make sure that $user_id (instead of $userid) is true (i.e. > 0).
Probably better is to pass a user object rather than a user ID.

>+    return Bugzilla->user->id;

What's the point to pass $userid to this method if you ignore it?


Do not forget to add POD to this module.



>=== modified file 'Bugzilla/User.pm'

>+sub reports {

Add POD for this new method.



>=== modified file 'report.cgi'

>+if ($action eq 'add') {

Should be elsif as there is already an if () previously.


>+    my $report_name = trim($cgi->param('name') || '');
>+    my $report = trim($cgi->param('query') || '');
>+
>+    trick_taint($report_name);
>+    trick_taint($report);
>+
>+    if ($report_name eq '') {
>+        ThrowUserError("report_name_missing")
>+    }
>+
>+    if ($report eq '') {
>+        ThrowUserError("missing_report")
>+    }

Leave all these sanity checks for Bugzilla::Report.


>+    my @reports = @{$dbh->selectcol_arrayref('SELECT name FROM reports WHERE userid = ' . $user->id)};

Do not run SQL queries from here. That's the job of Bugzilla::Report.


>+$vars->{'url'} = $cgi->url(-relative=>1, ,-query=>1);

What is this parameter used for?



>=== modified file 'template/en/default/global/useful-links.html.tmpl'

>+      <div class="label">
>+        Saved Reports :
>+      </div>
>+      <ul class="links">
>+        [% FOREACH r = user.reports %]

I think it would be better to have saved reports on the same line as the label.


>+          <a href="[% r.query FILTER none %]

The query must be HTML filtered.


>+         [% IF saved %]
>+           <a href="report.cgi?action=del&name=[% saved FILTER none %]">Forget this report</a>

The report name must be HTML filtered. Never use FILTER none for untrusted user data.
Attachment #569082 - Flags: review?(mkanat)
Attachment #569082 - Flags: review?(justdave)
Attachment #569082 - Flags: review?(LpSolit)
Attachment #569082 - Flags: review-

Updated

6 years ago
Attachment #245618 - Attachment is obsolete: true
(Assignee)

Comment 43

6 years ago
> Also, you must make sure that the name doesn't already exist when saving a
> new report.
Yes, I check it, and if the name report already exist for this user, it update the saved report.

I attach a new patch integrating your comments.
(Assignee)

Comment 44

6 years ago
Created attachment 623059 [details] [diff] [review]
Save report patch
Attachment #569082 - Attachment is obsolete: true
Attachment #623059 - Flags: review?(LpSolit)

Comment 45

6 years ago
Comment on attachment 623059 [details] [diff] [review]
Save report patch

>=== modified file 'Bugzilla/DB/Schema.pm'

>+            reports_userid_idx => {FIELDS => [qw(user_id name)],

The index should be named reports_user_id_idx as you renamed the userid column to user_id.



>=== added file 'Bugzilla/Report.pm'

Add these lines so that reports are not logged into the audit_log table:

# Do not track reports saved by users.
use constant AUDIT_CREATES => 0;
use constant AUDIT_UPDATES => 0;
use constant AUDIT_REMOVES => 0;


>+use constant UPDATE_COLUMNS => qw(
>+    query
>+);

It should be possible to rename reports, even if the UI doesn't let you do it yet.


>+use constant REQUIRED_CREATE_FIELDS => qw(user_id name query);

This constant is no longer used. Bugzilla::Object->check_required_create_fields() gets the data for us from the DB schema itself.


>+    user_id => \&_check_user_id,

For now, we should behave the same way as saved searches, i.e. only allow the logged in user to create saved reports for himself. This means that we don't need _check_user_id, and create() will look at Bugzilla->user->id to get the user ID. This will make our life easier for now.


>+sub _check_name {

I know this code is copied from Bugzilla/Search/Saved.pm, but it would be fine to check if the name is already in use from here, as we do e.g. from Product.pm. But I suppose this can be done in a separate bug.


>+sub _check_user_id {

Remove this validator. We will take the user ID from Bugzilla->user->id, as suggested above.


>+    my $report = new Bugzilla::Report(
>+        { name => $name, query => $query, user_id => $user->id });

This example is wrong. You can only create a Bugzilla::Report object using the report ID. Bugzilla::Object->new() won't understand what query and user_id are.


>+    my $report = Bugzilla::Report->create(
>+        { name => $name, query => $query, user_id => $user->id });

Remove the user_id argument, for the reasons above.



>=== modified file 'report.cgi'

>+} elsif ($action eq 'add') {

>+    my $name = $cgi->param('name');

You have to call clean_text() on it, else you could bypass the check e.g. by appending a whitespace to the name.


>+        my $report = Bugzilla::Report->check({name => $name});

This won't work. Several reports from different users can have the exact same name. You have to pass the report ID instead.


>+        my $report = Bugzilla::Report->create({name => $name, query => $query, user_id => $user->id});

Remove the user_id argument.


>+} elsif ($action eq 'del') {

>+    my $report = Bugzilla::Report->check({name => $cgi->param('name')});

Pass the report ID to check().



>=== modified file 'template/en/default/global/messages.html.tmpl'

>+  [% ELSIF message_tag == "query_new_saved_report" %]
>+  [% ELSIF message_tag == "report_gone" %]
>+  [% ELSIF message_tag == "update_saved_report" %]

All messages about reports should start with report_. Maybe report_created, report_deleted and report_updated, respectively.



>=== modified file 'template/en/default/global/user-error.html.tmpl'

>+  [% ELSIF error == "missing_report" %]

This error message is unused. It should go away.



>=== modified file 'template/en/default/reports/report.html.tmpl'

>+       </td>
>+      <td>&nbsp;</td>
>+       <td>

Fix the indentation.


>+         [% IF saved %]
>+           <a href="report.cgi?action=del&name=[% saved FILTER html %]">Forget this report</a>

You have to generate and append a token to the URL to prevent CSRF. Something like &amp;token=[% issue_hash_token(['delete_report', saved]) FILTER html %] should do it. Of course, this means that report.cgi must check this token thanks to check_hash_token($token, ...) before deleting the report.



>+         [% ELSE %]
>+           <form method="get" action="report.cgi">

Same here. You must store a token in a hidden field to avoid CSRF. And add the corresponding check in report.cgi.


>+             <input type="text" id="name" name="name" size="20" value=""> 

Also add maxlength=64 as it's the largest string allowed.


Your patch looks better and better, and is already working pretty well. :)
Attachment #623059 - Flags: review?(LpSolit) → review-
(Assignee)

Comment 46

6 years ago
Created attachment 632185 [details] [diff] [review]
Save report patch

Hi,

I send you a new patch integrating your comments.
Attachment #623059 - Attachment is obsolete: true
Attachment #632185 - Flags: review?(LpSolit)

Comment 47

5 years ago
Comment on attachment 632185 [details] [diff] [review]
Save report patch

>=== added file 'Bugzilla/Report.pm'

>+sub name { return $_[0]->{'name'}; }
>+sub id { return $_[0]->{'id'}; }

No need to define these methods here. Bugzilla::Report inherits them from Bugzilla::Object automatically.


>+    my $report = Bugzilla::Report->check({id => $id);

Missing }.



>=== modified file 'report.cgi'

>+    if (my ($reports) = grep($_->name eq $name, @{$user->reports})) {

We should do the comparison case-independent as MySQL is case-insensitive. So use lc(...) eq lc(...).


>+        my $report = Bugzilla::Report->check({id => $reports->id});

This check is useless as we got $reports from $user->reports already. So we already have a valid Bugzilla::Report object in hands.


>+    check_hash_token($token, ['delete_report']);

The report ID should be included to generate the token so that the token is unique for each report, which is much safer.


>+    $vars->{'reportname'} = $cgi->param('name');

There is no parameter named 'name'. This information can be accessed using $report->name.



>=== modified file 'template/en/default/global/useful-links.html.tmpl'

>+      <ul class="links">
>+      <span class="label">Saved Reports :</span>

You should shift both lines. <span> should be outside <ul>.


>+          <a href="report.cgi?[% r.query FILTER html %]&amp;saved_report_id=[% r.id FILTER uri %]">[% r.name FILTER html %]</a></li>

In a followup bug, we should change report.cgi to be able to extract the query from the report name instead of passing the whole URL here, as we do for saved searches.



>=== modified file 'template/en/default/global/user-error.html.tmpl'


>+  [% ELSIF error == "report_access_denied" %]
>+    [% title = "Access to this report denied" %]
>+    You can't access to this report.
>+
>   [% ELSIF error == "number_not_numeric" %]

The title should be: "Report Access Denied". Also: s/access to/access/.
Also, the error messages must be sorted alphabetically. Here "report_" is right before "number_".



>=== modified file 'template/en/default/reports/report.html.tmpl'

>+           <a href="report.cgi?action=del&amp;saved_report_id=[% saved_report_id FILTER html %]&amp;token=[% issue_hash_token(['delete_report']) FILTER html %]">Forget this report</a>

Use FILTER uri instead of FILTER html.
Also, the token should be generated using the report ID, as explained above.



I made a lot of comments above, but they are all easy to fix and your patch is working fine. So r=LpSolit, and I will fix them on checkin. Thanks a lot for your contribution! :)
Attachment #632185 - Flags: review?(LpSolit) → review+

Updated

5 years ago
Flags: testcase?
Flags: documentation?
Flags: approval+
Keywords: relnote

Comment 48

5 years ago
Created attachment 649824 [details] [diff] [review]
Save report patch (checked in)

Here is the patch I'm going to check in, with all comments addressed. I had to remove "Saved Reports" from global/useful-links.html.tmpl because the CSS code related to these labels is gone since Bugzilla 3.4, see bug 478173.
Attachment #632185 - Attachment is obsolete: true
Attachment #649824 - Flags: review+

Updated

5 years ago
Attachment #649824 - Attachment description: Save report patch → Save report patch (checked in)

Comment 49

5 years ago
There are several improvements which can be made, but we will implement them in separate bugs. Thanks again for your great contribution, Julien! :)

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/trunk/
modified report.cgi
added Bugzilla/Report.pm
modified Bugzilla/User.pm
modified Bugzilla/DB/Schema.pm
modified template/en/default/global/messages.html.tmpl
modified template/en/default/global/useful-links.html.tmpl
modified template/en/default/global/user-error.html.tmpl
modified template/en/default/reports/report.html.tmpl
Committed revision 8336.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Blocks: 781075

Comment 50

5 years ago
Added to relnotes for 4.4.
Keywords: relnote
You need to log in before you can comment on or make changes to this bug.