django-silk icon indicating copy to clipboard operation
django-silk copied to clipboard

Fix when Session, Authentication or Message middlewares are not present

Open mgaligniana opened this issue 1 year ago • 6 comments

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!

mgaligniana avatar Jul 27 '23 21:07 mgaligniana

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.

codecov[bot] avatar Jul 28 '23 18:07 codecov[bot]

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.

yoonthegoon avatar Jul 28 '23 19:07 yoonthegoon

@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?

mgaligniana avatar Aug 14 '23 12:08 mgaligniana

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!

mgaligniana avatar Dec 13 '23 03:12 mgaligniana

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

MattyCZ avatar Jul 17 '24 09:07 MattyCZ

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!

mgaligniana avatar Jul 17 '24 11:07 mgaligniana

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

mgaligniana avatar Aug 13 '24 17:08 mgaligniana