Launchpad itself

Code review comment for lp:~adiroiban/launchpad/bug-427319

bug-427319
Merge into devel

Revision history for this message

Adi Roiban (adiroiban) wrote on 2009-12-12:

În data de Vi, 11-12-2009 la 20:18 +0000, Guilherme Salgado a scris:
> Review: Approve
> Hi Adi,
>
> Just a couple more suggestions, but these are really trivial. Once you
> push the changes I'll submit your branch to ec2 to run the full test
> suite.
>
> Once again, thanks a lot for this improvement and the very nice cleanup!
>
> review approve
> status approved
[snip]
> In this case I think the best is to document that in the method's
> docstring (e.g. "This method must not be called if
> self.translation_group is None.") and turn the if/elif into an if/else
> with an assert, like this:
>
> if self.translation_team:
> ...
> else:
> assert self.translation_group is not None, (
> "Must not be called when there's no translation group.")
> ...
OK. Done.

> This check can go away once the if/elif is converted into an if/else
> with an assertion. The latter is preferred because it will fail
> horribly (instead of silently, like it currently does) when the property
> is used incorrectly (i.e. when translation_group is None).
Yep.

> > +
> > if not translations_person.translations_relicensing_agreement:
> > translation_license_url = PersonFormatterAPI(
> > - translations_person).url() + '/translations/+licensing'
> > - return ("To make translations in Launchpad you need to " +
> > - "agree with the " +
> > + translations_person).url(
> > + view_name='+licensing',
> > + rootsite='translations')
>
> You might not need rootsite='translations' (as this page is already on
> the translations rootsite), but I might be wrong.
I've checked the implementation for PersonFormatterAPI.url, and as far
I can understand that code, the rootsite is not automatically detected.

rootsite is required... as +licensing is not valid for main.

Also tested without rootsite and it was not created for translations.

> > +Users will see three references to the team managing these translations.
> > +
> > + >>> print user_browser.getLink(
> > + ... 'Ubuntu Spanish Translator',index=0).url
> > + http://launchpad.dev/~ubuntu-l10n-es
> > +
> > + >>> print user_browser.getLink(
> > + ... 'Ubuntu Spanish Translator',index=1).url
> > + http://launchpad.dev/~ubuntu-l10n-es
> > +
> > + >>> print user_browser.getLink(
> > + ... 'Ubuntu Spanish Translator',index=2).url
> > + http://launchpad.dev/~ubuntu-l10n-es
>
> Is it really important to show that we have 3 links to the team managing
> the translations?
Fixed.

I added new tests for checking when a translator has not agreed the
translation license and when translations are CLOSED.

Merged with devel since I modified the Factory in a branch that was just
commited and those changes also affects these tests.

Cheers.

Here is the diff.

     @property
     def access_level_description(self):
+ """Must not be called when there's no translation group."""
+
         if self.user is None:
             return ("You are not logged in. Please log in to work "
                     "on translations.")
@@ -88,18 +90,13 @@
         elif self.translation_group:
             translations_contact_link = PersonFormatterAPI(
                 self.translation_group.owner).link(None)
-
- if translations_contact_link is None:
- #Having no translation group is a valid case, but the
- #template should not call access_level_description for
- #this condition.
- #We return a blank screen since the information about
- #missing group is displaying in a different section
- return ""
+ else:
+ assert self.translation_group is not None, (
+ "Must not be called when there's no translation
group.")

         if not translations_person.translations_relicensing_agreement:
             translation_license_url = PersonFormatterAPI(
- translations_person).url(
+ self.user).url(
                     view_name='+licensing',
                     rootsite='translations')
             return ("To make translations in Launchpad you need to "
@@ -123,7 +120,7 @@
             permission = sample_pofile.translationpermission
             if permission == TranslationPermission.CLOSED:
                 return ("These templates can be translated only by "
- "its managers.")
+ "their managers.")

if self.translation_team is None:
return ("Since there is nobody to manage translation "

Spanish has a translation team for managing its translations and all
@@ -100,15 +103,7 @@
Users will see three references to the team managing these
translations.

>>> print user_browser.getLink(
- ... 'Ubuntu Spanish Translator',index=0).url
- http://launchpad.dev/~ubuntu-l10n-es
-
- >>> print user_browser.getLink(
- ... 'Ubuntu Spanish Translator',index=1).url
- http://launchpad.dev/~ubuntu-l10n-es
-
- >>> print user_browser.getLink(
- ... 'Ubuntu Spanish Translator',index=2).url
+ ... 'Ubuntu Spanish Translator').url
http://launchpad.dev/~ubuntu-l10n-es

Catalan has no translation team for managing translations and since
@@ -133,10 +128,7 @@
translations, please contact Ubuntu Translation Coordinators...

>>> print user_browser.getLink(
- ... 'Ubuntu Translation Coordinators',index=0).url
- http://launchpad.dev/~utc-team
- >>> print user_browser.getLink(
- ... 'Ubuntu Translation Coordinators',index=1).url
+ ... 'Ubuntu Translation Coordinators').url
http://launchpad.dev/~utc-team

Members of translation team and translations admins have full access to
@@ -148,9 +140,44 @@
... admin_browser.contents, 'translation-access-level'))
You can add and review translations...

-
-For project with no translation teams, translators see a note stating
-there is no group. No access level information is displayed in this
case.
+For projects using closed translations policy, a translator that is not
+member of the translation team appointed for that language will not
+be allowed to make any changes.
+
+ >>> login('<email address hidden>')
+ >>> ubuntu.translationpermission = TranslationPermission.CLOSED
+ >>> logout()
+
+ >>> user_browser.open(
+ ... 'http://translations.launchpad.dev/ubuntu/hoary/+lang/ro')
+ >>> print extract_text(find_tag_by_id(
+ ... user_browser.contents, 'translation-access-level'))
+ These templates can be translated only by their managers...
+
+Translation policy is rolled back to not affect other tests.
+
+ >>> login('<email address hidden>')
+ >>> ubuntu.translationpermission = TranslationPermission.RESTRICTED
+ >>> logout()
+
+Translators that have not agreed with the license can not make
+translations, and will see a link to the license page.
+
+ >>> no_license_browser = setupBrowser(
+ ... auth='Basic <email address hidden>:test')
+ >>> no_license_browser.open(
+ ... 'http://translations.launchpad.dev/ubuntu/hoary/+lang/ro')
+ >>> print extract_text(find_tag_by_id(
+ ... no_license_browser.contents, 'translation-access-level'))
+ To make translations in Launchpad you need to agree with
+ the Translations licensing...
+
+ >>> print no_license_browser.getLink(
+ ... 'Translations licensing').url
+ http://translations.launchpad.dev/~dude/+licensing
+
+For project with no translation group, translators see a note stating
+this fact. No access level information is displayed.

     >>> login('<email address hidden>')
     >>> ubuntu.translationgroup = None
@@ -166,4 +193,10 @@
     ... user_browser.contents, 'translation-access-level'))
     Templates which are more important to translate are listed first.

+Translation group configuration is rolled back to not affect other
tests.
+
+ >>> login('<email address hidden>')
+ >>> ubuntu.translationgroup = utg
+ >>> logout()
+
The details of the page are tested at the view level.

--
Adi Roiban

În data de Vi, 11-12-2009 la 20:18 +0000, Guilherme Salgado a scris:
> Review: Approve
> Hi Adi,
> 
> Just a couple more suggestions, but these are really trivial. Once you
> push the changes I'll submit your branch to ec2 to run the full test
> suite.
> 
> Once again, thanks a lot for this improvement and the very nice cleanup!
> 
>  review approve
>  status approved
[snip]
> In this case I think the best is to document that in the method's
> docstring (e.g. "This method must not be called if
> self.translation_group is None.") and turn the if/elif into an if/else
> with an assert, like this:
> 
>     if self.translation_team:
>         ...
>     else:
>         assert self.translation_group is not None, (
>             "Must not be called when there's no translation group.")
>         ...
OK. Done.

> This check can go away once the if/elif is converted into an if/else
> with an assertion.  The latter is preferred because it will fail
> horribly (instead of silently, like it currently does) when the property
> is used incorrectly (i.e. when translation_group is None).
Yep.

> > +
> >          if not translations_person.translations_relicensing_agreement:
> >              translation_license_url = PersonFormatterAPI(
> > -                translations_person).url() + '/translations/+licensing'
> > -            return ("To make translations in Launchpad you need to " +
> > -                    "agree with the " +
> > +                translations_person).url(
> > +                    view_name='+licensing',
> > +                    rootsite='translations')
> 
> You might not need rootsite='translations' (as this page is already on
> the translations rootsite), but I might be wrong.
I've checked the implementation for PersonFormatterAPI.url, and as far
I can understand that code, the rootsite is not automatically detected.

rootsite is required... as +licensing is not valid for main.

Also tested without rootsite and it was not created for translations.

> > +Users will see three references to the team managing these translations.
> > +
> > +    >>> print user_browser.getLink(
> > +    ...     'Ubuntu Spanish Translator',index=0).url
> > +    http://launchpad.dev/~ubuntu-l10n-es
> > +
> > +    >>> print user_browser.getLink(
> > +    ...     'Ubuntu Spanish Translator',index=1).url
> > +    http://launchpad.dev/~ubuntu-l10n-es
> > +
> > +    >>> print user_browser.getLink(
> > +    ...     'Ubuntu Spanish Translator',index=2).url
> > +    http://launchpad.dev/~ubuntu-l10n-es
> 
> Is it really important to show that we have 3 links to the team managing
> the translations?
Fixed.

I added new tests for checking when a translator has not agreed the
translation license and when translations are CLOSED.

Merged with devel since I modified the Factory in a branch that was just
commited and those changes also affects these tests.

Cheers.

Here is the diff.

=== modified file 'lib/lp/translations/browser/serieslanguage.py'
--- lib/lp/translations/browser/serieslanguage.py	2009-12-11 20:11:06
+0000
+++ lib/lp/translations/browser/serieslanguage.py	2009-12-12 06:19:24
+0000
@@ -75,6 +75,8 @@
 
     @property
     def access_level_description(self):
+        """Must not be called when there's no translation group."""
+        
         if self.user is None:
             return ("You are not logged in. Please log in to work "
                     "on translations.")
@@ -88,18 +90,13 @@
         elif self.translation_group:
             translations_contact_link = PersonFormatterAPI(
                 self.translation_group.owner).link(None)
-
-        if translations_contact_link is None:
-            #Having no translation group is a valid case, but the
-            #template should not call access_level_description for
-            #this condition.
-            #We return a blank screen since the information about
-            #missing group is displaying in a different section
-            return ""
+        else:
+            assert self.translation_group is not None, (
+                "Must not be called when there's no translation
group.")
 
         if not translations_person.translations_relicensing_agreement:
             translation_license_url = PersonFormatterAPI(
-                translations_person).url(
+                self.user).url(
                     view_name='+licensing',
                     rootsite='translations')
             return ("To make translations in Launchpad you need to "
@@ -123,7 +120,7 @@
             permission = sample_pofile.translationpermission
             if permission == TranslationPermission.CLOSED:
                 return ("These templates can be translated only by "
-                        "its managers.")
+                        "their managers.")
 
         if self.translation_team is None:
             return ("Since there is nobody to manage translation "

=== modified file
'lib/lp/translations/stories/standalone/xx-serieslanguage-index.txt'
--- lib/lp/translations/stories/standalone/xx-serieslanguage-index.txt
2009-12-11 20:11:06 +0000
+++ lib/lp/translations/stories/standalone/xx-serieslanguage-index.txt
2009-12-12 06:49:16 +0000
@@ -65,12 +65,15 @@
     >>> st_coordinator = factory.makePerson(
     ...     name="ubuntu-l10n-es",
     ...     displayname='Ubuntu Spanish Translators')
-
+    >>> dude = factory.makePerson(
+    ...     name="dude", email="dude@ex.com", password="test")
     >>> ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
     >>> ubuntu.translationgroup = utg
     >>> ubuntu.translationpermission = TranslationPermission.RESTRICTED
     >>> translators = factory.makeTranslator(
     ...     'es', group=utg, person=st_coordinator)
+    >>> no_license_translator = factory.makeTranslator(
+    ...     'es', person=dude, license=False)
     >>> logout()
 
 Spanish has a translation team for managing its translations and all
@@ -100,15 +103,7 @@
 Users will see three references to the team managing these
translations.
 
     >>> print user_browser.getLink(
-    ...     'Ubuntu Spanish Translator',index=0).url
-    http://launchpad.dev/~ubuntu-l10n-es
-
-    >>> print user_browser.getLink(
-    ...     'Ubuntu Spanish Translator',index=1).url
-    http://launchpad.dev/~ubuntu-l10n-es
-
-    >>> print user_browser.getLink(
-    ...     'Ubuntu Spanish Translator',index=2).url
+    ...     'Ubuntu Spanish Translator').url
     http://launchpad.dev/~ubuntu-l10n-es
 
 Catalan has no translation team for managing translations and since
@@ -133,10 +128,7 @@
     translations, please contact Ubuntu Translation Coordinators...
 
     >>> print user_browser.getLink(
-    ...     'Ubuntu Translation Coordinators',index=0).url
-    http://launchpad.dev/~utc-team
-    >>> print user_browser.getLink(
-    ...     'Ubuntu Translation Coordinators',index=1).url
+    ...     'Ubuntu Translation Coordinators').url
     http://launchpad.dev/~utc-team
 
 Members of translation team and translations admins have full access to
@@ -148,9 +140,44 @@
     ...     admin_browser.contents, 'translation-access-level'))
     You can add and review translations...
 
-
-For project with no translation teams, translators see a note stating
-there is no group. No access level information is displayed in this
case.
+For projects using closed translations policy, a translator that is not
+member of the translation team appointed for that language will not
+be allowed to make any changes.
+
+    >>> login('foo.bar@canonical.com')
+    >>> ubuntu.translationpermission = TranslationPermission.CLOSED
+    >>> logout()
+
+    >>> user_browser.open(
+    ...     'http://translations.launchpad.dev/ubuntu/hoary/+lang/ro')
+    >>> print extract_text(find_tag_by_id(
+    ...     user_browser.contents, 'translation-access-level'))
+    These templates can be translated only by their managers...
+
+Translation policy is rolled back to not affect other tests.
+
+    >>> login('foo.bar@canonical.com')
+    >>> ubuntu.translationpermission = TranslationPermission.RESTRICTED
+    >>> logout()
+
+Translators that have not agreed with the license can not make
+translations, and will see a link to the license page.
+
+    >>> no_license_browser = setupBrowser(
+    ...     auth='Basic dude@ex.com:test')
+    >>> no_license_browser.open(
+    ...     'http://translations.launchpad.dev/ubuntu/hoary/+lang/ro')
+    >>> print extract_text(find_tag_by_id(
+    ...     no_license_browser.contents, 'translation-access-level'))
+    To make translations in Launchpad you need to agree with
+    the Translations licensing...
+
+    >>> print no_license_browser.getLink(
+    ...     'Translations licensing').url
+    http://translations.launchpad.dev/~dude/+licensing
+
+For project with no translation group, translators see a note stating
+this fact. No access level information is displayed.
 
     >>> login('foo.bar@canonical.com')
     >>> ubuntu.translationgroup = None
@@ -166,4 +193,10 @@
     ...     user_browser.contents, 'translation-access-level'))
     Templates which are more important to translate are listed first.
 
+Translation group configuration is rolled back to not affect other
tests.
+
+    >>> login('foo.bar@canonical.com')
+    >>> ubuntu.translationgroup = utg
+    >>> logout()
+
 The details of the page are tested at the view level.

-- 
Adi Roiban

« Back to merge proposal