Skip to content

[70$]Logout callback #18

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
gondzo opened this issue Dec 31, 2016 · 16 comments
Closed

[70$]Logout callback #18

gondzo opened this issue Dec 31, 2016 · 16 comments

Comments

@gondzo
Copy link
Contributor

gondzo commented Dec 31, 2016

When logging out, the dropdown doesn't always close and userInfo from local storage isn't cleared. UserInfo key is set in global.js and in AuthService.js, that should be refactored to a common method

@gvir
Copy link

gvir commented Jan 4, 2017

Can we pick these issues ?

@gondzo
Copy link
Contributor Author

gondzo commented Jan 4, 2017

yes, you can assign the issue to yourself

@gvir gvir self-assigned this Jan 4, 2017
@gvir
Copy link

gvir commented Jan 4, 2017

Took master branch , but log out button does not do anything functionality wise , so do we have to add the functionality of logging out ?

@gvir gvir removed their assignment Jan 4, 2017
@birdofpreyru
Copy link
Collaborator

Hey, can I pick up this one? (It seems GitHub does not show me the option to self-assign myself, or I don't know where to look for it :)

@gondzo
Copy link
Contributor Author

gondzo commented Jan 4, 2017

Logout button should remove the userInfo from local storage, clear the access token from the api service and close that dropdown

@gondzo
Copy link
Contributor Author

gondzo commented Jan 4, 2017

If you are working on this, assign it to yourself in the right side pane (assignees section)

@birdofpreyru birdofpreyru self-assigned this Jan 4, 2017
@birdofpreyru
Copy link
Collaborator

@gondzo I see you have created issue-logout-callback branch at the commit 171b4ee, but don't you think it is better to work from the latest commit in the dev branch (f5d94cb), as between those two commits a bunch of login related functionality have been merged in (judging by commit names)?

@gondzo
Copy link
Contributor Author

gondzo commented Jan 4, 2017

I'm not sure what the issue-logout-callback branch was for, but I've forwarded it to dev. You can make a PR against dev too.

@gvir
Copy link

gvir commented Jan 4, 2017 via email

birdofpreyru added a commit to birdofpreyru/dsp-frontend that referenced this issue Jan 4, 2017
It resolves the issue topcoderinc#18, but, probably, some further work on
refactoring of the related code is necessary.
@birdofpreyru
Copy link
Collaborator

@gondzo Hey, will you check the above mentioned commit in my fork?

It fixes the dropdown, so that it is closed when you click Profile or Logout options.
It fixes the logout, so that user info is removed from the local storage, and once you click the Logout the header switches to the one corresponding to the Not logged in state.
Thus, it kind of solve the problem.

It does not do refactoring of the AuthService as (1) the service (like all other services in the app) is quite a mess, and it is quite a more big task to refactor it; (2) actually, now when you login with username/password, the AuthService is not used at all, everything is handled by the code in global.js. However, it seems that AuthService is used for social login functionality, so, as I told already, it is quite a separate big task to refactor everything that it uses AuthService in consistent way, and does not break anything.

@gondzo
Copy link
Contributor Author

gondzo commented Jan 4, 2017

Looking at it right now.
As for the ApiService, there are quite a few challenges running in parallel, so I don't think we could manage to pull off refactoring for the ApiService.

@birdofpreyru
Copy link
Collaborator

Yeap. But, in future, it will be nice to implement a general APIService, which takes care about authorisation and staff, and allows to send any HTTP requests through it, not caring about the auth.

@gondzo
Copy link
Contributor Author

gondzo commented Jan 4, 2017

Agreed.
This looks fine. Can you add a redirect to /home on logout and create a PR

birdofpreyru added a commit to birdofpreyru/dsp-frontend that referenced this issue Jan 4, 2017
Adds redirect to /home on logout.

Fixes issue topcoderinc#18
@birdofpreyru birdofpreyru mentioned this issue Jan 4, 2017
@birdofpreyru
Copy link
Collaborator

Done

@gondzo
Copy link
Contributor Author

gondzo commented Jan 4, 2017

Thanks

@kbowerma
Copy link
Contributor

kbowerma commented Feb 1, 2017

Payment Scheduled: https://apps.topcoder.com/bugs/browse/DRONESERIES-7

@kbowerma kbowerma closed this as completed Feb 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants