jazzband / django-user-sessions

Extend Django sessions with a foreign key back to the user, allowing enumerating all user's sessions.
https://pypi.python.org/pypi/django-user-sessions
MIT License
628 stars 128 forks source link

Various fixes and improvements #166

Closed blag closed 1 year ago

blag commented 1 year ago

Fix and improve a many small things.

Description

I noticed that some tests weren't running because we didn't have a valid mmdb database from MaxMind. But, we would need a MaxMind account to download their mmdb files. Additionally, even if we did manage to download those files from MaxMind within tests, we would be testing against real world data that could change out from under us - this actually happened in some of the tests with an IP address that the tests expected to be in San Diego. I wrote a Perl script using MaxMind's mmdb writer Perl library to write minimized mmdb fixture files. I had to reverse engineer the Perl data structures so that the generated files would be properly readable by Python's geoip2 library, but the result is two very small binary mmdb files that only contain IP addresses we need for tests.

Motivation and Context

Silences various warnings during tests, improves GHA test time, modernizes Python syntax, ensures all tests are run in a repeatable manner without relying on third party non-static data, and adds some nice-to-haves.

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

codecov[bot] commented 1 year ago

Codecov Report

Merging #166 (840b263) into master (8ebdfe8) will increase coverage by 2.29%. The diff coverage is 90.74%.

@@            Coverage Diff             @@
##           master     #166      +/-   ##
==========================================
+ Coverage   92.62%   94.92%   +2.29%     
==========================================
  Files          16       16              
  Lines         746      768      +22     
  Branches       50       65      +15     
==========================================
+ Hits          691      729      +38     
+ Misses         45       29      -16     
  Partials       10       10              
Files Coverage Δ
tests/settings.py 100.00% <100.00%> (ø)
tests/tests.py 100.00% <100.00%> (+1.63%) :arrow_up:
tests/utils.py 97.05% <100.00%> (ø)
user_sessions/admin.py 100.00% <100.00%> (ø)
user_sessions/templatetags/user_sessions.py 90.27% <66.66%> (+18.61%) :arrow_up:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

WhyNotHugo commented 1 year ago

I think that the remaining changes/commits are fine :+1:

blag commented 1 year ago

@WhyNotHugo Thanks for your review! I reformatted the usage section a bit and it's looking much cleaner.

I like your other suggestions, but I think this PR has already caused enough churn. Instead, I have captured your ideas in issues #167 and #168. Both of those should be fairly straightforward to implement, so perhaps we can get a new contributor to submit PRs for those.

I will wait one week or until you give a final 👍 for this before merging it myself (unless I'm told otherwise).