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

Command for migrating existing sessions to the new session store #33

Merged
merged 5 commits into from
Nov 25, 2017

Conversation

ivorbosloper
Copy link
Contributor

When converting an in-production site from django.contrib.sessions to django-user-sessions this python manage.py sessions_to_usersessions.py command allows you to maintain existing sessions (keep already logged-in users logged-in) and make the transition smooth.

@coveralls
Copy link

Coverage Status

Coverage decreased (-9.33%) to 84.59% when pulling 24a9830 on ivorbosloper:convert_sessions into 555144f on Bouke:master.

@Bouke
Copy link
Collaborator

Bouke commented Feb 12, 2015

Hi @ivorbosloper, thanks for your contribution. Could you also write a small explanation of this command in the documentation, for helping other users making the switch? Furthermore, the coverage would decrease to 85% after merging ☔. Would it be feasible to also provide a simple unit test? Otherwise I could write one, but might take a while to find the free time required.

@Bouke
Copy link
Collaborator

Bouke commented May 14, 2015

Hi @ivorbosloper, thanks again for your contribution. Would it be feasible to also provide a unit test anywhere in the near future? Without a validation of the contribution I cannot merge this pull request.

@codecov
Copy link

codecov bot commented Nov 21, 2017

Codecov Report

Merging #33 into master will decrease coverage by 0.42%.
The diff coverage is 82.35%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #33      +/-   ##
==========================================
- Coverage   93.75%   93.32%   -0.43%     
==========================================
  Files          14       15       +1     
  Lines         608      659      +51     
  Branches       31       35       +4     
==========================================
+ Hits          570      615      +45     
- Misses         30       34       +4     
- Partials        8       10       +2
Impacted Files Coverage Δ
user_sessions/templatetags/user_sessions.py 95.08% <100%> (ø) ⬆️
...er_sessions/management/commands/migratesessions.py 81.25% <81.25%> (ø)
user_sessions/utils/tests.py 98.35% <0%> (+0.1%) ⬆️

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 2719255...6372c4b. Read the comment docs.

Copy link
Collaborator

@Bouke Bouke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice PR and great to see there's a test for it as well. If you could resolve the two comments, then I'd be happy to merge this!

@@ -98,7 +98,7 @@ def location(value):
"""
try:
location = geoip() and geoip().city(value)
except:
except Exception as e:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This variable isn't used, no need to assign it.

.gitignore Outdated
@@ -10,3 +10,5 @@

/docs/_build/
/GeoLite2-City.mmdb
.idea/
*.pyc
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for adding these ignores, I prefer to have these in my global gitignore.

Copy link
Contributor Author

@ivorbosloper ivorbosloper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid points, I've improved the pull-request...

@Bouke Bouke merged commit a695473 into jazzband:master Nov 25, 2017
@Bouke
Copy link
Collaborator

Bouke commented Nov 25, 2017

Thank you, I've merged the PR! 👍

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

Successfully merging this pull request may close these issues.

3 participants