Closed Bug 1050226 Opened 5 years ago Closed 5 years ago

Offer web devs a more focused set of product/components to file bugs in

Categories

(bugzilla.mozilla.org :: Extensions, defect)

Production
x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: Gijs, Assigned: dylan)

References

Details

Attachments

(2 files, 8 obsolete files)

We could probably ask something like:
"Are you reporting a bug with:
- HTML, CSS, JS, SVG, or some other web technology or combination of web
technologies?
- Firefox's developer tools
- Firefox's user interface (for example, an issue with bookmarks, tabbed
browsing or the location bar)
"

and automatically select the right product/component for them (being
Core::Untriaged, Firefox::Developer Tools, and Firefox::Untriaged,
respectively).



(adapted from https://groups.google.com/forum/#!topic/mozilla.tools.bmo/RJYPz3ax9z4 based on feedback from Zack Weinberg)
Depends on: 1050235
Blocks: 1050235
No longer depends on: 1050235
Blocks: 1080933
Assignee: nobody → dylan
Status: NEW → ASSIGNED
Due Date: 2014-10-29
Context (from earlier offline discussion)

> Will need to add an optional parameter to guided bug entry, mode=webdev.
> When that is passed, the initial step in guided.js is set to "webdev" instead from "product".
> The webdev step will show the question listed in the bug, set the product and skip to the step that
> comes after "product" normally.

Status update:

1) Added "webdev" step which is the default step when ?webdev=1 is passed. (So this will not be directly accessible until the other parts of guided bug entry land)
2) Adding a step to the current system requires a javascript object that implements an .onShow and .onInit() at minimum. I've basically copied the 'product' step to do this. Added a step div to the template. After the webdev.onShow() happens, the next step is 'dupes'. 

Still need to remove the (now quite useless) step counter, fix js (this file uses 2-space indent...).
Might need to do something to the "show advanced form" link during the webdev step.
Attached patch bug-1050226-v1.patch (obsolete) — Splinter Review
This ended up being both harder to do, but easier to implement than I had imagined. Originally I thought I would need hooks in webdev.onShow() to set the default step back to product, and that when a ?webdev=1 query parameter was added, set the default first step to "webdev". However, this is wrong. It is entirely possible to just navigate to the form with #h=webdev appended to the end.

Meanwhile, such navigation does not work if you use <a href="#" onclick="blah" links (duh), which is why the existing code uses href="javascript:void(0)". This is probably sub-optimal.

Also, the current way that all steps for the guided bug process are in one file is probably not going to scale if we add a lot more custom steps in the future.

Note, in testing you'll need to navigate to enter_bug.cgi?format=guided#h=webdev, and make sure Firefox::Untriaged, Core::Untriaged, and Firefox::Developer Tools exist.
At least that last one had to be created on my box.
Attachment #8513543 - Flags: review?(glob)
Attached patch bug-1050226-v1.1.patch (obsolete) — Splinter Review
woops. I hadn't added back the generic product selector at the bottom -- which I think should be there.
Attachment #8513543 - Attachment is obsolete: true
Attachment #8513543 - Flags: review?(glob)
Attachment #8513545 - Flags: review?(glob)
Comment on attachment 8513545 [details] [diff] [review]
bug-1050226-v1.1.patch

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

t/009bugwords.t ...... 1/627
#   Failed test 'extensions/GuidedBugEntry/template/en/default/guided/guided.html.tmpl contains invalid bare words (e.g. 'bug') --WARNING'

the bugs are not filed into the 'developer tools' component when selecting the second option, most likely due to the 'defaultComponent' being set in products.js.

::: extensions/GuidedBugEntry/template/en/default/pages/guided_products.js.tmpl
@@ +8,5 @@
>  
>  [%# this file allows us to pull in data defined in the BMO ext %]
>  
> +[% IF webdev %]
> +guided.webdev = true;

nit: indent
Attachment #8513545 - Flags: review?(glob) → review-
Due Date: 2014-10-29 → 2014-11-03
(In reply to Byron Jones ‹:glob› from comment #4)
> Comment on attachment 8513545 [details] [diff] [review]
> bug-1050226-v1.1.patch
> 
> Review of attachment 8513545 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> t/009bugwords.t ...... 1/627
> #   Failed test
> 'extensions/GuidedBugEntry/template/en/default/guided/guided.html.tmpl
> contains invalid bare words (e.g. 'bug') --WARNING'
> 
> the bugs are not filed into the 'developer tools' component when selecting
> the second option, most likely due to the 'defaultComponent' being set in
> products.js.

Defeating the default component logic was easy, if I always defeat it. It looks like I'm going to need the ?webdev=1 query string param after all. Quite frustrating!
Due Date: 2014-11-03 → 2014-11-04
Attached patch bug-1050226-v2.patch (obsolete) — Splinter Review
Revised patch, defaults to the right component.
Note I've noticed some unrelated odd behavior with the component selection drop down, but not sure if that is a bug.
Attachment #8513545 - Attachment is obsolete: true
Attachment #8519465 - Flags: review?(glob)
Attached patch bug-1050226-v2.1.patch (obsolete) — Splinter Review
Fixed typo
Attachment #8519465 - Attachment is obsolete: true
Attachment #8519465 - Flags: review?(glob)
Attachment #8520003 - Flags: review?(glob)
Comment on attachment 8520003 [details] [diff] [review]
bug-1050226-v2.1.patch

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

this is better, however..

- we shouldn't allow the component to be changed on the last step
- clicking on 'change' from the last step takes you back to 'product' step, however it should take you to the webdev step

::: extensions/GuidedBugEntry/template/en/default/guided/guided.html.tmpl
@@ +112,5 @@
> +  <li><a href="javascript:void(0)"
> +         onclick="product.select('Firefox', 'Untriaged');">
> +         Firefox's user interface (for example, an issue with bookmarks,
> +         tabbed browsing or the location bar)</a>
> +  </li>

i'm thinking these options could do with some icons to make them stand out.
i suggest:

html/css/js/svg : use the "Core" icon
dev tools : use the devedition firefox icon (https://db.tt/clqtPzhr)
ui : use the normal firefox icon

::: extensions/GuidedBugEntry/web/js/guided.js
@@ +204,4 @@
>      }
>  
>      // show/hide component selection row
> +    if (products[productName] && products[productName].noComponentSelection && !guided.webdev) {

would it be possible/viable for the webdev step to instead override/set the product's defaultComponent?

if it's too fiddly, continuing with this approach is ok.
Attachment #8520003 - Flags: review?(glob) → review-
(In reply to Byron Jones ‹:glob› from comment #8)
> Comment on attachment 8520003 [details] [diff] [review]
> bug-1050226-v2.1.patch
> 
> Review of attachment 8520003 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> this is better, however..
> 
> - we shouldn't allow the component to be changed on the last step
> - clicking on 'change' from the last step takes you back to 'product' step,
> however it should take you to the webdev step
Ah! right, I meant to detect webdevness there and redirect to the webdev step.

> ::: extensions/GuidedBugEntry/template/en/default/guided/guided.html.tmpl
> @@ +112,5 @@
> > +  <li><a href="javascript:void(0)"
> > +         onclick="product.select('Firefox', 'Untriaged');">
> > +         Firefox's user interface (for example, an issue with bookmarks,
> > +         tabbed browsing or the location bar)</a>
> > +  </li>
> 
> i'm thinking these options could do with some icons to make them stand out.
> i suggest:
> 
> html/css/js/svg : use the "Core" icon
> dev tools : use the devedition firefox icon (https://db.tt/clqtPzhr)
> ui : use the normal firefox icon

Ah, yes. Should each list item be more boxy as a result?

> ::: extensions/GuidedBugEntry/web/js/guided.js
> @@ +204,4 @@
> >      }
> >  
> >      // show/hide component selection row
> > +    if (products[productName] && products[productName].noComponentSelection && !guided.webdev) {
> 
> would it be possible/viable for the webdev step to instead override/set the
> product's defaultComponent?
> 
> if it's too fiddly, continuing with this approach is ok.
It is a little fiddly, I tried doing that before. When I'm working #1 above I'll see what can be done.
(it does feel crufty right now).
Due Date: 2014-11-04
This feature now depends on the improvements made in bug 1050232, so that the questions asked can be shown in little boxes.
Attached patch bug-1050226-v3.patch (obsolete) — Splinter Review
With product icons, component non-editable and other enhancements. I also removed some suspected dead code (exit_block) in the templates.
Attachment #8520003 - Attachment is obsolete: true
Attachment #8527244 - Flags: review?(glob)
Comment on attachment 8527244 [details] [diff] [review]
bug-1050226-v3.patch

as this relies on bug 1050232, i'm clearing this review until that bug has an r+'d patch.
Attachment #8527244 - Flags: review?(glob)
Attached patch bug-1050226-v4.patch (obsolete) — Splinter Review
To review, apply after Bug 1050232.

No changes, just rebased on current Bug 1050232,
Attachment #8527244 - Attachment is obsolete: true
Attachment #8537867 - Flags: review?(glob)
Right! I was also going to mention: I sent this in for review before the r+ of the other patch so that one doesn't block the other. Mark thought it was a good idea. :-)
Comment on attachment 8537867 [details] [diff] [review]
bug-1050226-v4.patch

as this relies on bug 1050232, i'm clearing this review until that bug has an r+'d patch.
Attachment #8537867 - Flags: review?(glob)
Comment on attachment 8537867 [details] [diff] [review]
bug-1050226-v4.patch

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

the iconified page looks much better, great work :)

however looks like the fixes for these two issues were missed in this revision:
- we shouldn't allow the component to be changed on the last step
- clicking on 'change' from the last step takes you back to 'product' step, however it should take you to the webdev step

::: extensions/GuidedBugEntry/template/en/default/guided/guided.html.tmpl
@@ +269,4 @@
>        </div>
>      </td>
>      <td class="exit_text_last">
> +      <a href="enter_bug.cgi?format=guided&amp;webdev=1">Report an issue with Firefox on a site that I've developed</a><br>

this should probably take you direct to core|untraiged

it seems odd for a link that says it's for reporting issues with a site then then ask if they want to report a bug in the devtools or firefox ui.

::: extensions/GuidedBugEntry/web/js/guided.js
@@ +133,5 @@
> +    if (componentName) {
> +      this.setPreselectedComponent(componentName);
> +      if (prod && prod.defaultComponent) {
> +        prod.originalDefaultComponent = prod.originalDefaultComponent || prod.defaultComponent;
> +        prod.defaultComponent = componentName; 

nit: there be a trailing space here.
Attachment #8537867 - Flags: review-
Hmm, I remember specifically addressing "we shouldn't allow the component to be changed on the last step" before, but you're right, that is not done here.

Now that the UI-changing patch has landed, I can clean this one cleaned up properly.
(In reply to Byron Jones ‹:glob› (limited availability until 19th jan) from comment #16)
> Comment on attachment 8537867 [details] [diff] [review]
> bug-1050226-v4.patch
>
> however looks like the fixes for these two issues were missed in this
> revision:
> - we shouldn't allow the component to be changed on the last step
In all cases? I have verified that the *existing* behavior (with respect to noComponentSelection) is still in place. That is, if a product in products.js has noComponentSelection, no component can be selected (by the user). Did you encounter something different than this?

Screenshot to follow
Flags: needinfo?(glob)
Attached image last-step.png
Attached patch bug-1050226-v5.patch (obsolete) — Splinter Review
This works for me -- making setStep('default') be the action of the "Change" links. This results in the default step being selected, which is changed via the webdev mode already.
Attachment #8537867 - Attachment is obsolete: true
Flags: needinfo?(glob)
Attachment #8545486 - Flags: review?(glob)
(In reply to Dylan William Hardison [:dylan] from comment #18)
> > - we shouldn't allow the component to be changed on the last step
> In all cases?

depends on what you mean by "in all cases" :)

> I have verified that the *existing* behavior (with respect to
> noComponentSelection) is still in place. That is, if a product in
> products.js has noComponentSelection, no component can be selected (by the
> user). Did you encounter something different than this?

from comment 0:
> and automatically select the right product/component for them (being
> Core::Untriaged, Firefox::Developer Tools, and Firefox::Untriaged,
> respectively).

currently selecting "devtools" or "firefox ui" results in the component select being hidden, however selecting "web technologies" does not.  selecting any of the three web-dev options should pre-select and hide the component select.

the behaviour of non-web-dev shouldn't change.
Comment on attachment 8545486 [details] [diff] [review]
bug-1050226-v5.patch

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

as per comment 21, the component select needs to be hidden.

the other changes look good, except i get a weird syntax error when selecting the "report an issue with a website.." option:
SyntaxError: expected expression, got ')'
Attachment #8545486 - Flags: review?(glob) → review-
(In reply to Byron Jones ‹:glob› (limited availability until 19th jan) from comment #21)
> (In reply to Dylan William Hardison [:dylan] from comment #18)
> > > - we shouldn't allow the component to be changed on the last step
> > In all cases?
> 
> depends on what you mean by "in all cases" :)
> 
> > I have verified that the *existing* behavior (with respect to
> > noComponentSelection) is still in place. That is, if a product in
> > products.js has noComponentSelection, no component can be selected (by the
> > user). Did you encounter something different than this?
> 
> from comment 0:
> > and automatically select the right product/component for them (being
> > Core::Untriaged, Firefox::Developer Tools, and Firefox::Untriaged,
> > respectively).
> 
> currently selecting "devtools" or "firefox ui" results in the component
> select being hidden, however selecting "web technologies" does not. 
> selecting any of the three web-dev options should pre-select and hide the
> component select.
> 
> the behaviour of non-web-dev shouldn't change.
I will have to shadow the noComponentSelection when in webdev mode, I suppose, so any webdev component is "fixed" in webdev mode but not outside of it.
I've been in a style hole, for no other reason that I realized the alternating row colors is broken when component selection is hidden. If that unsightliness is acceptable I may just go ahead with it (it's hidden for all components if and only if webdev mode is on) and worry about redoing the bugform in a more modern, HTML5-y way later.
Maybe throw it up on bugzilla-dev so we can see how ugly it may or may not be?
(In reply to Mark Côté [:mcote] from comment #25)
> Maybe throw it up on bugzilla-dev so we can see how ugly it may or may not
> be?

I'd appreciate being able to poke at a test install myself -- not being familiar with the guts of Bugzilla, it's almost impossible for me to tell what the end result will be like from the patches.
(In reply to Zack Weinberg (:zwol) from comment #26)
> (In reply to Mark Côté [:mcote] from comment #25)
> > Maybe throw it up on bugzilla-dev so we can see how ugly it may or may not
> > be?
> 
> I'd appreciate being able to poke at a test install myself -- not being
> familiar with the guts of Bugzilla, it's almost impossible for me to tell
> what the end result will be like from the patches.

That's a good idea.
Code (finally) up on dev. Here's a link to the form in question. This is normally the last step in the guided bug process.

https://bugzilla-dev.allizom.org/enter_bug.cgi?format=guided&webdev=1#h=bugForm|Firefox|Developer+Tools
Attached patch bug-1050226-v6.patch (obsolete) — Splinter Review
Attachment #8545486 - Attachment is obsolete: true
Flags: needinfo?(zackw)
Zack, what do you think of the form in the final step? Is the even/odd row coloring (which is broken by hiding a row) worth fixing now, or should it remain until the form can be overhauled?
Attachment #8546833 - Flags: review?(glob)
(In reply to Dylan William Hardison [:dylan] from comment #30)
> Zack, what do you think of the form in the final step? Is the even/odd row
> coloring (which is broken by hiding a row) worth fixing now, or should it
> remain until the form can be overhauled?

If you're talking about what I think you're talking about (the "product" row having the same background as the "What did you do?" row), it's a little weird but I wouldn't worry about it right now, considering that the form itself is still presenting the same generic steps-to-reproduce template that is a poor fit for webdev Core bugs.  I think fixing that is much more important than a cosmetic issue with the background colors.

I'm not sure I would use even/odd row coloring for this form at all -- I don't think it works especially well, especially the way the large textareas are given a different color than their prompts -- but I'm not a graphic designer.

(As long as we're talking graphical nits, can you make the "Clear" button and "File description" row hidden when no file has been attached?  That's orthogonal, but seems like it would be an improvement across the board.)
Flags: needinfo?(zackw)
Comment on attachment 8546833 [details] [diff] [review]
bug-1050226-v6.patch

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

dropping the background banding makes sense :)  #E0E0E0 for all rows looks nice.

> can you make the "Clear" button and "File description" row hidden when no file has been attached?
+1

soft r- to address these nits.

::: extensions/GuidedBugEntry/template/en/default/guided/guided.html.tmpl
@@ +53,4 @@
>  
>  <script type="text/javascript">
>  YAHOO.util.Dom.addClass('loading', 'hidden');
> +guided.init({ webdev: [% IF webdev; GET "true"; ELSE; GET "false"; END %] });

[% webdev ? "true" : "false" %]
Attachment #8546833 - Flags: review?(glob) → review-
Changes per soft r- -- unsure if that meant "commit after fixing" or if another review cycle should be done.
Attachment #8546833 - Attachment is obsolete: true
Attachment #8554825 - Flags: review?(glob)
(In reply to Dylan William Hardison [:dylan] from comment #33)
> Changes per soft r- -- unsure if that meant "commit after fixing" or if
> another review cycle should be done.

an r- always means another review cycle :)
Comment on attachment 8554825 [details] [diff] [review]
bug-1050226-v7.patch

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

r=glob
Attachment #8554825 - Flags: review?(glob) → review+
I realize this is not discoverable until bug 1050224 is complete. Should it still be commited?
Flags: needinfo?(glob)
(In reply to Dylan William Hardison [:dylan] from comment #36)
> I realize this is not discoverable until bug 1050224 is complete. Should it
> still be commited?

yes.  it will allow us to give the direct url to people who want to both file firefox bugs and help test bmo :)
Flags: needinfo?(glob)
To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   1ccc683..2344279  master -> master
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Component: Extensions: GuidedBugEntry → Extensions
You need to log in before you can comment on or make changes to this bug.