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

= 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

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.

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.

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.

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
+

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.

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