bug 1431259: caching headers/tests for dashboards views#4676
bug 1431259: caching headers/tests for dashboards views#4676escattone merged 3 commits intomdn:masterfrom escattone:cache-control-headers-dashboards-1431259
Conversation
* add "shared_cache_control" decorator as a thin wrapper around Django's "cache_control" to encapsulate default settings for shared caching * add tests for "shared_cache_control" decorator * delete custom "never_cache" decorator, and replace with Django's * delete tests for custom "never_cache" decorator * decorate the dashboards views and add tests
|
I rebased with fixes for import ordering. |
|
I forgot to add in the introduction, that this PR only affects public caching (e.g., CDN's) not browser caching since by default it uses |
jwhitlock
left a comment
jwhitlock
left a comment
There was a problem hiding this comment.
Thanks @escattone, this looks good and works well in my development environment.
I identified a nit on import order, that we discussed in IRC. The plan is to get these changes merged, then use a flake8 plugin to standardize and enforce import order (we both seem to like the cryptography style).
There's a few more style nits, but I think this is ready to go
kuma/core/tests/test_views.py
Outdated
| assert '429.html' in [t.name for t in response.templates] | ||
| assert response['Retry-After'] == '60' | ||
| assert response['Cache-Control'] == 'no-cache, no-store, must-revalidate' | ||
| assert 'Cache-Control' in response |
There was a problem hiding this comment.
Nit: if Cache-Control isn't in the response, then the next assertion will fail with a KeyError, so it isn't required to test separately, and a line can be removed.
There was a problem hiding this comment.
Yeah, I did this everywhere thinking it would make for a nicer error message than KeyError, but I'm happy to remove it as well.
| # https://developer.mozilla.org/en-US/docs/Web/HTTP/Caching_FAQ | ||
| # Fixed in Django 1.9: | ||
| # https://docs.djangoproject.com/en/1.9/topics/http/decorators/#caching | ||
| def never_cache(view_func): |
There was a problem hiding this comment.
The comment is out-of-date, the @never_cache header was updated in Django 1.8.8, so we have Django's fixed @never_cache. Glad to take this off my to-do list ✅
kuma/core/tests/test_decorators.py
Outdated
| permission_required, shared_cache_control) | ||
|
|
||
| from kuma.core.tests import KumaTestCase, eq_, ok_ | ||
| from kuma.core.tests import KumaTestCase, eq_ |
|
|
||
|
|
||
| @shared_cache_control | ||
| @vary_on_headers('X-Requested-With') |
There was a problem hiding this comment.
good catch on X-Requested-With, I would have missed that
|
|
||
|
|
||
| @pytest.fixture | ||
| def wiki_user_2(db, django_user_model): |
There was a problem hiding this comment.
Time to graduate to the globals! 🎓
| shared_cache_control, | ||
| skip_in_maintenance_mode) | ||
|
|
||
| from kuma.core.tests import KumaTestCase, eq_, ok_ |
| from .urlresolvers import reverse | ||
|
|
||
|
|
||
| def shared_cache_control(func=None, **kwargs): |
There was a problem hiding this comment.
Nit: The name "shared cache" confused me a bit. Is the intent is to save a copy in the proxy, but to tell the browser to not cache the results? If so, a better name doesn't come to mind yet...
There was a problem hiding this comment.
Yes, that was the intent. I'm happy to use whatever name you'd prefer, as I had a difficult time coming up with something better.
There was a problem hiding this comment.
proxy_cache_yes_browser_cache_no_with_overrides is my current best choice, which is so awful. Let's merge with this name and revisit in a few weeks when we're smarter.
Codecov Report
@@ Coverage Diff @@
## master #4676 +/- ##
==========================================
+ Coverage 95.27% 95.29% +0.01%
==========================================
Files 261 261
Lines 23560 23632 +72
Branches 1691 1698 +7
==========================================
+ Hits 22447 22519 +72
Misses 902 902
Partials 211 211
Continue to review full report at Codecov.
|
3 participants
This is the first of a series of PR's that add the appropriate caching headers and related tests to all Kuma endpoints as part of the effort of placing a CDN in front of MDN. This PR adds caching headers and tests for the
kuma.dashboards.urls.shared_cache_controldecorator as a thin wrapper around Django'scache_controldecorator to encapsulate default settings for shared cachingshared_cache_controldecoratornever_cachedecorator, and replace with Django'snever_cachedecoratorkuma.dashboards.urlsand add tests