Guilherme Salgado wrote:
> Review: Approve
> Hi Michael,
> 
> Your changes look good.  I have a few suggestions and I'd like somebody
> else to review the JS changes, as I don't feel comfortable doing so.
> 

Great, thanks Salgado.

>  review approve
> 
>> === modified file 'lib/lp/soyuz/browser/archivesubscription.py'
>> --- lib/lp/soyuz/browser/archivesubscription.py	2009-08-12 08:40:41 +0000
>> +++ lib/lp/soyuz/browser/archivesubscription.py	2009-09-14 08:08:51 +0000
>> @@ -9,6 +9,8 @@
>>  
>>  __all__ = [
>>      'ArchiveSubscribersView',
>> +    'PersonalArchiveSubscriptionBreadcrumb',
>> +    'PersonArchiveSubscriptionView',
>>      'PersonArchiveSubscriptionsView',
>>      'traverse_archive_subscription_for_subscriber'
>>      ]
>> @@ -34,10 +36,11 @@
>>      IArchiveAuthTokenSet)
>>  from lp.soyuz.interfaces.archivesubscriber import (
>>      IArchiveSubscriberSet, IPersonalArchiveSubscription)
>> +from canonical.launchpad.webapp.breadcrumb import Breadcrumb
>>  from canonical.launchpad.webapp.launchpadform import (
>>      action, custom_widget, LaunchpadFormView, LaunchpadEditFormView)
>>  from canonical.launchpad.webapp.publisher import (
>> -    canonical_url, LaunchpadView)
>> +    canonical_url, LaunchpadView, Navigation)
>>  from canonical.widgets import DateWidget
>>  from canonical.widgets.popup import PersonPickerWidget
>>  
>> @@ -65,6 +68,20 @@
>>          """See `IPersonalArchiveSubscription`."""
>>          return "Access to %s" % self.archive.displayname
>>  
>> +    @property
>> +    def title(self):
> 
> I know of page_title and label, which are used for this purpose, did you mean
> one of them or am I not aware of this third one?

No, the helper class to which this belongs (PersonalArchiveSubscription)
is used as the context for these views, and so like normal content
classes, it needs a title attribute for when the base template tries to
access context.title.

> 
>> +        """Required for default headings in templates."""
>> +        return self.displayname
>> +
>> +
>> +class PersonalArchiveSubscriptionNavigation(Navigation):
>> +    """Navigation for `IPersonalArchiveSubscription`.
>> +
>> +    Without this, a breadcrumb will not be created for personal
>> +    archive subscription objects.
> 
> This is no longer needed, actually -- a fix for the bug (423898) is included
> in my breadcrumbs branch that is on PQM now.

Great, removed.

> 
>> +    """
>> +    usedfor = IPersonalArchiveSubscription
>> +
>>  def traverse_archive_subscription_for_subscriber(subscriber, archive_id):
>>      """Return the subscription for a subscriber to an archive."""
>>      subscription = None
>> @@ -79,6 +96,14 @@
>>          return PersonalArchiveSubscription(subscriber, archive)
>>  
>>  
>> +class PersonalArchiveSubscriptionBreadcrumb(Breadcrumb):
>> +    """Builds a breadcrumb for `PersonalArchiveSubscription`."""
>> +
>> +    @property
>> +    def text(self):
>> +        return self.context.displayname
> 
> You can use webapp.breadcrumbs.DisplaynameBreadcrumb in the zcml and get rid
> of this adapter altogether.  It'd be great if you could do a quick check for
> other places where that generic adapter could replace other specific ones. :)

Done - there was only one other that I could replace in the
soyuz/configure with a generic breadcrumb. Thanks!


> 
>> +
>> +
>>  class IArchiveSubscriberUI(Interface):
>>      """A custom interface for user interaction with archive subscriptions.
>>  
>> === modified file 'lib/lp/soyuz/browser/tests/test_breadcrumbs.py'
>> --- lib/lp/soyuz/browser/tests/test_breadcrumbs.py	2009-09-02 16:32:06 +0000
>> +++ lib/lp/soyuz/browser/tests/test_breadcrumbs.py	2009-09-11 10:52:54 +0000
>> @@ -11,6 +11,8 @@
>>  from canonical.launchpad.webapp.tests.breadcrumbs import (
>>      BaseBreadcrumbTestCase)
>>  from lp.registry.interfaces.distribution import IDistributionSet
>> +from lp.soyuz.browser.archivesubscription import PersonalArchiveSubscription
>> +from lp.testing import login, login_person
>>  
>>  
>>  class TestDistroArchSeriesBreadcrumb(BaseBreadcrumbTestCase):
>> @@ -55,5 +57,38 @@
>>          self.assertEquals(texts[-1], "0.1-1")
>>  
>>  
>> +class TestArchiveSubscriptionBreadcrumb(BaseBreadcrumbTestCase):
>> +
>> +    def setUp(self):
>> +        super(TestArchiveSubscriptionBreadcrumb, self).setUp()
>> +
>> +        # Create a private ppa
>> +        self.ppa = self.factory.makeArchive()
>> +        login('<email address hidden>')
>> +        self.ppa.private = True
>> +        self.ppa.buildd_secret = 'secret'
>> +
>> +        owner = self.ppa.owner
>> +        login_person(owner)
>> +        self.ppa_subscription = self.ppa.newSubscription(owner, owner)
>> +        self.ppa_token = self.ppa.newAuthToken(owner)
>> +        self.personal_archive_subscription = PersonalArchiveSubscription(
>> +            owner, self.ppa)
>> +
>> +    def test_personal_archive_subscription(self):
>> +
> 
> I find it weird to have a blank line here, although I do that for class
> definitions.

Gone.

> 
>> +        self.traversed_objects = [
>> +            self.root, self.ppa.owner, self.personal_archive_subscription]
>> +        subscription_url = canonical_url(self.personal_archive_subscription)
>> +
>> +        urls = self._getBreadcrumbsURLs(
>> +            subscription_url, self.traversed_objects)
>> +        texts = self._getBreadcrumbsTexts(
>> +            subscription_url, self.traversed_objects)
>> +
>> +        self.assertEquals(subscription_url, urls[-1])
>> +        self.assertEquals(
>> +            "Access to %s" % self.ppa.displayname, texts[-1])
>> +
>>  def test_suite():
>>      return unittest.TestLoader().loadTestsFromName(__name__)
>>
>> === modified file 'lib/lp/soyuz/templates/archive-subscribers.pt'
>> --- lib/lp/soyuz/templates/archive-subscribers.pt	2009-08-05 10:02:22 +0000
>> +++ lib/lp/soyuz/templates/archive-subscribers.pt	2009-09-18 08:09:23 +0000
>> @@ -12,22 +12,19 @@
>>        use-macro="context/@@launchpad_widget_macros/yui2calendar-dependencies" />
>>    </metal:block>
>>  
>> -  <div metal:fill-slot="heading">
>> -    <h1 tal:content="CONTEXTS/fmt:pagetitle">
>> -      Manage access to Blah PPA
>> -    </h1>
>> -  </div>
>>    <div metal:fill-slot="main">
>>      <div class="top-portlet">
>>        <p>You can grant access to people or teams to install software
>>           from your PPA.
>>        </p>
>>  
>> -      <div class="subscribers">
>> -        <p tal:condition="not: view/has_subscriptions" id="no-subscribers">
>> -          No one has access to install software from this PPA.
>> -        </p>
>> -
>> +      <p tal:condition="not: view/has_subscriptions" id="no-subscribers">
>> +        No one has access to install software from this PPA.
>> +      </p>
>> +
>> +      <div id="add-subscriber-placeholder"></div>
>> +
>> +      <div id="subscribers">
>>          <p class="error message" tal:condition="view/errors"
>>             tal:content="view/error_count" />
>>  
>> @@ -44,13 +41,13 @@
>>                 id="archive-subscribers" class="listing">
>>            <thead>
>>              <tr class="archive_subscriber_row">
>> -              <th>Name</th>
>> +              <th style="width:30%">Name</th>
>>                <th>Expires</th>
>>                <th colspan="2">Comment</th>
>>              </tr>
>>            </thead>
>>            <tbody>
>> -            <tr>
>> +            <tr class="add-subscriber" style="background-color:#eeeeff;">
>>                <tal:single-row-form define="widgets widgets|view/widgets"
>>                                     repeat="widget widgets">
>>  
>> @@ -96,6 +93,49 @@
>>          </form>
>>        </div><!-- class="portlet" -->
>>      </div>
>> +    <script type="text/javascript">
>> +YUI({
>> +        base: '../../lib/yui/current/build/',
>> +        filter: 'raw'
>> +    }).use('node', 'event', 'lazr.effects', function(Y) {
>> +
>> +    // If there are no errors then we hide the add-subscriber row and
>> +    // potentially the whole table if there are no subscribers.
>> +    if (Y.Lang.isNull(Y.get('p.error.message'))) {
>> +
>> +        // Hide the add-subscriber row.
>> +        var add_subscriber_row = Y.get(
>> +            '#archive-subscribers .add-subscriber');
>> +        add_subscriber_row.setStyle('display', 'none');
>> +
>> +        // If there are no subscribers, then hide the complete section.
>> +        var subscribers = Y.get('#subscribers');
>> +        if (Y.Lang.isObject(Y.get('#no-subscribers'))) {
>> +            subscribers.setStyle('display', 'none');
>> +        }
>> +    }
>> +
>> +    // Add a link to open the add-subscriber row.
>> +    var placeholder = Y.get('#add-subscriber-placeholder');
>> +    placeholder.set(
>> +        'innerHTML', '<a class="js-action sprite add" href="#" />');
>> +    var add_subscriber_node = Y.get("#add-subscriber-placeholder a");
>> +    add_subscriber_node.set('innerHTML', 'Add access');
> 
> Do you really need to use JS to set the innerHTML of the <a> tag?  Can't you
> do something like
> 
>     placeholder.set('innerHTML', '<a class="...">Add access</a>');
> 
> ?
> 
> (Note I haven't done any JS hacking since the epic, so I may have
> misunderstood what the code is doing)

Yes, you can, but not in a pt file otherwise it's not valid xml... and I
was too lazy to extract the js code to a separate file.

I've now moved it to a separate file and simplified the node creation,
and added unit tests, which you can run with:

firefox-3.5
lib/canonical/launchpad/javascript/soyuz/tests/archivesubscribers_index.html

I've also added an intergration test which can be run with:

bin/test --layer=SoyuzWindmillLayer

> 
> And btw, it'd be nice if you could get a second pair of eyes to have a look at
> this, as I don't feel comfortable reviewing something I wouldn't know how to
> write.

Will do. Thanks Guillherme!


> 
>> +
>> +    // Unfortunately we can't use the lazr slider, as it uses display:block
>> +    // which breaks table rows (they use display:table-row).
>> +    function show_add_subscriber(e) {
>> +        e.preventDefault();
>> +        subscribers.setStyle('display', 'block');
>> +        add_subscriber_row.setStyle('display', 'table-row');
>> +    }
>> +
>> +    Y.on('click', show_add_subscriber,
>> +         '#add-subscriber-placeholder a');
>> +
>> +});
>> +    </script>
>> +
>>  
>>    </div>
>>  </body>
>>
> 
> 


-- 
Michael