Closed Bug 367394 Opened 18 years ago Closed 17 years ago

show_bug should include a class bz_group_<whatever> so pages can be styled

Categories

(Bugzilla :: User Interface, defect)

Product:
Bugzilla
Component:
User Interface
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Bugzilla 3.2

People

(Reporter: dveditz, Assigned: reed)

References

Details

(Whiteboard: [applied to b.m.o])

Attachments

(2 files, 4 obsolete files)

what I will check-in
4.88 KB, patch
Details | Diff | Splinter Review
Fixing bustage
2.01 KB, patch
Details | Diff | Splinter Review
I would like to be able to style bugs that are in certain groups so users editing security bugs, for example, are aware of that fact. The style itself can be left up to the installation or even the user, but to accomplish this I'd like a bz_group_SECURITY (or whatever the group) added to the class on the body element just as currently the status, component, and other states have classes.
Assignee: general → ui
Component: Bugzilla-General → User Interface
Whiteboard: [wanted-bmo]
Attached patch patch - v1 (obsolete) — — Splinter Review
Add "bz_group_<group>" to bodyclasses for all active groups on a bug. You can see this in action at https://landfill.bugzilla.org/prodpatches/show_bug.cgi?id=417.
Assignee: ui → reed
Status: NEW → ASSIGNED
Attachment #307293 - Flags: review?(LpSolit)
Target Milestone: --- → Bugzilla 3.0
Version: unspecified → 3.0
Comment on attachment 307293 [details] [diff] [review] patch - v1 >+ [% IF bug.groups.size > 0 %] No need to do this check. If bug.groups is empty, the FOREACH loop will immediately exit. Moreover, you have to fix attachment/updated.html.tmpl as well. As an enhancement, it's not something for 3.0.x.
Attachment #307293 - Flags: review?(LpSolit) → review-
Target Milestone: Bugzilla 3.0 → Bugzilla 3.2
Attached patch patch - v2 (obsolete) — — Splinter Review
Address review comments... Well, it was worth a shot trying to get this tiny change into 3.0.x. We'll still take it downstream on bmo.
Attachment #307293 - Attachment is obsolete: true
Attachment #307320 - Flags: review?(LpSolit)
It's very much wanted on BMO to prevent mistakes in handling security bugs. (In reply to comment #2) > You can see this in action at > https://landfill.bugzilla.org/prodpatches/show_bug.cgi?id=417. I don't. On that bug I only see class="landfill-bugzilla-org-prodpatches". On a typical BMO bug (e.g. this one) I see class="bugzilla-mozilla-org bz_bug bz_status_ASSIGNED bz_component_User_Interface bz_bug_367394"
(In reply to comment #5) > (In reply to comment #2) > > You can see this in action at > > https://landfill.bugzilla.org/prodpatches/show_bug.cgi?id=417. > > I don't. On that bug I only see class="landfill-bugzilla-org-prodpatches". On a > typical BMO bug (e.g. this one) I see class="bugzilla-mozilla-org bz_bug > bz_status_ASSIGNED bz_component_User_Interface bz_bug_367394" <body onload="" class="landfill-bugzilla-org-prodpatches bz_bug bz_status_REOPENED bz_component_SaltSprinkler bz_bug_417 bz_group_taragroup"> That's what I see... you don't see that?
Were you logged in at the time?
Without logging in, I get: class="landfill-bugzilla-org-prodpatches" Once I log into landfill, I'm able to access the bug, and I get: class="landfill-bugzilla-org-prodpatches bz_bug bz_status_REOPENED bz_component_SaltSprinkler bz_bug_417 bz_group_taragroup" Makes sense, given the policy of not saying what group an inaccessible bug is in.
Comment on attachment 307320 [details] [diff] [review] patch - v2 Works fine. But I realized you also have to fix attachment/created.html.tmpl which doesn't have bodyclasses set at all. Else these classes would be omitted everytime you add an attachment. Please fix this template before I a+ it.
Attachment #307320 - Flags: review?(LpSolit) → review+
Attached patch patch - v3 (obsolete) — — Splinter Review
Add bodyclasses to created.html.tmpl and carry over review+.
Attachment #307663 - Flags: review+
Attachment #307320 - Attachment is obsolete: true
Flags: approval?
Flags: approval3.2?
Comment on attachment 307663 [details] [diff] [review] patch - v3 >Index: template/en/default/attachment/created.html.tmpl >+[% bodyclasses = ['bz_bug', >+ "bz_status_$bug.bug_status", >+ "bz_component_$bug.component", >+ "bz_bug_$bug.bug_id" >+ ] >+%] >+[% FOREACH group = bug.groups %] >+ [% bodyclasses.push("bz_group_" _ group.name) IF group.ison %] >+[% END %] No, this doesn't work. $bug is undefined. You need to set [% bug = bugs.0 %] as in other templates. Please test your patch! ;)
Attachment #307663 - Flags: review+ → review-
Flags: approval?
Flags: approval3.2?
Attached patch patch - v3.1 (obsolete) — — Splinter Review
I don't have a tip testing place set up currently, and 3.0 branch doesn't have most of this code.
Attachment #307663 - Attachment is obsolete: true
Attachment #307758 - Flags: review?(LpSolit)
Comment on attachment 307758 [details] [diff] [review] patch - v3.1 This one works fine. r=LpSolit
Attachment #307758 - Flags: review?(LpSolit) → review+
Flags: approval+
Flags: approval3.2?
This seems overengineered to me. Why put a class for *each group* if all you care about is whether or not the bug is secure? Just put a single class in there that indicates this bug is restricted to a group.
(In reply to comment #14) > This seems overengineered to me. Why put a class for *each group* if all you > care about is whether or not the bug is secure? Just put a single class in > there that indicates this bug is restricted to a group. I for one would like the ability to do different things based on different groups. Not all groups are "security" group, so it doesn't make sense to just have a "bz_group" class. bmo has 12 groups-used-for-bugs, and only 3 of them are specifically mentioned as security groups. I wonder if it's possible to do class matching on partial names (i.e., you could possibly match on bz_group yourself even though it's really bz_group_<whatever>). Is that possible?
You should be able do that with the CSS3 selector[class*="bz_group"].
> Why put a class for *each group* if all you care about is [security] Currently I use a greasemonkey script to look at the group checkbox states, and it's useful to style various groups differently. Different products might be useful, too (but we don't have that). I don't know why we have a separate class for each component, though -- _that_ seems overkill, and yet someone wanted it enough to add it.
(In reply to comment #17) > > Why put a class for *each group* if all you care about is [security] I agree that style based on the group a bug is restricted to makes sense, so you need to know the name of the group(s) a bug is restricted to. It's not overengineered, especially when you know that bugs are most of the time restricted to one group only. So I maintain my a+. > it's useful to style various groups differently. Different products might be > useful, too (but we don't have that). I don't know why we have a separate class > for each component, though -- _that_ seems overkill, and yet someone wanted it > enough to add it. Good point! Gerv implemented it in bug 223078 in late 2003, and nobody complained. Let's check this in.
Flags: approval3.2?
(In reply to comment #15) > Not all groups are "security" group, Any group a bug is in restricts it to being viewed by only that group. (In reply to comment #18) > Good point! Gerv implemented it in bug 223078 in late 2003, and nobody > complained. Because I wasn't here. Also, you should be using bug.groups_in, not bug.groups. In any case, the "overengineering for little benefit" aspect of this bug isn't so great that I'd deny checkin, so you can go ahead and check it in, yeah.
Use bug.groups_in and drop the |IF group.ison| check.
Attachment #307758 - Attachment is obsolete: true
Checking in template/en/default/bug/show.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/show.html.tmpl,v <-- show.html.tmpl new revision: 1.24; previous revision: 1.23 done Checking in template/en/default/attachment/created.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/created.html.tmpl,v <-- created.html.tmpl new revision: 1.20; previous revision: 1.19 done Checking in template/en/default/attachment/updated.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/updated.html.tmpl,v <-- updated.html.tmpl new revision: 1.18; previous revision: 1.17 done
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
(In reply to comment #19) > (In reply to comment #15) > > Not all groups are "security" group, > > Any group a bug is in restricts it to being viewed by only that group. Yes, but we don't treat all groups as "security" groups. I purposely put the word "security" in quotes to emphasize this fact. Some groups are used for MoCo-only things while others are security-related. They aren't the same thing, so there should be a way to differentiate the groups themselves, and just having "bz_group" doesn't allow for that. > Also, you should be using bug.groups_in, not bug.groups. Done. > In any case, the "overengineering for little benefit" aspect of this bug > isn't so great that I'd deny checkin, so you can go ahead and check it in, > yeah. I disagree with your thinking about "overengineering" here. It's a perfectly reasonable request, and it makes complete sense for the groups to be listed since they aren't all the same thing.
Attached patch Fixing bustage — — Splinter Review
No idea why Tinderbox is burning, but this patch should fix the problem. Checking in template/en/default/attachment/created.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/created.html.tmpl,v <-- created.html.tmpl new revision: 1.21; previous revision: 1.20 done Checking in template/en/default/attachment/updated.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/updated.html.tmpl,v <-- updated.html.tmpl new revision: 1.19; previous revision: 1.18 done Checking in template/en/default/bug/show.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/show.html.tmpl,v <-- show.html.tmpl new revision: 1.25; previous revision: 1.24 done
Whiteboard: [wanted-bmo] → [applied to b.m.o]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: