Skip to content
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

Add logout confirmation screen #322

Merged
merged 18 commits into from Sep 16, 2019

Conversation

@pniedzwiedzinski
Copy link
Contributor

commented Aug 10, 2019

I added logout confirmation screen (#262)

@monikakrawczak monikakrawczak added this to the 2019-09 milestone Aug 21, 2019

frontend/src/store/actions.js Outdated Show resolved Hide resolved
@adamgajzlerowicz

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2019

Please fix linter

@pniedzwiedzinski

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2019

Should I update this?

Feature: Logout
@skip
Scenario: User logs out from pah-fm website using Logout button
Given User is logged into pah-fm website
When User clicks on Logout button
Then User's session details are removed
And User is redirected to the Login view
@skip
Scenario: User logs out from pah-fm website using URL
Given User is logged into Ppah-fm website
When User enters URI '/logout' directly in the address bar
Then User's session details are removed
And User is redirected to the Login view

@arturtamborski

This comment has been minimized.

Copy link
Contributor

commented Aug 31, 2019

Should I update this?

No, we do not require contributions with behave changes. Though unit tests would be cool but as far as I know we don't plan adding frontend testing suite.

@arturtamborski
Copy link
Contributor

left a comment

There's just one more error - when user logs out a stack exception is thrown:
image
That's certainly a bug. I've traced it down to line #33 in NavigationItems.vue. Variable user is used there which won't exist when user is logged out (/logout_success route).

frontend/src/App.vue Outdated Show resolved Hide resolved
@pniedzwiedzinski

This comment has been minimized.

Copy link
Contributor Author

commented Sep 2, 2019

Fixed

@arturtamborski

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2019

I really appreciate your work here though we still need few changes.

First thing - logout view is not aligned correctly. That's an issue because our app is used almost exclusively on mobile devices. Currently logout view is aligned to left whereas it should be on the middle.
image

Second thing - /logout_success is not blocked / hidden in any way. I can now open up the project website, not log in and go straight to /logout_success and it will show me that I've logged out.
That's technically not an issue but it's also not desirable.

Feel free to ask for help and keep up the good work, we're almost there ;)

@arturtamborski
Copy link
Contributor

left a comment

Looks great, thank you for your work on this.
@adamgajzlerowicz can you take a look?

.prettierrc Show resolved Hide resolved
@pniedzwiedzinski

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2019

There’s a Bug with menu on logout that I have not fixed yet

@adamgajzlerowicz

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2019

PR is looking good. Lets finish if there is anything left - we have been holding this pr for ages now. I am happy to fix whatever is left in a separate pr

@pniedzwiedzinski

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2019

I'm not sure about this last commit. It doesn't seem like the most elegant solution, but it works! What do you guys think? Is it ok?

@arturtamborski

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2019

Looks good enough for me though I'm not a front-end guy.

As for improving it I was thinking what if you'd display the sidebar only when user is logged in (in other words only when we have a session key).

You could remove the current functions isLogin and isLogout as they only check url and replace the v-if to something like this:
v-if=isLoggedIn.

This function could then check if said token was in localStorage then user could be considered as logged in so we could show the sidebar.

@arturtamborski arturtamborski self-requested a review Sep 12, 2019

@arturtamborski

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2019

It doesn't seem like the most elegant solution, but it works! What do you guys think? Is it ok?

Well it's not bad, but I was thinking, maybe it would be better to take out

const userLoggedIn = getItem(tokenKey);

to external function and use that for deciding if the side bar should or shouldn't be shown.

Also, I'm not sure if the cause is in your commits but I see some 403 after logging in and sometimes I can't log in at all. I'll have to test this.

@pniedzwiedzinski

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2019

@pniedzwiedzinski

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2019

It should be ready for merge.

@arturtamborski

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2019

Yup, just tested it, looks great!

@arturtamborski arturtamborski merged commit 5001276 into CodeForPoznan:develop Sep 16, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.