706680 - Can we integrate the orange factor link with BMO's bug view page

Status: RESOLVED FIXED

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

Description

[cmtalbert]

Heather made a change to the bugzilla tweaks extension that tied together our orangefactor bug view page with a specific bug, provided that bug was already in the orange factor database.  

This way bugs that are tracking "oranges" (intermittent test failures) can be linked to their corresponding orangefactor bug page so that someone investigating that bug can quickly tell whether the bug is getting better or worse.

For example, if you were investigating the intermittent orange tracked by bug 687972 then it would be nice to have a link to that bug's orange factor page: http://brasstacks.mozilla.com/orangefactor/?display=Bug&endday=2011-11-30&startday=2011-10-01&bugid=687972 on the bug details view (i.e. this page: https://bugzilla.mozilla.org/show_bug.cgi?id=687972).

That's what heather had created in her code.  You can steal her code from here: https://github.com/harthur/bztweaks/tree/orange

And if you have questions about querying the orange factor database, jgriffin can answer those.

If the query fails for any reason, simply defaulting to not show the link is a perfectly fine fallback in my opinion.  So, I wouldn't expect this to adversely impact bugzilla.

Comment 1

[cmtalbert]

blassey, you'd asked for this functionality earlier today in the mobile automation meeting.  when you asked me about I thought, "didn't we already do that?"

Turns out we did, but hardly anyone now has bugzilla tweaks installed since so much of what bugzilla tweaks gave us is now in bugzilla proper.  So we should move this into bugzilla proper too, I think.

Comment 2

[David Lawrence [:dkl]]

Attachment: Patch to add OrangeFactor BMO extension for showing WOO stats on bug page (v1)

Ok, new native BMO extension that gives similar functionality to Harth's bztweaks code that displays OrangeFactor information on the show_bug.cgi page.

1. Converted from jquery to YUI/plain javascript.
2. Uses brasstack's new JSONP support since FF extensions can access directly but Bugzilla pages cannot.
3. When the whiteboard does not contain [orange], then the information is not displayed and brasstacks is not contacted.
4. If the user has the Orange Factor Bugzilla preference set to off, then it is not displayed and brasstacks is not contacted.
5. Using a different javascript lib for displaying Sparkline graphs. We can tweak the colors, size, etc. to our hearts content.

Currently http://brasstacks.mozilla.com/orangefactor is intermittently very slow at times and so it may take a while for the stats to show up in the orange factor box. Sometimes it is rather quick as well. Mcotes says it may need to be rebooted but has not been able to yet.

I will put this on https://bugzilla-stage-tip.mozilla.org for you guys to play around with.

Please review
dkl

Comment 3

[Reed Loden [:reed]]

Comment on attachment 578710 [details] [diff] [review]
Patch to add OrangeFactor BMO extension for showing WOO stats on bug page (v1)

Loading stuff from an HTTP site is a no-no. All connections *MUST* be over SSL. https://brasstacks.mozilla.com/ seems to work, so just s/http/https/ in most places. Need to make sure that brasstacks isn't loading anything over HTTP itself, though.

Comment 4

[Reed Loden [:reed]]

In fact, this needs a security review before it goes live.

Comment 5

[David Lawrence [:dkl]]

Attachment: Patch to add OrangeFactor BMO extension for showing WOO stats on bug page (v2)

Thanks reed. I have changed to https as suggested. The brasstacks API does not support https currently but it is being fixed. Also I will schedule a sec review for this as well before going live.

dkl

Comment 6

[Mark Côté [:mcote]]

https is now supported.

By the way, Orange Factor has already had a security review (bug 679072).  There haven't been any major changes to it since.

Comment 7

[David Lawrence [:dkl]]

(In reply to Mark Côté ( :mcote ) from comment #6)
> https is now supported.
> 
> By the way, Orange Factor has already had a security review (bug 679072). 
> There haven't been any major changes to it since.

Yeah but this is new code in Bugzilla itself, so it may require a fresh look from BMO standpoint.

dkl

Comment 8

[:glob ✱]

Comment on attachment 578715 [details] [diff] [review]
Patch to add OrangeFactor BMO extension for showing WOO stats on bug page (v2)

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

where does sparklines come from, and what license does it use?

this doesn't work on IE8, probably due to it using Date.now(), but there may be other issues.

::: extensions/OrangeFactor/Extension.pm
@@ +55,5 @@
> +        }
> +        return;
> +    } elsif ($file ne 'bug/edit.html.tmpl') {
> +        return;
> +    }

this elsif block does nothing, remove.

::: extensions/OrangeFactor/template/en/default/hook/bug/edit-after_custom_fields.html.tmpl
@@ +26,5 @@
> +
> +[% IF orange_factor %]
> +  <tr>
> +    <td class="" colspan="2">
> +      <input type="hidden" id="orange-bug-id" value="[% bug.bug_id FILTER html %]">

you don't need to add 'orange-bug-id', use the existing document.forms['changeform']['id'] value.

::: extensions/OrangeFactor/template/en/default/hook/global/setting-descs-settings.none.tmpl
@@ +28,5 @@
> +  # ***** END LICENSE BLOCK *****
> +  #%]
> +
> +[%
> +  setting_descs.orange_factor = "When viewing a $terms.bug, show it's corresponding Orange Factor page"

s/it's/its/ :)

::: extensions/OrangeFactor/web/orange_factor.js
@@ +89,5 @@
> +    var script = document.createElement('script');
> +    Dom.setAttribute(script, 'src', url);
> +    Dom.setAttribute(script, 'type', 'text/javascript');
> +    var head = document.getElementsByTagName('head')[0];
> +    head.appendChild(script);

is it worth doing this in show-header-end hook, rather than waiting for dom to load first?

::: extensions/OrangeFactor/web/style.css
@@ +6,5 @@
> +    float: left;
> +    margin: 7px 0 15px 0;
> +    box-shadow: 2px 2px 5px orange; 
> +    text-decoration: none;
> +}

i'd prefer for this box to look more like the rest of the bmo ui, in terms of font-size and shading.  right now the header and text are larger than all other bugzilla fields, and the box-shadow makes it stand out unnecessarily.

it should be a normal <th><td> without the border, using the standard bmo font size and colours.

Comment 9

[David Lawrence [:dkl]]

(In reply to Byron Jones ‹:glob› from comment #8)
> where does sparklines come from, and what license does it use?

I found it at https://github.com/lethain/sparklines.js. I have added the licensing information to the top of the js file.

> ::: extensions/OrangeFactor/web/orange_factor.js
> @@ +89,5 @@
> > +    var script = document.createElement('script');
> > +    Dom.setAttribute(script, 'src', url);
> > +    Dom.setAttribute(script, 'type', 'text/javascript');
> > +    var head = document.getElementsByTagName('head')[0];
> > +    head.appendChild(script);
> 
> is it worth doing this in show-header-end hook, rather than waiting for dom
> to load first?

No. Any javascript libs loaded in show-header-end will not be loaded tile later so javascript code in the same template will not execute properly. Also as soon as the <script src=""> tag is inserted it will try to run the returned function that renders the information in the orange factor box, so the DOM needs to be complete for it to find it.

> ::: extensions/OrangeFactor/web/style.css
> @@ +6,5 @@
> > +    float: left;
> > +    margin: 7px 0 15px 0;
> > +    box-shadow: 2px 2px 5px orange; 
> > +    text-decoration: none;
> > +}
>
> i'd prefer for this box to look more like the rest of the bmo ui, in terms
> of font-size and shading.  right now the header and text are larger than all
> other bugzilla fields, and the box-shadow makes it stand out unnecessarily.
> 
> it should be a normal <th><td> without the border, using the standard bmo
> font size and colours.

Agreed. I copied over what was already there to get the first iteration up and running for you to look at the basic structure. I will work on making it look more BMO like.

dkl

Comment 10

[David Lawrence [:dkl]]

Attachment: Patch to add OrangeFactor BMO extension for showing WOO stats on bug page (v3)

New patch with suggest changes. See if this works better for IE. Problem is Date.now only works for Firefox. So I switched to using dateTime() instead which works across more browsers.

Problem is it is hard to test as brasstacks API has been pretty unreliable this week or so. Sometimes it times out and sometimes it comes back quickly.

dkl

Comment 11

[:glob ✱]

Comment on attachment 579222 [details] [diff] [review]
Patch to add OrangeFactor BMO extension for showing WOO stats on bug page (v3)

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

> I found it at https://github.com/lethain/sparklines.js
> I have added the licensing information to the top of the js file.

you need to include the full license file, not just a link to it.
it'd be better to do this in a separate file rather than in the js file itself.


the layout looks *much* nicer now.

it may be better to display them all on the same line, but i'm equally happy with the current layout.

current:
(link)
[canvas]
5 failures in the past day

proposed:
[canvas] 5 failures in the past day (link)

::: extensions/OrangeFactor/web/orange_factor.js
@@ +47,5 @@
> +    OrangeFactor.displayCount(days[days.length - 1]);
> +}
> +
> +OrangeFactor.displayGraph = function (dayCounts) {
> +    var max = dayCounts.reduce(function(max, count) {

array.reduce isn't supported until IE9.

@@ +65,5 @@
> +
> +OrangeFactor.displayCount = function (count) {
> +    var count = YAHOO.util.Dom.get('orange-count');
> +    count.appendChild(document.createTextNode(count + ' failures in the past day'));
> +}

oops, you're using the same variable for the element and the count, resulting in "[object HTMLDivElement] failures in the past day".

Comment 12

[Dave Lawrence]

Attachment: Patch to add OrangeFactor BMO extension for showing WOO stats on bug page (v4)

Thanks glob. I have made a new patch with the suggested changes/fixes for review.

dkl

Comment 13

[Dave Lawrence]

Attachment: Screenshot of Orange Factor stats on bug page

Here is a screen shot showing the new layout.

Comment 14

[:glob ✱]

Comment on attachment 579421 [details] [diff] [review]
Patch to add OrangeFactor BMO extension for showing WOO stats on bug page (v4)

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

still fails on IE:

Object doesn't support this property or method
sparklines.min.js line 28
you probably need to include excanvas in the sparklines minified script.

that said, this breaks other functionality, such as the expanding of tracking flags, by messing with Array's prototype :(

i vote for detecting if the current browser supports canvas, array.reduce and array.map, and displaying an 'unsupported' blurb if it doesn't.
if that pans out to be too tricky (i'm not sure how you'd do that with the jsonp script loading dance), i'd settle for making this feature disabled on IE (any version).

::: extensions/OrangeFactor/template/en/default/hook/bug/show-header-end.html.tmpl
@@ +29,5 @@
> +  #%]
> +
> +[% IF orange_factor %]
> +  [% style_urls.push('extensions/OrangeFactor/web/style.css') %]
> +  [% yui.push('json') %]

you aren't using yui's json lib, so this line can safely be removed.

::: extensions/OrangeFactor/web/orange_factor.js
@@ +119,5 @@
> +    var endday = OrangeFactor.dateString(new Date(OrangeFactor.getCurrentDateMs() - 1 * OrangeFactor.dayMs));
> +    var startday = OrangeFactor.dateString(new Date(OrangeFactor.getCurrentDateMs() - (OrangeFactor.limit + 1) * OrangeFactor.dayMs));
> +    var url = "https://brasstacks.mozilla.com/orangefactor/api/count?startday=" + encodeURIComponent(startday) +
> +              "&endday=" + encodeURIComponent(endday) + "&bugid=" + encodeURIComponent(bugid) + 
> +               "&callback=OrangeFactor.getOrangeCount";

nit: indentation is wrong.

@@ +129,5 @@
> +    head.appendChild(script);
> +
> +    var count = YAHOO.util.Dom.get('orange-count');
> +    Dom.removeClass(count, 'bz_default_hidden');
> +    count.innerHTML = 'Loading stats...';

the abbreviation "stats" may not be well understood by esl users, go with 'Loading...'

::: extensions/OrangeFactor/web/sparklines.min.js
@@ +4,5 @@
> + * More information: https://github.com/lethain/sparklines.js
> + *
> + * Processing.js - John Resig (http://ejohn.org/)
> + * See LICENSE.processing.js and AUTHORS.processing.js
> + * More information: http://processing.org/

the correct url is processingjs.org

Comment 15

[David Lawrence [:dkl]]

Attachment: Patch to add OrangeFactor BMO extension for showing WOO stats on bug page (v5)

Thanks for the review. I have addressed your changes as well as disabled the chart/counts for all browsers except Gecko for now. We can change this later as things improve. Non-gecko browsers will still get the link generated so it is still useful IMO.

dkl

Comment 16

[:glob ✱]

Comment on attachment 579863 [details] [diff] [review]
Patch to add OrangeFactor BMO extension for showing WOO stats on bug page (v5)

+[% IF orange_factor && cgi.user_agent.match('(?i)gecko') %]

this check of the useragent string won't restrict display to gecko only; all webkit based browsers, including safari and chrome, include "like gecko" in their ua.

however this isn't a problem; the code doesn't break on webkit, so it's all good.

r=glob

Comment 17

[David Lawrence [:dkl]]

Committing to: bzr+ssh://[e-mail]/bmo/4.0
added extensions/OrangeFactor
added extensions/OrangeFactor/Config.pm
added extensions/OrangeFactor/Extension.pm
added extensions/OrangeFactor/template
added extensions/OrangeFactor/web
added extensions/OrangeFactor/template/en
added extensions/OrangeFactor/template/en/default
added extensions/OrangeFactor/template/en/default/hook
added extensions/OrangeFactor/template/en/default/hook/bug
added extensions/OrangeFactor/template/en/default/hook/global
added extensions/OrangeFactor/template/en/default/hook/bug/edit-after_custom_fields.html.tmpl
added extensions/OrangeFactor/template/en/default/hook/bug/show-header-end.html.tmpl
added extensions/OrangeFactor/template/en/default/hook/global/setting-descs-settings.none.tmpl
added extensions/OrangeFactor/web/js
added extensions/OrangeFactor/web/style
added extensions/OrangeFactor/web/js/AUTHORS.processing.js
added extensions/OrangeFactor/web/js/LICENSE.processing.js
added extensions/OrangeFactor/web/js/LICENSE.sparklines.js
added extensions/OrangeFactor/web/js/orange_factor.js
added extensions/OrangeFactor/web/js/sparklines.min.js
added extensions/OrangeFactor/web/style/orangefactor.css
Committed revision 8314.

Committing to: bzr+ssh://[e-mail]/bmo/4.2
added extensions/OrangeFactor
added extensions/OrangeFactor/Config.pm
added extensions/OrangeFactor/Extension.pm
added extensions/OrangeFactor/template
added extensions/OrangeFactor/web
added extensions/OrangeFactor/template/en
added extensions/OrangeFactor/template/en/default
added extensions/OrangeFactor/template/en/default/hook
added extensions/OrangeFactor/template/en/default/hook/bug
added extensions/OrangeFactor/template/en/default/hook/global
added extensions/OrangeFactor/template/en/default/hook/bug/edit-after_custom_fields.html.tmpl
added extensions/OrangeFactor/template/en/default/hook/bug/show-header-end.html.tmpl
added extensions/OrangeFactor/template/en/default/hook/global/setting-descs-settings.none.tmpl
added extensions/OrangeFactor/web/js
added extensions/OrangeFactor/web/style
added extensions/OrangeFactor/web/js/AUTHORS.processing.js
added extensions/OrangeFactor/web/js/LICENSE.processing.js
added extensions/OrangeFactor/web/js/LICENSE.sparklines.js
added extensions/OrangeFactor/web/js/orange_factor.js
added extensions/OrangeFactor/web/js/sparklines.min.js
added extensions/OrangeFactor/web/style/orangefactor.css
Committed revision 8337.