bug 1431259: caching headers/tests for dashboards views #4676

escattone · 2018-02-22T20:54:02Z

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.

add shared_cache_control decorator as a thin wrapper around Django's cache_control decorator 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 views for kuma.dashboards.urls and add tests

* 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

escattone · 2018-02-22T23:23:34Z

I rebased with fixes for import ordering.

escattone · 2018-02-22T23:33:10Z

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 s-maxage instead of max-age.

jwhitlock

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

jwhitlock · 2018-02-22T22:51:13Z

kuma/core/tests/test_views.py

    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


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.

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.

jwhitlock · 2018-02-22T22:57:01Z

kuma/core/decorators.py

-# 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):


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 ✅

jwhitlock · 2018-02-22T22:57:23Z

kuma/core/tests/test_decorators.py

+                                  permission_required, shared_cache_control)

-from kuma.core.tests import KumaTestCase, eq_, ok_
+from kuma.core.tests import KumaTestCase, eq_


Another _ok gone! 🎉

jwhitlock · 2018-02-22T23:07:21Z

kuma/dashboards/views.py



+@shared_cache_control
+@vary_on_headers('X-Requested-With')


good catch on X-Requested-With, I would have missed that

jwhitlock · 2018-02-22T23:09:23Z

kuma/conftest.py



+@pytest.fixture
+def wiki_user_2(db, django_user_model):


Time to graduate to the globals! 🎓

jwhitlock · 2018-02-23T16:17:40Z

kuma/core/tests/test_decorators.py

+                                  shared_cache_control,
+                                  skip_in_maintenance_mode)

-from kuma.core.tests import KumaTestCase, eq_, ok_


Another ok_ gone! 🎉

jwhitlock · 2018-02-23T16:29:19Z

kuma/core/decorators.py

 from .urlresolvers import reverse


+def shared_cache_control(func=None, **kwargs):


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...

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.

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-io · 2018-02-23T17:38:11Z

Codecov Report

Merging #4676 into master will increase coverage by 0.01%.
The diff coverage is 98.54%.

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

Impacted Files	Coverage Δ
kuma/wiki/tests/conftest.py	`100% <ø> (ø)`	⬆️
kuma/users/views.py	`96.82% <100%> (+0.01%)`	⬆️
kuma/core/tests/test_views.py	`100% <100%> (ø)`	⬆️
kuma/settings/common.py	`92.6% <100%> (+0.02%)`	⬆️
kuma/wiki/views/create.py	`92.2% <100%> (+0.1%)`	⬆️
kuma/core/views.py	`95.45% <100%> (ø)`	⬆️
kuma/wiki/views/edit.py	`70.34% <100%> (+0.2%)`	⬆️
kuma/core/decorators.py	`93.61% <100%> (ø)`	⬆️
kuma/conftest.py	`100% <100%> (ø)`	⬆️
kuma/dashboards/urls.py	`100% <100%> (ø)`	⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a29dc0a...afc82b8. Read the comment docs.

escattone requested a review from jwhitlock February 22, 2018 20:54

escattone added 2 commits February 22, 2018 15:21

fix Cache-Control in test_ratelimit_429

05a0d6e

This was referenced Feb 22, 2018

bug 1431259: caching headers/tests for landing views #4679

Merged

bug 1431259: caching headers/tests for search/events views #4680

Merged

jwhitlock approved these changes Feb 23, 2018

View reviewed changes

remove unnecessary assert for cache-control in response

afc82b8

escattone merged commit 7839d19 into mdn:master Feb 23, 2018

escattone deleted the cache-control-headers-dashboards-1431259 branch February 23, 2018 17:54

escattone added the CDN for developer.mozilla.org label Mar 22, 2018

jwhitlock mentioned this pull request Apr 2, 2018

bug 957802: Enforce import order #4727

Merged

		from .urlresolvers import reverse


		def shared_cache_control(func=None, **kwargs):



		@shared_cache_control
		@vary_on_headers('X-Requested-With')

bug 1431259: caching headers/tests for dashboards views #4676

bug 1431259: caching headers/tests for dashboards views #4676

Uh oh!

Conversation

escattone commented Feb 22, 2018

Uh oh!

escattone commented Feb 22, 2018

Uh oh!

escattone commented Feb 22, 2018

Uh oh!

jwhitlock left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Feb 23, 2018

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants