Merge into devel : bug-475435 : Code : Launchpad itself

Status:

Merged

Approved by:

Данило Шеган on 2010-07-16

Approved revision:

no longer in the source branch.

Merged at revision:

11176

Proposed branch:

lp://qastaging/~adiroiban/launchpad/bug-475435

Merge into:

lp://qastaging/launchpad

Diff against target:

300 lines (+74/-58)

5 files modified

lib/canonical/launchpad/security.py (+15/-7)
lib/lp/translations/browser/potemplate.py (+6/-7)
lib/lp/translations/model/potemplate.py (+1/-1)
lib/lp/translations/stories/standalone/xx-series-templates.txt (+30/-22)
lib/lp/translations/templates/object-templates.pt (+22/-21)

To merge this branch:

bzr merge lp://qastaging/~adiroiban/launchpad/bug-475435

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Данило Шеган (community)		2010-07-14	Approve on 2010-07-15
Review via email: mp+29889@code.qastaging.launchpad.net

Commit message

Reduce the usage of URL formatters on +templates page in order to avoid timeouts.

Description of the change

= Bug 475435 =
+template page times out on pages with more than 100 templates

== Proposed fix ==
* reduce the amount of SQL queries
* reduce the usage of URL formatters
== Pre-implementation notes ==
I have discussed with Danilo at LP Epic and he agreed with using "hardcoded" link instead of URL formatters for admin links.

== Implementation details ==
Since the metal:fill-slot="head_epilogue" is going to be placed in <head> tag, I have changed it from <div> to a generic tal tag. DIVs are not allowed in <head>.

I have removed the language_count, since they are not really that important and are adding more queries. If this branch solve the timeout problem, we might consider adding the language count again.

To easy the review process I plan to fix the lint errors after the review is done, since those warnings were not introduces by the changes from this branch.

== Tests ==
./bin/test -t xx-series-templates.txt

== Demo and Q/A ==

Testing is a bit difficult since the problem appears only when there are many templates.
On lp.dev you should add more than 1000 templates to a distribution series and then access
https://translations.launchpad.dev/ubuntu/hoary/+templates

The page should load in less than 10 seconds.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/translations/browser/potemplate.py
  lib/lp/translations/stories/standalone/xx-series-templates.txt
  lib/lp/translations/templates/object-templates.pt

./lib/lp/translations/browser/potemplate.py
     833: W291 trailing whitespace
     833: Line has trailing whitespace.
./lib/lp/translations/stories/standalone/xx-series-templates.txt
      71: want exceeds 78 characters.
      73: want exceeds 78 characters.
      87: source exceeds 78 characters.
      89: want exceeds 78 characters.
      93: source exceeds 78 characters.
      95: want exceeds 78 characters.
     102: source exceeds 78 characters.
     104: want exceeds 78 characters.
     108: source exceeds 78 characters.
     110: want exceeds 78 characters.
     190: want exceeds 78 characters.
     191: want exceeds 78 characters.
     192: want exceeds 78 characters.

Revision history for this message

Данило Шеган (danilo) wrote on 2010-07-15:

#

Things we agreed on in live discussion (for reference):
* template_edit_permission should be determined in initialize() once, based on context, not on first template; name should be more appropriate as well ("can_edit_templates"?)
* we should have "can_admin_translations" as well for launchpad.TranslationsAdmin permission
* it would be nice if we can get languages back in the table; we'd have to insert a count() in the get*Templates call on HasTranslationTemplates so we get it all with one query.

review: Needs Fixing

Revision history for this message

Adi Roiban (adiroiban) wrote on 2010-07-15:

#

Download full text (5.3 KiB)

Hi,

Thanks for the review.

Here is the latest diff :)

=== modified file 'lib/canonical/launchpad/security.py'
--- lib/canonical/launchpad/security.py 2010-07-13 10:57:31 +0000
+++ lib/canonical/launchpad/security.py 2010-07-15 09:33:17 +0000
@@ -1192,15 +1192,12 @@
         template = self.obj
         if template.distroseries is not None:
             # Template is on a distribution.
- distribution = template.distroseries.distribution
- return (
- AdminDistributionTranslations(
- distribution).checkAuthenticated(user))
-
+ return AdminDistroSeriesTranslations(
+ template.distroseries).checkAuthenticated(user)
         else:
             # Template is on a product.
- return OnlyRosettaExpertsAndAdmins.checkAuthenticated(
- self, user)
+ return AdminProductSeriesTranslations(
+ template.productseries).checkAuthenticated(user)

class EditPOTemplateDetails(AdminPOTemplateDetails, EditByOwnersOrAdmins):
@@ -1769,6 +1766,16 @@
self.obj.distribution).checkAuthenticated(user))

+class AdminProductSeriesTranslations(AuthorizationBase):
+ permission = 'launchpad.TranslationsAdmin'
+ usedfor = IProductSeries
+
+ def checkAuthenticated(self, user):
+ """Is the user able to manage `IProductSeries` translations."""
+
+ return OnlyRosettaExpertsAndAdmins(self.obj).checkAuthenticated(user)
+
+
class BranchMergeProposalView(AuthorizationBase):
permission = 'launchpad.View'
usedfor = IBranchMergeProposal

=== modified file 'lib/lp/translations/browser/potemplate.py'
--- lib/lp/translations/browser/potemplate.py 2010-07-14 15:01:28 +0000
+++ lib/lp/translations/browser/potemplate.py 2010-07-15 08:58:01 +0000
@@ -806,7 +806,8 @@
     productseries = None
     label = "Translation templates"
     page_title = "All templates"
- template_edit_permission = None
+ can_edit = None
+ can_admin = None

     def initialize(self, series, is_distroseries=True):
         self.is_distroseries = is_distroseries
@@ -814,6 +815,10 @@
             self.distroseries = series
         else:
             self.productseries = series
+ self.can_admin = check_permission(
+ 'launchpad.TranslationsAdmin', series)
+ self.can_edit = (
+ self.can_admin or check_permission('launchpad.Edit', series))

     def iter_templates(self):
         potemplateset = getUtility(IPOTemplateSet)
@@ -827,18 +832,3 @@
             return "active-template"
         else:
             return "inactive-template"
-
- def isVisible(self, template):
- '''Returns True if the template should be display to users.
-
- Since all templates from a distroseries have the same permissions,
- security checking is cached and the condition is based on `iscurrent`
- attribute.
- '''
- if self.template_edit_permission is None:
- self.template_edit_permission = (
- check_permission('launchpad.Edit', template))
- if (template.iscurrent or self.template_edit_permission):
- return True
- else:
...

Hi,

Thanks for the review.

Here is the latest diff :)

=== modified file 'lib/canonical/launchpad/security.py'
--- lib/canonical/launchpad/security.py	2010-07-13 10:57:31 +0000
+++ lib/canonical/launchpad/security.py	2010-07-15 09:33:17 +0000
@@ -1192,15 +1192,12 @@
         template = self.obj
         if template.distroseries is not None:
             # Template is on a distribution.
-            distribution = template.distroseries.distribution
-            return (
-                AdminDistributionTranslations(
-                    distribution).checkAuthenticated(user))
-
+            return AdminDistroSeriesTranslations(
+                template.distroseries).checkAuthenticated(user)
         else:
             # Template is on a product.
-            return OnlyRosettaExpertsAndAdmins.checkAuthenticated(
-                self, user)
+            return AdminProductSeriesTranslations(
+                template.productseries).checkAuthenticated(user)
 
 
 class EditPOTemplateDetails(AdminPOTemplateDetails, EditByOwnersOrAdmins):
@@ -1769,6 +1766,16 @@
             self.obj.distribution).checkAuthenticated(user))
 
 
+class AdminProductSeriesTranslations(AuthorizationBase):
+    permission = 'launchpad.TranslationsAdmin'
+    usedfor = IProductSeries
+
+    def checkAuthenticated(self, user):
+        """Is the user able to manage `IProductSeries` translations."""
+
+        return OnlyRosettaExpertsAndAdmins(self.obj).checkAuthenticated(user)
+
+
 class BranchMergeProposalView(AuthorizationBase):
     permission = 'launchpad.View'
     usedfor = IBranchMergeProposal

=== modified file 'lib/lp/translations/browser/potemplate.py'
--- lib/lp/translations/browser/potemplate.py	2010-07-14 15:01:28 +0000
+++ lib/lp/translations/browser/potemplate.py	2010-07-15 08:58:01 +0000
@@ -806,7 +806,8 @@
     productseries = None
     label = "Translation templates"
     page_title = "All templates"
-    template_edit_permission = None
+    can_edit = None
+    can_admin = None
 
     def initialize(self, series, is_distroseries=True):
         self.is_distroseries = is_distroseries
@@ -814,6 +815,10 @@
             self.distroseries = series
         else:
             self.productseries = series
+        self.can_admin = check_permission(
+            'launchpad.TranslationsAdmin', series)
+        self.can_edit = (
+            self.can_admin or check_permission('launchpad.Edit', series))
 
     def iter_templates(self):
         potemplateset = getUtility(IPOTemplateSet)
@@ -827,18 +832,3 @@
             return "active-template"
         else:
             return "inactive-template"
-
-    def isVisible(self, template):
-        '''Returns True if the template should be display to users.
-        
-        Since all templates from a distroseries have the same permissions,
-        security checking is cached and the condition is based on `iscurrent`
-        attribute.
-        '''
-        if self.template_edit_permission is None:
-            self.template_edit_permission = (
-                check_permission('launchpad.Edit', template))
-        if (template.iscurrent or self.template_edit_permission):
-            return True
-        else:
-            return False

=== modified file 'lib/lp/translations/templates/object-templates.pt'
--- lib/lp/translations/templates/object-templates.pt	2010-07-14 15:01:28 +0000
+++ lib/lp/translations/templates/object-templates.pt	2010-07-15 10:57:09 +0000
@@ -90,7 +90,7 @@
             <tr tal:define="
                   inactive_css_class python:view.rowCSSClass(template);
                   template_url template/fmt:url"
-                tal:condition="python: view.isVisible(template)"
+                tal:condition="python: view.can_edit or template.iscurrent"
                 tal:attributes="
                   class string: template_row ${inactive_css_class}">
               <td class="priority_column"
@@ -128,14 +128,21 @@
               <td class="actions_column"
                   tal:condition="context/required:launchpad.AnyPerson">
               <div class="template_links">
-                <a class="sprite edit" tal:attributes="
-                    href string: ${template_url}/+edit">Edit</a>
-                <a class="sprite add" tal:attributes="
-                    href string: ${template_url}/+upload">Upload</a>
-                <a class="sprite download" tal:attributes="
-                    href string: ${template_url}/+export">Download</a>
-                <a class="sprite edit" tal:attributes="
-                    href string: ${template_url}/+admin">Administer</a>
+                <a class="sprite edit"
+                    tal:condition="view/can_edit"
+                    tal:attributes="
+                        href string: ${template_url}/+edit">Edit</a>
+                <a class="sprite add"
+                    tal:condition="view/can_edit"
+                    tal:attributes="
+                        href string: ${template_url}/+upload">Upload</a>
+                <a class="sprite download"
+                    tal:attributes="
+                        href string: ${template_url}/+export">Download</a>
+                <a class="sprite edit"
+                    tal:condition="view/can_admin"
+                    tal:attributes="
+                        href string: ${template_url}/+admin">Administer</a>
               </div>
               </td>
             </tr>

Revision history for this message

Данило Шеган (danilo) wrote on 2010-07-15:

#

Ok, so this is a big improvement. Thank you for working on it :)

review: Approve

Revision history for this message

Adi Roiban (adiroiban) wrote on 2010-07-15:

#

OK. I will fix the lint warnings and ask for an ec2-test.

Launchpad itself

Merge lp://qastaging/~adiroiban/launchpad/bug-475435 into lp://qastaging/launchpad

Commit message

Description of the change

Preview Diff

Subscribers