Login/out button cleanups #1079

Merged
merged 7 commits into from Dec 14, 2011

Conversation

Projects
None yet
3 participants
@stefanv
Contributor

stefanv commented Dec 1, 2011

  • Display a login button when viewing the notebook in read-only mode.
  • On the login page, focus on the password field by default.
  • Correctly style login / logout buttons.
  • In read-only mode, redirect to "/" after logout, not "/login".
@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Dec 1, 2011

Member

The login button should not appear in 'default' mode, where it does exactly nothing. Do you know why your PR disables the hidden logic that was there before?

Member

minrk commented Dec 1, 2011

The login button should not appear in 'default' mode, where it does exactly nothing. Do you know why your PR disables the hidden logic that was there before?

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Dec 1, 2011

Member

Also, what should happen in the case where read-only mode is the only one available (no password is set)? In your case, you can enter any password, and login just redirects you to the front page. This suggests that login was successful, but it obviously wasn't, as the login button remains (and login is impossible).

I should think that at the very least, trying to login should always fail, and ideally the login button should be disabled in this case as well.

Member

minrk commented Dec 1, 2011

Also, what should happen in the case where read-only mode is the only one available (no password is set)? In your case, you can enter any password, and login just redirects you to the front page. This suggests that login was successful, but it obviously wasn't, as the login button remains (and login is impossible).

I should think that at the very least, trying to login should always fail, and ideally the login button should be disabled in this case as well.

@stefanv

This comment has been minimized.

Show comment
Hide comment
@stefanv

stefanv Dec 1, 2011

Contributor

@minrk The current logic is already broken; it never shows a login button in read-only mode. The problem is that we only make a distinction between read-only vs. non-read-only modes, whereas there is a third case: read-only but with the option to log in. So maybe we can give the read-only property some more states. I'll have a look at how to best accomplish this.

Contributor

stefanv commented Dec 1, 2011

@minrk The current logic is already broken; it never shows a login button in read-only mode. The problem is that we only make a distinction between read-only vs. non-read-only modes, whereas there is a third case: read-only but with the option to log in. So maybe we can give the read-only property some more states. I'll have a look at how to best accomplish this.

@stefanv

This comment has been minimized.

Show comment
Hide comment
@stefanv

stefanv Dec 1, 2011

Contributor

OK, I see we do have three states in there: None, True and False. I'll check for these separately.

Contributor

stefanv commented Dec 1, 2011

OK, I see we do have three states in there: None, True and False. I'll check for these separately.

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Dec 1, 2011

Member

I think the existence of a non-empty password is the thing that tells you whether login is possible.

Member

minrk commented Dec 1, 2011

I think the existence of a non-empty password is the thing that tells you whether login is possible.

@stefanv

This comment has been minimized.

Show comment
Hide comment
@stefanv

stefanv Dec 1, 2011

Contributor

Yes, but we don't want to do that logic inside of the templates, if possible.

Contributor

stefanv commented Dec 1, 2011

Yes, but we don't want to do that logic inside of the templates, if possible.

@stefanv

This comment has been minimized.

Show comment
Hide comment
@stefanv

stefanv Dec 1, 2011

Contributor

@minrk This is the basic idea I had in mind. There are different ways to implement it (e.g., have read_only return a string instead of True, False, None), but this seems to work for now.

Contributor

stefanv commented Dec 1, 2011

@minrk This is the basic idea I had in mind. There are different ways to implement it (e.g., have read_only return a string instead of True, False, None), but this seems to work for now.

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Dec 4, 2011

Member

There is actually one more case to handle: no password set, and not read-only. This is by far the most common case, and should not draw any login/logout buttons.

Member

minrk commented Dec 4, 2011

There is actually one more case to handle: no password set, and not read-only. This is by far the most common case, and should not draw any login/logout buttons.

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Dec 13, 2011

Member

@stefanv, this PR needs a rebase as it's now conflicting. Do you think you'll have time to finish it up before we cut the 0.12 RC? That basically means finish it today...

Member

fperez commented Dec 13, 2011

@stefanv, this PR needs a rebase as it's now conflicting. Do you think you'll have time to finish it up before we cut the 0.12 RC? That basically means finish it today...

@stefanv

This comment has been minimized.

Show comment
Hide comment
@stefanv

stefanv Dec 13, 2011

Contributor

@fperez I'm on it..
On Dec 13, 2011 11:28 AM, "Fernando Perez" <
reply@reply.github.com>
wrote:

@stefanv, this PR needs a rebase as it's now conflicting. Do you think
you'll have time to finish it up before we cut the 0.12 RC? That basically
means finish it today...


Reply to this email directly or view it on GitHub:
#1079 (comment)

Contributor

stefanv commented Dec 13, 2011

@fperez I'm on it..
On Dec 13, 2011 11:28 AM, "Fernando Perez" <
reply@reply.github.com>
wrote:

@stefanv, this PR needs a rebase as it's now conflicting. Do you think
you'll have time to finish it up before we cut the 0.12 RC? That basically
means finish it today...


Reply to this email directly or view it on GitHub:
#1079 (comment)

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Dec 13, 2011

Member

ok!

Member

fperez commented Dec 13, 2011

ok!

@stefanv

This comment has been minimized.

Show comment
Hide comment
@stefanv

stefanv Dec 14, 2011

Contributor

@fperez @minrk This change should make the logic a lot clearer. There are three states that need to be monitored, each now associated with a method:

  • Was the notebook started with the "read-only" flag (method: read_only)
  • Can the user log in, thereby making the notebook rw (method: login_available)
  • Is the current user logged in? (method: logged_in)

I also removed display logic from the javascript, and moved it to the template. Finally, the password field now receives automatic focus.

Contributor

stefanv commented Dec 14, 2011

@fperez @minrk This change should make the logic a lot clearer. There are three states that need to be monitored, each now associated with a method:

  • Was the notebook started with the "read-only" flag (method: read_only)
  • Can the user log in, thereby making the notebook rw (method: login_available)
  • Is the current user logged in? (method: logged_in)

I also removed display logic from the javascript, and moved it to the template. Finally, the password field now receives automatic focus.

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Dec 14, 2011

Member

Awesome, much cleaner than it was before, and I've tried every combination of being logged in / out having password defined / undefined, and readonly, and it seems to always behave how I expect.

The only thing that didn't seem perfect was the fact that login/logout buttons are still drawn on the login page. If that's an easy fix, go for it, otherwise I'm +1 on merging right now.

Member

minrk commented Dec 14, 2011

Awesome, much cleaner than it was before, and I've tried every combination of being logged in / out having password defined / undefined, and readonly, and it seems to always behave how I expect.

The only thing that didn't seem perfect was the fact that login/logout buttons are still drawn on the login page. If that's an easy fix, go for it, otherwise I'm +1 on merging right now.

@stefanv

This comment has been minimized.

Show comment
Hide comment
@stefanv

stefanv Dec 14, 2011

Contributor

That's a trivial fix. Give me one second.

Contributor

stefanv commented Dec 14, 2011

That's a trivial fix. Give me one second.

@stefanv

This comment has been minimized.

Show comment
Hide comment
@stefanv

stefanv Dec 14, 2011

Contributor

@minrk That should do it.

Contributor

stefanv commented Dec 14, 2011

@minrk That should do it.

Correctly set read_only meta name for use in javascript. Note that th…
…is has a different meaning than in the Python code: not whether the read-only flag was specified upon launch, but whether the notebook is in read-only mode and input should be disabled.

fperez added a commit that referenced this pull request Dec 14, 2011

Merge pull request #1079 from stefanv/htmlnotebook_login_button
Login/out button cleanups:

- Display a login button when viewing the notebook in read-only mode.
- On the login page, focus on the password field by default.
- Correctly style login / logout buttons.
- In read-only mode, redirect to "/" after logout, not "/login".

@fperez fperez merged commit 2bfc449 into ipython:master Dec 14, 2011

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014

Merge pull request #1079 from stefanv/htmlnotebook_login_button
Login/out button cleanups:

- Display a login button when viewing the notebook in read-only mode.
- On the login page, focus on the password field by default.
- Correctly style login / logout buttons.
- In read-only mode, redirect to "/" after logout, not "/login".
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment