Fix redirection to previous state after required authentication #901

igorauad · 2015-09-10T00:46:30Z

$stateChangeSuccess event should be signaled in order to allow "recording" of previous state.

This part of the implementation was missing, I just realized that.

ilanbiala · 2015-09-10T01:03:45Z

@igorauad did we not have tests for this?

igorauad · 2015-09-10T12:24:30Z

Not yet, @ilanbiala . I don't think so.

ilanbiala · 2015-09-10T16:04:28Z

@igorauad can you also add tests to this PR then? Depending on how quickly we can get this rolling (hopefully for 0.4.2), then I'll milestone it right before we merge.

igorauad · 2015-09-10T18:01:47Z

Sure, @ilanbiala . Just give me some time. I will probably find time tomorrow.

rhutchison · 2015-09-11T05:47:44Z

@igorauad great find, but the proposed solution needs some work. We should avoid firing the success event, when the state was not successful (it was interrupted). There is a better way to preserve the previous (interrupted state) without an event trigger.

The reason I am against reusing the event is because your assumption is that it will only trigger the listener to record the previous state - you are not considering that there may be other listeners.

mleanos · 2015-09-11T07:13:01Z

@igorauad I agree with @rhutchison that we don't want to fire the $stateChangeSuccess again. This would end up firing it twice in that cycle. Among the other reasons that Ryan mentioned.

I pulled down your branch locally, and made modifications as an alternative to this solution. Let me know if you think this will suffice?

mleanos@f86c948

rhutchison · 2015-09-11T13:32:22Z

@mleanos the commit looks good.

@igorauad can you review his solution and let us know if you think this is a better approach? If you agree, can you update your PR to reflect those changes, plus any tests you can put around it.

igorauad · 2015-09-11T20:27:59Z

modules/core/client/app/init.js

@@ -30,7 +30,9 @@ angular.module(ApplicationConfiguration.applicationModuleName).run(function ($ro
        if (Authentication.user !== undefined && typeof Authentication.user === 'object') {
          $state.go('forbidden');
        } else {
-          $state.go('authentication.signin');
+          $state.go('authentication.signin').then(function () {


We could also use something like $state.go('authentication.signin', {warn: 'Please, sign in first.'}) here. Then, print that in alert in the signin template. What do you think?

@igorauad What would that accomplish?

@mleanos just UX, for me it seems better to let the user know why the signin page is being displayed rather than the requested one.

I think most users know nowadays, no?

Yes @ilanbiala Ok, in fact that depends on the audience and the application, so we shouldn't worry about it here.

codydaig · 2015-09-12T23:24:38Z

LGTM

ilanbiala · 2015-09-13T20:11:24Z

@igorauad any tests you can throw in?

igorauad · 2015-09-14T02:17:01Z

Yes @ilanbiala , I just need to learn a bit about tests, because I'm not experienced on this yet :)

These tests should be included in authentication.client.controller.tests.js, correct?

If anybody want to contribute first, feel free.

ilanbiala · 2015-09-14T16:41:44Z

@igorauad yeah probably, seems most appropriate there. Add whatever you can and we'll go from there.

igorauad · 2015-09-15T01:34:21Z

@ilanbiala just pushed my first attempt. Not yet successful :( any hints?

mleanos · 2015-09-15T01:42:14Z

@igorauad Do the tests pass locally for you?

igorauad · 2015-09-15T01:49:47Z

@mleanos nop, I'm on the learning curve yet. Just pushed following the advice from @ilanbiala: "Add whatever you can and we'll go from there."

mleanos · 2015-09-15T01:51:50Z

@igorauad Ok, that's fine. I'll try on my end.. I'm not proficient with the Karma tests yet either. Maybe together we'll both learn from this :)

igorauad · 2015-09-15T02:21:32Z

Cool @mleanos :)

ilanbiala · 2015-09-15T03:29:29Z

Error for reference:

Expected spy transitionTo to have been called with [ Object({ state: Object({ name: '', url: '^', views: null, abstract: true }), params: Object({  }), href: null }) ] but it was never called.
        at /home/travis/build/meanjs/mean/modules/users/tests/client/authentication.client.controller.tests.js:82

@igorauad I think either the spy isn't on the right thing or scope.signin isn't calling, but I'll look into it more tomorrow when I have time.

mleanos · 2015-09-16T07:26:41Z

modules/users/tests/client/authentication.client.controller.tests.js

+
+            scope.signin(true);
+            $httpBackend.flush();
+            spyOn($state, 'transitionTo');


This should be set before scope.signin(true) (line 77).

However, even after that modification, the test doesn't pass. The actuals returned from the below expect don't match up with the previous state defined in this test. Upon further investigation, I logged $state.previous to the console in the signin method of the AuthenticationController & it was equal to the previous state defined here in this test. Lastly, I logged a test to the console from ChatController & it didn't log. So it appears, it's never getting to the previous state. Somewhat of a head scratcher.

Anyone else wanna take a shot at this? I'll keep working on it, but probably could use a fresh set of eyes.

mleanos · 2015-09-16T21:23:31Z

@ilanbiala @igorauad I was able to get the test to pass.. It appears that both the $state.transitionTo & $state.go must be spied on.. I can't tell you why, but after spending quite a bit of time fiddling with this test, this is the only way I got it to work.

it('should be redirected to previous state after successful login',
  inject(function (_$state_) {
    $state = _$state_;
    $state.previous = {
      state: {
        name: 'articles.create'
        },
        params: {},
        href: '/articles/create'
     };

     spyOn($state, 'transitionTo');
     spyOn($state, 'go');

     // Test expected GET request
     $httpBackend.when('POST', '/api/auth/signin').respond(200, 'Fred');

     scope.signin(true);
     $httpBackend.flush();

     // Test scope value
     expect($state.go).toHaveBeenCalledWith($state.previous.state.name, $state.previous.params);
}));

ilanbiala · 2015-09-17T01:33:36Z

@igorauad can you see if that solves it for you and if so push it?

igorauad · 2015-09-17T02:56:47Z

@ilanbiala just pushed. Yes, it solves. If somebody understand exactly why, that would be interesting to know. Thank you very much @mleanos

mleanos · 2015-09-17T03:12:48Z

@igorauad Looks like timeout errors, during the build. Hopefully, it was due to a hiccup with Mongo.

@ilanbiala @lirantal Do you have the ability to re-run the CI builds for this PR?

mleanos · 2015-09-17T03:42:28Z

@igorauad It's nice that at least the coveralls increased :)

ilanbiala · 2015-09-17T13:08:39Z

MongoError: topology was destroyed and timeout error on Node 0.12, but not 0.10. Not sure why. 4.0 is going crazy with Nan.

lirantal · 2015-09-18T07:16:28Z

Yes, I re-ran the build now, let's see how it goes.

lirantal · 2015-09-18T07:25:03Z

CI LGTM now.

igorauad · 2015-09-19T18:47:47Z

Is there anything else we should do on this?

codydaig · 2015-09-19T18:50:11Z

@igorauad Squash, and it should be good.

mleanos · 2015-09-19T19:29:54Z

LGTM. Just needs squashing.

Fixes the issue with the previous state not being recorded, when the unauthenticated user is redirected to the signin state, when trying to access a restricted route. Added a function that stores the provided state & state params, in the $state.previous object. This has been implemented in the $stateChangeSuccess event, and the callback of the $state.go transition when the user is not allowed to access the requested route.

igorauad · 2015-09-19T19:36:44Z

Done, @codydaig @mleanos

mleanos · 2015-09-19T23:12:33Z

@igorauad Thanks

igorauad · 2015-09-20T00:19:36Z

Thanks, @mleanos

Fix redirection to previous state after required authentication

ilanbiala self-assigned this Sep 10, 2015

ilanbiala added issue:bug enhance:feature labels Sep 10, 2015

ilanbiala added this to the 0.4.x milestone Sep 10, 2015

igorauad reviewed Sep 11, 2015
View reviewed changes

mleanos reviewed Sep 16, 2015
View reviewed changes

igorauad force-pushed the fixRedirection branch from a9cd4ce to 2b8bee0 Compare September 19, 2015 19:36

ilanbiala added a commit that referenced this pull request Sep 26, 2015

Merge pull request #901 from igorauad/fixRedirection

5901b17

Fix redirection to previous state after required authentication

ilanbiala merged commit 5901b17 into meanjs:master Sep 26, 2015

ilanbiala modified the milestones: 0.4.2, 0.4.x Sep 26, 2015

Fix redirection to previous state after required authentication #901

Fix redirection to previous state after required authentication #901

Conversation

igorauad commented Sep 10, 2015

ilanbiala commented Sep 10, 2015

igorauad commented Sep 10, 2015

ilanbiala commented Sep 10, 2015

igorauad commented Sep 10, 2015

rhutchison commented Sep 11, 2015

mleanos commented Sep 11, 2015

rhutchison commented Sep 11, 2015

igorauad Sep 11, 2015

Choose a reason for hiding this comment

mleanos Sep 12, 2015

Choose a reason for hiding this comment

igorauad Sep 12, 2015

Choose a reason for hiding this comment

ilanbiala Sep 12, 2015

Choose a reason for hiding this comment

igorauad Sep 12, 2015

Choose a reason for hiding this comment

codydaig commented Sep 12, 2015

ilanbiala commented Sep 13, 2015

igorauad commented Sep 14, 2015

ilanbiala commented Sep 14, 2015

igorauad commented Sep 15, 2015

mleanos commented Sep 15, 2015

igorauad commented Sep 15, 2015

mleanos commented Sep 15, 2015

igorauad commented Sep 15, 2015

ilanbiala commented Sep 15, 2015

mleanos Sep 16, 2015

Choose a reason for hiding this comment

mleanos commented Sep 16, 2015

ilanbiala commented Sep 17, 2015

igorauad commented Sep 17, 2015

mleanos commented Sep 17, 2015

mleanos commented Sep 17, 2015

ilanbiala commented Sep 17, 2015

lirantal commented Sep 18, 2015

lirantal commented Sep 18, 2015

igorauad commented Sep 19, 2015

codydaig commented Sep 19, 2015

mleanos commented Sep 19, 2015

igorauad commented Sep 19, 2015

mleanos commented Sep 19, 2015

igorauad commented Sep 20, 2015