validateUsername() reports 'Users is already in use' when Server or Internet offline.

[tohagan]

OK ... This one could drive your users nuts!

validateUsername()/validateEmail() as used within superlogin-demo will always return false when your superlogin Server is down or the client computer is offline. This will mean they will keep trying new usernames but never find one that is not used :( . Nice one for April 1st ;)

Ahh but you say ... How would they ever get to display the web site if the server is down? Pretty rare yeah? Well in my case, I'm coding an Ionic/Angular mobile app client so the client code is already on the device - not loaded from the superlogin server so now it would then be quite a common occurrence - especially for mobile devices that are frequently dropping offline.

Solution? ... Well Angular Messages does not really provide a clean solution for this. Personally I'd prefer if their $asyncValidators could return a message key - not just true/false - then (sensibly) a single server call could deliver multiple validation message types which is what we need in this case. Maybe ... one ... day

Hack? Here's the solution I came up with for superlogin-demo:

I added this check into validateUsername() and validateEmail()

    // login server or internet offline?
    if(err.status === 404 || err.status === -1) {
        ctrl.$setValidity('offline', false);
        return $q.when(true);
    }

... and add the offline messages in the signup form ...

 <div ng-message="checkUsername">Username is already in use.</div>
 <div ng-message="offline">Cannot check Username. Try again later.</div>
...
 <div ng-message="checkEmail">Email is already in use.</div>
 <div ng-message="offline">Cannot check Email. Try again later.</div>

Final versions of ... validateUsername() and validateEmail()

       validateUsername: function(username) {
          return $http.get(superloginSession.getConfig().baseUrl + 'validate-username/' + encodeURIComponent(username))
            .then(function() {
              return $q.when(true);
            }, function(err) {
              // login server or internet offline?
              if(err.status === 404 || err.status === -1) {
                ctrl.$setValidity('offline', false);
                return $q.when(true);
              }
              if(err.status === 409) {
                return $q.reject(false);
              }
              return $q.reject(err.data);
            });
        },
        validateEmail: function(email) {
          return $http.get(superloginSession.getConfig().baseUrl + 'validate-email/' + encodeURIComponent(email))
            .then(function () {
              return $q.when(true);
            }, function (err) {
              // login server or internet offline?
              if(err.status === 404 || err.status === -1) {
                ctrl.$setValidity('offline', false);
                return $q.when(true);
              }
              if(err.status === 409) {
                return $q.reject(false);
              }
              return $q.reject(err.data);
            });
        }

[colinskow]

Excellent handling... Care to make a PR?

[tohagan]

One other nice addition is to report when the check is pending ...

   <div ng-show="regForm.username.$pending" class="ng-messages">Checking if available...</div>
...
   <div ng-show="regForm.email.$pending" class="ng-messages">Checking email...</div>

[tohagan]

Thought I'd run it past you first - especially as you'd need to up the version of ng-superlogin used by superlogin-demo.

[colinskow]

For pending what would be cool is a tiny material spinner next to username or email that turns into a checkmark or X after validation.

[tohagan]

Agreed. Not sure if I can spare the time for that right now. I'm not using MD in my Ionic app.

[tohagan]

Ahh ... just spotted a bug with my solution. The gotcha is that the call to ctrl.$setValidity('offline', false); needs a reference to the ctrl. So we'd need to send that to the checkUsername() call. We make it the 2nd param and test it to ensure backward compatibility.

       validateUsername: function(username, ctrl) {
          return $http.get(superloginSession.getConfig().baseUrl + 'validate-username/' + encodeURIComponent(username))
            .then(function() {
              return $q.when(true);
            }, function(err) {
              // login server or internet offline?
              if(err.status === 404 || err.status === -1) {
                if (ctrl) ctrl.$setValidity('offline', false);
                return $q.when(true);
              }
              if(err.status === 409) {
                return $q.reject(false);
              }
              return $q.reject(err.data);
            });
        },
        validateEmail: function(email, ctrl) {
          return $http.get(superloginSession.getConfig().baseUrl + 'validate-email/' + encodeURIComponent(email))
            .then(function () {
              return $q.when(true);
            }, function (err) {
              // login server or internet offline?
              if(err.status === 404 || err.status === -1) {
                if (ctrl) ctrl.$setValidity('offline', false);
                return $q.when(true);
              }
              if(err.status === 409) {
                return $q.reject(false);
              }
              return $q.reject(err.data);
            });
        }

[colinskow]

But update SL Demo here in the validation directives. https://github.com/colinskow/superlogin-demo/blob/master/client/www/src/login/login.js

NgSL shouldn't need any changes.

[colinskow]

Catch the 404 and set the validation state.

[tohagan]

Can't catch it ... too late since it returns return $q.reject(err.data); not return $q.reject(err); so I cannot test err.status.

[tohagan]

So yes ... another solution would be to make this change (but it would be a breaking change to ng-superlogin).

[tohagan]

The fix above would have no effect on existing apps.

[tohagan]

However you might argue that it would be better to return err in case we find that we need to deal with other issues.

[tohagan]

My actual tested code does this ... but I'd argue, the fix probably belongs in ng-superlogin Your call Colin - I'm happy to PR the change to your preference.

  .directive('checkUsername', function($q, $http, superlogin) {
    return {
      require: 'ngModel',
      link: function(scope, elm, attrs, ctrl) {

        ctrl.$asyncValidators.checkUsername = function(modelValue) {
          return $http.get(superlogin.getConfig().baseUrl + 'validate-username/' + encodeURIComponent(modelValue))
            .then(function() {
              return $q.when(true);
            }, function(err) {
              // login server or internet offline?
              if(err.status === 404 || err.status === -1) {
                ctrl.$setValidity('offline', false);
                return $q.when(true);
              }
              if(err.status === 409) {  // existing
                return $q.reject(false);
              }
              return $q.when(true);
            });
        };
      }
    };
  })

[tohagan]

Sorry Colin ... Your spinner suggestion was dead easy in Ionic and much better than my div which suffers from a poor flashing on/off UX.

[colinskow]

I think putting the directives directly in NG-SuperLogin is the right move. But leave the existing checkUsername function alone so it is not a breaking change:

.directive('checkUsername', function($q, $http, superlogin) {
    return {
      require: 'ngModel',
      link: function(scope, elm, attrs, ctrl) {

        ctrl.$asyncValidators.checkUsername = function(modelValue) {
          return superlogin.checkUsername(modelValue)
            .then(function() {
              return $q.when(true);
            }, function(err) {
              // login server or internet offline?
              if(err.status === 404 || err.status === -1) {
                ctrl.$setValidity('offline', false);
                return $q.when(true);
              }
              if(err.status === 409) {  // existing
                return $q.reject(false);
              }
              return $q.when(true);
            });
        };
      }
    };
  })

[tohagan]

You code above won't work. The reason is that superlogin.checkUsername() returns an error as $q.reject(err.data); not $q.reject(err); so the code above cannot access err.status. Also the 409 case has already been tested inside superlogin.checkUsername() - but it returns $q.reject(false); not $q.when(false); !!

Arguably both superlogin.checkUsername() and superlogin.checkEmail() should return err (unlike the other methods nearby in superlogin that are not ngMessage validators that also return err.data) but this would be a breaking change - though a fairly minor one since most app will only be relying on the true/false return values.

[tohagan]

Whats also confusing is that both superlogin.checkUsername() and superlogin.checkEmail() can return an error using ...

if (err.status === 409) {
  return $q.reject(false);
}

which I think should have been ...

if (err.status === 409) {
  return $q.when(false);
}

[colinskow]

Go ahead and patch superlogin.checkUsername() to return $q.reject(err) if that is necessary. According to the Angular documentation, rejecting the promise is correct when an async validation fails.

[tohagan]

OK. Glad you knew that! Thanks I'll patch checkUsername() and checkEmail().