> On May 14, 2014, 4:26 a.m., Suman Karumuri wrote:
> > src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js,
> > line 386
> > <https://reviews.apache.org/r/21247/diff/5/?file=581259#file581259line386>
> >
> > I see this function call (function definition itself is good) as a
> > potential land mine. For example, moving this function from line 358 to
> > 356, would cause buildGroupSummary to break and this error will be very
> > hard to debug. Your comment here is an indication that this needs special
> > attention.
> >
> > Moving this function call into the getTasksForJob(line 398) function
> > will obviate the need for this comment and will also make the code flow
> > more straightforward.
> >
> > I agree that this controller code can use some refactoring, but I think
> > this is a potential land mine and there is nothing to be gained from
> > calling it outside the normal call path.
> >
> > Please feel free to leave a TODO to refactor getTasksForJob.
buildGroupSummary needs $scope.taskSummary to be defined. If I move it to line
398, then every reason for this being 'a land mine' still exists. This is
synchronous, imperative code. Why would it be surprising that you can't just
move code around without breaking stuff?
Also, it's not going to be hard to debug. The error will say "Cannot call map
on undefined" and
TypeError: Cannot read property 'map' of undefined
at buildGroupSummary (http://localhost:8081/js/controllers.js:372:47)
at new <anonymous> (http://localhost:8081/js/controllers.js:356:5)
at invoke (http://localhost:8081/js/angular.js:3697:17)
at Object.instantiate (http://localhost:8081/js/angular.js:3708:23)
at http://localhost:8081/js/angular.js:6758:28
at link (http://localhost:8081/js/angular-route.js:906:26)
at nodeLinkFn (http://localhost:8081/js/angular.js:6212:13)
at compositeLinkFn (http://localhost:8081/js/angular.js:5622:15)
at publicLinkFn (http://localhost:8081/js/angular.js:5527:30)
at boundTranscludeFn (http://localhost:8081/js/angular.js:5641:21) <ng-view
class="ng-scope">
- David
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21247/#review42937
-----------------------------------------------------------
On May 14, 2014, 1:30 a.m., David McLaughlin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21247/
> -----------------------------------------------------------
>
> (Updated May 14, 2014, 1:30 a.m.)
>
>
> Review request for Aurora, Suman Karumuri and Bill Farner.
>
>
> Bugs: AURORA-378
> https://issues.apache.org/jira/browse/AURORA-378
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Replace the button to show/hide configs with a bar when there are multiple.
> Tidied up the display of the configuration data.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/http/ServletModule.java
> 983101277ffbd1c4017b29f4c86e61315f1bcc78
> src/main/resources/org/apache/aurora/scheduler/http/ui/configSummary.html
> PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/http/ui/css/app.css
> dc515ac818a9af36522bb07a125cd92ff9230fa2
> src/main/resources/org/apache/aurora/scheduler/http/ui/groupSummary.html
> PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/http/ui/job.html
> cd78c20bee7891a6cdfd19943a6f7e8a9dce33df
> src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js
> 7c80bd769b7e80bef0822846166959925b1bf7f8
> src/main/resources/org/apache/aurora/scheduler/http/ui/js/directives.js
> a79276d2486fc1122a8b084ea0614cdae759f88c
>
> Diff: https://reviews.apache.org/r/21247/diff/
>
>
> Testing
> -------
>
>
> File Attachments
> ----------------
>
> Job page with only one config group
>
> https://reviews.apache.org/media/uploaded/files/2014/05/08/908570f2-bef2-4370-bf08-1b5c7400c3c6__Screen_Shot_2014-05-08_at_4.26.02_PM.png
> Showing config for only one config group
>
> https://reviews.apache.org/media/uploaded/files/2014/05/08/1424f2c3-cf66-4ce4-a595-dc3f7647d6d2__Screen_Shot_2014-05-08_at_4.26.12_PM.png
> Job page with multiple config groups
>
> https://reviews.apache.org/media/uploaded/files/2014/05/08/bc9c1616-3c71-4bcc-ab40-fefc56d5e19a__Screen_Shot_2014-05-08_at_4.25.21_PM.png
> Viewing one of the groups from config bar
>
> https://reviews.apache.org/media/uploaded/files/2014/05/08/6b8e6077-c8c8-48c9-94e9-732e782204b6__Screen_Shot_2014-05-08_at_4.25.36_PM.png
> Viewing multiple configs at the same time
>
> https://reviews.apache.org/media/uploaded/files/2014/05/08/b7b2cba1-034b-43e6-9eaa-3ba1a78f87f5__Screen_Shot_2014-05-08_at_4.25.42_PM.png
> screenshot of really small group
>
> https://reviews.apache.org/media/uploaded/files/2014/05/12/2d7999ac-ba1f-45b0-bf9e-99f9c7a10009__Screen_Shot_2014-05-12_at_1.27.59_PM.png
>
>
> Thanks,
>
> David McLaughlin
>
>
