Merge lp:~adiroiban/launchpad/bug-340662-take-2 into lp:launchpad/db-devel

[Adi Roiban]

Remove "name" field from POTemplate edit view. On POTemplateSubset url traversal, check permission on POFile. Remove EditPOTemplateSubset from security.py

[Adi Roiban (adiroiban)]

= Bug #340662 and #507498=
This branch should fix the issues raised in this MP
https://code.edge.launchpad.net/~adiroiban/launchpad/bug-340662/+merge/16629

== Proposed fix ==
Remove "name" field from PoTemplate edit page.

In the PoTemplateSubset URL traversal code, check permissions for the POTemplate and not for the context.

== Pre-implementation notes ==
Danilo asked to remove the "name" field from the edit page since it is to fragile to be edited by project owners.

Henning suggest that permission checking for POTemplateSubset URL traversal can be done for POTemplate and in this way we can remove the EditPOTemplateSubSet from security.py

== Implementation details ==
Nothing special here.

== Tests ==
lp-test -t templates

== Demo and Q/A ==
As a product owner (ex. <email address hidden>) for a project (ex. evolution) go to a template page for that project
https://translations.launchpad.dev/evolution/trunk/+pots/evolution-2.2/

You should see the „Change details” that links to the +edit page

From this page you should be able to change template name, domain name, path, details, priority, owner and „accept translation”.

Change some values and then save them.

From the series +template page you will see „Edit” links for each template, including disabled templates:
https://translations.launchpad.dev/evolution/trunk/+templates

Disabling a template, it should still be in the list (with a red background) and you can still edit it.

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/canonical/launchpad/security.py
  lib/lp/translations/configure.zcml
  lib/lp/translations/browser/potemplate.py
  lib/lp/translations/stories/standalone/xx-potemplate-edit.txt

[Brad Crittenden (bac)]

Hi Adi,

Thanks for this branch.

As noted in IRC, there is an import violation that is clearly not your fault but needs to be fixed before landing this branch. Thanks for agreeing to look into it.

Also, when attempting the demo you describe in the MP I get an unauthorized error changing any field b/c date_last_updated requires launchpad.TranslationsAdmin. We need to beef up the tests to ensure the permissions are set as we need.

review: Needs Fixing (code)

[Adi Roiban (adiroiban)]

Hi,

Thanks for the review.

The permission problem should be fixed now and test extended to check all fields.

Not sure if there will be branch for fixing the import problem. I will leave the import problem as a final commit.

[Brad Crittenden (bac)]

Hi Adi,

Thanks again for your changes. Your use of removeSecurityProxy was a bit flawed, though, so date_last_updated was not being changed. This flaw also exposes a hole in the testing as there is not test to ensure the value actually got changed.

If you apply the following change it will work:

=== modified file 'lib/lp/translations/browser/potemplate.py'
--- lib/lp/translations/browser/potemplate.py 2010-01-19 16:48:51 +0000
+++ lib/lp/translations/browser/potemplate.py 2010-01-20 14:45:58 +0000
@@ -533,9 +533,8 @@
             # We only update date_last_updated when translation_domain field
             # is changed because is the only significative change that,
             # somehow, affects the content of the potemplate.
- UTC = pytz.timezone('UTC')
- date_last_updated = removeSecurityProxy(context.date_last_updated)
- date_last_updated = datetime.datetime.now(UTC)
+ naked_context = removeSecurityProxy(context)
+ naked_context.date_last_updated = datetime.datetime.now(pytz.UTC)

Also, while you're there could you fix the preceding comment so that it reads:

We only change date_last_updated when the translation_domain field is changed because it is the only relevant field we care about regarding the date of last update.

In your test it would be great to grab the value of date_last_updated before and after the change and show that new > old. It's kind of messy in a story test but should be ok.

Please post a diff when you're done and I'll have a final look at it.

review: Needs Fixing (code)

[Adi Roiban (adiroiban)]

Hi,
Here is the diff. Hope everything is OK now.

=== modified file 'lib/lp/translations/browser/potemplate.py'
--- lib/lp/translations/browser/potemplate.py 2010-01-19 16:48:51 +0000
+++ lib/lp/translations/browser/potemplate.py 2010-01-20 18:12:49 +0000
@@ -530,12 +530,12 @@
                 product=context.product, distribution=context.distribution,
                 sourcepackagename=context.sourcepackagename)
         if old_translation_domain != context.translation_domain:
- # We only update date_last_updated when translation_domain field
- # is changed because is the only significative change that,
- # somehow, affects the content of the potemplate.
+ # We only change date_last_updated when the translation_domain
+ # field is changed because it is the only relevant field we
+ # care about regarding the date of last update.
             UTC = pytz.timezone('UTC')
- date_last_updated = removeSecurityProxy(context.date_last_updated)
- date_last_updated = datetime.datetime.now(UTC)
+ naked_context = removeSecurityProxy(context)
+ naked_context.date_last_updated = datetime.datetime.now(UTC)

     @property
     def cancel_url(self):

=== modified file 'lib/lp/translations/stories/standalone/xx-potemplate-edit.txt'
--- lib/lp/translations/stories/standalone/xx-potemplate-edit.txt 2010-01-19 16:48:51 +0000
+++ lib/lp/translations/stories/standalone/xx-potemplate-edit.txt 2010-01-20 19:10:54 +0000
@@ -90,6 +90,22 @@
   >>> browser.getControl(name='field.translation_domain').value
   'evolution-2.2'

+We remember the 'last_update_date' in order to check if it was changed
+after updating the template.
+
+ >>> from zope.component import getUtility
+ >>> from canonical.launchpad.interfaces import ILaunchpadCelebrities
+ >>> from lp.translations.model.potemplate import POTemplateSubset
+ >>> from lp.registry.interfaces.product import IProductSet
+ >>> login('<email address hidden>')
+ >>> evolution = getUtility(IProductSet).getByName('evolution')
+ >>> evolution_trunk = evolution.getSeries('trunk')
+ >>> hoary_subset = POTemplateSubset(productseries=evolution_trunk)
+ >>> evolution_template = hoary_subset.getPOTemplateByName(
+ ... 'evolution-2.2')
+ >>> previous_date_last_updated = evolution_template.date_last_updated
+ >>> logout()
+
 The visible fields can be changed and saved.

   >>> browser.getControl(name='field.translation_domain').value = u'evo'
@@ -120,3 +136,6 @@
   'name12'
   >>> browser.getControl(name='field.description').value
   'foo'
+ >>> previous_date_last_updated != evolution_template.date_last_updated
+ True
+

[Brad Crittenden (bac)]

Adi it looks very good now. Thanks for the fixes.

One tiny thing: remove the UTC = line and just use 'pytz.UTC' instead, as shown in my previous comment. pytz is nice enough to export a pre-defined UTC constant so we should use it rather than looking it up again.

review: Approve (code)

[Adi Roiban (adiroiban)]

Here is the diff after running the syntactic check for modified files.

=== modified file 'lib/canonical/launchpad/security.py'
--- lib/canonical/launchpad/security.py 2010-01-18 16:54:07 +0000
+++ lib/canonical/launchpad/security.py 2010-01-20 20:29:00 +0000
@@ -469,7 +469,7 @@
         return (user.inTeam(self.obj.person) or
                 user.isOneOf(
                     self.obj.specification,
- ['owner','drafter', 'assignee', 'approver']) or
+ ['owner', 'drafter', 'assignee', 'approver']) or
                 user.in_admin)

@@ -530,6 +530,7 @@
             return True
         return user.isOwner(self.obj.target)

+
 class AdminMilestoneByLaunchpadAdmins(AuthorizationBase):
     permission = 'launchpad.Admin'
     usedfor = IMilestone
@@ -574,7 +575,7 @@

     def checkAuthenticated(self, user):
         """Is the user a privileged team member or Launchpad staff?
-
+
         Return true when the user is a member of Launchpad admins,
         registry experts, team admins, or the team owners.
         """
@@ -1431,7 +1432,6 @@

     # This code MUST match the logic in IBuildSet.getBuildsForBuilder()
     # otherwise users are likely to get 403 errors, or worse.
-
     def checkAuthenticated(self, user):
         """Private restricts to admins and archive members."""
         if not self.obj.archive.private:

=== modified file 'lib/lp/translations/browser/configure.zcml'
--- lib/lp/translations/browser/configure.zcml 2009-12-12 05:47:41 +0000
+++ lib/lp/translations/browser/configure.zcml 2010-01-20 20:31:52 +0000
@@ -391,9 +391,9 @@
             <browser:page
                 name="+chart"
                 template="../templates/potemplate-chart.pt"/>
-
+
             <!-- Potemplate Portlets -->
-
+
             <browser:page
                 name="+portlet-details"
                 template="../templates/potemplate-portlet-details.pt"/>

=== modified file 'lib/lp/translations/browser/potemplate.py'
--- lib/lp/translations/browser/potemplate.py 2010-01-20 20:16:29 +0000
+++ lib/lp/translations/browser/potemplate.py 2010-01-20 20:33:54 +0000
@@ -533,9 +533,8 @@
             # We only change date_last_updated when the translation_domain
             # field is changed because it is the only relevant field we
             # care about regarding the date of last update.
- UTC = pytz.timezone('UTC')
             naked_context = removeSecurityProxy(context)
- naked_context.date_last_updated = datetime.datetime.now(UTC)
+ naked_context.date_last_updated = datetime.datetime.now(pytz.UTC)

     @property
     def cancel_url(self):

=== modified file 'lib/lp/translations/configure.zcml'
--- lib/lp/translations/configure.zcml 2010-01-17 02:07:16 +0000
+++ lib/lp/translations/configure.zcml 2010-01-20 20:32:41 +0000
@@ -415,7 +415,7 @@
             <require
                 permission="launchpad.TranslationsAdmin"
                 set_attributes="
- name productseries distroseries sourcepackagename
+ name productseries distroseries sourcepackagename
                     sourcepackageversion binaryp...

=== modified file 'lib/canonical/launchpad/security.py'
--- lib/canonical/launchpad/security.py	2010-01-14 11:36:51 +0000
+++ lib/canonical/launchpad/security.py	2010-01-20 20:38:18 +0000
@@ -469,7 +469,7 @@
         return (user.inTeam(self.obj.person) or
                 user.isOneOf(
                     self.obj.specification,
-                    ['owner','drafter', 'assignee', 'approver']) or
+                    ['owner', 'drafter', 'assignee', 'approver']) or
                 user.in_admin)
 
 
@@ -530,6 +530,7 @@
             return True
         return user.isOwner(self.obj.target)
 
+
 class AdminMilestoneByLaunchpadAdmins(AuthorizationBase):
     permission = 'launchpad.Admin'
     usedfor = IMilestone
@@ -574,7 +575,7 @@
 
     def checkAuthenticated(self, user):
         """Is the user a privileged team member or Launchpad staff?
-        
+
         Return true when the user is a member of Launchpad admins,
         registry experts, team admins, or the team owners.
         """
@@ -1144,8 +1145,6 @@
                 self, user)
 
 
-# Please keep EditPOTemplateSubset in sync with this, unless you
-# know exactly what you are doing.
 class EditPOTemplateDetails(AdminPOTemplateDetails, EditByOwnersOrAdmins):
     permission = 'launchpad.Edit'
     usedfor = IPOTemplate
@@ -1433,7 +1432,6 @@
 
     # This code MUST match the logic in IBuildSet.getBuildsForBuilder()
     # otherwise users are likely to get 403 errors, or worse.
-
     def checkAuthenticated(self, user):
         """Private restricts to admins and archive members."""
         if not self.obj.archive.private:
@@ -1648,29 +1646,6 @@
             return OnlyRosettaExpertsAndAdmins.checkAuthenticated(self, user)
 
 
-# Please keep EditPOTemplate in sync with this, unless you
-# know exactly what you are doing. Note that this permission controls
-# access to browsing into individual potemplates, but it's on a different
-# object (POTemplateSubset) from EditPOTemplateDetails,
-# even though it looks almost identical
-class EditPOTemplateSubset(AuthorizationBase):
-    permission = 'launchpad.Edit'
-    usedfor = IPOTemplateSubset
-
-    def checkAuthenticated(self, user):
-        """Allow anyone with admin rights; owners, product owners and
-        distribution owners; and for distros, translation group owners.
-        """
-        if (self.obj.productseries is not None and
-            user.inTeam(self.obj.productseries.product.owner)):
-            # The user is the owner of the product.
-            return True
-
-        return (
-            AdminPOTemplateSubset(self.obj).checkAuthenticated(user) or
-            EditByOwnersOrAdmins(self.obj).checkAuthenticated(user))
-
-
 class AdminDistroSeriesTranslations(AuthorizationBase):
     permission = 'launchpad.TranslationsAdmin'
     usedfor = IDistroSeries

=== modified file 'lib/lp/translations/browser/configure.zcml'
--- lib/lp/translations/browser/configure.zcml	2009-12-12 05:47:41 +0000
+++ lib/lp/translations/browser/configure.zcml	2010-01-20 20:38:18 +0000
@@ -391,9 +391,9 @@
             <browser:page
                 name="+chart"
                 template="../templates/potemplate-chart.pt"/>
-            
+
             <!-- Potemplate Portlets -->
-            
+
             <browser:page
                 name="+portlet-details"
                 template="../templates/potemplate-portlet-details.pt"/>

=== modified file 'lib/lp/translations/browser/potemplate.py'
--- lib/lp/translations/browser/potemplate.py	2009-12-28 22:58:18 +0000
+++ lib/lp/translations/browser/potemplate.py	2010-01-20 20:38:18 +0000
@@ -31,6 +31,7 @@
 from zope.component import getUtility
 from zope.interface import implements
 from zope.publisher.browser import FileUpload
+from zope.security.proxy import removeSecurityProxy
 
 from canonical.lazr.utils import smartquote
 
@@ -291,7 +292,7 @@
                 # SourcePackageName is needed to avoid hardcoding this URL.
                 url = (canonical_url(
                     self.context.distroseries, rootsite="translations") +
-                    "/+source/" + self.context.sourcepackagename.name + 
+                    "/+source/" + self.context.sourcepackagename.name +
                     "/+translations")
             else:
                 url = canonical_url(
@@ -512,7 +513,7 @@
     """View class that lets you edit a POTemplate object."""
 
     schema = IPOTemplate
-    field_names = ['name', 'translation_domain', 'description', 'priority',
+    field_names = ['translation_domain', 'description', 'priority',
         'path', 'owner', 'iscurrent']
     label = 'Edit translation template details'
     page_title = 'Edit details'
@@ -529,11 +530,11 @@
                 product=context.product, distribution=context.distribution,
                 sourcepackagename=context.sourcepackagename)
         if old_translation_domain != context.translation_domain:
-            # We only update date_last_updated when translation_domain field
-            # is changed because is the only significative change that,
-            # somehow, affects the content of the potemplate.
-            UTC = pytz.timezone('UTC')
-            context.date_last_updated = datetime.datetime.now(UTC)
+            # We only change date_last_updated when the translation_domain
+            # field is changed because it is the only relevant field we
+            # care about regarding the date of last update.
+            naked_context = removeSecurityProxy(context)
+            naked_context.date_last_updated = datetime.datetime.now(pytz.UTC)
 
     @property
     def cancel_url(self):
@@ -713,7 +714,7 @@
             raise AssertionError('Unknown context for %s' % potemplate.title)
 
         if ((official_rosetta and potemplate.iscurrent) or
-            check_permission('launchpad.Edit', self.context)):
+            check_permission('launchpad.Edit', potemplate)):
             # The target is using officially Launchpad Translations and the
             # template is available to be translated, or the user is a is a
             # Launchpad administrator in which case we show everything.

=== modified file 'lib/lp/translations/configure.zcml'
--- lib/lp/translations/configure.zcml	2010-01-12 23:25:16 +0000
+++ lib/lp/translations/configure.zcml	2010-01-20 20:38:18 +0000
@@ -411,11 +411,11 @@
                 permission="launchpad.Edit"
                 set_attributes="
                     owner priority description translation_domain path
-                    iscurrent name"/>
+                    iscurrent"/>
             <require
                 permission="launchpad.TranslationsAdmin"
                 set_attributes="
-                    productseries distroseries sourcepackagename 
+                    name productseries distroseries sourcepackagename
                     sourcepackageversion binarypackagename languagepack
                     source_file_format source_file date_last_updated
                     from_sourcepackagename header"/>

=== modified file 'lib/lp/translations/stories/standalone/xx-potemplate-edit.txt'
--- lib/lp/translations/stories/standalone/xx-potemplate-edit.txt	2009-12-29 18:21:37 +0000
+++ lib/lp/translations/stories/standalone/xx-potemplate-edit.txt	2010-01-20 20:38:18 +0000
@@ -33,6 +33,8 @@
   >>> print browser.url
   http://translations.launchpad.dev/evolution/trunk/+pots/evolution-2.2/+edit
 
+The owner of a product has access to edit page for PO templates.
+
   >>> browser = setupBrowser(auth='Basic [e-mail]:test')
   >>> browser.open(
   ...     'http://translations.launchpad.dev/evolution/trunk/+pots/'
@@ -40,20 +42,17 @@
   >>> print browser.url
   http://translations.launchpad.dev/evolution/trunk/+pots/evolution-2.2/+edit
 
-We should be sure that the edit form get only the non admin fields.
+Owner will not see admin fields, only those fields designated for the
+edit page.
 
   >>> browser.getControl(name='field.name').value
-  'evolution-2.2'
-  >>> browser.getControl(name='field.description').value
-  'Template for evolution in hoary'
+  Traceback (most recent call last):
+  ...
+  LookupError:...
   >>> browser.getControl(name='field.header').value
   Traceback (most recent call last):
   ...
   LookupError:...
-  >>> browser.getControl(name='field.iscurrent').value
-  True
-  >>> browser.getControl(name='field.owner').value
-  'rosetta-admins'
   >>> browser.getControl(name='field.productseries').value
   Traceback (most recent call last):
   ...
@@ -78,20 +77,65 @@
   Traceback (most recent call last):
   ...
   LookupError:...
+  >>> browser.getControl(name='field.description').value
+  'Template for evolution in hoary'
   >>> browser.getControl(name='field.path').value
   'po/evolution-2.2.pot'
-
-And that we are able to POST it.
-
+  >>> browser.getControl(name='field.iscurrent').value
+  True
+  >>> browser.getControl(name='field.owner').value
+  'rosetta-admins'
+  >>> browser.getControl(name='field.priority').value
+  '0'
+  >>> browser.getControl(name='field.translation_domain').value
+  'evolution-2.2'
+
+We remember the 'last_update_date' in order to check if it was changed
+after updating the template.
+
+  >>> from zope.component import getUtility
+  >>> from canonical.launchpad.interfaces import ILaunchpadCelebrities
+  >>> from lp.translations.model.potemplate import POTemplateSubset
+  >>> from lp.registry.interfaces.product import IProductSet
+  >>> login('[e-mail]')
+  >>> evolution = getUtility(IProductSet).getByName('evolution')
+  >>> evolution_trunk = evolution.getSeries('trunk')
+  >>> hoary_subset = POTemplateSubset(productseries=evolution_trunk)
+  >>> evolution_template = hoary_subset.getPOTemplateByName(
+  ...     'evolution-2.2')
+  >>> previous_date_last_updated = evolution_template.date_last_updated
+  >>> logout()
+
+The visible fields can be changed and saved.
+
+  >>> browser.getControl(name='field.translation_domain').value = u'evo'
+  >>> browser.getControl(name='field.priority').value = '100'
+  >>> browser.getControl(name='field.iscurrent').value = False
+  >>> browser.getControl(name='field.path').value = 'po/evolution.pot'
+  >>> browser.getControl(name='field.owner').value = u'name12'
   >>> browser.getControl(name='field.description').value = u'foo'
   >>> browser.getControl('Change').click()
   >>> print browser.url
   http://translations.launchpad.dev/evolution/trunk/+pots/evolution-2.2
 
-And the changed value is stored.
+The changed values will be stored and visible by accesing again the
+edit page.
 
   >>> browser.open(
   ...     'http://translations.launchpad.dev/evolution/trunk/+pots/'
   ...     'evolution-2.2/+edit')
+  >>> browser.getControl(name='field.translation_domain').value
+  'evo'
+  >>> browser.getControl(name='field.priority').value
+  '100'
+  >>> browser.getControl(name='field.iscurrent').value
+  False
+  >>> browser.getControl(name='field.path').value
+  'po/evolution.pot'
+  >>> browser.getControl(name='field.owner').value
+  'name12'
   >>> browser.getControl(name='field.description').value
   'foo'
+  >>> previous_date_last_updated != evolution_template.date_last_updated
+  True
+