Bug 1060322 - Move hardcoded Bugzilla & slave health URLs in the UI to thUrl #598

mandaltapesh · 2015-06-02T08:44:43Z

Bug 1060322 - Move hardcoded treeherder-ui URLs to a config file

Respected all,
I have moved only those URLs which have been repeated more than once in a angularJS app..There are a lot of changes( 8 i think) besides two file inclusions.I have tried my best to keep the basic code structure and philosophy intact.However i am a beginner so there can be mistakes..I have tested it locally(but partially) so i suggest the reviewer to kindly check it once.
In case of any suggestions/changes feel free to contact me at tapesh.mandal@gmail.com.My IRC nick tapesh and i hang out in #treeherder channel in irc.mozilla.org.

Regards
Tapesh Mandal

edmorley · 2015-06-02T09:10:57Z

hi :-) so we don't need to create a new angular app for this - we want to add to the files used by the existing apps. eg the best way I think is likely to add to services/main.js, two new factories - one for bugzilla and one for slave health (though @camd / @wlach feel free to suggest something better - I'm still not 100% sure of the difference between some of the angular terminology and/or the UI directory layout). Each of these factories would have a function to return the URL - eg getBugURL(), which in the markup would have the bug id passed in as a parameter.

If it helps, take a look at the treestatus file for reference:
https://github.com/mozilla/treeherder/blob/master/ui/js/services/treestatus.js
...except instead of a tree name being returned, we'd return a URL.

mandaltapesh · 2015-06-03T16:18:26Z

Respected Sir,
Was inspecting services/main.js . is there any place where treeherder factory format is defined?

Regards,
Tapesh Mandal

edmorley · 2015-06-30T15:38:47Z

Was inspecting services/main.js . is there any place where treeherder factory format is defined?

I gave a link to an example in the comment above.

camd · 2015-07-30T00:15:50Z

ui/js/tree_url_app.js

@@ -0,0 +1,4 @@
+
+
+var app = angular.module("tree_url_app", []);


this is the part that Ed was saying is not needed. We don't need to create a new app, we should add to the existing treeherderApp.

I suggest adding your functions to this factory: https://github.com/mozilla/treeherder/blob/master/ui/js/services/main.js#L13-24

Then you can reference that factory in any of the controllers that need to use it.

edmorley · 2015-07-30T10:07:37Z

Newer versions of the code here were pushed to different branches and new PRs opened.
I've closed those in favour of this older PR, since there are already review comments here, so it's helpful to keep everything in one place. Generally we try to avoid multiple PRs for the same fix, and instead rebase+force push to the branch, which updates the PR. For more info, see:
https://github.com/edx/edx-platform/wiki/How-to-Rebase-a-Pull-Request

Once you've pushed the latest version to this PR, and there are no accidentally added files / the tests pass, request review from one of us in the bug - I'm on holiday until Monday, so if you want a review before then, one of the others may be a better choice :-)

Good luck!

mandaltapesh · 2015-08-03T17:02:05Z

Hi Ed,

i hope you had a nice vacation. i followed the link in the above comment..was still unable to update the PR.. but i did manage to squash the commits as mentioned in the text.Even i am opposed to opening multiple PRs and totally understand that it creates a lot of inconvenience...i am at this juncture after the push ..https://github.com/mozilla/treeherder/compare/master...mandaltapesh:test?expand=1 ..will opening a new PR somehow merge the new one with the existing one ?

Regards
Tapesh Mandal

edmorley · 2015-08-03T17:08:40Z

PRs are linked to the branch name - so to update this PR you need to push to the same branch name it was created with - which in this case was treetest.

mandaltapesh · 2015-08-03T17:14:53Z

Hi Ed,
https://github.com/mandaltapesh/treeherder/tree/treetest ..i think i pushed to treetest

mandaltapesh · 2015-08-03T17:23:06Z

ok yeah i can see the "test" written ..sorry my mistake ..

mandaltapesh · 2015-08-03T17:32:04Z

Ed ,

i don't know what happened but it now says all checks have passed does that mean it is updated ?

Regards
Tapesh Mandal

camd · 2015-08-05T00:45:11Z

Hi Tapesh, the code all looks good to me. I'm going to test with it tomorrow, and if all goes well, I'll merge it in for you.

Thanks for doing this work! :)

edmorley · 2015-08-05T09:27:03Z

ui/js/services/main.js

@@ -21,6 +21,12 @@ treeherder.factory('thUrl', [
            },
            getLogViewerUrl: function(job_id) {
                return "logviewer.html#?job_id=" + job_id + "&repo=" + $rootScope.repoName;
+            },
+            getBugIdUrl:function(bug_id) {


There needs to be a space after the colon, and before function.

I think maybe the function name might be better as getBugUrl rather than getBugIdUrl?

mandaltapesh · 2015-08-05T16:57:08Z

Hi Ed,
i have made the changes ...:)
Regards,
Tapesh Mandal

edmorley · 2015-08-06T00:18:45Z

ui/index.html

@@ -202,7 +202,7 @@
                    <tr>
                        <th class="small">Machine name</th>
                        <td class="small">
-                            <a target="_blank" href="https://secure.pub.build.mozilla.org/builddata/reports/slave_health/slave.html?name={{ job.machine_name }}">{{ job.machine_name }}</a>
+                            <a target="_blank" href="{{ getMachineNameUrl(job.machine_name) }}">{{ job.machine_name }}</a>


This needs to be updated to getSlaveHealthUrl

mandaltapesh · 2015-08-06T07:51:41Z

Hi Ed,

      Made the required changes. :)

Regards
Tapesh Mandal

edmorley · 2015-08-07T10:23:51Z

Ah there's another slave health url instance that needs changing:

treeherder/ui/plugins/similar_jobs/main.html

Lines 70 to 71 in 84d8a71

    
           <a target="_blank" href="https://secure.pub.build.mozilla.org/builddata/reports/slave_health/slave.html?name={{ similar_job_selected.machine_name }}"> 
        
               {{ similar_job_selected.machine_name }}

And one more for the bug url:

treeherder/ui/js/filters.js

Lines 50 to 51 in 84d8a71

    
           var bug_url = '<a href="https://bugzilla.mozilla.org/show_bug.cgi?id=$1" ' + 
        
               'data-bugid=$1 ' + 'title=' + bug_title + '>$1</a>';

Could you also update the commit message to something like:
Bug 1060322 - Move hardcoded Bugzilla & slave health URLs in the UI to thUrl
(and remove the leftovers from squashing, from the multi-line commit message)

Apart from that looking great! :-)

…thUrl

mandaltapesh · 2015-08-07T12:49:54Z

Hi Ed,
Made the required changes :)
Regards,
Tapesh Mandal

edmorley · 2015-08-11T00:02:52Z

Great! :-)

I've rebased against latest master, added back the angular one-time binding syntax (the '::'), dropped the filters.js change (since it required a bit more than just a simple substitution, and it's maybe easier if we save that for another time) and merged to master manually in: 9a2497d

Thank you for the patch!

camd reviewed Jul 30, 2015
View reviewed changes

This was referenced Jul 30, 2015

Bug 1060322 - Move hardcoded treeherder-ui URLs/ test stage now #816

Closed

Bug 1060322 move hardcoded URLs into a config file. #814

Closed

mandaltapesh force-pushed the treetest branch from 1d5ab45 to 6417687 Compare August 3, 2015 17:21

edmorley reviewed Aug 5, 2015
View reviewed changes

mandaltapesh force-pushed the treetest branch from 6417687 to d6f1ea1 Compare August 5, 2015 16:49

edmorley reviewed Aug 6, 2015
View reviewed changes

mandaltapesh force-pushed the treetest branch from d6f1ea1 to c220934 Compare August 6, 2015 07:44

edmorley changed the title ~~Bug 1060322 - Move hardcoded treeherder-ui URLs to a config file~~ Bug 1060322 - Move hardcoded Bugzilla & slave health URLs in the UI to thUrl Aug 7, 2015

mandaltapesh force-pushed the treetest branch from c220934 to b0241ba Compare August 7, 2015 12:34

Bug 1060322-Move hardcoded Bugzilla & slave health URLs in the UI to …

be2e945

…thUrl

mandaltapesh force-pushed the treetest branch from b0241ba to be2e945 Compare August 7, 2015 12:40

edmorley closed this Aug 11, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1060322 - Move hardcoded Bugzilla & slave health URLs in the UI to thUrl #598

Bug 1060322 - Move hardcoded Bugzilla & slave health URLs in the UI to thUrl #598

mandaltapesh commented Jun 2, 2015

edmorley commented Jun 2, 2015

mandaltapesh commented Jun 3, 2015

edmorley commented Jun 30, 2015

camd Jul 30, 2015

edmorley commented Jul 30, 2015

mandaltapesh commented Aug 3, 2015

edmorley commented Aug 3, 2015

mandaltapesh commented Aug 3, 2015

mandaltapesh commented Aug 3, 2015

mandaltapesh commented Aug 3, 2015

camd commented Aug 5, 2015

edmorley Aug 5, 2015

edmorley Aug 5, 2015

mandaltapesh commented Aug 5, 2015

edmorley Aug 6, 2015

mandaltapesh commented Aug 6, 2015

edmorley commented Aug 7, 2015

mandaltapesh commented Aug 7, 2015

edmorley commented Aug 11, 2015

		@@ -0,0 +1,4 @@


		var app = angular.module("tree_url_app", []);

Bug 1060322 - Move hardcoded Bugzilla & slave health URLs in the UI to thUrl #598

Bug 1060322 - Move hardcoded Bugzilla & slave health URLs in the UI to thUrl #598

Conversation

mandaltapesh commented Jun 2, 2015

Bug 1060322 - Move hardcoded treeherder-ui URLs to a config file

edmorley commented Jun 2, 2015

mandaltapesh commented Jun 3, 2015

edmorley commented Jun 30, 2015

camd Jul 30, 2015

Choose a reason for hiding this comment

edmorley commented Jul 30, 2015

mandaltapesh commented Aug 3, 2015

edmorley commented Aug 3, 2015

mandaltapesh commented Aug 3, 2015

mandaltapesh commented Aug 3, 2015

mandaltapesh commented Aug 3, 2015

camd commented Aug 5, 2015

edmorley Aug 5, 2015

Choose a reason for hiding this comment

edmorley Aug 5, 2015

Choose a reason for hiding this comment

mandaltapesh commented Aug 5, 2015

edmorley Aug 6, 2015

Choose a reason for hiding this comment

mandaltapesh commented Aug 6, 2015

edmorley commented Aug 7, 2015

mandaltapesh commented Aug 7, 2015

edmorley commented Aug 11, 2015