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

🎨 #600 - changed the error on 403 without session #612

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Xaohs
Copy link
Contributor

@Xaohs Xaohs commented Jan 15, 2025

closes #600

  • Added a backend middleware to return a code on the session expiry edge case
  • Added two E2E tests
    • One tests for the issue itself
    • Other one tests our authentication implementation directly (and changed the CSRF test to not hit this edge case anymore)
  • Updated frontend to add a link to the modal when the session_expired code is provided as a response error

@Xaohs Xaohs requested a review from svenvandescheur January 15, 2025 14:48
@Xaohs Xaohs force-pushed the issue/#600-error-message branch from 646195d to d1f5aa6 Compare January 16, 2025 15:11
@Xaohs Xaohs force-pushed the issue/#600-error-message branch from 6240499 to 1732718 Compare January 16, 2025 17:14
@Xaohs Xaohs marked this pull request as ready for review January 16, 2025 17:14
@Xaohs
Copy link
Contributor Author

Xaohs commented Jan 16, 2025

@svenvandescheur if needed, we can add Silvia as a reviewer aswell

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 89.28571% with 3 lines in your changes missing coverage. Please review.

Project coverage is 86.81%. Comparing base (90849eb) to head (1732718).

Files with missing lines Patch % Lines
frontend/src/hooks/useAlertOnError.ts 72.72% 3 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #612   +/-   ##
=======================================
  Coverage   86.81%   86.81%           
=======================================
  Files         205      205           
  Lines        5968     5991   +23     
  Branches      619      621    +2     
=======================================
+ Hits         5181     5201   +20     
- Misses        787      790    +3     

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

Copy link
Contributor

@svenvandescheur svenvandescheur left a comment

Choose a reason for hiding this comment

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

Some small remarks, looks good otherwise.



@tag("e2e")
@override_settings(SESSION_COOKIE_AGE=3)
Copy link
Contributor

Choose a reason for hiding this comment

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

We can make this 0/1 no?


const flatten = Object.values(errors || {})
.flat()
.filter((error) => error !== "session_expired");
Copy link
Contributor

Choose a reason for hiding this comment

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

This only works for the "session_expired" code, I think we should just filter out any record with the key "code", also please add some documentation on why this is and update the test file with a testcase for it :)

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.

Onduidelijke foutmelding wanneer je een poos de pagina niet gebruikt
3 participants