174037 - need ability to hide flags from some users

Status: NEW

(Bugzilla :: Attachments & Requests, enhancement)

Description

[Jeff Hedlund]

I'd like the ability to hide certain flags from certain users.

We could add a 'can see' group(s) to the flag UI to restrict certain people from
seeing the flag on the edit bug screen.

Comment 1

[David Lawrence [:dkl]]

Attachment: Patch to allow flags to be private to a particular group (v1)

We here at Red Hat have quite a few flags that are heavily tied to our management and release process. These flags are generally not useful to or should be hidden from the public view. We have added support to our installation to be able to 
set a group that is allowed to view a flag for a bug/attachment. If the user is not in the proper group if one is set then it does not show up. If no group is set for the flag then the flag is visible to everyone similar to how it is now.

Basically it does the following:

1. Adds a new drop down in editflagtypes.cgi that allows an existing group to be selected as able to view the flag. No group selected means flag is public.
2. From show_bug.cgi or attachment details page, flags are filtered based on the group not displayed if the user cannot see the flag. Updated Bugzilla::FlagType::match() to only return flags the user has permission to see.
3. When viewing bug history, flags that the user cannot see are filtered from the list.
4. When email notifications are sent, flags that the user cannot see are also filtered from the email text.
5. Admin and those with 'editcomponents' permissions are allowed to see all flags.
6. Does not affect the ability to grant/request a flag, just visibility.

Comment 2

[David Lawrence [:dkl]]

Attachment: Patch to allow flags to be private to a particular group (v2)

Fixed error message that is displayed when a flag is trying to be validated that the user is not allowed to view.

Comment 3

[Max Kanat-Alexander]

Comment on attachment 300818 [details] [diff] [review]
Patch to allow flags to be private to a particular group (v2)

LpSolit owns flags. It should say that on the reviewer list--if it doesn't, let me know and I'll fix it.

Comment 4

[Frédéric Buclin]

In which circumstances is this implementation useful? I mean, even timetracking fields should IMO be visible, but not editable if you are not in the timetracking group. About flags, it's the same: you should still be allowed to view them, even if you cannot edit thme. Else what's the interest to be able to view only some bug information, but not all of them? Also, if a user moves a bug into another product or component where an invisible flag is no longer applicable, it would silently be dropped and the user would not even know he was removing it. Another concern is about midair collisions. What is the midair page supposed to say if it cannot talk about flags?

I want some more input about this before reviewing it.

Comment 5

[David Lawrence [:dkl]]

(In reply to comment #3)
> (From update of attachment 300818 [details] [diff] [review])
> LpSolit owns flags. It should say that on the reviewer list--if it doesn't, let
> me know and I'll fix it.
> 

Sorry for that. Trying to get back into the swing of things and am not familiar yet with who owns what.

Will consult with http://www.bugzilla.org/docs/reviewer-list.html#expertise
next time.

Comment 6

[David Lawrence [:dkl]]

(In reply to comment #4)
> In which circumstances is this implementation useful? I mean, even timetracking
> fields should IMO be visible, but not editable if you are not in the
> timetracking group. About flags, it's the same: you should still be allowed to
> view them, even if you cannot edit thme. Else what's the interest to be able to
> view only some bug information, but not all of them? 

We have flags that have names of products/versions that are not yet available to the public. For example before rhel 5.1 was released, we used the flags to track  which features/bugs were going to make it into the release by using rhel-5.1.0+, rhel-5.1.0-, etc. When a feature/bug is requested by sales/eng/marketing, they set the rhel-5.1.0? flag. We then have three other flags, pm_ack, qa_ack, and devel_ack that all must be set to + by their respective parties before the rhel-5.1.0+ can be set. If any one of the ack flags are set to - then the rhel-5.1.0- flag is set.

Long story short, we do not allow the general public to see this process happening due to the sensitive nature of the data regarding what will or will not be in the next release of our products. The bug can stay public but the process happens privately.

I admit for the general open source community and Mozilla especially, private flags would generally not be encouraged or prove useful. We would just like to give the Bugzilla admins the option of doing this.

> Also, if a user moves a
> bug into another product or component where an invisible flag is no longer
> applicable, it would silently be dropped and the user would not even know he
> was removing it.

This would be fine for our purposes as we have prducts now based on major product versions such as RHEL4, RHEL5, etc. So if a bug moves to another product the rhel-X.X.X flags would no longer be relevant anyway. But to address this concern maybe a warning should be displayed or a statement next to the flags?

> Another concern is about midair collisions. What is the midair
> page supposed to say if it cannot talk about flags?

Unfortunately I do not address the midair issue in my current patch and will add that fix in for the next version. I can see maybe a message stating that 
a flag change has occurred since your the last page load and conflicts with the current changes. The actual flag names would not need to be revealed if they are set to private.

Dave

Comment 7

[Frédéric Buclin]

(In reply to comment #6)
> Long story short, we do not allow the general public to see this process
> happening due to the sensitive nature of the data regarding what will or will
> not be in the next release of our products. The bug can stay public but the
> process happens privately.

If the process must remain private, how do you communicate as any comment would be visible to the public? If you say qa_ack- because such or such feature is definitely broken, users will see this information anyway in the comment, so they get as much information as looking at flags themselves.


> this concern maybe a warning should be displayed or a statement next to the
> flags?

This bothers me. Not knowing what your changes are going to involve is not something I would like to see upstream.


> Unfortunately I do not address the midair issue in my current patch

Sorry, but I really don't like to be cryptic to users. "Your changes are going to affect flags you cannot see, but I cannot tell you more, sorry!" would be the kind of messages you would have to display.

In comparison to other bug fields, flags are already the most complex part and adding another attribute honestly bothers me.


I need an answer to my first question about comments before deciding what to do with this patch.

Comment 8

[David Lawrence [:dkl]]

(In reply to comment #7)
> If the process must remain private, how do you communicate as any comment would
> be visible to the public? If you say qa_ack- because such or such feature is
> definitely broken, users will see this information anyway in the comment, so
> they get as much information as looking at flags themselves.

Any comment made while making the private flag change is marked private to the insider group manually by the submitter. We have a group called private_comment which users must be in to see private comments. Those in that group currently are Red Hat engineering only.

Comment 9

[David Lawrence [:dkl]]

Attachment: Patch to allow flags to be private to a particular group (v3)

Comment 10

[David Lawrence [:dkl]]

(In reply to comment #9)
> Created an attachment (id=301370) [details]
> Patch to allow flags to be private to a particular group (v3)
> 

Hit enter before I could finish typing a comment for this new patch.

1. Fixed patch where admin/flag-type/{list,edit}.html.tmpl was mistakenly left out.
2. Fixed Bugzilla::Flag::notify() where it will filter the recipients of the flag change notification based on whether they can see the flag.
3. Fixed editflagtypes.cgi where it was not checking for view_group in validateGroups() so changes were being dropped.
4. Fixed POD documentation in Bugzilla::User by adding section about can_see_flag().

Comment 11

[Frédéric Buclin]

We are frozen already. This won't go into 3.2.

Comment 12

[Frédéric Buclin]

Comment on attachment 301370 [details] [diff] [review]
Patch to allow flags to be private to a particular group (v3)

Must be written based on current CVS HEAD code.

Comment 13

[David Lawrence [:dkl]]

Attachment: Patch to allow flags to be private to a particular group (v4)

Updated to latest CVS HEAD.

Comment 14

[Frédéric Buclin]

dkl: wouldn't it be easier if the view_group was always the same as the insidergroup? This way we would be sure that comments made while editing private flags are private as the user would have the required privs for sure. This means the UI in editflagtypes.cgi would be a simple checkbox instead of a drop-down menu.

Comment 15

[Frédéric Buclin]

Comment on attachment 305064 [details] [diff] [review]
Patch to allow flags to be private to a particular group (v4)

As discussed on IRC with dkl, flags are not going to have their own visible_group, but will reuse the existing insidergroup.

Comment 16

[Jeff Hedlund]

I'd like to chime in here real quick - something we are looking for is the ability to hide certain flags from the insidergroup as well.

I know dkl mentioned (in comment 1) that some flags are tied to management. 

We are looking at some flags that would be management viewable only - but that's only a subset of the insidergroup.  dkl: Do you not have a need for that?  Or are you merely wanting to keep certain flags from being visible to public/non-insiders?  (I would assume that's the case from the IRC discussion that took place).

I'll probably look into applying the patch written by dkl,  but it might be helpful to know why the decision was made the only use the insidergroup.

Comment 17

[David Lawrence [:dkl]]

(In reply to comment #16)
> I'd like to chime in here real quick - something we are looking for is the
> ability to hide certain flags from the insidergroup as well.
> 
> I know dkl mentioned (in comment 1) that some flags are tied to management. 
> 
> We are looking at some flags that would be management viewable only - but
> that's only a subset of the insidergroup.  dkl: Do you not have a need for
> that?  Or are you merely wanting to keep certain flags from being visible to
> public/non-insiders?  (I would assume that's the case from the IRC discussion
> that took place).
> 

Mainly I was considering to use the insidergroup to make the patch simpler and also help get accepted. I am discussion now internally to make sure this would still work with our internal processes. I still feel that this may need to be more configurable in the future and that possibly leaving this as a selectable group may be better long term. FWIW, with my patch you can still choose the group that the insidergroup param is mapped to anyway for all flags which would accomplish the same thing. If people feel insidergroup is adequate and our managers agree then I can go either way. But I do see how it may be preferable to choose different groups on a per flag basis in the future.

Dave 

Comment 18

[Frédéric Buclin]

(In reply to comment #16)
> helpful to know why the decision was made the only use the insidergroup.

Because I don't want to make Bugzilla too complex. Having each flag type having their own viewable group would be the first step towards requesting the same feature for each other field (priority, keywords, status whiteboard, etc...), which is something I don't want for now (custom fields may be another story, depends how things will go in the future). I think a public/private bit as we currently have for comments is enough for now.

Comment 19

[David Lawrence [:dkl]]

One issue brought up in our discussion at Red Hat is that we also host Fedora on our Bugzilla. If for whatever reason the Fedora people wanted a flag to be private (unlikely but could happen I suppose) it would not be possible for us to grant that ability since our insidergroup is only for Red Hat employees. In that case having separate groups for flag privacy would be a plus.

Dave

Comment 20

[David Lawrence [:dkl]]

(In reply to comment #18) 
> Because I don't want to make Bugzilla too complex. Having each flag type having
> their own viewable group would be the first step towards requesting the same
> feature for each other field (priority, keywords, status whiteboard, etc...),
> which is something I don't want for now (custom fields may be another story,
> depends how things will go in the future). I think a public/private bit as we
> currently have for comments is enough for now.

Call me stubborn but I still fail to see how having a single insidergroup boolean checkbox for flag visibility is that much less complex than having a simple drop down of current active groups where a single value can be selected. 
As more and more Bugzilla's are being used for mixed business/community use, I can see that having it set to the insidergroup will be very limiting to some, us included as that group is usually reserved for internal eyes only. As for database schema it is simply an integer value instead of a boolean which is not
really too different. Maybe further discussion on the mailing lists is in order to bring in different views on this topic. 

As for Red Hat, I think we may need to stick with the group drop down due to our Bugzilla being used by different groups inside and outside of Red Hat. But I don't mind checking in the insidergroup version of the patch if developers feel that will be sufficient.

Dave

Comment 21

[Simon Green]

(In reply to David Lawrence [:dkl] from comment #20)
> Call me stubborn but I still fail to see how having a single insidergroup
> boolean checkbox for flag visibility is that much less complex than having a
> simple drop down of current active groups where a single value can be
> selected. 

Agreed, but then I've taken over from (some of) dkl's role at Red Hat :P

I'm keen to get this upstream if possible. The less customisation I have to carry the better, and I would imagine that other Bugzilla installations might want to make use of it. If I make a patch for Bugzilla 4.3, would you be interested in using it?

  -- simon

Comment 22

[Frédéric Buclin]

(In reply to Simon Green from comment #21)
> might want to make use of it. If I make a patch for Bugzilla 4.3, would you
> be interested in using it?

As I said in comment 18, I fear that people start requesting this feature for all bug fields, which I don't think would make sense. So I'm still ambivalent about this.

Comment 23

[Frédéric Buclin]

We are going to branch for Bugzilla 4.4 next week and this bug is either too invasive to be accepted for 4.4 at this point or shows no recent activity. The target milestone is reset and will be set again *only* when a patch is attached and approved.

I ask the assignee to reassign the bug to the default assignee if you don't plan to work on this bug in the near future, to make it clearer which bugs should be fixed by someone else.

Comment 24

[Simon Green]

(In reply to Frédéric Buclin from comment #23)
> I ask the assignee to reassign the bug to the default assignee if you don't
> plan to work on this bug in the near future, to make it clearer which bugs
> should be fixed by someone else.

I assume dkl has no interest in continuing with this. I'm going to have a crack at getting this into Bugzilla 5.0. There are some major changes, and getting it all upstream reduces the amount of code we need to support locally. As mentioned in comment 16, there are other installations that want the ability to hide flags to a certain group.

Comment 25

[Simon Green]

Adding the dependency so I don't forget to patch the new code too.

Comment 26

[Simon Green]

Attachment: incomplete untest patch

Here's an incomplete and slight bit rotted patch that someone can use as a basis for a working patch