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.)

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 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.

(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.

All issues addressed, nothing else of any significance changed.

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'.

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 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. :(

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

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. :)

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

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

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.

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?

(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.

ok, sounds good to me :)

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