[GitHub] [brooklyn-ui] tbouron commented on a change in pull request #211: Login page fixes - WIP - please do not merge

[GitBox <gi...@apache.org>]

tbouron commented on a change in pull request #211:
URL: https://github.com/apache/brooklyn-ui/pull/211#discussion_r645467670



##########
File path: ui-modules/login/app/views/main/main.controller.js
##########
@@ -47,18 +47,28 @@ export function loginStateController($scope, $http, $window, brBrandInfo) {
     $scope.$emit(HIDE_INTERSTITIAL_SPINNER_EVENT);
     $scope.getBrandedText = brBrandInfo.getBrandedText;
 
+    let loginController = this;
+    loginController.error = {};
+
+    var testAuthReq = {
+        method: 'HEAD',
+        url: '/v1/server/up/extended'
+    }
+
+    // If the user is already logged in then redirect to home
+    $http(testAuthReq).then( () => {$window.location.href = '/';} );
+
     $scope.login = (user)=> {
         var req = {
-            method: 'GET',
-            url: '/',
+            method: 'HEAD',
+            url: '/v1/server/up/extended',
             headers: {
                 'Authorization': 'Basic ' + btoa(user.name +":" + user.password)
             }
         }
 // ui registry metadata
         $http(req)
-            .then(function () {
-                $window.location.href = '/';
-            });
+            .then(() => { $window.location.href = '/'; },
+                () => { $window.alert("Login Failed") ; loginController.error.loginFailed = "Login Failed" });

Review comment:
       Yup, would be nicer to use the `uibModal` service to display a popup rather than the default browser alert.
   
   Also, it is just code style but I find the syntax
   ```
   promise
     .then(() => {})
     .catch(() => {})
   ```
   
   nicer than the one you are using

##########
File path: ui-modules/login/app/views/main/main.template.html
##########
@@ -20,6 +20,9 @@
     <div class="row">
         <div class="col-md-4 col-md-offset-4 col-sm-12">
             <form>
+                <p ng-if="error['loginFailed']">
+                    fkjldsfdlskjfdslkj

Review comment:
       Hum, did a cat wrote that? :)

##########
File path: ui-modules/utils/server-status/server-status.js
##########
@@ -65,6 +65,9 @@ export function BrServerStatusDirective() {
                 }else if(response.status === 404) {
                     state = BrServerStatusModalController.STATES.NO_CONNECTION;
                 }else if(response.status === 401 || response.status === 403 ) {
+                    if( response.headers("LOGIN_PAGE")) {

Review comment:
       I don't think it is. But by convention, there should start with `X_`. 
   
   For instance, I think `X_BROOKLYN_LOGIN_PAGE` would be nicer

##########
File path: ui-modules/login/app/views/main/main.controller.js
##########
@@ -47,18 +47,28 @@ export function loginStateController($scope, $http, $window, brBrandInfo) {
     $scope.$emit(HIDE_INTERSTITIAL_SPINNER_EVENT);
     $scope.getBrandedText = brBrandInfo.getBrandedText;
 
+    let loginController = this;
+    loginController.error = {};
+
+    var testAuthReq = {
+        method: 'HEAD',
+        url: '/v1/server/up/extended'
+    }
+
+    // If the user is already logged in then redirect to home
+    $http(testAuthReq).then( () => {$window.location.href = '/';} );

Review comment:
       There would be @duncangrant if within the same UI module. But you want to redirect to home, i.e. `/` so this is the right thing to do




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org