mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 58872: Ensured sandbox URI request reroute after fetched `$scope.state`.
Date Tue, 09 May 2017 20:54:23 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58872/#review174374
-----------------------------------------------------------


Fix it, then Ship it!




Before you commit this, it would be great if you could test this (I can't tell how you tested
this from the testing done section), e.g.:

* Clicking on a link from the top level task page (state should already be loaded)
* Navigating directly to a valid URL (state should not be loaded yet)
* Navigating directly to an invalid URL (e.g. bad agent ID) (state should not be loaded)

To make sure that these all work as you expect.


src/webui/master/static/js/controllers.js
Lines 1016-1017 (patched)
<https://reviews.apache.org/r/58872/#comment247501>

    I'm a bit confused as to why we would want to set up the listener if we already called
reroute(). Does this belong in the else case?
    
    ```
        if ($scope.state) {
          reroute();
        } else {
          var removeListener = $scope.$on('state_updated', reroute);
          $scope.$on('$routeChangeStart', removeListener);
        }
    ```
    
    Also, why is it safe to call reroute on every state update? It seems there are a few cases
to consider:
    
    Case 1: reroute actually routes away from the page, then we're ok because the listener
is removed
    
    Case 2: reroute calls goBack, we're ok because the listener is removed (there is a separate
question we should consider later, which is whether going back is the best thing to do)
    
    Case 3: reroute navigates "home" (/) if we get an error response, this is ok since the
listener is removed
    
    How about adding a comment here that reroute is expected to always route away from the
page and so the listener is removed after the first state update?


- Benjamin Mahler


On May 3, 2017, 4:15 a.m., haosdent huang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58872/
> -----------------------------------------------------------
> 
> (Updated May 3, 2017, 4:15 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Tomasz Janiszewski.
> 
> 
> Bugs: MESOS-4992
>     https://issues.apache.org/jira/browse/MESOS-4992
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Open sandbox link in a new tab would fail since it depends on agents
> information loading finish. This patch fixes it by ensuring the agents
> information loaded first and then rerouting the sandbox request.
> 
> 
> Diffs
> -----
> 
>   src/webui/master/static/js/controllers.js a021962573d452de1581e6a7717016eac7d0cd85

> 
> 
> Diff: https://reviews.apache.org/r/58872/diff/2/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> haosdent huang
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message