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

Allow staying logged in for longer #4236

Merged
merged 5 commits into from Jun 22, 2018

Conversation

Projects
None yet
5 participants
@davidfischer
Contributor

davidfischer commented Jun 13, 2018

  • Makes it more obvious that a user won't see ads when they are a donor
  • Give donors an easy way to stay logged in for longer (3 months)

GOLD MEMBER/DONOR

screen shot 2018-06-13 at 1 56 38 pm

NON-DONOR

screen shot 2018-06-13 at 1 56 10 pm

@davidfischer davidfischer requested a review from rtfd/core Jun 13, 2018

@humitos

I like these changes.

It's not still clear to me what's the workflow for this. I suppose once the user gets it's gold membership a one week session is saved. Then the user has to go to this page and increase the session's time.

After those three months, the user will start seeing ads again and it has to do the same process, right?: Go to .org site, log in, go to admin, click this button again or just log in will be enough for next 3 months?

@ericholscher

This comment has been minimized.

Member

ericholscher commented Jun 14, 2018

This feels like a change that nobody will ever find or really use. Can we not catch this when the user logs in, and just extend gold user sessions automatically? Another question -- is there a reason that we have such short login sessions to begin with? Is there a reason not to just leave everyone logged in for 3 months?

@agjohnson

This comment has been minimized.

Contributor

agjohnson commented Jun 14, 2018

Also going to echo the concern of if we really need this in the UI. I don't think users will find this easily, and I'd like to see what doc-viewing-only user subscription rates look like before optimizing this all.

Is there a view where we can set_expiry on the session to extend this date for a doc-viewing-only user? If we have the session at footer_html, we could update the expiry there.

@stsewd

This comment has been minimized.

Member

stsewd commented Jun 14, 2018

Maybe related to #3259

@davidfischer

This comment has been minimized.

Contributor

davidfischer commented Jun 14, 2018

I'd be fine removing the "your login session will expire" part and the "stay logged in" button. My goal is to give gold members a way to not see ads for longer than 2 weeks without having to login constantly.

First, I'll describe our current setup and then I'll discuss options to meet that goal.

Current setup

  • When logging in, if you click the "remember me" checkbox, your login session will last for 2 weeks. 2 weeks is the Django default.
  • Merely visiting the website within that 2 week time period does not refresh the 2 week time period. However, anything that writes to request.session will restart a new 2 week window. We don't have anything that does that though.
  • If you do not click the "remember me" checkbox, your login cookie is a session cookie and is deleted when the browser closes. You'll need to login again the next time you open your browser.

Possible options

  • Increase the login cookie expiry. What is a reasonable expiry might depend on whether we combine this with any options below. I think 30 days at a minimum is fine.
  • Save the session cookie on every request. This is a setting in Django and is as simple as setting a flag. This means that anytime somebody visits readthedocs.org (including via the footer API call on docs pages) their session cookie expiry will refresh. This will involve an extra cache write (our production session settings use the cache) in addition to the existing cache read for the session on every request.
  • We could detect a gold member in the advertising API call (we already do this) and refresh their session cookie expiry.

My recommendation

  • Remove the "your login session will expire" part and the "stay logged in" button on the advertising screen
  • Change the login cookie expiry to 30 days
  • Save the session on every request

This means that somebody who visits readthedocs.org (including via the footer call) will never need to login again as long as they visit once every 30 days. Anybody who doesn't click the "remember me" button will still logout when they close their browser.

This would solve #3259 and reduce the frequency that .com users need to login.

Side note

I have a preference that our production session settings used signed cookies as opposed to cache based sessions. Since we don't write much of anything to the session there are almost no downsides and the upside is to avoid having to connect to the cache on every request which we do now. It would also free up cache space. Making this change though would log everyone out which isn't a big deal right now since people get logged out every 2 weeks anyway.

@ericholscher

This comment has been minimized.

Member

ericholscher commented Jun 15, 2018

I'm +1 on the suggested implementation (30 days & re-uping on request). It seems like moving to signed cookies would be a nice performance win too. Is there anything else on the footer call that depends on the cache, or would this allow us to continue serving footer requests even when the cache is down? IIRC we hit it in middleware for CNAME's, and perhaps a couple other places, but reducing the usage of it makes a lot of sense to me.

davidfischer added some commits Jun 15, 2018

@davidfischer

This comment has been minimized.

Contributor

davidfischer commented Jun 15, 2018

I made the changes to remove the UI elements to extend the log in session and just made them the defaults.

  • 30 day session cookie expiry
  • resave the session on every request (the 30 day expiry becomes 30 days after the last request)
  • Switch to signed cookie sessions

NOTE: Deploying this will log everyone out although after that they will stay logged in much longer.

Fixes #3259

Is there anything else on the footer call that depends on the cache

There doesn't appear to be but advertising does rely on the cache.

@@ -310,5 +310,6 @@ def account_advertising(request):
'form': form,
'profile': profile_obj,
'user': profile_obj.user,
'session_expiry': request.session.get_expiry_date(),

This comment has been minimized.

@stsewd

stsewd Jun 15, 2018

Member

I believe this isn't used with the recent changes

@humitos

These changes make sense to me and will help Gold users to not see ads for a long time :)

I took a look at other places where we use sessions and I found this middleware that do not creates a session for non-logged in users when hitting the footer. I think it stills makes sense, but wanted to mention to you just in case you see that something need to be touched there:

class FooterNoSessionMiddleware(SessionMiddleware):

SESSION_COOKIE_DOMAIN = 'readthedocs.org'
SESSION_COOKIE_HTTPONLY = True
SESSION_COOKIE_AGE = 30 * 24 * 60 * 60 # 30 days
SESSION_ENGINE = 'django.contrib.sessions.backends.signed_cookies'

This comment has been minimized.

@humitos

humitos Jun 18, 2018

Member

If we are going to use signed cookies, don't we want to remove django.contrib.sessions from INSTALLED_APPS?. What I understand from the documentation it's only used for database-cookies.

https://docs.djangoproject.com/en/1.11/topics/http/sessions/#using-database-backed-sessions

This comment has been minimized.

@davidfischer

davidfischer Jun 21, 2018

Contributor

I'm thinking of actually taking this change out and making this a setting that is set in the production settings rather than in the base settings.

This comment has been minimized.

@davidfischer

davidfischer Jun 21, 2018

Contributor

There are advantages to signed cookies but they aren't huge and they are what's holding this up due to the complications with the corporate site.

This comment has been minimized.

@davidfischer

davidfischer Jun 21, 2018

Contributor

I removed the SESSION_ENGINE setting here. If we decide to change the production SESSION_ENGINE, we'll do that in the production settings file. This will ensure that anyone taking our code and using it don't have a SESSION_ENGINE change unless they want one.

This comment has been minimized.

@ericholscher

ericholscher Jun 22, 2018

Member

Makes sense. 👍

@ericholscher

Looks good.

@davidfischer

This comment has been minimized.

Contributor

davidfischer commented Jun 18, 2018

No changes should be required by the FooterNoSessionMiddleware

@ericholscher ericholscher merged commit 08591ea into master Jun 22, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@humitos humitos deleted the davidfischer/advertising-preferences-tweak branch Jun 22, 2018

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