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

Flush session on logout #2828

Merged
merged 22 commits into from
Mar 7, 2024
Merged

Conversation

stveit
Copy link
Contributor

@stveit stveit commented Feb 23, 2024

Instead of deleting stuff from session manually, use flush function instead. This is how Django does stuff internally.
Also puts the code into its own function so it isnt spread several places

Makes changes that are awkward to test in unit tests, so some tests have been ported to integration tests

related to #2804

Copy link

github-actions bot commented Feb 23, 2024

Test results

     12 files       12 suites   11m 39s ⏱️
3 320 tests 3 320 ✔️ 0 💤 0
9 435 runs  9 435 ✔️ 0 💤 0

Results for commit 45cea1f.

♻️ This comment has been updated with latest results.

@stveit
Copy link
Contributor Author

stveit commented Feb 27, 2024

This needs some changes to testing and additional tests, so ill let it wait until #2813 is done, as that adds some useful test additions

@stveit stveit added the blocked label Feb 27, 2024
@stveit stveit force-pushed the flush-session-on-logout branch 2 times, most recently from 83b5498 to 8b9a98c Compare March 1, 2024 09:39
@stveit stveit mentioned this pull request Mar 4, 2024
@stveit stveit removed the blocked label Mar 4, 2024
@stveit stveit self-assigned this Mar 4, 2024
@stveit stveit force-pushed the flush-session-on-logout branch from d5d2595 to 149b11b Compare March 4, 2024 13:59
Copy link

codecov bot commented Mar 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 57.18%. Comparing base (459cb29) to head (45cea1f).
Report is 30 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2828      +/-   ##
==========================================
+ Coverage   57.15%   57.18%   +0.02%     
==========================================
  Files         568      568              
  Lines       41282    41301      +19     
==========================================
+ Hits        23596    23617      +21     
+ Misses      17686    17684       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@stveit stveit marked this pull request as ready for review March 4, 2024 14:06
@stveit stveit force-pushed the flush-session-on-logout branch from 149b11b to 67679a4 Compare March 4, 2024 14:16
python/nav/web/auth/utils.py Show resolved Hide resolved
python/nav/web/auth/utils.py Show resolved Hide resolved
@hmpf hmpf linked an issue Mar 5, 2024 that may be closed by this pull request
@stveit stveit force-pushed the flush-session-on-logout branch from 2d5519e to ce85442 Compare March 5, 2024 13:13
@hmpf hmpf mentioned this pull request Mar 6, 2024
Copy link
Member

@lunkwill42 lunkwill42 left a comment

Choose a reason for hiding this comment

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

The functionality changes here seem fine (have not tested manually, yet), however, I have objections to the way the tests have been reorganized.

We try to adhere to a pattern where test modules are named after the Python modules they are testing. These modules do not exist in NAV, yet the new test modules seem to indicate they exist:

  • nav.web.auth.ensure_account
  • nav.web.auth.logout

If you have a need to group multiple tests for a single function or class (or group of co-operating functions), I find it's better to group those tests as methods in a class.

E.g.

class TestFoobar:
    def test_when_color_is_green_it_should_return_ok_status(self ...):
        ...

Something like this would tell me the function or functionality under test is Foobar, and the test names tell me something about which conditions are being tested and which behaviors are expected
.

Copy link

sonarcloud bot commented Mar 6, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@lunkwill42 lunkwill42 self-requested a review March 6, 2024 14:20
@stveit stveit merged commit 58b6db8 into Uninett:master Mar 7, 2024
11 checks passed
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.

Rework session ids
4 participants