276446 - Initial description cannot be made private on new bug creation

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect, P2)

Description

[Shane H. W. Travis]

When a new bug is created, there is no way to make the first comment 'private' 
(i.e. visible only to the insidergroup) right off the bat. True, it's possible 
to make the whole bug visible only to that group, but that doesn't fix the 
problem.

After a bug has been created, it's possible to go back and set the 0th comment 
to 'private' if one is part of the insider group. If that happens, then this 
privacy needs to be maintained when a bug is cloned... but right now it cannot 
be done since there's no way to pass that information to post_bug, nor does 
post_bug know what to do with it anyway. This patch fixes that.

(Yes, it's a weird situation, but I realized that it could happen when trying 
to account for all the states a bug could get into as I worked through bug 
81642. And really... if we can do it later, may as well be able to do it right 
off the bat.)

Comment 1

[Shane H. W. Travis]

Attachment: Allows initial comment to be private (tip only)

Comment 2

[Shane H. W. Travis]

As this is small, localized, inoffensive, and (IMHO) should have been done at 
the time that the insidergroup stuff was done, I'd love to see it go into 2.18 
after it passes review.

Comment 3

[Jouni Heikniemi]

Comment on attachment 169891 [details] [diff] [review]
Allows initial comment to be private (tip only)

The patch doesn't apply for me; hunk 1 of post_bug fails (why? I don't know and
don't have the time to find out), so I'm reviewing by inspection only. I'll
actually test the next one :-)

> [% product_name = product FILTER html %]
>+[% isinsider = Param("insidergroup") && UserInGroup(Param("insidergroup")) %]

For the sake of consistency, is_insider?

>+  [% IF Param("insidergroup") && UserInGroup(Param("insidergroup")) %]

... and then, you should use the variable or drop it totally :-)

>+        &nbsp;&nbsp;<input type="checkbox" name="commentprivacy" value="1"
>+               id="newcommentprivacy"> Initial Description is Private

Use <label> for the checkbox text.

>+    <input type="hidden" name="commentprivacy" value="1">

Wouldn't "0" be more sensible a value for those not in the insider group?


>--- tip/post_bug.cgi    2004-12-28 22:59:30.000000000 -0600
>+++ tip.clone/post_bug.cgi      2004-12-29 22:30:54.000000000 -0600

>+my $privacy = (exists $::FORM{'commentprivacy'}) ? $::FORM{'commentprivacy'} : 0;

my $privacy = $::FORM{'commentprivacy'} || 0; 

is a cleaner way to say this :-) (we also use this elsewhere in Bugzilla code)

Here, you need to verify that the user actually has the permission to mark the
first comment as insider; it's not sufficient to check this in the template.

Comment 4

[Shane H. W. Travis]

(In reply to comment #3)
> (From update of attachment 169891 [details] [diff] [review] [edit])
> The patch doesn't apply for me; hunk 1 of post_bug fails (why? I don't know
> and don't have the time to find out)

I just checked out a clean copy of the tip, and it applies cleanly for me... 
just FYI.

> ... and then, you should use the variable or drop it totally :-)

lol! oops...

> >+    <input type="hidden" name="commentprivacy" value="1">
> Wouldn't "0" be more sensible a value for those not in the insider group?

You'd think so, but I've seen it done this way in other places, and my testing 
showed that it worked. Still, I agree that it doesn't make any *sense*, so I'm 
changing it to 0 anyway.

> my $privacy = $::FORM{'commentprivacy'} || 0; 
> is a cleaner way to say this :-) (we also use this elsewhere in Bugzilla code)

Does this syntax check for existence too?

> Here, you need to verify that the user actually has the permission to mark
> the first comment as insider; it's not sufficient to check this in the
> template.

Why is it insufficient? A value is sent in either case, so it's not as though 
it can be overridden by a URI parameter or anything... can it?

I'll take you at your word and add some checking, but I'm interested in hearing 
why it's needed so I understand for next time; I really didn't think the 
template check could be bypassed.

Follow-up patch forthcoming.

Comment 5

[Shane H. W. Travis]

Attachment: Code patch for tip, take 2

All issues addressed, nothing else of any significance changed.

Comment 6

[Shane H. W. Travis]

Attachment: Code patch for tip, take 3

I guess I should have changed something significant. I forgot to pass the
commentprivacy information from enter_bug to create.html.tmpl. As such, this
wouldn't have worked with bookmarkable templates... or for cloning, which is
the whole reason I needed this bug in the first place! :)

I also undid part of the "|| 0" fix that Jouni recommended, because I realized
I needed the answer to be exactly 0 or 1, not '0 or whatever the user passes
in'.

Comment 7

[Shane H. W. Travis]

Attachment: patch for a completely different bug: ignore.

Bah, forgot to get rid of the extraneous lines in the patch. Don't know if
it'll affect anything, but they're ugly and unnecessary, so here's one without
them.

Comment 8

[Shane H. W. Travis]

Comment on attachment 170373 [details] [diff] [review]
patch for a completely different bug: ignore.

Argh, I'm an idiot. This is from bug 108870, which I've been pushing to get
finished. Fingers typed the wrong thing when I wasn't looking. :(

Comment 9

[Shane H. W. Travis]

Attachment: Code patch for tip, take 3 (for real)

Don'tcha just love it when you try and fix a small mistake, and make an even
bigger mistake? :(

Comment 10

[Frédéric Buclin]

Comment on attachment 170379 [details] [diff] [review]
Code patch for tip, take 3 (for real)

>+if (Param("insidergroup") && UserInGroup(Param("insidergroup"))) {
>+  $privacy = $::FORM{'commentprivacy'} ? 1 : 0;
>+}

nit: you should have 4 spaces before $privacy.


>+
>+SendSQL("INSERT INTO longdescs (bug_id, who, bug_when, thetext, isprivate) 
>+         VALUES ($id, $::userid, now(), " . SqlQuote($comment) . ", $privacy)");

Instead of $::userid, use SqlQuote($user->id). And replace now() by
SqlQuote($timestamp) for consistency with the activity table.

Otherwise, it looks *very* good and works great. :)

Comment 11

[Shane H. W. Travis]

Attachment: Code patch for tip, take 4

Fixed the nits and non-nits from above comments, otherwise changed nothing.

Comment 12

[Jouni Heikniemi]

Comment on attachment 170417 [details] [diff] [review]
Code patch for tip, take 4

> > my $privacy = $::FORM{'commentprivacy'} || 0; 
> > is a cleaner way to say this :-) (we also use this elsewhere in Bugzilla code)
> Does this syntax check for existence too?

$::FORM{'commentprivacy'} evaluates to undef if it isn't set. The or operation
will then end up setting it to 0. As you say later on:

> I also undid part of the "|| 0" fix that Jouni recommended, because I realized
> I needed the answer to be exactly 0 or 1, not '0 or whatever the user passes
> in'.

True; my syntax proposal was nothing but equivalent of your original if exists
-construct, no more or less validation.


> > Here, you need to verify that the user actually has the permission to mark
> > the first comment as insider; it's not sufficient to check this in the
> > template.
> Why is it insufficient? A value is sent in either case, so it's not as though 
> it can be overridden by a URI parameter or anything... can it?

You don't need to use the form to make a post. Although browsers don't support
arbitrary posts, there's no reason why a tool such as curl
<http://curl.haxx.se/> couldn't be used to post in a "1" even when the user
wouldn't actually see the insider group checkbox on the form. Always validate
when you save; there's no reason to assume the user would be using the UI you
create. Actually, even some of our tools such as bugzilla-submit use this
approach.

But yeah, this version looks good to me. 
r=jouni

Comment 13

[Shane H. W. Travis]

Thanks, Jouni, for doing the review even though I'd taken your name off it; I 
wasn't sure how much time you had on your hands lately. Thanks also to Frederic 
for doing the review of v3, and for several IRC suggestions too.

I'm requesting approval for 2.18 if possible, as the privacy stuff is new to 
2.18 (compared to 2.16, anyway) and IMHO this could have/should have been 
something done in the initial proposal. This is a fairly small, well-contained 
and easy-to-decipher patch that shouldn't have any larger ramifications that I 
can think of, so it should be safe.

Comment 14

[Dave Miller [:justdave]]

I have questions about the operation of this patch (and the insidergroup in
general when you hide the initial comment after the fact) before I approve this...

What happens if there are no additional comments yet (only the description) and
the initial comment is hidden?  What does the bug look like to someone not in
the insidergroup?  Does the bug just stop after the Format For Printing link? 
Is the Opened date still displayed, since it's part of that initial header?  Is
the Description header still there, just with nothing under it?  Once there is
an additional comment, but the initial one is hidden, does the second comment
show up as if it were the initial comment, and thus attributed to the reporter?

Comment 15

[Shane H. W. Travis]

(In reply to comment #14)

> What happens if there are no additional comments yet (only the description)
> and the initial comment is hidden?  What does the bug look like to someone
> not in the insidergroup?

It looks roughly like this:

View Bug Activity   |   Format For Printing 
____________________________________________________________________________

Description [reply]                                 Opened: 2004-12-14 14:13
____________________________________________________________________________

Bug List: (1 of 1) First Last Prev (etc.)


> Does the bug just stop after the Format For Printing link?
No.

> Is the Opened date still displayed, since it's part of that initial header?
Yes.

> Is the Description header still there, just with nothing under it?
Correct.

> Once there is an additional comment, but the initial one is hidden, does the
> second comment show up as if it were the initial comment, and thus attributed
> to the reporter?

No, it does not. In the example below, there are four comments on the bug: the 
initial description and comment #2 are private, and the bug is being viewed by 
a non-insider. (This is a real example taken from my test DB.)

____________________________________________________________________________

Description [reply]                                 Opened: 2004-12-14 14:13

--- Additional Comment #1 From Shane H. W. Travis 2005-01-04 13:43 [reply] --- 
A new, public comment


--- Additional Comment #3 From Shane H. W. Travis 2005-01-06 00:50 [reply] --- 
Another public comment. This is the third comment on this bug, but the initial 
description and comment #2 are both private.

____________________________________________________________________________

As you can see, the non-insider can tell that private comments *exist* (by the 
skip-numbering) but simply cannot see them; The same is true for the initial 
description. It must exist -- you can't make a bug without one -- but it just 
cannot be seen by a non-insider.

Comment 16

[Dave Miller [:justdave]]

ok, sounds good to me :)

Comment 17

[Shane H. W. Travis]

2.18:

Checking in enter_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/enter_bug.cgi,v  <--  enter_bug.cgi
new revision: 1.94.2.2; previous revision: 1.94.2.1
done
Checking in post_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
new revision: 1.88.2.3; previous revision: 1.88.2.2
done
Checking in template/en/default/bug/create/create.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/create/create.html.tm
pl,v  <--  create.html.tmpl
new revision: 1.30.2.3; previous revision: 1.30.2.2
done


Tip:
Checking in enter_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/enter_bug.cgi,v  <--  enter_bug.cgi
new revision: 1.99; previous revision: 1.98
done
Checking in post_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
new revision: 1.93; previous revision: 1.92
done
Checking in template/en/default/bug/create/create.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/create/create.html.tm
pl,v  <--  create.html.tmpl
new revision: 1.36; previous revision: 1.35
done