401957 - Move comment "isprivate" updating from process_bug to Bugzilla::Bug

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[Max Kanat-Alexander]

There's some code that validates and updates the "isprivate" settings for current comments, and it needs to be in Bugzilla::Bug instead.

Comment 1

[Max Kanat-Alexander]

Attachment: v1

I was also able to change it from using bug_when to using comment_id, which was nice.

I tested this, and it works.

Comment 2

[Frédéric Buclin]

Comment on attachment 286900 [details] [diff] [review]
v1

>diff -u process_bug.cgi process_bug.cgi

>+    # You can only mark/unmark comments as private on single bugs. If
>+    # you're not in the insider group, this code won't do anything.
>+    foreach my $field (grep(/^defined_isprivate/, $cgi->param())) {

I can still hack the URL when editing several bugs at once. You should make sure @idlist only has one element. Else the first bug of the list would be altered.



>diff -u Bugzilla/Bug.pm Bugzilla/Bug.pm

>+sub set_comment_is_private {

Wouldn't set_comment_privacy be a better name?


>+    return unless Bugzilla->user->in_group(Bugzilla->params->{insidergroup});

Use Bugzilla->user->is_insider instead.


>+    $self->{comment_isprivate}->{$comment_id} = $isprivate;

Please only set $self->{comment_isprivate}->{$comment_id} when the new value is different from the old one (you can get this information from $self->longdescs). Else you are going to do tens to hundreds useless UPDATE calls when editing bugs with many comments.



>diff -u template/en/default/bug/comments.html.tmpl 

>+	  <input type="hidden" value="1"
>+		 name="defined_isprivate_[% comment.id %]">

I'm not sure defined_isprivate_$id is really useful. We should store the number of comments and loop over 1..max($id) in process_bug.cgi.



>+++ template/en/default/global/user-error.html.tmpl

>+  [% ELSIF error == "comment_invalid_isprivate" %]
>+    You tried to make comment id [% id FILTER html %] private, but that
>+    is not a valid comment on this bug.

s/comment id/comment #/ ? Moreover: bug -> [% terms.bug %].

Comment 3

[Max Kanat-Alexander]

(In reply to comment #2)
> I can still hack the URL when editing several bugs at once. You should make
> sure @idlist only has one element. Else the first bug of the list would be
> altered.

  That's fine. You'd have to choose to make a comment on that bug private, the checker checks for that.

> Please only set $self->{comment_isprivate}->{$comment_id} when the new value is
> different from the old one (you can get this information from
> $self->longdescs). Else you are going to do tens to hundreds useless UPDATE
> calls when editing bugs with many comments.

  Okay, that makes sense.

> I'm not sure defined_isprivate_$id is really useful. We should store the number
> of comments and loop over 1..max($id) in process_bug.cgi.

  No, with this it means you don't have to pass in every single field on the page just to make a simple modification. That is, I'm trying to make it so that you can use process_bug.cgi without having to pass in a lot of "magical" arguments.

> >+  [% ELSIF error == "comment_invalid_isprivate" %]
> >+    You tried to make comment id [% id FILTER html %] private, but that
> >+    is not a valid comment on this bug.
> 
> s/comment id/comment #/ ? Moreover: bug -> [% terms.bug %].

  No, it's not the comment number, it really is the ID. It'll be something like 105423.

Comment 4

[Frédéric Buclin]

(In reply to comment #3)

>   No, with this it means you don't have to pass in every single field on the
> page just to make a simple modification.

Ah right. Forget my comment, then. :)


> > s/comment id/comment #/ ? Moreover: bug -> [% terms.bug %].
> 
>   No, it's not the comment number, it really is the ID. It'll be something like
> 105423.

Huh? This is the internal comment ID, isn't it? As the user will have no idea what this ID is, couldn't you reword the error message so that this ID is never displayed? Something like "You tried to change the privacy of a comment which doesn't belong to bug XXX". Also, if you try to mark an invalid comment ID as public, the error message would be wrong isn't it (private vs public)?

Comment 5

[Max Kanat-Alexander]

(In reply to comment #4)
> Huh? This is the internal comment ID, isn't it? As the user will have no idea
> what this ID is, couldn't you reword the error message so that this ID is never
> displayed? Something like "You tried to change the privacy of a comment which
> doesn't belong to bug XXX". Also, if you try to mark an invalid comment ID as
> public, the error message would be wrong isn't it (private vs public)?

  Yeah, I thought about everything but your last point there. I'll change that to "modify the privacy of".

  Users should never ever get this message unless they've been removed from the insidergroup while the bug was open. Otherwise, the only people who will get it are people hacking the URL for their own custom tools, and I want those people to be able to quickly find their error.

Comment 6

[Max Kanat-Alexander]

Attachment: v2

Okay, this fixes everything.

Comment 7

[Frédéric Buclin]

Comment on attachment 287463 [details] [diff] [review]
v2

>Index: Bugzilla/Bug.pm

>+sub set_comment_is_private {

>+    ThrowUserError('comment_invalid_isprivate', { id => $comment_id }) 
>+        if !$comment;

Nit: for readability, you could write this on a single line.



>Index: template/en/default/filterexceptions.pl

>+  'comment.time',

comment.time is no longer in use except in a single place where it's already filtered. This makes runtests.pl unhappy:

Failed Test    Stat Wstat Total Fail  Failed  List of Failed
---------------------------------------------------------------
t/005no_tabs.t    1   256   431    1   0.23%  182
t/008filter.t     1   256   281    1   0.36%  32



>Index: template/en/default/bug/comments.html.tmpl

>+	  <input type="hidden" value="1"
>+		 name="defined_isprivate_[% comment.id %]">
>+          <input type="checkbox"
>+		 name="isprivate_[% comment.id %]" value="1"

Weird indentation due to tabs, as reported by runtests.pl above.


Your patch is working great, but it doesn't pass tests, though my r-.

Comment 8

[Max Kanat-Alexander]

Attachment: v3

That one thing is on two lines because otherwise the line is longer than 80 chars.

I fixed the test failures.

Comment 9

[Frédéric Buclin]

Comment on attachment 288385 [details] [diff] [review]
v3

r=LpSolit

Comment 10

[Max Kanat-Alexander]

Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.393; previous revision: 1.392
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.215; previous revision: 1.214
done
Checking in template/en/default/filterexceptions.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v  <--  filterexceptions.pl
new revision: 1.108; previous revision: 1.107
done
Checking in template/en/default/bug/comments.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/comments.html.tmpl,v  <--  comments.html.tmpl
new revision: 1.34; previous revision: 1.33
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v  <--  user-error.html.tmpl
new revision: 1.236; previous revision: 1.235
done