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

Don't empty _session_cache during create on SessionStore #63

Closed
wants to merge 2 commits into from
Closed

Don't empty _session_cache during create on SessionStore #63

wants to merge 2 commits into from

Conversation

Remiz
Copy link

@Remiz Remiz commented Dec 15, 2016

Starting Django 1.7 the create() method of db/SessionStore removes this line.

I noticed an issue when using the SessionStore manually with Django 1.9 and django-user-sessions:

ipdb>session = import_module(settings.SESSION_ENGINE).SessionStore(some_session_key)
ipdb>session['foo'] = 'bar'
ipdb>print session.get('foo')
bar
ipdb> session.create()
ipdb> print session.get('foo')
None

I'm not 100% sure how it'll impact Django 1.6 and older but considering the support was dropped in 2014, maybe it won't impact so many people.

Starting Django 1.7 the create() method of db/SessionStore removes this line.

I noticed an issue when using the SessionStore manually with Django 1.9 and django-user-sessions:
```
ipdb>session = import_module(settings.SESSION_ENGINE).SessionStore(some_session_key)
ipdb>session['foo'] = 'bar'
ipdb>print session.get('foo')
bar
ipdb> session.create()
ipdb> print session.get('foo')
None
```
I'm not 100% sure how it'll impact Django 1.6 and older but considering the support was dropped in 2014, maybe it won't impact so many people.
@coveralls
Copy link

coveralls commented Dec 15, 2016

Coverage Status

Coverage decreased (-0.03%) to 89.931% when pulling ae5080b on Remiz:patch-1 into 8b7a336 on Bouke:master.

@Bouke
Copy link
Collaborator

Bouke commented Dec 15, 2016

This is the upstream commit where this line was removed on the master branch. And it was also backported in this commit to version 1.8. Seeing that this change was made to the versions that this package currently supports (1.8 and up), this change can be merged. However as there are quite a few upstream changes in those commits, it is prudent to review those changes and see if/how they should be merged.

@codecov
Copy link

codecov bot commented Mar 23, 2018

Codecov Report

Merging #63 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #63      +/-   ##
==========================================
- Coverage   93.84%   93.81%   -0.03%     
==========================================
  Files          10       10              
  Lines         276      275       -1     
  Branches       31       31              
==========================================
- Hits          259      258       -1     
  Misses         11       11              
  Partials        6        6
Impacted Files Coverage Δ
user_sessions/backends/db.py 89.39% <ø> (-0.16%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update db94bfd...0bd8137. Read the comment docs.

@Remiz Remiz closed this Sep 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants