-
Notifications
You must be signed in to change notification settings - Fork 679
bug 1431259: do not cache docs if logged-in user or ks errors #4734
bug 1431259: do not cache docs if logged-in user or ks errors #4734
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to work locally, and will take care of issues around rendering documents.
I think the tests are OK, although I'm getting dumb to the repeated testing code.
One nit around no-cache for the redirect around a fallback doc.
assert 'max-age=0' in response['Cache-Control'] | ||
assert 'no-cache' in response['Cache-Control'] | ||
assert 'no-store' in response['Cache-Control'] | ||
assert 'must-revalidate' in response['Cache-Control'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm reconsidering if we should have a function like assert_no_cache_header
. This repetition seems harmful to understanding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're right. I think it's worth introducing both assert_no_cache_header
and assert_cache_header
and submitting in a new PR.
kuma/wiki/views/document.py
Outdated
return redirect(redirect_url) | ||
response = redirect(redirect_url) | ||
if request.user.is_authenticated(): | ||
add_never_cache_headers(response) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the fallback_doc
vary for logged-in users? If not, I don't see why we shouldn't cache the redirect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My original intention was simply to be thorough in not caching when the user is logged-in, but you're right, I don't think this can vary for logged-in users. I will remove it.
Thanks for the review @jwhitlock! I removed the code that added the never cache header to the default-locale fallback response, and then re-based and force pushed. |
👍 I'm OK to merge, if you want to save the test utility functions for a new PR. |
This PR adjusts the
Cache-Control
header for documents. We will no longer cache if the user is logged-in or there are Kumascript errors. This should help with issues due to caching when a user deletes, restores, or re-renders a document.