669314 - tidy up enter_bug when all fields are visible

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: User Interface, enhancement)

Description

[:glob ✱]

enter_bug when all fields are visible has a number of problems:

- the most important field, the description, is pushed down the page a lot, mostly by the custom fields
- the first textarea field on the page is the crash-signature custom field, not the description field
- the vertical spacing of the assignee, qa, cc, alias and url fields is massive, due to the number of flags

a quick fix for the first two issues is to move the custom fields to be displayed after the 'blocks' field.

Comment 1

[David Lawrence [:dkl]]

I agree that this would make it simpler for those that need to use the Advanced view for whatever reason but not necessarily need to see the custom fields. I think quite a few people would welcome this minor change.

dkl

Comment 2

[:glob ✱]

we should also add a link to the guided form for the selected product.

Comment 3

[:glob ✱]

also:

  - only run platform and op_sys detection on selected products (as
    per the guided form)

  - if a platform and/or op_sys is provided in the query_string, don't
    run the detection routine

Comment 4

[:glob ✱]

Attachment: patch v1

changing the form as per comment 3 had a greater impact than i was comfortable doing at this stage, so those changes aren't included here.

Comment 5

[:glob ✱]

Attachment: guided.png

Comment 6

[:glob ✱]

Attachment: screenshot (advanced, flags collapsed)

Comment 7

[:glob ✱]

Attachment: screenshot (advanced, flags expanded)

Comment 8

[:glob ✱]

Attachment: patch v2

fixes bitrot, adds a 'take' button for the assignee.

Comment 9

[David Lawrence [:dkl]]

Initial review comments, more to come...

t/008filter.t ........ 222/278 
#   Failed test '(en/default) template/en/default/bug/create/create.html.tmpl has unfiltered directives:
#   800: product.name FILTER url 
#   802: product.name FILTER url 
# --ERROR'
#   at t/008filter.t line 130.
# Looks like you failed 1 test of 278.
t/008filter.t ........ Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/278 subtests

s/url/url_quote/
I realize that 'url' is a valid filter in TT2 but the rest of Bugzilla uses url_quote for consistency. For 4.2, everything switched to 'uri' instead so we will need to do the same when porting this to 4.2.

t/009bugwords.t ...... 260/278 
#   Failed test 'template/en/default/bug/create/create.html.tmpl contains invalid bare words (e.g. 'bug') --WARNING'
#   at t/009bugwords.t line 90.
# Looks like you failed 1 test of 278.

Also no need for a filter here:
+<input type="button" id="btn_no_bug_flags" value="Don't set [% terms.bug FILTER html %] flags"

Comment 10

[David Lawrence [:dkl]]

Comment on attachment 569299 [details] [diff] [review]
patch v2

Review of attachment 569299 [details] [diff] [review]:
-----------------------------------------------------------------

I would also move the deadline and estimated time fields to the bottom. I realize that it is not widely
seen due to the timetracking permission restriction, but it does make the page look better for the 47 people
who can see it (and us admins) :) They are not even using it that much. Those fields have been changed
a total of 18 times in the activity table (sanitized db).

Otherwise definitely an improvement so far. 
dkl

::: js/create_bug.js
@@ +9,5 @@
> +function handleWantsBugFlags(wants) {
> +    if (wants) {
> +        hideElementById('bug_flags_false');
> +        showElementById('bug_flags_true');
> +    }

Nit: inconsistent indentation.

::: template/en/default/bug/create/create.html.tmpl
@@ +53,5 @@
> +
> +function hideCrashSignatureField() {
> +  var el = document.getElementById('cf_crash_signature');
> +  if (!el) return;
> +  hideEditableField('cf_crash_signature_container','cf_crash_signature_input','cf_crash_signature_action', 'cf_crash_signature', '');

Nit: break this line up.

@@ +384,5 @@
>           emptyok => 1
>           custom_userlist => assignees_list
>         %]
> +       &nbsp;(<a title="Assign to yourself" href="#"
> +                 onclick="return take_bug('[% user.login FILTER js %]')">take</a>)

It would be better if this was hidden once clicked similar to the (edit) links on show_bug.cgi. Also if the assigned to is later changes to something other than the user, put the (take) link back.

@@ +400,5 @@
> +       value => qa_contact
> +       disabled => qa_contact_disabled
> +       size => 30
> +       emptyok => 1
> +       custom_userlist => qa_contacts_list

Nit: If these lines are changing anyway, line up the =>'s :)

@@ +644,5 @@
> +</tbody>
> +
> +<tbody class="expert_fields">
> +[%# crash-signature handling %]
> +[% UNLESS cf_hidden_in_product('cf_crash_signature', product.name, component.name, 1) %]

Move the <tbody> tags inside [% UNLESS %] check.

@@ +672,5 @@
> +[% END %]
> +
> +[% display_flags = 0 %]
> +[% any_flags_requesteeble = 0 %]
> +[% IF product.flag_types(is_active=>1).bug.size > 0 %]

This check is not really needed.

@@ +676,5 @@
> +[% IF product.flag_types(is_active=>1).bug.size > 0 %]
> +  [% FOREACH flag_type = product.flag_types(is_active=>1).bug %]
> +    [% display_flags = 1 %]
> +    [% SET any_flags_requesteeble = 1 IF flag_type.is_requestable && flag_type.is_requesteeble %]
> +  [% END %]

To short circuit this loop you can add

  [% LAST IF display_flags && any_flags_requesteeable %]

Comment 11

[:glob ✱]

Attachment: patch v3

thanks dkl; here's an updated version which addresses your review points.

Comment 12

[David Lawrence [:dkl]]

Still failing t/009bugwords.t:

not ok 267 - template/en/default/bug/create/create.html.tmpl contains invalid bare words (e.g. 'bug') --WARNING
#   Failed test 'template/en/default/bug/create/create.html.tmpl contains invalid bare words (e.g. 'bug') --WARNING'
#   at t/009bugwords.t line 90.
801: "
   >Switch to the Bugzilla Helper</a>
</div>

s/Bugzilla/[% terms.Bugzilla %]/

Otherwise, code looks complete and the functionality is working as expected. While we are picking at this I have a couple small cosmetic changes I think would add a little more polish to the page. 

Currently the section with Status, Assignee, QA Contact and CC looks awkward the way it is laid out. How about:

Bug Status: [NEW]                      QA Contact: [                ]
  Assignee: [                ]                 CC: [                ]

That would compress the page some more.

Also the new layout for deadline and estimated time is laid out wrong as well due to the vertical gap that is being added between columns. Things should be laid out together in their logical groups. I think instead we should do:

Depends on: [                ]              Orig. Est: [                ]
    Blocks: [                ]               Deadline: [                ]

Thanks
dkl

Comment 13

[:glob ✱]

(In reply to David Lawrence [:dkl] from comment #12)
> Still failing t/009bugwords.t:

gah, sorry.

> Currently the section with Status, Assignee, QA Contact and CC looks awkward
> the way it is laid out. How about:
> 
> Bug Status: [NEW]                      QA Contact: [                ]
>   Assignee: [                ]                 CC: [                ]
>
> That would compress the page some more.

the idea is to group similar items together, not just to compact the page.

you're probably thinking about the blank space to the right of CC -- this is where the "default cc" list is display.  try adding a bug to "bugzilla > documentation".

> Also the new layout for deadline and estimated time is laid out wrong as
> well due to the vertical gap that is being added between columns. Things
> should be laid out together in their logical groups. I think instead we
> should do:
> 
> Depends on: [                ]              Orig. Est: [                ]
>     Blocks: [                ]               Deadline: [                ]

in 99% of cases the time tracking fields are not displayed, and this layout would result in wasted space.  i'll play with making the depends/blocks fields look better in their current layout.

Comment 14

[:glob ✱]

Attachment: patch v4

Comment 15

[David Lawrence [:dkl]]

Comment on attachment 569905 [details] [diff] [review]
patch v4

Looks good to me for now. If we think of anything else we can tweak it more later. I like the time fields now that the are separate from the dependency fields. r=dkl

Comment 16

[:glob ✱]

\o/ thanks dkl :)

Committing to: bzr+ssh://[e-mail]/bmo/4.0/
modified extensions/BMO/template/en/default/bug/create/user-message.html.tmpl
added extensions/BMO/web/images/guided.png
modified js/TUI.js
added js/create_bug.js
added skins/custom/create_bug.css
modified template/en/default/bug/create/create.html.tmpl
Committed revision 7916.