Merge lp:~michael.nelson/launchpad/509370-access-non-unique-ppa-name into lp:launchpad

[Michael Nelson]

Ensures that email sent regarding private ppa subscriptions uniquely identifies the ppa by including the ppa ref (ppa:cprov/myppa), and additionally the description (to help the person receiving the email to understand what the purpose of the PPA is.

[Michael Nelson (michael.nelson)]

This branch addresses the issues in bug 509370, by ensuring that when a person is added as a subscriber to a private PPA, the email they receive informing them will include not only the display name of the PPA, but also the ppa name, and the description of the PPA.

It also updates the archive subscriptions page to include the PPA name (in addition to the display name) and a link to the owner (but not currently the description of the PPA as it's not viewable by the subscriber, but this could be addressed by bug 336779).

Matt: can you check the wording change to the email template? You can see the change in expected output in doc/archivesubscriber.txt

An example of the archivesubscriptions page:
http://people.canonical.com/~michaeln/tmp/Screenshot-Private%20PPA%20access%20:%20Joe%20Smith%20-%20Chromium.png

== Demo ==
To setup the subscription, run the following in a harness:

http://pastebin.ubuntu.com/373304/

and then login as <email address hidden>:test and browse toz;

https://launchpad.dev/~joesmith

then click on "View your private PPA subscriptions"

== Test ==
bin/test -vv -t xx-private-ppa-subscription -t archivesubscriber.txt

== Pylint notices ==

lib/canonical/launchpad/mailnotification.py
    15: [F0401] Unable to import 'email.Header' (No module named Header)
    16: [F0401] Unable to import 'email.MIMEText' (No module named MIMEText)
    17: [F0401] Unable to import 'email.MIMEMultipart' (No module named MIMEMultipart)
    18: [F0401] Unable to import 'email.MIMEMessage' (No module named MIMEMessage)
    19: [F0401] Unable to import 'email.Utils' (No module named Utils)

[Matthew Revell (matthew.revell)]

> Matt: can you check the wording change to the email template? You can see the
> change in expected output in doc/archivesubscriber.txt

Looks good to me. +1

As an aside, should we link to some help in the email?

review: Approve (archivesubscriber.txt)

[Curtis Hovey (sinzui)]

Hi Michael.

This looks nice. I have a few remarks about the reset password button.
1) It should be in title case: [Reset Password]
2) Why is it missing padding between the text and the border in webkit?
   I see a very narrow button in Epiphany, looks fine in Firefox.

review: Approve (ui)

[Abel Deuring (adeuring)]

review: Approve (code)

[Michael Nelson (michael.nelson)]

> Hi Michael.
>
> This looks nice. I have a few remarks about the reset password button.
> 1) It should be in title case: [Reset Password]

Thanks Curtis, I updated this one.
> 2) Why is it missing padding between the text and the border in webkit?
> I see a very narrow button in Epiphany, looks fine in Firefox.

Chromium is fine, but yeah, I installed Epiphany and took a look. Eek.

So we've got a button {padding:0} in our style.css.

Take a look at:
http://people.canonical.com/~michaeln/tmp/button_default_padding.png

Both images are chromium, the first with the default 0 padding, the second with 8px padding on the button. As you can see, Chromium (and FF, etc.) even with zero padding, the content of the button (text in this case) has its own padding or margin, whereas epiphany doesn't. I couldn't find a quick solution (modifying the button padding obviously affects the other browsers), so I've switched the button, replacing it with a hidden input (for the value passed to the view) and a normal <input type="submit"..> for the button. This ensures that Epiphany appears just like Chromium/FF now.

Thanks.
http://pastebin.ubuntu.com/374707/

[Curtis Hovey (sinzui)]

On Fri, 2010-02-12 at 12:43 +0000, Michael Nelson wrote:
> > Hi Michael.
...
> Take a look at:
> http://people.canonical.com/~michaeln/tmp/button_default_padding.png
>
> Both images are chromium, the first with the default 0 padding, the
> second with 8px padding on the button. As you can see, Chromium (and
> FF, etc.) even with zero padding, the content of the button (text in
> this case) has its own padding or margin, whereas epiphany doesn't. I
> couldn't find a quick solution (modifying the button padding obviously
> affects the other browsers), so I've switched the button, replacing it
> with a hidden input (for the value passed to the view) and a normal
> <input type="submit"..> for the button. This ensures that Epiphany
> appears just like Chromium/FF now.
>
> Thanks.
> http://pastebin.ubuntu.com/374707/

Your solution explains other bug reports if have seen about narrow
buttons, such as in the help pane.

I see that webkit has a plethora of styles, and I suspect the wrong one
is chosen by default. I only recently discovered that MacOS supports
extra narrow buttons and scrollbars:

    -webkit-appearance: <push-button|square-button|button|button-bevel>

May investigate this if I do another round of webkitisms.

--
__Curtis C. Hovey_________
http://launchpad.net/

=== modified file 'lib/canonical/launchpad/doc/tales.txt'
--- lib/canonical/launchpad/doc/tales.txt	2010-01-14 12:09:34 +0000
+++ lib/canonical/launchpad/doc/tales.txt	2010-02-12 13:51:23 +0000
@@ -123,11 +123,14 @@
     <img ... src="/@@/distribution" />
 
 PPAs have a 'link' formatter, which returns the appropriate HTML used
-for referring to them in other pages.
+for referring to them in other pages and a 'reference' formatter which
+displays the unique ppa reference.
 
     >>> print test_tales("ppa/fmt:link", ppa=mark.archive)
     <a href="/~mark/+archive/ppa"
        class="sprite ppa-icon">PPA for Mark Shuttleworth</a>
+    >>> print test_tales("ppa/fmt:reference", ppa=mark.archive)
+    ppa:mark/ppa
 
 Disabled PPAs links use a different icon and are only linkified for
 users with launchpad.View on them.
@@ -160,10 +163,22 @@
 
     >>> print test_tales("ppa/fmt:link", ppa=mark.archive)
 
+Similarly, references to private PPAs are not rendered unless the user has
+a subscription to the PPA.
+
+    >>> login('[e-mail]')
+    >>> print test_tales("ppa/fmt:reference", ppa=mark.archive)
+
+    >>> login('[e-mail]')
+    >>> foo = getUtility(IPersonSet).getByName('name16')
+    >>> ignore = mark.archive.newSubscription(foo, mark)
+    >>> login('[e-mail]')
+    >>> print test_tales("ppa/fmt:reference", ppa=mark.archive)
+    ppa:mark/ppa
+
 We also have icons for builds which may have different dimensions.
 
     >>> from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
-    >>> login('[e-mail]')
     >>> stp = SoyuzTestPublisher()
     >>> stp.prepareBreezyAutotest()
     >>> source = stp.getPubSource()

=== modified file 'lib/canonical/launchpad/emailtemplates/ppa-subscription-new.txt'
--- lib/canonical/launchpad/emailtemplates/ppa-subscription-new.txt	2009-07-13 09:22:52 +0000
+++ lib/canonical/launchpad/emailtemplates/ppa-subscription-new.txt	2010-02-12 13:51:23 +0000
@@ -3,7 +3,9 @@
 Launchpad: access to a private archive
 --------------------------------------
 
-%(registrant_name)s has granted you access to a private software archive "%(ppa_name)s", which is hosted by Launchpad.
+%(registrant_name)s has granted you access to a private software archive "%(ppa_displayname)s" (%(ppa_reference)s), which is hosted by Launchpad and has the following description:
+
+%(ppa_description)s
 
 To start downloading and using software from this archive you need to view your access details by visiting this link:
 

=== modified file 'lib/canonical/launchpad/mailnotification.py'
--- lib/canonical/launchpad/mailnotification.py	2010-01-14 17:53:22 +0000
+++ lib/canonical/launchpad/mailnotification.py	2010-02-12 13:51:23 +0000
@@ -1203,9 +1203,13 @@
     """Notification that a new PPA subscription can be activated."""
     non_active_subscribers = subscription.getNonActiveSubscribers()
 
+    archive = subscription.archive
     registrant_name = subscription.registrant.displayname
-    ppa_name = subscription.archive.displayname
-    subject = 'PPA access granted for ' + ppa_name
+    ppa_displayname = archive.displayname
+    ppa_reference = "ppa:%s/%s" % (
+        archive.owner.name, archive.name)
+    ppa_description = archive.description
+    subject = 'PPA access granted for ' + ppa_displayname
 
     template = get_email_template('ppa-subscription-new.txt')
 
@@ -1222,7 +1226,9 @@
             'recipient_name': person.displayname,
             'registrant_name': registrant_name,
             'registrant_profile_url': canonical_url(subscription.registrant),
-            'ppa_name': ppa_name,
+            'ppa_displayname': ppa_displayname,
+            'ppa_reference': ppa_reference,
+            'ppa_description': ppa_description,
             'recipient_subscriptions_url': recipient_subscriptions_url,
             }
         body = MailWrapper(72).format(template % replacements,

=== modified file 'lib/canonical/launchpad/webapp/tales.py'
--- lib/canonical/launchpad/webapp/tales.py	2010-01-25 14:50:42 +0000
+++ lib/canonical/launchpad/webapp/tales.py	2010-02-12 13:51:23 +0000
@@ -45,6 +45,7 @@
 from lp.blueprints.interfaces.specification import ISpecification
 from lp.code.interfaces.branch import IBranch
 from lp.soyuz.interfaces.archive import ArchivePurpose, IPPA
+from lp.soyuz.interfaces.archivesubscriber import IArchiveSubscriberSet
 from canonical.launchpad.interfaces.launchpad import (
     IHasIcon, IHasLogo, IHasMugshot, IPrivacy)
 from lp.registry.interfaces.person import IPerson, IPersonSet
@@ -1589,6 +1590,13 @@
 
     _link_summary_template = '%(display_name)s'
     _link_permission = 'launchpad.View'
+    _reference_template = "ppa:%(owner_name)s/%(ppa_name)s"
+
+    final_traversable_names = {
+        'reference': 'reference',
+        }
+    final_traversable_names.update(
+        CustomizableFormatter.final_traversable_names)
 
     def _link_summary_values(self):
         """See CustomizableFormatter._link_summary_values."""
@@ -1618,6 +1626,24 @@
             else:
                 return ''
 
+    def reference(self, view_name=None, rootsite=None):
+        """Return the text PPA reference for a PPA."""
+        # XXX: noodles 2010-02-11 bug=336779: This following check
+        # should be replaced with the normal check_permission once
+        # permissions for archive subscribers has been resolved.
+        if self._context.private:
+            request = get_current_browser_request()
+            person = IPerson(request.principal)
+            subscriptions = getUtility(IArchiveSubscriberSet).getBySubscriber(
+                person, self._context)
+            if subscriptions.is_empty():
+                return ''
+
+        return self._reference_template % {
+            'owner_name': self._context.owner.name,
+            'ppa_name': self._context.name,
+            }
+
 
 class SpecificationBranchFormatterAPI(CustomizableFormatter):
     """Adapter for ISpecificationBranch objects to a formatted string."""
@@ -3112,7 +3138,7 @@
 
     traversable_names = {
         'link': 'link',
-        'url': 'url', 
+        'url': 'url',
         'displayname': 'displayname',
     }
 

=== modified file 'lib/lp/soyuz/browser/archive.py'
--- lib/lp/soyuz/browser/archive.py	2010-02-02 21:28:50 +0000
+++ lib/lp/soyuz/browser/archive.py	2010-02-12 13:51:23 +0000
@@ -561,17 +561,6 @@
         can_edit = check_permission('launchpad.Edit', self.context)
         return can_edit and len(disabled_dependencies) > 0
 
-    @property
-    def ppa_reference(self):
-        """PPA reference as supported by `dput` and `software-properties`.
-
-        :raises AssertionError: if the context `IArchive` is not a PPA.
-        :return: a `str` as 'ppa:%(ppa.owner.name)/%(ppa.name)'
-        """
-        assert self.context.is_ppa, (
-            'PPA reference should not be used for non-PPA archives.')
-        return 'ppa:%s/%s' % (self.context.owner.name, self.context.name)
-
     @cachedproperty
     def package_copy_requests(self):
         """Return any package copy requests associated with this archive."""

=== modified file 'lib/lp/soyuz/browser/tests/archive-views.txt'
--- lib/lp/soyuz/browser/tests/archive-views.txt	2010-02-02 21:28:50 +0000
+++ lib/lp/soyuz/browser/tests/archive-views.txt	2010-02-12 13:51:23 +0000
@@ -46,18 +46,6 @@
     >>> print copy_archive_view.archive_label
     Archive
 
-There is also the 'ppa_reference' property, which mimics the syntax
-supported by `dput` and `software-properties` Ubuntu tools.
-
-    >>> print ppa_archive_view.ppa_reference
-    ppa:cprov/ppa
-
-    >>> print copy_archive_view.ppa_reference
-    Traceback (most recent call last):
-    ...
-    AssertionError: PPA reference should not be used for non-PPA
-    archives.
-
 The ArchiveView is provides the html for the inline description
 editing widget.
 
@@ -492,7 +480,7 @@
     Packages in ...PPA for Celso Providelo...
 
     >>> print copy_archive_view.page_title
-    Packages in ...Copy archive intrepid-security-rebuild... 
+    Packages in ...Copy archive intrepid-security-rebuild...
 
 This view inherits from ArchiveViewBase and has all the
 corresponding properties such as archive_url, build_counters etc.

=== modified file 'lib/lp/soyuz/doc/archivesubscriber.txt'
--- lib/lp/soyuz/doc/archivesubscriber.txt	2009-12-24 01:41:54 +0000
+++ lib/lp/soyuz/doc/archivesubscriber.txt	2010-02-12 13:51:23 +0000
@@ -116,7 +116,10 @@
     --------------------------------------
     <BLANKLINE>
     Celso Providelo has granted you access to a private software archive
-    "PPA for Celso Providelo", which is hosted by Launchpad.
+    "PPA for Celso Providelo" (ppa:cprov/ppa), which is hosted by Launchpad
+    and has the following description:
+    <BLANKLINE>
+    packages to help my friends.
     <BLANKLINE>
     To start downloading and using software from this archive you need to
     view your access details by visiting this link:

=== modified file 'lib/lp/soyuz/stories/ppa/xx-private-ppa-subscription-stories.txt'
--- lib/lp/soyuz/stories/ppa/xx-private-ppa-subscription-stories.txt	2009-12-24 01:41:54 +0000
+++ lib/lp/soyuz/stories/ppa/xx-private-ppa-subscription-stories.txt	2010-02-12 13:51:23 +0000
@@ -197,8 +197,8 @@
     >>> joe_browser.open("http://launchpad.dev/~joesmith")
     >>> joe_browser.getLink('View your private PPA subscriptions').click()
     >>> print_subscriptions_for_person(joe_browser.contents)
-    Archive
-    PPA for Celso Providelo            View
+    Archive      Owner
+    PPA for ...  Celso Providelo  View
 
 When Joe clicks on the View button for Celso's PPA then the
 details of the subscription are displayed with the newly created
@@ -223,8 +223,9 @@
     >>> joe_browser.open(
     ...     "http://launchpad.dev/~joesmith/+archivesubscriptions")
     >>> print_subscriptions_for_person(joe_browser.contents)
-    Archive
-    PPA for Celso Providelo     View
+    Archive      Owner
+    PPA for ...  Celso Providelo  View
+
     >>> joe_browser.getLink('View').click()
     >>> print(extract_text(joe_browser.contents))
     Access to PPA for Celso Providelo...
@@ -244,13 +245,12 @@
     >>> print(extract_text(regeneration_info))
     Reset password
     If you believe...
-    Reset password
     Note: after ...
 
 When Joe clicks on the 'Generate new personal subscription' link then
 the page is redisplayed with new sources.list entries and a notification.
 
-    >>> joe_browser.getControl(name='regenerate').click()
+    >>> joe_browser.getControl(name='regenerate_btn').click()
     >>> for msg in get_feedback_messages(joe_browser.contents):
     ...     print msg
     Launchpad has generated the new password you requested for your
@@ -278,7 +278,7 @@
     >>> transaction.commit()
     >>> logout()
 
-When Mark, a member of the Launchpad team, visits his profile and clicks 
+When Mark, a member of the Launchpad team, visits his profile and clicks
 'View your private PPA subscriptions', then he'll see a list of his current
 subscriptions.
 
@@ -288,8 +288,8 @@
 
     >>> mark_browser.getLink('View your private PPA subscriptions').click()
     >>> print_subscriptions_for_person(mark_browser.contents)
-    Archive
-    PPA for Celso Providelo            View
+    Archive      Owner
+    PPA for ...  Celso Providelo  View
 
 When Mark clicks on the view button, then he is taken to the page for
 his personal subscription for Celso's private PPA and the newly-created
@@ -314,8 +314,8 @@
     >>> mark_browser.open(
     ...     "http://launchpad.dev/~mark/+archivesubscriptions")
     >>> print_subscriptions_for_person(mark_browser.contents)
-    Archive
-    PPA for Celso Providelo     View
+    Archive      Owner
+    PPA for ...  Celso Providelo  View
 
     >>> mark_browser.getLink('View').click()
     >>> print(extract_text(mark_browser.contents))

=== modified file 'lib/lp/soyuz/stories/ppa/xx-private-ppa-subscriptions.txt'
--- lib/lp/soyuz/stories/ppa/xx-private-ppa-subscriptions.txt	2009-09-30 14:15:39 +0000
+++ lib/lp/soyuz/stories/ppa/xx-private-ppa-subscriptions.txt	2010-02-12 13:51:23 +0000
@@ -138,9 +138,9 @@
     >>> for row in find_tags_by_class(joe_browser.contents,
     ...                               'archive-subscription-row'):
     ...     print extract_text(row)
-    Archive
-    PPA for Mark Shuttleworth    View
-    PPA for Celso Providelo      View
+    Archive                                     Owner
+    PPA for Mark Shuttleworth (ppa:mark/ppa)  Mark Shuttleworth       View
+    PPA for Celso Providelo (ppa:cprov/ppa)     Celso Providelo         View
 
 == Confirming a subscription ==
 

=== modified file 'lib/lp/soyuz/templates/archive-index.pt'
--- lib/lp/soyuz/templates/archive-index.pt	2010-02-10 14:19:23 +0000
+++ lib/lp/soyuz/templates/archive-index.pt	2010-02-12 13:51:23 +0000
@@ -84,7 +84,7 @@
         <p>You can update your system with unsupported packages from
           this untrusted PPA by <a href="/+help/soyuz/ppa-sources-list.html"
           target="help">adding</a> <strong
-          tal:content="view/ppa_reference">ppa:cprov/ppa</strong>
+          tal:content="context/fmt:reference">ppa:cprov/ppa</strong>
           to your system's Software Sources.
           <span id="pre-karmic-systems-slide-trigger"
             >Not using Ubuntu 9.10 (Karmic)?</span></p>
@@ -316,7 +316,7 @@
          spurious build failures or binaries with unexpected
          contents.</p>
       <p>You can upload packages to this PPA using:</p>
-      <p><tt>dput <tal:ppa-ref replace="view/ppa_reference"
+      <p><tt>dput <tal:ppa-ref replace="context/fmt:reference"
                     >ppa:cprov/ppa</tal:ppa-ref> &lt;source.changes&gt;</tt>
             (<a href="https://help.launchpad.net/Packaging/PPA/Uploading"
                >Read about uploading</a>)</p>

=== modified file 'lib/lp/soyuz/templates/person-archive-subscription.pt'
--- lib/lp/soyuz/templates/person-archive-subscription.pt	2009-09-14 08:44:08 +0000
+++ lib/lp/soyuz/templates/person-archive-subscription.pt	2010-02-12 13:51:23 +0000
@@ -55,9 +55,8 @@
          on this page. You'll need to update them on your computer.
       </p>
       <form action="" method="post">
-        <button type="submit" name="regenerate" value="1">
-          Reset password
-        </button>
+        <input type="hidden" name="regenerate" value="1" />
+        <input type="submit" name="regenerate_btn" value="Reset Password" />
       </form>
       <p class="discreet">
          Note: after resetting the password for your personal

=== modified file 'lib/lp/soyuz/templates/person-archive-subscriptions.pt'
--- lib/lp/soyuz/templates/person-archive-subscriptions.pt	2009-11-02 21:44:55 +0000
+++ lib/lp/soyuz/templates/person-archive-subscriptions.pt	2010-02-12 13:51:23 +0000
@@ -16,10 +16,11 @@
              been granted access are listed below.
           </p>
           <table summary="CONTEXTS/fmt:pagetitle" id="archive-subscriptions"
-            class="listing" style="width:70%">
+            class="listing">
             <thead>
               <tr class="archive-subscription-row">
                 <th>Archive</th>
+                <th>Owner</th>
                 <th></th>
               </tr>
             </thead>
@@ -29,8 +30,15 @@
                 <tal:subscription_and_token
                     define="subscription subscription_with_token/subscription;
                             token subscription_with_token/token">
-                <td tal:content="subscription/archive/displayname"
-                    class="ppa_display_name">Private PPA for Celso
+                <td class="ppa_display_name">
+                  <tal:display_name content="subscription/archive/displayname">
+                    Private PPA for Celso
+                  </tal:display_name>
+                  (<tal:reference content="subscription/archive/fmt:reference">
+                    ppa:cprov/ppa</tal:reference>)
+                </td>
+                <td><tal:owner content="structure subscription/archive/owner/fmt:link">
+                        Celso Providelo</tal:owner>
                 </td>
                 <td>
                   <tal:active condition="token">