Users without the capability to edit a bug should not be able to remove other users from the CC list, but should still be able to remove themselves. A good way to solve this would be add a checkbox for "Email me when this bug changes". If the user has the capability to edit bugs, then they should still get the existing textbox like we do currently, as well, but if the user doesn't have that capability, or if they haven't logged in, then the CC list can be included in a hidden field instead of a textbox, so that it still gets submitted with the update, but the users don't have the opportunity to change it. (This also helps to protect the privacy of people who want themselves CC'd on a bug). Then the cgi can look at that checkbox and add or remove that person's name as necessary when processing it. (It would also have to be preset if their name was in the CC list when they viewed it, if they were logged in at the time).

This bug is similar to bug 7345.

tara@tequilarista.org is the new owner of Bugzilla and Bonsai. (For details, see my posting in netscape.public.mozilla.webtools, news://news.mozilla.org/38F5D90D.F40E8C1A%40geocast.com .)

See also bug 34488.

Adding default QA contact to all open Webtools/Bugzilla bugs lacking one. Sorry for the spam.

On a sort of related note, if a user doesn't have permissions to edit things on a bug, then shouldn't all the fields the user can't edit be made read-only? e.g. the keywords field sits there tempting me to add things on all bugs, but I can only make changes to it on bugs I created.

moving

correcting version field lost in product move

Taking. Very similar to bug 7710 which is also mine.

Gavin: that is not the gist of this bug, but is worthy of a proper bug itself. If you could file another one with your summary, it would be great. Are there UI proposals for this bug to compare with my proposal on bug 34488?

->2.18

Christian: Re my comment#5 and your reply in comment#9. I've only just seen this, as I wasn't CC'd - doh! Bug#95923 is about not letting the user change fields they aren't allowed to.

Sigh. No time to work on this anymore... ->Default owner.

Enhancements which don't currently have patches on them which are targetted at 2.18 are being retargetted to 2.20 because we're about to freeze for 2.18. Consideration will be taken for moving items back to 2.18 on a case-by-case basis (but is unlikely for enhancements)

Bugzilla 2.20 feature set is now frozen as of 15 Sept 2004. Anything flagged enhancement that hasn't already landed is being pushed out. If this bug is otherwise ready to land, we'll handle it on a case-by-case basis, please set the blocking2.20 flag to '?' if you think it qualifies.

*** Bug 207895 has been marked as a duplicate of this bug. ***

Dave - The whiteboard says blocker will fix and the blocker is done. So can this be closed?

No.

yeah, what ended up getting implemented in bug 95923 isn't quite was was being asked for here. That ended up blocking all changes, and only for the product restrictions (i.e. the canedit bit on a product). With that change, users can no longer CC themselves, even, on a bug in a product in which they don't have canedit. Users should probably still be able to CC/unCC themselves from a bug, even without canedit privs, as that's our notification method. Likewise, random users who haven't gotten editbugs yet probably shouldn't be able to CC/unCC anyone other than themselves, which they CAN do now if the product doesn't have canedit restrictions.

I think being able to CC people is fine. But unCC'ing is probably not unless you have editbugs privs.

Comment on attachment 527271 [details] [diff] [review] patch for 3.6 No need to attach 3 times the exact same patch. :)

Comment on attachment 527269 [details] [diff] [review] patch for trunk >=== modified file 'Bugzilla/Bug.pm' >+ if (!$currentUser->in_group('editbugs', $self->{'product_id'})) { >+ if ($user->id != $currentUser->id) { You could combine both conditions into a single one. Also, please use $self->product_id instead of the internal value directly. >=== modified file 'template/en/default/global/user-error.html.tmpl' >+ [% ELSIF error == "cc_remove_denied" %] >+ [% title = "Access Denied" %] Maybe "Change Denied" would be more appropriate? Also, you have to enforce this restriction in the UI. For powerless users, I think we should make the CC list read-only, and the label of the checkbox should be changed from "Remove selected CCs" to "Remove me from the CC list".

Comment on attachment 527296 [details] [diff] [review] patch v2 Hmm. Perhaps this should be implemented via check_can_change_field instead? Also, I'm suspicious of your UI logic, as it doesn't check all the ways to user can edit the bug.

(In reply to comment #29) > Also, I'm suspicious of your UI logic, as it doesn't check all the ways to user > can edit the bug. i tested it as anon, a user without editbugs and a user with editbugs. what did i miss?

(In reply to comment #29) > Hmm. Perhaps this should be implemented via check_can_change_field instead? The problem is that you can always edit the CC field, as you can add/remove yourself.

Comment on attachment 527296 [details] [diff] [review] patch v2 >=== modified file 'template/en/default/bug/edit.html.tmpl' >+ [% UNLESS user.id && !user.in_group('editbugs', bug.product.id) && !bug.cc.contains(user.email) %] > (<a href="#" id="cc_edit_area_showhide">[% IF user.id %]edit[% ELSE %]show[% END %]</a>) >+ [% END %] This totally prevents us from displaying the CC list. Instead of your UNLESS block, it should rather be: [% IF user.id AND bug.user.canedit %]edit[% ELSE %]show[% END %] >- [% IF user.id %] >+ [% IF user.id && user.in_group('editbugs', bug.product.id) %] This change should be reverted. We don't want to prevent users from adding other users as they may legitimately have to do it. And we never saw abuse this way, isn't it? ;) >- <select id="cc" name="cc" multiple="multiple" size="5"> >+ <select id="cc" multiple="multiple" size="5" >+ name="cc[% IF !user.in_group('editbugs', bug.product.id) %]_select[% END %]"> Shouldn't we omit the name entirely, in this case? cc_select is not understood by process_bug.cgi. Also, it should be bug.user.canedit instead. >+ [% IF user.id && !user.in_group('editbugs', bug.product.id) -%] >+ <input type="hidden" name="cc" value="[% user.email FILTER html %]"> >+ [% END %] Same here. >- [%%]<label for="removecc">Remove selected CCs</label> >+ [%-%]<label for="removecc"> >+ [% IF user.in_group('editbugs', bug.product.id) %] >+ [%-%]Remove selected CCs >+ [% ELSE %] >+ [%-%]Remove me from the CC list >+ [% END %] >+ </label> Please fix the indentation. Also, I'm not sure we need all these [%-%].

Comment on attachment 527296 [details] [diff] [review] patch v2 >=== modified file 'template/en/default/bug/edit.html.tmpl' > [% IF user.id %] > <br> > <input type="checkbox" id="removecc" name="removecc"> >- [%%]<label for="removecc">Remove selected CCs</label> >+ [%-%]<label for="removecc"> >+ [% IF user.in_group('editbugs', bug.product.id) %] >+ [%-%]Remove selected CCs >+ [% ELSE %] >+ [%-%]Remove me from the CC list >+ [% END %] >+ </label> > <br> > [% END %] One more thing. If the user isn't in the CC list and has no privs, this checkbox shouldn't be displayed at all. So the first IF condition should be changed to [% IF user.id AND (bug.user.canedit OR bug.cc.contains(user.login)) %]

thanks for the review; this revision address the review points.

Comment on attachment 527506 [details] [diff] [review] patch v3 >=== modified file 'template/en/default/bug/edit.html.tmpl' >- (<a href="#" id="cc_edit_area_showhide">[% IF user.id %]edit[% ELSE %]show[% END %]</a>) >+ (<a href="#" id="cc_edit_area_showhide"> >+ [% IF user.id AND (bug.user.canedit OR bug.cc.contains(user.login)) %]edit >+ [% ELSE %]show[% END %]</a>) I just realized that we don't want this change as a logged in user can always add people to the list. So technically, "edit" is correct. Please revert this change on checkin. >+ [% IF user.id && !bug.user.canedit %] >+ <input type="hidden" name="cc" value="[% user.email FILTER html %]"> >+ [% END %] Please fix the indentation. Also, we must pass user.login, not user.email, else this will break process_bug.cgi if emailsuffix is in use. And as in the code above it, it must be FILTER email FILTER html. >+ [% IF (bug.user.canedit) %] Useless parens. Otherwise looks good. Next shoot will be the last one. :)

Comment on attachment 528531 [details] [diff] [review] patch v4 >=== modified file 'Bugzilla/Bug.pm' >+ my $currentUser = Bugzilla->user; >+ if (!$currentUser->in_group('editbugs', $self->product_id) >+ && $user->id != $currentUser->id) Now that we use bug.user.canedit everywhere in the template, we have to use it here to. So we want if (!$self->user->{canedit} && $user->id != Bugzilla->user->id) here. >=== modified file 'template/en/default/bug/edit.html.tmpl' >+ <input type="hidden" name="cc" value="[% user.login FILTER html %]"> You forgot FILTER email: [% user.login FILTER email FILTER html %].

Comment on attachment 529948 [details] [diff] [review] patch v5 >=== modified file 'Bugzilla/Bug.pm' >+ if (!$self->user->{'canedit'} && $user->id != $currentUser->id) >+ { Nit: { comes at the end of the previous line. >=== modified file 'template/en/default/bug/edit.html.tmpl' >- <select id="cc" name="cc" multiple="multiple" size="5"> >- [% FOREACH c = bug.cc %] >- <option value="[% c FILTER email FILTER html %]"> >- [% c FILTER email FILTER html %]</option> >- [% END %] >+ <select id="cc" multiple="multiple" size="5" >+ [% IF bug.user.canedit %]name="cc"[% END %]> >+ [% FOREACH c = bug.cc %] >+ <option value="[% c FILTER email FILTER html %]"> >+ [% c FILTER email FILTER html %]</option> >+ [% END %] > </select> There is no reason to change the indentation of the FOREACH block. It was correct as it was; the change isn't. Please revert this on checkin. Otherwise looks good and works fine. r=LpSolit

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/ modified Bugzilla/Bug.pm modified template/en/default/bug/edit.html.tmpl modified template/en/default/global/user-error.html.tmpl Committed revision 7811.

Updated the patch in Bug #704753

Added to relnotes in bug 713346.