Merge lp:~sinzui/launchpad/timeout-portlet-package-summary-2 into lp:launchpad

[Curtis Hovey]

[Curtis Hovey (sinzui)]

This is my branch to guarantee that the distroseries page does not
timeout.

    lp:~sinzui/launchpad/timeout-portlet-package-summary-2
    Diff size: 88
    Launchpad bug: https://bugs.launchpad.net/bugs/526583
    Test command: ./bin/test -vv -t xx-distroseries-index
    Pre-implementation: no one
    Target release: 10.02

Guarantee that the distroseries page does not timeout
-----------------------------------------------------

+portlet-package-summary still causes timeouts after SQL tuning. Take
drastic measures like removing content to avoid making SQL calls that
are to expensive.

Rules
-----

    * Remove the list of packages that most need linking.
    * Factor out calls to getPrioritizedUnlinkedSourcePackages() that
      are used by view.num_unlinked_packages. The number of packages
that
      need linking can be derived from the total source packages minus
      the number of linked packages.
      * /me thinks the number of packages that need linking as
calculated
        now is wrong. It is too high and anyone looking at the numbers
in
        the page can work that out. 16829 - 1498 = 15331, not 15414. I
        suspect the difference is unpublished packages in the series.

QA
--

    * Visit https://edge.launchpad.net/ubuntu/lucid
    * Reload the page 10 times and do not see a timeout.

Lint
----

Linting changed files:
  lib/lp/registry/browser/distroseries.py
  lib/lp/registry/stories/distroseries/xx-distroseries-index.txt
  lib/lp/registry/templates/distroseries-portlet-packaging.pt

Test
----

    * lib/lp/registry/stories/distroseries/xx-distroseries-index.txt
      * Updated the test to show that packages that most need linking
are
        not listed.
      * Update the test to show the insane -2 packages need linking.
This
        is caused by stale distroseries.sourcecounts in sampledata. I
        decided to not force the number to be sane so that anyone
looking
        at the page in dev will see a test explaining why the number is
        wrong.

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

    * lib/lp/registry/browser/distroseries.py
      * Updated num_unlinked_packages to use the fast and logical way to
        learn the number: total source packages - number of packagings.
      * Inlined _unlinked_packages to the only method that needs to use
        the expensive query. The template does not call this method. We
will
        work on getPrioritizedUnlinkedSourcePackages after this release.
    * lib/lp/registry/templates/distroseries-portlet-packaging.pt
      * Removed the needs linking section to avoid calling
        getPrioritizedUnlinkedSourcePackages.

[Brad Crittenden (bac)]

The changes look good for this pass. I hope we intend to solve the query problem and restore the "needs linking" section. Should you open a bug and put in an XXX comment to that effect? It might be good to comment out the existing TAL with an XXX rather than deleting it outright. Your call as to the details.

review: Approve (code)

=== modified file 'lib/lp/registry/browser/distroseries.py'
--- lib/lp/registry/browser/distroseries.py	2010-02-20 23:35:12 +0000
+++ lib/lp/registry/browser/distroseries.py	2010-02-26 19:30:38 +0000
@@ -337,25 +337,22 @@
         """The number of linked packagings for this distroseries."""
         return len(self.context.packagings)
 
-    @cachedproperty
-    def _unlinked_packages(self):
-        """Get a prioritized list of unlinked source packages."""
-        return self.context.getPrioritizedUnlinkedSourcePackages()
-
     @property
     def num_unlinked_packages(self):
         """The number of unlinked packagings for this distroseries."""
-        return len(self._unlinked_packages)
+        return self.context.sourcecount - self.num_linked_packages
 
     @cachedproperty
     def recently_linked(self):
         """Return the packages that were most recently linked upstream."""
         return self.context.getMostRecentlyLinkedPackagings()
 
-    @property
+    @cachedproperty
     def needs_linking(self):
-        """Return a list of 10 packages most in need of upstream linking."""
-        return self._unlinked_packages[:10]
+        """Return a list of 10 packages most in need of upstream linking.""" 
+        # XXX sinzui 2010-02-26 bug=528648: This method causes a timeout.
+        # return self.context.getPrioritizedUnlinkedSourcePackages()[:10]
+        return None
 
     milestone_can_release = False
 

=== modified file 'lib/lp/registry/stories/distroseries/xx-distroseries-index.txt'
--- lib/lp/registry/stories/distroseries/xx-distroseries-index.txt	2010-02-19 20:19:28 +0000
+++ lib/lp/registry/stories/distroseries/xx-distroseries-index.txt	2010-02-26 19:30:38 +0000
@@ -86,18 +86,18 @@
 The distroseries page contains a portlet with information on the
 upstream packaging.
 
+    >>> # Note that warty's sourcecount is stale in sample data
+    >>> # which causes -2 need linking.
     >>> anon_browser.open('http://launchpad.dev/ubuntu/warty')
     >>> print extract_text(
-    ...     find_portlet(anon_browser.contents, 'Upstream packaging'))
+    ...     find_tag_by_id(anon_browser.contents, 'series-packaging'))
     Upstream packaging
     5 source packages are linked to registered upstream projects.
-    3 need linking.
+    -2 need linking.
     Recently linked to upstream:
     alsa-utils linked...
     a52dec linked...
     evolution linked...
     mozilla-firefox linked...
-    netapplet linked...
-    Needs more information or linking to upstream:
-    linux-source-2.6.15 cdrkit iceweasel
-    Needs upstream links All upstream links
+    netapplet linked 2005-07-05
+    Needs upstream links  All upstream links

=== modified file 'lib/lp/registry/templates/distroseries-portlet-packaging.pt'
--- lib/lp/registry/templates/distroseries-portlet-packaging.pt	2010-02-19 20:19:28 +0000
+++ lib/lp/registry/templates/distroseries-portlet-packaging.pt	2010-02-26 19:30:38 +0000
@@ -53,15 +53,13 @@
         tal:condition="view/needs_linking">
       Needs more information or linking to upstream:
     </dt>
-    <dd>
+    <dd tal:condition="view/needs_linking">
       <ul class="horizontal">
-        <tal:package repeat="package view/needs_linking">
-          <li>
+          <li repeat="package view/needs_linking">
             <a class="sprite package-source"
                tal:attributes="href package/package/fmt:url"
                tal:content="package/package/name">evolution</a>
           </li>
-        </tal:package>
       </ul>
     </dd>
   </dl>