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

Fixed #32493 -- Removed redundant never_cache uses from admin views. #14066

Merged
merged 1 commit into from Mar 3, 2021

Conversation

tim-mccurrach
Copy link
Contributor

@tim-mccurrach tim-mccurrach commented Mar 1, 2021

Decorating logout and index with never_cache is not needed, since admin_view already applies never_cache.

https://code.djangoproject.com/ticket/32493#ticket

Copy link
Member

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

OK, this seems reasonable to me.

  • Coverage is provided by admin_views.tests.NeverCacheTests
  • logout inherits never_cache from django.contrib.auth.views.LogoutView

I don't think it's supported usage to directly access the index or logout views, but I added a small release note for the behaviour change.

@tim-mccurrach
Copy link
Contributor Author

@carltongibson Thanks for the review :)

I didn't think about accessing these methods directly, but agree with everything you've said.

Shouldn't the release note only mention index, not logout, since (as you mention) logout is covered by LogoutView?

@carltongibson
Copy link
Member

Hey @tim-mccurrach — yes, that's a good point. I've adjusted the release note accordingly, squashed, rebased and used the project commit message format.

@felixxm Can I leave this here for you to glance at, in case you have a concern. (As we discussed I don't think there's an issue — if you're overriding, say, admin_view and not applying never_cache then you're on your own I'd argue...) Thanks!

@tim-mccurrach
Copy link
Contributor Author

It's just occurred to me that login doesn't need never_cache either since that will be applied by auth.views.LoginView. I'll remove that one as well.

@carltongibson
Copy link
Member

@tim-mccurrach I'd leave that one.

That never_cache is applied is obvious from the use of the admin_view decorator. login is one case we don't apply the wrap, so that it's decorated is clear. If we're relying on the decoration of the superclass it becomes mysterious.

@tim-mccurrach
Copy link
Contributor Author

@tim-mccurrach I'd leave that one.

That never_cache is applied is obvious from the use of the admin_view decorator. login is one case we don't apply the wrap, so that it's decorated is clear. If we're relying on the decoration of the superclass it becomes mysterious.

Yes, that's a good point. Thank you

Copy link
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

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

Thanks both 👍

docs/releases/4.0.txt Outdated Show resolved Hide resolved
@felixxm felixxm changed the title Fixed #32493 - Remove unnecessary uses of never_cache from admin views Fixed #32493 -- Removed redundant never_cache uses from admin views. Mar 2, 2021
Co-authored-by: Carlton Gibson <carlton.gibson@noumenal.es>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants