----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19833/#review40608 -----------------------------------------------------------
src/main/resources/org/apache/aurora/scheduler/http/ui/job.html <https://reviews.apache.org/r/19833/#comment73678> Repeating the comment from Bill. Make this an anchor with no href and Angular will call preventDefault for you. src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js <https://reviews.apache.org/r/19833/#comment73669> Better to move these default values to getTasksForJob (is the empty string default value even required in the Angular templating language?). src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js <https://reviews.apache.org/r/19833/#comment73637> Why are you passing $scope.role, $scope.environment and $scope.job into a function which reads and writes to the $scope object anyway? Why not just pass $scope? Or why have parameters at all if it's just a closure? If the intent is to break a large controller into functions to make it easier to read (which I think is a good idea), it is best to define this function outside of the controller itself, so that the function body is just the control flow. I'm not sure of the idiomatic way to do this in Angular.JS, but normally you just use JavaScript function scope to break your code into smaller functions without polluting your API. I mean intuitively I would just do something like: (function() { function getTasksForJob($scope) { // do some side-effecty things to $scope } function jobController($scope, ..) { // only control-flow code here getTasksForJob($scope); } auroraUIControllers.controller('JobController', jobController); })(); src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js <https://reviews.apache.org/r/19833/#comment73670> I feel like this might be clearer as: if(response.error) { $scope.error = "Error fetching tasks: " + response.error; return []; } src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js <https://reviews.apache.org/r/19833/#comment73641> Why isn't this part of taskUtil? src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js <https://reviews.apache.org/r/19833/#comment73668> Style: prefer ScheduleStatus.RUNNING here. src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js <https://reviews.apache.org/r/19833/#comment73645> I realise this code doesn't have bugs right now, but the 'this' keyword can cause subtle problems given how easy it is to change the calling scope. You can avoid having to worry about it here by pulling the return object into a named variable and using that instead. i.e. function() { var taskUtil = { summarizeActiveConfigs: function(..) { //... this will still work taskUtil.isActiveTask(task); // .. }, isActiveTask: function(..) { .. } } return taskUtil; } src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js <https://reviews.apache.org/r/19833/#comment73671> Can be rewritten as: .filter(taskUtil.isActiveTask) src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js <https://reviews.apache.org/r/19833/#comment73673> The terseness of this code makes it quite difficult to read. Would be nice to split this into multiple lines with descriptive variable names for the intermediate steps. Alternatively, keep it the same and add some inline comments to explain how the data is being transformed. src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js <https://reviews.apache.org/r/19833/#comment73676> Style: Beyond a one liner, it is best to extract these values into temporary variables. i.e. var constraints = _.chain(task.constraints) // etc. var metadata = _.chain(task.metadata) // etc. return { // .. constraints: constraints, metadata: metadata, // .. } src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js <https://reviews.apache.org/r/19833/#comment73675> Equivalent to .map(taskUtil.formatConstraint) src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js <https://reviews.apache.org/r/19833/#comment73677> This code path will execute if the value is undefined. Why not just use: if(taskConstraint.value) here? I'm also assuming if taskConstraint.value exists, taskConstraint.value.values will always exist and will always be an array? Otherwise you should add checks for that too. - David McLaughlin On April 16, 2014, 8:37 a.m., Suman Karumuri wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/19833/ > ----------------------------------------------------------- > > (Updated April 16, 2014, 8:37 a.m.) > > > Review request for Aurora, Kevin Sweeney and Bill Farner. > > > Bugs: AURORA-281 > https://issues.apache.org/jira/browse/AURORA-281 > > > Repository: aurora > > > Description > ------- > > *** Please review Diff 2. Diff 1 is https://reviews.apache.org/r/19565/. > Added it here so only new code can be reviewed. *** > > > Migrated job page to AngularJS. > Removed old scheduler job page. > Removed js related to the old scheduler page. Removed data tables. > Added moment.js library for date time manipulation. > Refactored code a bit for reusability. > Stats link should still be added to the new jobs page. > > Since the role/env code is not committed, this diff contains that code as > well. > Code changes start from page 4 in diff between version 1 and 2. > > > Diffs > ----- > > build.gradle 109c193da3324bd5534b409bfabb6aeb0adda7b1 > src/main/java/org/apache/aurora/scheduler/app/AppModule.java > eeafc784e915137cacd5f64df1252ccbaf6c0f6c > src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java > ec56c649116c03ef148bac916bd6691a94685bc3 > src/main/java/org/apache/aurora/scheduler/http/DisplayUtils.java > 19df7889f15b4cf44e386d8ce0626cc94fdcdfba > src/main/java/org/apache/aurora/scheduler/http/SchedulerzJob.java > 2ccc6f367b9715a0abb3e0673069289ae4860087 > src/main/java/org/apache/aurora/scheduler/http/ServletModule.java > e3ff2571d95effcf72b2047cc5840d56143a180c > src/main/java/org/apache/aurora/scheduler/http/UIRedirectFilter.java > b35aad8db006133ba70692e43db1f083d4950914 > > src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/css/jquery.dataTables.css > ee6da23c3948a87b0d4df73e82e5c4e2ab2bc803 > > src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_disabled.png > 881de7976ff98955e2a5487dca66e618a0655f3d > > src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_enabled.png > c608682b04a6d9b8002602450c8ef7e80ebba099 > > src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_enabled_hover.png > d300f1064b3beac1d7d5274e294494d3143e53a2 > > src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_disabled.png > 6a6ded7de821619aedc71d1738c0b73463a4452e > > src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_enabled.png > a4e6b5384b8454ee7f44a8f7c75b0321b7eeb9b1 > > src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_enabled_hover.png > fc46c5ebf0524b72a509fe2d7c1bc74995cb8a9d > > src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_asc.png > a88d7975fe9017e4e5f2289a94bd1ed66a5f59dc > > src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_asc_disabled.png > 4e144cf0b1f786a9248a2998311e8109998d8a2d > > src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_both.png > 18670406bc01ab2721781822dd6478917745ff54 > > src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_desc.png > def071ed5afd264a036f6d9e75856366fd6ad153 > > src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_desc_disabled.png > 7824973cc60fc1841b16f2cb39323cefcdc3f942 > > src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/dataTables.bootstrap.js > 420e507086f7a2af08f68787f67208546745260d > > src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/dataTables.htmlNumberType.js > 885ad347f3076dcf75558ef44806dc25a8171884 > > src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/dataTables.localstorage.js > 3c0b39aefbd5a82adee49f2ac7539b6974576b2f > > src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/jquery.dataTables.min.js > 02694a4a56eb247baa2398d971927dfbd1ac3e60 > src/main/resources/org/apache/aurora/scheduler/http/assets/dictionary.js > 92045c41706ee9315f3522c3b5bbff7f6e7d2c64 > src/main/resources/org/apache/aurora/scheduler/http/assets/util.js > 5dd0d17849c7116d70925275b9c6c0715b441f14 > src/main/resources/org/apache/aurora/scheduler/http/schedulerzjob.st > 28b56671b2e825912a6427e609c2bbe1e7758e26 > src/main/resources/org/apache/aurora/scheduler/http/ui/breadcrumb.html > PRE-CREATION > src/main/resources/org/apache/aurora/scheduler/http/ui/css/app.css > ade850ce624964693e9bd55946464983c4b9f8c2 > src/main/resources/org/apache/aurora/scheduler/http/ui/index.html > 36225d1e5147e30ba2cb4ddda96dec9f0f2f1dce > src/main/resources/org/apache/aurora/scheduler/http/ui/job.html > PRE-CREATION > src/main/resources/org/apache/aurora/scheduler/http/ui/js/app.js > db6ea99aeb749fd8674613e3620dc3012872e13c > src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js > 7cd534479dd2f17ffd46248ce9af1f8fe89beb97 > src/main/resources/org/apache/aurora/scheduler/http/ui/js/directives.js > d2b2017a0efc70d425fd6c89ad6caaf46cb8ded5 > src/main/resources/org/apache/aurora/scheduler/http/ui/js/filters.js > PRE-CREATION > src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js > 8681bbe840b6285b15dd6766016258c2c8115632 > src/main/resources/org/apache/aurora/scheduler/http/ui/role.html > PRE-CREATION > > src/main/resources/org/apache/aurora/scheduler/http/ui/schedulingDetail.html > PRE-CREATION > src/main/resources/org/apache/aurora/scheduler/http/ui/taskLink.html > PRE-CREATION > src/main/resources/org/apache/aurora/scheduler/http/ui/taskSandbox.html > PRE-CREATION > src/main/resources/org/apache/aurora/scheduler/http/ui/taskStatus.html > PRE-CREATION > src/main/thrift/org/apache/aurora/gen/api.thrift > c0618e4edebd6f282698abfd9bdc3c36fff16920 > src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java > a4e9464f7d5d3f5a640b62557c3e29f2f1566985 > > src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java > fae2de11235dd059718e1023fdcfb0e8fc4deadd > > src/test/java/org/apache/aurora/scheduler/thrift/aop/ServerInfoInterceptorTest.java > dd991fb90889c3f221e537a78283eb4a31e5f0dd > src/test/resources/org/apache/aurora/gen/api.thrift.md5 > 05c6e8a1e2407f2822b3844c555d8995f1cd1d49 > > Diff: https://reviews.apache.org/r/19833/diff/ > > > Testing > ------- > > ./gradlew clean build run on local laptop. > > > Thanks, > > Suman Karumuri > >
