django-silk
django-silk copied to clipboard
Fix when Session, Authentication or Message middlewares are not present
Hello!
In this PR I try fix the issue https://github.com/jazzband/django-silk/issues/347
Doesn't fail when one of these middlewares are not present:
'django.contrib.sessions.middleware.SessionMiddleware',
'django.contrib.auth.middleware.AuthenticationMiddleware',
'django.contrib.messages.middleware.MessageMiddleware'
and if SILKY_AUTHENTICATION
is True
, an exception is raised: SILKY_AUTHENTICATION can not be enabled without Session, Authentication or Message Django's middlewares
Let me know any change required!
Thank you!
Codecov Report
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 87.07%. Comparing base (
75cbbcf
) to head (8bb96b5
). Report is 39 commits behind head on master.
Additional details and impacted files
@@ Coverage Diff @@
## master #667 +/- ##
==========================================
+ Coverage 86.51% 87.07% +0.56%
==========================================
Files 52 52
Lines 2091 2113 +22
==========================================
+ Hits 1809 1840 +31
+ Misses 282 273 -9
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
profiling.py
, requests.py
, and summary.py
look good to me.
The tests look good at a glance.
Appears to resolve the issue on the ticket. Hopefully Seb or someone else comes in and checks it too.
@SebCorbin First, thank you for your quick responses and the pacience 🙏
Regarding the review:
Thanks for your work, it's a great beginning, but if I'm not mistaking, the filters are not working when selecting them. They should work if you fall back to
request.GET or request.POST
here
I followed the session
middleware variable approach and defined a new one for the silk's filters: silk_filters as a fallback.
With that + the custom filter manager we can get filters from session
or silk_filters
depending on if the first one is not defined.
What do you think?
Other question: would it be worth adding selenium tests?
Hi @albertyw! Sorry to bother you, just wondering if there is something I can do to push forward the pull request. As I mentioned in my last comment, perhaps add selenium tests could give more confident to the change? Thank you!
Hi!
Sorry to bother you, but any progress on the review? We would really appreciate this feature, since it would allow us to use silk without the middleware needed. Thank you very much
Hi @MattyCZ!
This pull request was marked as part of the last release but never merged. I think we should wait.
I'm available in case of changes required!
Hi @albertyw! I just want to let you know that this PR was marked as part of the release 5.1.0
but was never merged! https://github.com/jazzband/django-silk/releases/tag/5.1.0