launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/registry-cache-4 into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)


This is my branch to remove caching from milestone pages.

    lp:~sinzui/launchpad/registry-cache-3
    Diff size: 93
    Launchpad bug:
          https://bugs.launchpad.net/bugs/id
    Test command: ./bin/test -vv -t test_milestone
    Pre-implementation: bjorn, jelmer, Danilos
    Target release: 10.08


Remove caching from milestone pages
-----------------------------------

On Fri, 2010-08-06 at 10:07 +0300, Bjorn Tillenius wrote:
> I'd be interested in knowing this as well. I've always wondered
> whether
> the caching on the milestone list actually helps, and how it helps.
> Considering it's cache:private, it means that each user has its own
> cache, right? So it doesn't help when multiple people look at the page
> the first time. It only helps when the same person reloads the page
> multiple times. Now, which use cases exist for that, except for
> checking
> if something changed, usually after having modified bugs himself, or
> someone on IRC told him that they changed bugs? For the use case when
> the user knows something has changed, he will try to reload until he
> actually sees the changes, and is a really bad user experience. 

This is a well thought argument. I think anonymous users are the only
ones who benefit from the cache. The page spends most of its time in
python formatting objects and anonymous users/bots do benefit from a
fast loading page.


Rules
-----

Change the cache rules in the milestone template to anonymous. Only anonymous
users see a cached page.


QA
--

Using two browsers, one logged in, the other not.

    * View a milestone in both browsers.
    * Visit a bug listed in the page in the logged browser
    * Change the status or assignee of the bug in the logged browser
    * Visit the milestone gain in the logged browser
    * Verify you see the change.
    * Reload the anonymous browser
    * Verify it has not changed, it does not show the bug has changed.


Lint
----

Linting changed files:
  lib/lp/registry/browser/tests/test_milestone.py
  lib/lp/registry/templates/milestone-index.pt


Test
----

    * lib/lp/registry/browser/tests/test_milestone.py
      * Renamed an existing test to show it tests the anonymous condition
      * Added a new test for the logged in condition


Implementation
--------------

    * lib/lp/registry/templates/milestone-index.pt
      * Changed the cache instructions to anonymous.
-- 
https://code.launchpad.net/~sinzui/launchpad/registry-cache-4/+merge/31998
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/registry-cache-4 into lp:launchpad/devel.

=== modified file 'lib/lp/registry/browser/tests/test_milestone.py'
--- lib/lp/registry/browser/tests/test_milestone.py	2010-06-30 19:37:09 +0000
+++ lib/lp/registry/browser/tests/test_milestone.py	2010-08-06 20:06:08 +0000
@@ -7,7 +7,7 @@
 
 import unittest
 
-from lp.testing import login_person
+from lp.testing import ANONYMOUS, login_person, login
 from lp.testing.views import create_initialized_view
 from lp.testing.memcache import MemcacheTestCase
 
@@ -26,25 +26,43 @@
         bugtask.milestone = self.milestone
         self.observer = self.factory.makePerson()
 
-    def test_milestone_index_memcache(self):
+    def test_milestone_index_memcache_anonymous(self):
         # Miss the cache on first render.
+        login(ANONYMOUS)
         view = create_initialized_view(
-            self.milestone, name='+index', principal=self.observer)
+            self.milestone, name='+index', principal=None)
         content = view.render()
         self.assertCacheMiss('<dt>Assigned to you:</dt>', content)
         self.assertCacheMiss('id="milestone_bugtasks"', content)
         # Hit the cache on the second render.
         view = create_initialized_view(
-            self.milestone, name='+index', principal=self.observer)
+            self.milestone, name='+index', principal=None)
         self.assertTrue(view.milestone.active)
         self.assertEqual(10, view.expire_cache_minutes)
         content = view.render()
         self.assertCacheHit(
             '<dt>Assigned to you:</dt>',
-            'private, view/expire_cache_minutes minute', content)
+            'anonymous, view/expire_cache_minutes minute', content)
         self.assertCacheHit(
             'id="milestone_bugtasks"',
-            'private, view/expire_cache_minutes minute', content)
+            'anonymous, view/expire_cache_minutes minute', content)
+
+    def test_milestone_index_memcache_no_cache_logged_in(self):
+        login_person(self.observer)
+        # Miss the cache on first render.
+        view = create_initialized_view(
+            self.milestone, name='+index', principal=self.observer)
+        content = view.render()
+        self.assertCacheMiss('<dt>Assigned to you:</dt>', content)
+        self.assertCacheMiss('id="milestone_bugtasks"', content)
+        # Hit the cache on the second render.
+        view = create_initialized_view(
+            self.milestone, name='+index', principal=self.observer)
+        self.assertTrue(view.milestone.active)
+        self.assertEqual(10, view.expire_cache_minutes)
+        content = view.render()
+        self.assertCacheMiss('<dt>Assigned to you:</dt>', content)
+        self.assertCacheMiss('id="milestone_bugtasks"', content)
 
     def test_milestone_index_active_cache_time(self):
         # Verify the active milestone cache time.

=== modified file 'lib/lp/registry/templates/milestone-index.pt'
--- lib/lp/registry/templates/milestone-index.pt	2010-06-29 14:52:51 +0000
+++ lib/lp/registry/templates/milestone-index.pt	2010-08-06 20:06:08 +0000
@@ -129,7 +129,7 @@
 
         <div class="yui-u">
           <div id="milestone-activities" class="portlet"
-            tal:content="cache:private, view/expire_cache_minutes minute">
+            tal:content="cache:anonymous, view/expire_cache_minutes minute">
             <h2>Activities</h2>
 
             <dl>
@@ -208,7 +208,7 @@
         </h2>
 
         <tal:has_specs condition="specs"
-          tal:content="cache:public, view/expire_cache_minutes minute">
+          tal:content="cache:anonymous, view/expire_cache_minutes minute">
           <table class="listing sortable" id="milestone_specs"
             style="margin-bottom: 2em;">
             <thead>
@@ -266,7 +266,7 @@
         </tal:has_specs>
 
         <tal:has_bugtasks condition="bugtasks">
-          <tal:cache content="cache:private, view/expire_cache_minutes minute">
+          <tal:cache content="cache:anonymous, view/expire_cache_minutes minute">
             <tal:milestone-bugtasks
               metal:use-macro="context/@@+milestone-macros/milestone_bugtasks" />
           </tal:cache>