424215 - show_bug.cgi should hide unset flags unless needing to edit them

Status: RESOLVED FIXED

(Bugzilla :: User Interface, enhancement)

Description

[David Lawrence [:dkl]]

Attachment: Patch to hide full list of flags/groups unless you need to edit them (v1)

Similar to how the assigned_to, qa_contact, etc. fields are readonly unless you click on the (edit) link to the right, the group checkboxes and flag form fields need to be hidden the same way. We have numerous groups (especially if you are the admin) and a large number of flags that can display for a single bug report. These group boxes and flags can take up alot of screen space when not needed. It would be nice to see just a readonly list of the flags/groups that are set and then provide an (edit) link that will display the full list of form fields that can
be edited.

Attaching a patch that does this using the standard hideEditableField() javascript already available in js/field.js. Prints None Set if no flags or groups are currently set.

Please let me know what you think.
Dave

Comment 1

[Max Kanat-Alexander]

I'm not sure I understand the point of this patch--flags are already displayed as uneditable if you aren't logged in or aren't in editbugs.

As far as hiding the group controls entirely if you can't edit them, that sounds fine to me.

Comment 2

[David Lawrence [:dkl]]

It just seemed like a good idea to clean up the show_bug.cgi a little more. We have quite a few flags 20-30 for some bugs which causes the flag section to push the section below it down on the page. Just showing the current flags set and only showing the form fields if the user wants to edit a flag state clears that up quite a bit. Also we can have a long list of groups listed for a single bug (30-40) which pushes everything even lower causing the user to scroll down quite a long way to get to the last comment made. Having the group list hidden unless the user actually wants to make a group change also help quite a bit to clean up the page. It just seemed to go along with the same theme similar to the way we do assigned to and qa contact now when a value is set. They are readonly even when the user is logged in and can edit those values until they click on (edit).

Comment 3

[Guy Pyrzak]

I like the idea. However, there is at least 1 bug you haven't tested. That bug (which is annoying) is what happens when a  user makes an error on entry and clicks back (or refresh). The way to handle this is to have an on load that checks to see if any of the values have changed. You can guess why this might be especially annoying for flags and groups, but if you want to make that work, go for it! I have handled this in the following lines of code on edit_bug (actually located in field.js): 

/* Hide input fields and show the text with (edit) next to it */
function hideEditableField( container, input, action, field_id, original_value ) {
 YAHOO.util.Dom.setStyle(container, 'display', 'inline');
 YAHOO.util.Dom.setStyle(input, 'display', 'none');
 YAHOO.util.Event.addListener(action, 'click', showEditableField,
 new Array(container, input));
 if(field_id != ""){
   YAHOO.util.Event.addListener(window, 'load', checkForChangedFieldValues,
   new Array(container, input, field_id, original_value));
 }
}

As you can tell this doesn't handle your situation of having multiplicable fields or flags. If you'd like me to finish off this idea i'd be happy to work on it when i have time, but that will not be before 3.2 is done, but it would be great for the next version. Let me know how you'd like to proceed.

Comment 4

[David Lawrence [:dkl]]

(In reply to comment #3)
> I like the idea. However, there is at least 1 bug you haven't tested. That bug
> (which is annoying) is what happens when a  user makes an error on entry and
> clicks back (or refresh). The way to handle this is to have an on load that
> checks to see if any of the values have changed. You can guess why this might
> be especially annoying for flags and groups, but if you want to make that work,
> go for it! I have handled this in the following lines of code on edit_bug
> (actually located in field.js): 
> 

Thanks for the pointer. I will definitely need to look at that before we release. this was definitely a proof of concept and in need of refinement.

> As you can tell this doesn't handle your situation of having multiplicable
> fields or flags. If you'd like me to finish off this idea i'd be happy to work
> on it when i have time, but that will not be before 3.2 is done, but it would
> be great for the next version. Let me know how you'd like to proceed.

As I have tons of stuff to work on, I definitely welcome the help if possible. And you definitely have a better grasp of these concepts due to all of the work you have done on the show_bug.cgi page already. If you think you will have some time in the near future to look at it let me know and I can help out. If not then I will fumble around with it some more to see if I can work out the kinks.

Thanks for looking at it!
Dave 

Comment 5

[Frédéric Buclin]

(In reply to comment #2)
> We have quite a few flags 20-30 for some bugs

Wow. Do you keep old flags active? Or do you really need them all for a single bug? This bug (on b.m.o) has 16 flags and I think it's already a lot, and only because we manage 4 branches + trunk.


> Also we can have a long list of groups listed for a single bug (30-40)

How is that possible? Unless you belong to all these groups at once, you pretty much cannot do anything with such bugs as soon as they are restricted to a group or two. Again, do you really need all these groups for a single bug?

Comment 6

[David Lawrence [:dkl]]

(In reply to comment #5)
> (In reply to comment #2)
> > We have quite a few flags 20-30 for some bugs
> 
> Wow. Do you keep old flags active? Or do you really need them all for a single
> bug? This bug (on b.m.o) has 16 flags and I think it's already a lot, and only
> because we manage 4 branches + trunk.
>

For RHEL bugs we do keep some old flags around. Since we broke up the one RHEL bugzilla product into separate major version products in Bugzilla, the amount of flags on a single bug did drop significantly. So for example for RHEL5 bugs
we have 12 flags currently. But as we release more updates the number will increase per bug.

> 
> > Also we can have a long list of groups listed for a single bug (30-40)
> 
> How is that possible? Unless you belong to all these groups at once, you pretty
> much cannot do anything with such bugs as soon as they are restricted to a
> group or two. Again, do you really need all these groups for a single bug?
> 

We are using OR groups instead of AND so a user needs to only be in one or more of the groups checked to see the bug.

Also for RHEL bugs we have a large amount of hardware and software partners whom all have their own group such as Oracle, IBM, Intel, etc. So for RHEL bugs the list of groups can be very long for Red Hat people who need access to all of them. With the 3.2 UI we are getting alot of feedback from users that they do not like to scroll down too far to get to the comments. So if we were not collapsing the group list already, they would have to scroll down significantly to get to the comments.

Comment 7

[Frédéric Buclin]

Comment on attachment 310840 [details] [diff] [review]
Patch to hide full list of flags/groups unless you need to edit them (v1)

Bitrotten. Also:


>Index: template/en/default/bug/edit.html.tmpl

>+            [% FOREACH type = bug.flag_types %]
>+              [% FOREACH flag = type.flags %]
>+                [% setflags = 1 %]
>+                [% flag.setter.nick FILTER html %]:
>+                [%+ type.name FILTER html FILTER no_break %][% flag.status %]
>+                [%+ IF flag.requestee %]
>+                  ([% flag.requestee.nick FILTER html %])
>+                [% END %]<br>
>+              [% END %]
>+            [% END %]


>         [% FOREACH type = bug.flag_types %]
>           [% FOREACH flag = type.flags %]
>+            [% flag.setter.nick FILTER html %]:
>+            [%+ type.name FILTER html FILTER no_break %][% flag.status %]
>+            [%+ IF flag.requestee %]
>+              ([% flag.requestee.nick FILTER html %])
>+            [% END %]<br>
>           [% END %]
>         [% END %]

Put this code in a BLOCK, to avoid code duplication.


That's all I can say for now as I cannot apply your patch on my installation. But ask pyrzak again for review for your updated patch.

Comment 9

[Frédéric Buclin]

dkl, could we get some traction? About your patch (which I couldn't apply as it's bitrotten), I want already set/requested flags to be always visible, and only hide unset ones by default (i.e. the "(edit)" link would only affect flags with no status). Is that doable for 3.8? We freeze in 3-4 weeks.

Comment 10

[Guy Pyrzak]

I might be able to work on this since we're freezing in 3 to 4 weeks, pressure helps to motivate patch writing. DKL let me know if this is not doable for you in the next few weeks.

Comment 11

[David Lawrence [:dkl]]

(In reply to comment #10)
> I might be able to work on this since we're freezing in 3 to 4 weeks, pressure
> helps to motivate patch writing. DKL let me know if this is not doable for you
> in the next few weeks.

It's up to you. I can do it given enough time but if you have spare cycles sooner then feel free. LpSolit and I discussed this in IRC where he described showing the current set/requested flags in their full UI elements and filter out those that are not set and not displaying them. Then have a (additional flags) link or similar which then fills in the extra flags. I took a different approach on b.r.c and in my patch where I show a read-only list of flags initially with a normal (edit) link that then displayed the full web form. Which idea to you like?

Comment 12

[Max Kanat-Alexander]

  I think showing a read-only list of set flags is simplest, in terms of implementation, because then all you have to do is re-use the existing read-only flag set that we already have in the template.

Comment 13

[Frédéric Buclin]

(In reply to comment #12)
>   I think showing a read-only list of set flags is simplest

I really prefer to have editable forms for already set/requested flags. This is not harder to implement.

Comment 14

[Max Kanat-Alexander]

  Okay. :-) LpSolit is the flags owner, so whatever he says goes. :-) I agree that it would be more convenient to show the already-set ones as editable.

Comment 15

[David Lawrence [:dkl]]

Attachment: Patch to hide full list of flags unless you need to edit them (v2)

Here is a new updated patch that hides the unset flags unless you hit (edit). I had to refactor hideEditableField and showEditableField some in field.js to allow for hiding/showing elements with class name instead of by id. I also refactored flag/list.html.tmpl to allow flag_row BLOCK to work for all flag rows and not just for rows displaying a set flag.

Let me know what you think.

dkl

Comment 16

[David Lawrence [:dkl]]

Changing the summary to make this bug just about hiding/showing flag fields and not groups. We can do the group section as a separate bug.

dkl

Comment 17

[Frédéric Buclin]

Comment on attachment 562164 [details] [diff] [review]
Patch to hide full list of flags unless you need to edit them (v2)

>=== modified file 'js/field.js'

>     YAHOO.util.Dom.addClass(input, 'bz_default_hidden');
>+    var inputList = YAHOO.util.Dom.getElementsByClassName(input);

I'm not a fan to see 'input' being both an ID and a class. I think this could be solved by enclosing unset flags between <tbody></tbody> and define an ID for it. This should automatically hide all included rows. This means all changes in this file would be useless.



>=== modified file 'template/en/default/bug/edit.html.tmpl'

>+        <span id="bz_flags_edit_container" class="bz_default_hidden">
>+          <span>(<a href="#" id="bz_flags_edit_action">edit</a>)</span>

Is it really required to have the 2nd <span>? Also, I don't like where (edit) is displayed. When you already have some flags set, they are already displayed and editable, and so (edit) is pretty confusing. I would prefer to see it moved below existing flags (i.e. below the flag table) and remamed to (more flags) if and only if there are more flags to display (there is no check about this here).



>=== modified file 'template/en/default/flag/list.html.tmpl'

>+    [% SET flag = undef %]

undef has no meaning in the TT world. [% flag = "" %] would be better.


>+        <tr class="bz_flags_type_input"><td colspan="3"><hr></td></tr>

As suggested above, I don't like to use bz_flags_type_input as a class name. It should be set as an ID on <tbody>.


>+  <tr [% IF !flag %]class="bz_flags_type_input"[% END %]>

Same here.

>+      <td>[% IF addl_text %][% addl_text FILTER html %][% ELSE %]&nbsp;[% END %]</td>

Either write [% addl_text || "" FILTER html %] or simply [% addl_text FILTER html %] and we don't care if it's undefined.


>+      <label title="[% type.description FILTER html %]"
>+             for="[% IF flag %]flag-[% flag.id %][% ELSE %]flag_type-[% type.id %][% END %]">
>+      [%- type.name FILTER html FILTER no_break -%]</label>

Keep the old indentation. We are within <label></label>.


>+      <select [% IF flag %]
>+              id="flag-[% flag.id %]" name="flag-[% flag.id %]"
>+              [% ELSE %]
>+              id="flag_type-[% type.id %]" name="flag_type-[% type.id %]"
>+              [% END %]

This indentation makes it hard to parse. Please indent id="" correctly.


>-              [% " disabled=\"disabled\"" UNLESS (type.is_requestable && user.can_request_flag(type)) || user.can_set_flag(type) %]

I don't see why this line has been removed. This would suddenly make some flagtypes editable despite they aren't for the current user.


>+      [% ELSIF flag %]

If we come here, then we already know that flag is defined, because we can never enter this block if the flagtype is inactive and there is no flag defined. So [% ELSE %] is enough.


>+        [% IF (type.is_requestable && type.is_requesteeble) || (flag && flag.requestee) %]

You must include type.is_active in the first parens. You removed it inadvertently, it looks like. Else you could set a requestee on an inactive flag.


>+            [% SET flag_multiple = flag ? 0 : type.is_multiplicable * 3 %]
>+            [% SET flag_empty_ok = flag ? 1 : !type.is_multiplicable %]
>+            [% INCLUDE global/userselect.html.tmpl
>+                       multiple => 0
>+                       emptyok  => 1

multiple and emptyok must use flag_multiple and flag_empty_ok, not hardcoded values.


It's a good thing you managed to refactor the code to avoid duplicated code. There are many "IF ELSE END", but this is still readable. :)

Comment 18

[David Lawrence [:dkl]]

Attachment: Patch to hide full list of flags unless you need to edit them (v3)

Thanks for the review. Here is a new patch that addresses your changes.

dkl

Comment 19

[Frédéric Buclin]

Comment on attachment 568291 [details] [diff] [review]
Patch to hide full list of flags unless you need to edit them (v3)

>=== modified file 'template/en/default/bug/edit.html.tmpl'

>+    [% IF !type.flags || type.flags.size < 1 %]
>+      [% show_more_flags = 1 %]
>+    [% END %]

The logic is wrong. You would never display "(more flags)" if all flag types already have flags *and* are multiplicable. So probably
[% IF !type.flags || type.is_multiplicable %] would be better. Also, you must check that the flag type is active.


>+          <span id="bz_flags_more_container" class="bz_default_hidden">
>+            (<a href="#" id="bz_flags_more_action">more flags</a>)
>+          </span>

I didn't check the JS code yet, but I hope all flag types are displayed if JS is turned off.



>=== modified file 'template/en/default/flag/list.html.tmpl'

>+  <tbody [% ' class="bz_flag_type"' IF !flag %]>

Remove the whitespace between <tbody and [% ... %], so that if a flag is passed, then you have <tbody> instead of <tbody >.


>+      [% IF flag %]
>+        <td>
>+          <span title="[% flag.setter.identity FILTER html %]">[% flag.setter.nick FILTER html %]</span>:
>+        </td>
>+      [% ELSE %]
>+        <td>[% addl_text FILTER html %]</td>
>+      [% END %]

Move <td> and </td> outside the IF-ELSE block, to make the code slightly shorter.


>+      <td>
>+        <label title="[% type.description FILTER html %]"
>+               for="[% IF flag %]flag-[% flag.id %][% ELSE %]flag_type-[% type.id %][% END %]">
>+          [%- type.name FILTER html FILTER no_break -%]</label>
>+      </td>
>+      <td>
>+        <select [% IF flag %]
>+                  id="flag-[% flag.id %]" name="flag-[% flag.id %]"
>+                [% ELSE %]
>+                  id="flag_type-[% type.id %]" name="flag_type-[% type.id %]"

You should define [% fid = flag ? "flag-$flag.id" : "flag_type-$type.id" %] and then use [% fid FILTER html %] in <label> and <select> above. This would avoid the duplication of the code, and would call flag.id or type.id only once, which is good.


>+                  [% " disabled=\"disabled\"" UNLESS (type.is_requestable && user.can_request_flag(type)) || user.can_set_flag(type) %]

Nit: write ' disabled="disabled"' to avoid \", which makes the code harder to parse.


Your patch looks good. I think we are very close. :)

Comment 20

[David Lawrence [:dkl]]

Attachment: Patch to hide full list of flags unless you need to edit them (v4)

Thanks. Here is a revised patch addressing your suggested changes.

dkl

Comment 21

[Frédéric Buclin]

Comment on attachment 577198 [details] [diff] [review]
Patch to hide full list of flags unless you need to edit them (v4)

A few things before I r+ your patch:

- every time I click the (more flags) link, the page jumps at the top. This problem doesn't happen with (take) and (edit) for the assignee field, for instance.

- when no flag is set yet, (more flags) alone looks weird, because it means "more than nothing"?! I suggest we replace it by:

  <em>no flag set yet</em>
  (more flags)

Comment 22

[David Lawrence [:dkl]]

Attachment: Patch to hide full list of flags unless you need to edit them (v5)

here is a new patch which fixes the issues/suggestions you found.

1. Added YAHOO.util.Event.preventDefault(e); to suppress the '#' from being added to the URL which made the page jump similar to (edit).
2. When no flags are currently set, show <em>None yet set</em> and also change like to (set flags) instead of (more flags).

dkl

Comment 23

[Frédéric Buclin]

Comment on attachment 577360 [details] [diff] [review]
Patch to hide full list of flags unless you need to edit them (v5)

>=== modified file 'template/en/default/bug/edit.html.tmpl'

>+    [% LAST IF show_bug_flags && show_more_flags %]

You must also add |&& bug_flags_set| here, else you may stop the loop before we had a chance to set this variable. This happened to me while testing your patch. Adding this check fixed the problem.


>+        [% IF show_more_flags %]
>+          <span id="bz_flags_more_container" class="bz_default_hidden">
>+            [% IF !bug_flags_set %]<em>None yet set</em>[% END %]
>+            (<a href="#" id="bz_flags_more_action">[% IF !bug_flags_set %]set[% ELSE %]more[% END %] flags</a>)
>+          </span>
>+        [% END %]

It looks a bit weird to have (more flags) displayed *before* already set flags. IMO, it makes more sense to be displayed *after* existing flags. This means you can merge this [% IF show_more_flags %] block with the one containing the JS code. Just make sure you put this code before <script></script>.


Everything else looks good to me and works fine. r=LpSolit with these two comments fixed on checkin.

Comment 24

[Frédéric Buclin]

Do not forget the fixes on checkin. :)

Comment 25

[David Lawrence [:dkl]]

Thanks. Fixed on checkin.

trunk:
Committing to: bzr+ssh://[e-mail]/bugzilla/trunk
modified template/en/default/filterexceptions.pl
modified template/en/default/flag/list.html.tmpl
modified template/en/default/bug/edit.html.tmpl
Committed revision 8044.

dkl

Comment 26

[Frédéric Buclin]

Added to relnotes for 4.4.