712040 - enter_bug.cgi should say if a product is visible only to people with permissions

Status: NEW

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[David Lawrence [:dkl]]

Attachment: Patch to add lock icon to choose products to show product group restricted (v1)

+++ This bug was initially created as a clone of Bug #705051 +++

https://bugzilla.mozilla.org/enter_bug.cgi?full=1 lists the products available to the bug reporter.  However, sometimes the bug reporter can see products that others can't.  But this can lead to confusion when talking about how to file a bug.

To avoid confusion, enter_bug.cgi should have text (and probably also a different background color) next to a product that's only visible to some users saying who it's visible to.

Patch based on Gerv's earlier work.

dkl

Comment 1

[Frédéric Buclin]

Comment on attachment 582861 [details] [diff] [review]
Patch to add lock icon to choose products to show product group restricted (v1)

>=== modified file 'skins/standard/global.css'

>+.group_secure > th > a {

Why don't you reuse tr.bz_secure defined in buglist.css? What's the point to define another class name which does mostly the same thing?


>=== modified file 'template/en/default/global/choose-product.html.tmpl'

>   [% FOREACH p = c.products %]
>+    [% class = "" %]
>+    [% has_entry_groups = 0 %]
>+    [% FOREACH gid = p.group_controls.keys %]
>+      [% IF p.group_controls.$gid.entry %]
>+        [% has_entry_groups = 1 %]
>+        [% class = class _ " group_$gid" %]
>+      [% END %]
>+    [% END %]

What's the performance penalty to call group_controls() on *all* products? Also, using group_$gid as classes is inconsistent with classes already used in bugs, which are of the form bz_group_$name. IMO, displaying the padlock only should be enough. I don't see the point to add more classes. This would fix the potential name/id leakage problem.


>+    [%- IF has_entry_groups %] title="This product requires one or more
>+ group memberships in order to enter [% terms.bugs %] in it. You have them, but be
>+ aware not everyone else does."[% END %]>

IMO, this warning is useless to 99.99% of users, which makes me wonder if this patch is really useful upstream. Some installations use group settings to only give you access to some very specific products, and so all products would be displayed with the padlock icon.

Comment 2

[David Lawrence [:dkl]]

Attachment: Patch to add lock icon to choose products to show product group restricted (v2)

New patch that uses bz_secure. Also no noticeably performance impact from adding this based on my benchmark tests. I admit when all products are product the usefulness decreases but BMO and Red Hat are pretty mixed with regard to private/public products so it would be useful for those at least.

dkl

Comment 3

[Frédéric Buclin]

Comment on attachment 604221 [details] [diff] [review]
Patch to add lock icon to choose products to show product group restricted (v2)

>+    [% FOREACH gid = p.group_controls.keys %]
>+      [% IF p.group_controls.$gid.entry %]
>+        [% has_entry_groups = 1 %]
>+      [% END %]

As I suspect some performance issues when listing several tens or hundreds of products (or simply several tens or hundreds of useless calls to the DB), we should write a specific SQL query in enter_bug.cgi and describecomponents.cgi right before calling this template which returns products with the ENTRY bit set:

  SELECT DISTINCT product_id FROM group_control_map WHERE entry != 0

Store the result in a hash so that we can easily use it. Your code above could then totally be removed.


>+    <tr class="[% "bz_secure" IF has_entry_groups %]"

This class is not needed here. Please remove it.


>+           class="[% "bz_secure" IF has_entry_groups +%]">

IMO, it would be better to write [% ' class="bz_secure"' IF has_entry_groups %] to not create an empty class.


I tested your patch, and the UI looks fine. So I'm fine with an updated patch with the comments above addressed.

Comment 4

[Frédéric Buclin]

(In reply to Frédéric Buclin from comment #3)
> we should write a specific SQL query in enter_bug.cgi and
> describecomponents.cgi right before calling this template

I just realized that the bug summary only mentions enter_bug.cgi, not describecomponents.cgi. So the SQL query would only be needed in enter_bug.cgi. For describecomponents.cgi, if the template cannot find the hash I mentioned in my review, it simply won't display any padlock, which is fine.

Comment 5

[Frédéric Buclin]

Too late for 4.4. We already released rc1.