Code review comment for lp:~michael.nelson/launchpad/409187-trivial-ui-fixes-for-p3a-access

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.

 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?

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

> + """
> + 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. :)

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

> + 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)

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.

> +
> + // 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>
>