Merge lp:~henninge/launchpad/devel-705176-sharing-info-link into lp:launchpad

[Henning Eggers]

[r=deryck][ui=sinzui][bug=705176] Add links to sharing details page to translation pages.

= Summary =

This is the final step in adding sharing information to translation pages. It adds a link to each "Sharing Information" portlet which were introduced in the last branch. The link points to a yet-to-be-created page that will provide detailed information about translation sharing and a place to take the necessary configuration step. Thus the link is named differently depending on the user's permissions to configure these things.

Here are the screenshots used in the UI review.
http://people.canonical.com/~henninge/screenshots/sharing-information/sharing-details-info.png
http://people.canonical.com/~henninge/screenshots/sharing-information/sharing-details-edit.png
http://people.canonical.com/~henninge/screenshots/sharing-information/sharing-details-setup.png

== Proposed fix ==

I added a Mixin to be used in all the view classes that creates an HTML snippet for the link. This was far easier than putting it together in TAL. The views that use this mixing implement some methods to provide the specific objects.

== Pre-implementation notes ==

Same as before.

== Implementation details ==

A lot of the implementation is extending the test to work with different users to verify that the correct link text is displayed. One trick here was to make sure that the user's password is 'test' because that is what getViewBrowser expects.

== Tests ==

bin/test -vvcm lp.translations.browser.test.test_sharing_information
bin/test -vvct xx-potemplate-index.txt

== Demo and Q/A ==

Try these as a privileged and an unprivileged user. The link should be "Edit sharing details" and "View sharing details" respectively.

You will have to enable the feature:
translations.sharing_information.enabled

View sharing information for a product series here:
http://translations.launchpad.dev/evolution/trunk

View sharing information for a template here:
http://translations.launchpad.dev/evolution/trunk/+pots/evolution-2.2

View sharing information for a pofile here:
http://translations.launchpad.dev/evolution/trunk/+pots/evolution-2.2/es
This will not show the link!

Once on the other side, you'll see the equivalent links there.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/translations/browser/potemplate.py
  lib/lp/translations/browser/productseries.py
  lib/lp/translations/browser/sourcepackage.py
  lib/lp/translations/browser/translationsharing.py
  lib/lp/translations/browser/tests/test_sharing_information.py
  lib/lp/translations/stories/standalone/xx-potemplate-index.txt
  lib/lp/translations/templates/potemplate-index.pt
  lib/lp/translations/templates/productseries-translations.pt
  lib/lp/translations/templates/sourcepackage-translations.pt

[Curtis Hovey (sinzui)]

The exclamation mark in sharing-details-setup.png looks odd (now!). Links do not have punctuation. Why does this one need an exclamation mark? Launchpad does not have any links that say "now". This link looks like it wants to be a sentence. There are no links that use "Set up". I see "Set thing" and "Configure thing" I would expect this to be presented as
a sentence:
    _(/) Configure sharing_ now!
or a navigational link
    _(/) Configure sharing_

review: Needs Information (ui)

[Henning Eggers (henninge)]

Well, the exclamation mark and the "now" were meant to encourage people to do the configuration. But I see your point that this is not common to do. Using "set up" is used on the same page, though: "Set up branch synchronization". Should that be "Configure branch synchronization", too? I understand "set up" to be more about "inital configuration" which is what this link points to.

I would like to do this:
    _(/) Set up sharing_

[Curtis Hovey (sinzui)]

Thank you for explaining more about what the user is going to do. _(/) Set up sharing_
is better than my suggestion.

review: Approve (ui)

[Deryck Hodge (deryck)]

Looks good. Approved with the following request for a minor update:

<deryck> henninge, looking at your MP.... should TranslationSharingDetailsMixin also raise NotImplementedError for is_sharing and can_edit_sharing_details?
<deryck> henninge, so the API is clear?
<henninge> deryck: Yes, I think that makes sense. I thought about it, too, but I don't know why I discarded the thought.

Cheers,
deryck

review: Approve (code)

=== modified file 'lib/lp/translations/browser/potemplate.py'
--- lib/lp/translations/browser/potemplate.py	2011-03-01 17:40:06 +0000
+++ lib/lp/translations/browser/potemplate.py	2011-03-08 17:15:57 +0000
@@ -71,6 +71,9 @@
 from lp.services.worlddata.interfaces.language import ILanguageSet
 from lp.translations.browser.poexportrequest import BaseExportView
 from lp.translations.browser.translations import TranslationsMixin
+from lp.translations.browser.translationsharing import (
+    TranslationSharingDetailsMixin,
+    )
 from lp.translations.interfaces.pofile import IPOFileSet
 from lp.translations.interfaces.potemplate import (
     IPOTemplate,
@@ -229,7 +232,8 @@
         self.request.response.redirect('../+translations')
 
 
-class POTemplateView(LaunchpadView, TranslationsMixin):
+class POTemplateView(LaunchpadView,
+                     TranslationsMixin, TranslationSharingDetailsMixin):
 
     SHOW_RELATED_TEMPLATES = 4
 
@@ -373,6 +377,20 @@
         obj, template = infos[0]
         return template
 
+    def getTranslationTarget(self):
+        """See `TranslationSharingDetailsMixin`."""
+        if self.is_upstream_template:
+            return self.context.productseries
+        else:
+            return self.context.sourcepackage
+
+    def can_edit_sharing_details(self):
+        if self.is_upstream_template:
+            obj = self.context.productseries
+        else:
+            obj = self.context.distroseries
+        return check_permission('launchpad.Edit', obj)
+
 
 class POTemplateUploadView(LaunchpadView, TranslationsMixin):
     """Upload translations and updated template."""

=== modified file 'lib/lp/translations/browser/productseries.py'
--- lib/lp/translations/browser/productseries.py	2011-02-25 11:45:11 +0000
+++ lib/lp/translations/browser/productseries.py	2011-03-08 17:15:57 +0000
@@ -49,6 +49,9 @@
 from lp.translations.browser.poexportrequest import BaseExportView
 from lp.translations.browser.potemplate import BaseSeriesTemplatesView
 from lp.translations.browser.translations import TranslationsMixin
+from lp.translations.browser.translationsharing import (
+    TranslationSharingDetailsMixin,
+    )
 from lp.translations.interfaces.productserieslanguage import (
     IProductSeriesLanguageSet,
     )
@@ -344,7 +347,9 @@
         return check_permission("launchpad.Edit", self.context)
 
 
-class ProductSeriesView(LaunchpadView, ProductSeriesTranslationsMixin):
+class ProductSeriesView(LaunchpadView,
+                        ProductSeriesTranslationsMixin,
+                        TranslationSharingDetailsMixin):
     """A view to show a series with translations."""
 
     label = "Translation status by language"
@@ -462,6 +467,12 @@
         sourcepackage, template = infos[0]
         return sourcepackage
 
+    def getTranslationTarget(self):
+        """See `TranslationSharingDetailsMixin`."""
+        return self.context
+
+    can_edit_sharing_details = can_configure_translations
+
 
 class SettingsRadioWidget(LaunchpadRadioWidgetWithDescription):
     """Remove the confusing hint under the widget."""

=== modified file 'lib/lp/translations/browser/sourcepackage.py'
--- lib/lp/translations/browser/sourcepackage.py	2011-02-25 11:45:11 +0000
+++ lib/lp/translations/browser/sourcepackage.py	2011-03-08 17:15:57 +0000
@@ -16,16 +16,21 @@
     Link,
     NavigationMenu,
     )
+from canonical.launchpad.webapp.authorization import check_permission
 from lp.registry.interfaces.sourcepackage import ISourcePackage
 from lp.translations.browser.poexportrequest import BaseExportView
 from lp.translations.browser.translations import TranslationsMixin
+from lp.translations.browser.translationsharing import (
+    TranslationSharingDetailsMixin,
+    )
 from lp.translations.utilities.translationsharinginfo import (
     has_upstream_template,
     get_upstream_sharing_info,
     )
 
 
-class SourcePackageTranslationsView(TranslationsMixin):
+class SourcePackageTranslationsView(TranslationsMixin,
+                                    TranslationSharingDetailsMixin):
 
     @property
     def potemplates(self):
@@ -47,6 +52,13 @@
         productseries, template = infos[0]
         return productseries
 
+    def getTranslationTarget(self):
+        """See `TranslationSharingDetailsMixin`."""
+        return self.context
+
+    def can_edit_sharing_details(self):
+        return check_permission('launchpad.Edit', self.context.distroseries)
+
 
 class SourcePackageTranslationsMenu(NavigationMenu):
     usedfor = ISourcePackage

=== modified file 'lib/lp/translations/browser/tests/test_sharing_information.py'
--- lib/lp/translations/browser/tests/test_sharing_information.py	2011-03-01 20:05:22 +0000
+++ lib/lp/translations/browser/tests/test_sharing_information.py	2011-03-08 17:15:57 +0000
@@ -62,12 +62,28 @@
 
     SHARING_TEXT = None
 
-    def _test_sharing_information(self, obj, expected_text):
+    def getAuthorizedUser(self, obj):
+        """Get a user that is authorized to edit sharing details on obj."""
+        raise NotImplementedError
+
+    SHARING_DETAILS_INFO = "View sharing details"
+    SHARING_DETAILS_SETUP = "Set up sharing"
+    SHARING_DETAILS_EDIT = "Edit sharing details"
+
+    def _test_sharing_information(self, obj,
+                                  id_under_test, expected_text,
+                                  authorized=False):
         self.useFixture(FeatureFixture(
             {'translations.sharing_information.enabled': 'on'}))
+        if authorized:
+            user = self.getAuthorizedUser(obj)
+        else:
+            user = None
         browser = self.getViewBrowser(
-            obj, no_login=True, rootsite="translations")
-        sharing_info = find_tag_by_id(browser.contents, 'sharing-information')
+                obj, user=user, no_login=(not authorized),
+                rootsite="translations")
+
+        sharing_info = find_tag_by_id(browser.contents, id_under_test)
         if expected_text is None:
             self.assertIs(None, sharing_info)
         else:
@@ -77,11 +93,36 @@
 
     def test_not_sharing(self):
         self._test_sharing_information(
-            self.makeNotSharingObject(), self.NOT_SHARING_TEXT)
+            self.makeNotSharingObject(),
+            'sharing-information', self.NOT_SHARING_TEXT)
 
     def test_sharing(self):
         self._test_sharing_information(
-            self.makeSharingObject(), self.SHARING_TEXT)
+            self.makeSharingObject(),
+            'sharing-information', self.SHARING_TEXT)
+
+    def test_sharing_details_info(self):
+        # For unauthorized users, the link to the sharing details page is
+        # informational.
+        self._test_sharing_information(
+            self.makeSharingObject(),
+            'sharing-details', self.SHARING_DETAILS_INFO)
+
+    def test_sharing_details_setup(self):
+        # For authorized users of not sharing objects, the link to the
+        # sharing details page encourages action.
+        self._test_sharing_information(
+            self.makeNotSharingObject(),
+            'sharing-details', self.SHARING_DETAILS_SETUP,
+            authorized=True)
+
+    def test_sharing_details_edit(self):
+        # For authorized users, the link to the sharing details page is for
+        # editing
+        self._test_sharing_information(
+            self.makeSharingObject(),
+            'sharing-details', self.SHARING_DETAILS_EDIT,
+            authorized=True)
 
 
 class TestUpstreamPOTemplateSharingInfo(BrowserTestCase,
@@ -104,6 +145,9 @@
     SHARING_TEXT = """
         This template is sharing translations with .*"""
 
+    def getAuthorizedUser(self, potemplate):
+        return potemplate.productseries.product.owner
+
 
 class TestPOFileSharingInfo(BrowserTestCase, TestSharingInfoMixin):
     """Test display of POFile sharing info."""
@@ -125,6 +169,13 @@
     SHARING_TEXT = """
         These translations are shared with .*"""
 
+    def getAuthorizedUser(self, productseries):
+        return None
+
+    SHARING_DETAILS_INFO = None
+    SHARING_DETAILS_SETUP = None
+    SHARING_DETAILS_EDIT = None
+
 
 class TestDummyPOFileSharingInfo(BrowserTestCase, TestSharingInfoMixin):
     """Test display of DummyPOFile sharing info."""
@@ -145,6 +196,13 @@
     SHARING_TEXT = """
         These translations are shared with .*"""
 
+    def getAuthorizedUser(self, productseries):
+        return None
+
+    SHARING_DETAILS_INFO = None
+    SHARING_DETAILS_SETUP = None
+    SHARING_DETAILS_EDIT = None
+
 
 class TestUpstreamSharingInfo(BrowserTestCase, TestSharingInfoMixin):
     """Test display of product series sharing info."""
@@ -167,6 +225,9 @@
     SHARING_TEXT = """
         This project series is sharing translations with .*"""
 
+    def getAuthorizedUser(self, productseries):
+        return productseries.product.owner
+
 
 class TestUbuntuPOTemplateSharingInfo(BrowserTestCase, TestSharingInfoMixin):
     """Test display of template sharing info in an Ubuntu source package."""
@@ -190,6 +251,12 @@
     SHARING_TEXT = """
         This template is sharing translations with .*"""
 
+    def getAuthorizedUser(self, potemplate):
+        with celebrity_logged_in('admin'):
+            potemplate.distroseries.owner = self.factory.makePerson(
+                password='test')
+        return potemplate.distroseries.owner
+
 
 class TestUbuntuSharingInfo(BrowserTestCase, TestSharingInfoMixin):
     """Test display of source package sharing info."""
@@ -213,3 +280,9 @@
 
     SHARING_TEXT = """
         This source package is sharing translations with .*"""
+
+    def getAuthorizedUser(self, sourcepackage):
+        with celebrity_logged_in('admin'):
+            sourcepackage.distroseries.owner = self.factory.makePerson(
+                password='test')
+        return sourcepackage.distroseries.owner

=== added file 'lib/lp/translations/browser/translationsharing.py'
--- lib/lp/translations/browser/translationsharing.py	1970-01-01 00:00:00 +0000
+++ lib/lp/translations/browser/translationsharing.py	2011-03-08 17:15:57 +0000
@@ -0,0 +1,57 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Views and mixins to use for translation sharing."""
+
+__metaclass__ = type
+
+__all__ = [
+    'TranslationSharingDetailsMixin',
+    ]
+
+
+from canonical.launchpad.webapp import canonical_url
+
+
+class TranslationSharingDetailsMixin:
+    """Mixin for views that need to display translation details link.
+
+    View using this need to implement is_sharing, can_edit_sharing_details
+    and getTranslationTarget().
+    """
+
+    def is_sharing(self):
+        """Whether this object is sharing translations or not."""
+        raise NotImplementedError
+
+    def can_edit_sharing_details(self):
+        """If the current user can edit sharing details."""
+        raise NotImplementedError
+
+    def getTranslationTarget(self):
+        """Return either a productseries or a sourcepackage."""
+        raise NotImplementedError
+
+    def sharing_details(self):
+        """Construct the link to the sharing details page."""
+        tag_template = (
+            '<a class="sprite %(icon)s" id="sharing-details"'
+            ' href="%(href)s/+sharing-details">%(text)s</a>')
+
+        if self.can_edit_sharing_details():
+            icon = 'edit'
+            if self.is_sharing():
+                text = "Edit sharing details"
+            else:
+                text = "Set up sharing"
+        else:
+            if self.is_sharing():
+                icon = 'info'
+                text = "View sharing details"
+            else:
+                return ""
+        href = canonical_url(
+            self.getTranslationTarget(),
+            rootsite='translations',
+            )
+        return tag_template % dict(icon=icon, text=text, href=href)

=== modified file 'lib/lp/translations/stories/standalone/xx-potemplate-index.txt'
--- lib/lp/translations/stories/standalone/xx-potemplate-index.txt	2011-03-02 09:20:38 +0000
+++ lib/lp/translations/stories/standalone/xx-potemplate-index.txt	2011-03-08 17:15:57 +0000
@@ -80,6 +80,7 @@
     Sharing Information
     This template is sharing translations with
     evolution in Ubuntu Hoary template evolution-2.2.
+    View sharing details
     >>> print sharing_info
     <div...<a href="/ubuntu/hoary/+source/evolution/+pots/evolution-2.2"...
 
@@ -95,9 +96,23 @@
     Sharing Information
     This template is sharing translations with
     Evolution trunk series template evolution-2.2.
+    View sharing details
     >>> print sharing_info
     <div...<a href="/evolution/trunk/+pots/evolution-2.2"...
 
+If the user has the right permissions, they are offered to edit the sharing
+information.
+
+    >>> with FeatureFixture(feature_flag):
+    ...     admin_browser.open('http://translations.launchpad.dev/evolution'
+    ...         '/trunk/+pots/evolution-2.2')
+    >>> sharing_details = find_tag_by_id(
+    ...     admin_browser.contents, 'sharing-details')
+    >>> print extract_text(sharing_details)
+    Edit sharing details
+    >>> print sharing_details['href']
+    http://.../evolution/trunk/+sharing-details
+
 
 Finding related templates
 -------------------------

=== modified file 'lib/lp/translations/templates/potemplate-index.pt'
--- lib/lp/translations/templates/potemplate-index.pt	2011-03-01 17:39:16 +0000
+++ lib/lp/translations/templates/potemplate-index.pt	2011-03-08 17:15:57 +0000
@@ -80,6 +80,9 @@
                tal:content="template/name"
              >evolution-2.2</a>.
         </p>
+        <a tal:replace="structure view/sharing_details">
+          View sharing details
+        </a>
       </div>
     </div>
 

=== modified file 'lib/lp/translations/templates/productseries-translations.pt'
--- lib/lp/translations/templates/productseries-translations.pt	2011-02-28 17:06:03 +0000
+++ lib/lp/translations/templates/productseries-translations.pt	2011-03-08 17:15:57 +0000
@@ -1,3 +1,4 @@
+
 <html
   xmlns="http://www.w3.org/1999/xhtml"
   xmlns:tal="http://xml.zope.org/namespaces/tal"
@@ -90,6 +91,9 @@
             tal:attributes="href package/fmt:url"
             tal:content="package/displayname">apache in ubuntu hoary</a>.
             </p>
+            <a tal:replace="structure view/sharing_details">
+              View sharing details
+            </a>
           </div>
         </div>
 

=== modified file 'lib/lp/translations/templates/sourcepackage-translations.pt'
--- lib/lp/translations/templates/sourcepackage-translations.pt	2011-02-28 17:06:03 +0000
+++ lib/lp/translations/templates/sourcepackage-translations.pt	2011-03-08 17:15:57 +0000
@@ -38,6 +38,9 @@
               tal:attributes="href productseries/fmt:url"
               tal:content="productseries/title">Evolution trunk</a>.
             </p>
+            <a tal:replace="structure view/sharing_details">
+              View sharing details
+            </a>
           </div>
         </div>
         <div class="yui-u">