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

Logout button #124

Open
wants to merge 16 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@emilojkovic
Collaborator

emilojkovic commented Apr 25, 2018

No description provided.

@nwalters512

Thanks Ema! Can you add a Changelog.md entry and fix the linter errors?

{userName}
</NavLink>
</Link>
<div className="d-flex justify-content-between">

This comment has been minimized.

@nwalters512

nwalters512 Apr 25, 2018

Member

What's the purpose of this div?

This comment has been minimized.

@emilojkovic

emilojkovic Apr 25, 2018

Collaborator

It was something I put in when I tried to add more space between the username and the logout button, but that is no longer relevant because I used "ml-3" in the button instead... deleting the div!

@@ -36,6 +38,10 @@ class Header extends React.Component {
})
}
handleLogout() {
window.location = 'https://edu.cs.illinois.edu/Shibboleth.sso/Logout'

This comment has been minimized.

@nwalters512

nwalters512 Apr 25, 2018

Member

This should probably grab the actual host that we're running on, given that this link would currently break in our staging environment. Check out https://github.com/illinois/queue/blob/master/src/components/CourseShortCodeInfo.js#L13

@@ -8,6 +8,7 @@ with the current date and the next changes should go under a **[Next]** header.
## [Next]
* Reorganize directory structure. ([@nwalters512](https://github.com/nwalters512) in [#108](https://github.com/illinois/queue/pull/108))
* Added logout button. ([@emilojkovic](https://github.com/emilojkovic) in [#124](https://github.com/illinois/queue/pull/124))

This comment has been minimized.

@nwalters512

nwalters512 Apr 25, 2018

Member

nitpick: present-tense 🙂

const styles = {
navbar: {
zIndex: '10',
},
}
const origin = (typeof this !== 'undefined' && this.location.origin) || ''

This comment has been minimized.

@nwalters512

nwalters512 Apr 25, 2018

Member

Why this and not window?

This comment has been minimized.

@emilojkovic

emilojkovic Apr 26, 2018

Collaborator

window isn't defined there -- the Travis build gave me the following error when I used window:
/home/travis/build/illinois/queue/build/components/Header.js 26:50 error 'window' is not defined no-undef.

Is there something that I can import to make window defined? I noticed that window is not used anywhere else in the header.js file.

I also used this in the handleLogout function for the same reason:
handleLogout() { this.location = logoutLink }

This comment has been minimized.

@nwalters512

nwalters512 Apr 26, 2018

Member

That's just a complaint from ESLint: it doesn't know the context of where the code will be run (it assumes a plain Node environment by default), and that doesn't have a window object. You can tell ESLint to assume a browser env, which should get rid of the error: https://github.com/illinois/queue/blob/master/src/util/index.js#L1

You can run the linter locally before committing+pushing with npm run lint-js! Same goes for tests: npm test.

const styles = {
navbar: {
zIndex: '10',
},
}
const origin = (typeof this !== 'undefined' && this.location.origin) || ''
const logoutLink = `${origin}${baseUrl}/Shibboleth.sso/Logout`

This comment has been minimized.

@nwalters512

nwalters512 Apr 25, 2018

Member

I think the logout link is for the host as a whole; it isn't going to be at our base url.

This comment has been minimized.

@emilojkovic

emilojkovic Apr 26, 2018

Collaborator

So the logout link that @wadefagen sent me (that also worked to log me out of the deployed queue) is https://edu.cs.illinois.edu/Shibboleth.sso/Logout I thought that edu.cs.illinois.edu was covered by the origin and baseUrl? How would this differ for the host?

This comment has been minimized.

@nwalters512

nwalters512 Apr 26, 2018

Member

Shib auth is done on a per-host basis, and our staging environment is edu-staging.cs.illinois.edu. It would be very confusing to click "Log out" in the staging website but be logged out of the actual queue! This also applies to cases where we're hypothetically deployed at UBC, for instance.

This comment has been minimized.

@nwalters512

nwalters512 Apr 26, 2018

Member

Also sorry, it's late, that didn't fully answer your question. You've got the host thing right by using location.origin, but the logout link doesn't need to include the base url. If it did, that would make the logout link https://edu.cs.illinois.edu/queue/Shibboleth.sso/Logout, since baseUrl is /queue in production.

emilojkovic and others added some commits Apr 26, 2018

@nwalters512

This comment has been minimized.

Member

nwalters512 commented Apr 26, 2018

Thanks Ema, this looks great! I'm not going to merge this just yet, I want to verify that this actually works in an actual deployment which will require me getting the staging environment back up and running.

@@ -23,7 +23,7 @@ export default class MyDocument extends Document {
// If we're deployed to anywhere other than the server root, we'll have to
// store our root path that here so that the client can access it.
const script = {
__html: `window.BASE_URL = '${baseUrl}';`,
__html: `window.BASE_URL = '${baseUrl}'; window.IS_DEV = '${isDev}';`,

This comment has been minimized.

@nwalters512

nwalters512 Apr 30, 2018

Member

Glad I tested this! We need the literal value false here, not the string "false". You should just be able to template ${isDev} instead of '${isDev}'.

@@ -36,6 +42,10 @@ class Header extends React.Component {
})
}
handleLogout() {
this.location = logoutLink

This comment has been minimized.

@nwalters512

nwalters512 Apr 30, 2018

Member

This should definitely be window

This comment has been minimized.

@emilojkovic

emilojkovic May 9, 2018

Collaborator

I'm getting a linter error when I use window instead of this. I think that was the reason I used this originally... The whole error is this:
45:15 error Expected 'this' to be used by class method 'handleLogout' class-methods-use-this

This comment has been minimized.

@nwalters512

nwalters512 May 9, 2018

Member

You can look up what that error means with its name: https://eslint.org/docs/rules/class-methods-use-this. In this case, that's just saying that if a class method doesn't use this, there's not really a reason for it to be a class method. In this case, you could either inline that in the onClick prop, or move the function outside the class.

@now

This comment has been minimized.

now bot commented Aug 18, 2018

This pull request is automatically deployed with Now.

To access deployments, click Details below or on the icon next to each push.

@nwalters512

This comment has been minimized.

Member

nwalters512 commented Aug 18, 2018

Hey @emilojkovic will you have time to wrap this up soon? Otherwise I'm happy to pick up where you left off.

@emilojkovic

This comment has been minimized.

Collaborator

emilojkovic commented Aug 20, 2018

Yeah! I have some time, I'll checkout it. Just to refresh, this.location = logoutLink should be changed (to what we discussed in the comments)?

@nwalters512

This comment has been minimized.

Member

nwalters512 commented Aug 20, 2018

Yep, that and the other bits!

redsn0w422 and others added some commits Apr 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment