709812 - Move Firefox versions to a global constant

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Result Dashboard, defect)

Description

[Dave Hunt [:davehunt] [he/him] ⌚BST]

This would reduce duplication and maintenance when updating the report filters, and also allow the upcoming endurance charts page to dynamically preset the latest version.

Comment 1

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Moved Firefox versions to a global constant. v1.0

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 580945 [details] [diff] [review]
Moved Firefox versions to a global constant. v1.0

>+++ b/_attachments/js/dashboard.js
>@@ -1,6 +1,7 @@
> var BYTE_TO_MEGABYTE = 1/1048576;
> var MAX_CHART_CHECKPOINTS = 450;
> var METRIC_UNAVAILABLE = "--";
>+var FIREFOX_VERSIONS = ["11.0", "10.0", "9.0", "8.0", "3.6"]

Please move it to the very top of this file or even better include it from a versions.js file so we have it separated from this large dashboard code.

>       var context = this;
>+      context.firefox_versions = FIREFOX_VERSIONS;
>       request({url: '/_view/functional_reports?' + $.param(query)}, function (err, resp) {
>         if (err) window.alert(err);

I would bind it directly to the Sammy app by placing it into it's constructor. That way we would only have to set it once.

>+        {{#firefox_versions}}
>+            <span>{{{.}}}</span>
>+        {{/firefox_versions}}

nit: 2 spaces indentation.

Comment 3

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Moved Firefox versions to a global constant. v1.1

(In reply to Henrik Skupin (:whimboo) from comment #2)
> Comment on attachment 580945 [details] [diff] [review]
> Moved Firefox versions to a global constant. v1.0
> 
> >+++ b/_attachments/js/dashboard.js
> >@@ -1,6 +1,7 @@
> > var BYTE_TO_MEGABYTE = 1/1048576;
> > var MAX_CHART_CHECKPOINTS = 450;
> > var METRIC_UNAVAILABLE = "--";
> >+var FIREFOX_VERSIONS = ["11.0", "10.0", "9.0", "8.0", "3.6"]
> 
> Please move it to the very top of this file or even better include it from a
> versions.js file so we have it separated from this large dashboard code.

Moving it to a separate file sounds great, but I'm not sure how I'd achieve this. I tried adding a versions.js but this didn't work - I assume I need to reference this new file somehow.

> 
> >       var context = this;
> >+      context.firefox_versions = FIREFOX_VERSIONS;
> >       request({url: '/_view/functional_reports?' + $.param(query)}, function (err, resp) {
> >         if (err) window.alert(err);
> 
> I would bind it directly to the Sammy app by placing it into it's
> constructor. That way we would only have to set it once.

I have tried doing this by adding this.firefox_versions to the app constructor, however this does not appear to be available in the event context. If I use context.firefox_versions = app.firefox_versions it works, but I might as well reference the global constant directly.

> >+        {{#firefox_versions}}
> >+            <span>{{{.}}}</span>
> >+        {{/firefox_versions}}
> 
> nit: 2 spaces indentation.

Fixed.

Comment 4

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Moved Firefox versions to a global constant. v1.2

Disregard my comment about moving to a separate file, we cleared this up on IRC.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 581229 [details] [diff] [review]
Moved Firefox versions to a global constant. v1.2

>+    <script language="javascript" type="text/javascript" src="js/versions.js"></script>

You will also have to add this file to the patch. It's missing right now.

>+    this.firefox_versions = FIREFOX_VERSIONS;
>+

So does that work now or is this property still not accessible from within the event callback and template?

Comment 6

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Pointer to Github pull request: https://github.com/whimboo/mozmill-dashboard/pull/5

Pointer to Github pull-request

Comment 7

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 581927 [details]
Pointer to Github pull request: https://github.com/whimboo/mozmill-dashboard/pull/5

Sorry, I submitted this pull request but didn't update the bug due to issues I was having with the github tweaks addon.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 581927 [details]
Pointer to Github pull request: https://github.com/whimboo/mozmill-dashboard/pull/5

Looks good to me. Just fix the outstanding review comments and it will be ready to land.

Comment 9

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Pointer to Github pull request: https://github.com/whimboo/mozmill-dashboard/pull/5

Pointer to Github pull-request

Comment 10

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 583112 [details]
Pointer to Github pull request: https://github.com/whimboo/mozmill-dashboard/pull/5

Implemented suggested changes.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 583112 [details]
Pointer to Github pull request: https://github.com/whimboo/mozmill-dashboard/pull/5

r=me with the one remaining issue fixed. Please squash all commits into a single one before merging your branch.

Comment 12

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Landed as:
https://github.com/whimboo/mozmill-dashboard/commit/3c36dad0387e935540c84c4551697caa25aeb655