Merge lp:~michael.nelson/launchpad/644328-blacklist-ui into lp:launchpad/db-devel

[Michael Nelson]

Adds UI for blacklisting distroseriesdifferences.

Overview
========

This branch implements the UI for blacklisting source package differences for derived distro series.

See the LEP here:
https://dev.launchpad.net/LEP/DerivativeDistributions

and the specific mockup here:
https://dev.launchpad.net/LEP/DerivativeDistributions?action=AttachFile&do=get&target=derived-series-diffs_uiround2.png

You can view a 1min demo of the UI here:

http://people.canonical.com/~michaeln/tmp/644328-blacklist-ui.ogv

updated after salgado's ui review to:
http://people.canonical.com/~michaeln/tmp/644328-review-salgado.ogv

(Note: the issue I mention about the default radio not being selected in chromium was resolved by wrapping the inputs in an empty form element).

To test
=======
bin/test -vv -m test_servers -m test_distroseriesdifference_views -m test_distroseriesdifference_webservice -m test_distroseriesdifference_expander

To demo locally:
================
Run http://pastebin.ubuntu.com/494656/ in bin/iharness and then open
https://launchpad.dev/ubuntu/hoary/+localpackagediffs

[Michael Nelson (michael.nelson)]

I forgot to mention, I've ventured slightly from that mockup after some earlier discussions with Henning... that's why 'Add to blacklist' on the mockup has been replaced by two separate links.

[Guilherme Salgado (salgado)]

<salgado> noodles775, I like the graying out because it makes it easy to see everything that was changed at a glance and maybe undo, but the text (as well as the links) is a bit too verbose (IMHO) and don't allow switching between all different states, so I was thinking of using 3 radio buttons there. "Blacklisted: (*) No, ( ) This version, ( ) All versions"
<salgado> but that's more like a nice-to-have thing, not that important
<noodles775> salgado: that actually sounds a bit like what I'd proposed on the mockup (a single 'Add to blacklist' that would then give you those options), but henninge convinced me on a previous MP that it was a bit too much... hangon, I'll find the discussion.
<salgado> well, except that the radio buttons allow you to see all the possible states without having to click anything
<salgado> just like the links do
<salgado> but sure, maybe he'll be able to convince me as well. :)
<noodles775> salgado: ok - if you search for blacklist at https://code.edge.launchpad.net/~michael.nelson/launchpad/distro-series-difference-browser2/+merge/34739 you'll see a bit of background.
<noodles775> salgado: but I think your radio button option you mention could work too... Also, if you've got suggestions for making the text less verbose while still being easy to understand, let me know :)
<noodles775> s/you/that you/
<salgado> what I like about the radio button is that you see all the options without having to click anything and there's no repetition
<salgado> the downside is that you need vertical real estate, but you seem to have plenty of that
<salgado> as for the text of the link, the only thing I can think of is something like "Blacklist: _this version_ or _all versions_"
<salgado> but I don't quite like that as it seems to suggest you need to blacklist something
<noodles775> salgado: Yeah. RE the radio buttons, I was going to say that it would require two clicks (ie select/submit), but it doesn't of course... we could do the api update when an option is selected quite easily. And that would mean no need for 'Undo'. I like it more and more :)
<salgado> indeed, I hadn't thought about the undo

[Curtis Hovey (sinzui)]

I radio button the suggestion since it provides context. If I blacklist all version, I can still see an option to black list one version (and know I have made a mistake). This assumes there are three buttons and each fires an ajax event on change.

I think we have a "Undo" problem in Lp. It needs a standard presentation, an icon and rules about where is goes (like "or _Cancel_") so that users know what to expect. As Henning remarked in the other review, the text of the action will vary in size and that could make the list of several open rows hard to scan.

[Michael Nelson (michael.nelson)]

Hi Guilherme,

Here's an updated screencast based on your UI review:
http://people.canonical.com/~michaeln/tmp/644328-review-salgado.ogv

Let me know if it's what you were thinking.

[Guilherme Salgado (salgado)]

That works really nice, thanks for doing it, Michael. It'd be important to try and make the default radio button be initially selected on chromium as well, but if it turns out to be too tricky, maybe you can just file a bug for now.

[Guilherme Salgado (salgado)]

review: Approve (ui*)

[Curtis Hovey (sinzui)]

This looks great. Thanks Michael and Salgado for doing such a good job on this.

review: Approve (ui)

[Henning Eggers (henninge)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Michael,
thank you for this cool feature. I don't see any problems with your branch
just a few suggestions some of which we already discussed in IRC

 review approve code

Cheers,
Henning

> === modified file 'lib/canonical/launchpad/icing/style-3-0.css.in'
> === modified file 'lib/canonical/launchpad/webapp/interfaces.py'
> === modified file 'lib/canonical/launchpad/webapp/servers.py'
> --- lib/canonical/launchpad/webapp/servers.py 2010-09-22 15:17:39 +0000
> +++ lib/canonical/launchpad/webapp/servers.py 2010-09-27 15:03:26 +0000
> @@ -526,7 +526,27 @@
> return decoded_qs
>
>
> -class BasicLaunchpadRequest:
> +class LaunchpadBrowserRequestMixin:
> + """A mixin for classes that share some method implementations."""

We already talked about how senseless this comment is ... Can you please
improve it?

> +
> + def getRootURL(self, rootsite):
> + """See IBasicLaunchpadRequest."""
> + if rootsite is not None:
> + assert rootsite in allvhosts.configs, (
> + "rootsite is %s. Must be in %r." % (
> + rootsite, sorted(allvhosts.configs.keys())))
> + root_url = allvhosts.configs[rootsite].rooturl
> + else:
> + root_url = self.getApplicationURL() + '/'
> + return root_url
> +
> + @property
> + def is_ajax(self):
> + """See `IBasicLaunchpadRequest`."""
> + return 'XMLHttpRequest' == self.getHeader('HTTP_X_REQUESTED_WITH')

I was just wondering: How much of a standard is the "X-Requested-With" header?
Is there a fall-back if browser does not send it?

> +
> +
> +class BasicLaunchpadRequest(LaunchpadBrowserRequestMixin):
> """Mixin request class to provide stepstogo."""
>
> implements(IBasicLaunchpadRequest)
> @@ -581,24 +601,8 @@
> return get_query_string_params(self)
>
>
> -class LaunchpadBrowserRequestMixin:
> - """A mixin for classes that share some method implementations."""
> -
> - def getRootURL(self, rootsite):
> - """See IBasicLaunchpadRequest."""
> - if rootsite is not None:
> - assert rootsite in allvhosts.configs, (
> - "rootsite is %s. Must be in %r." % (
> - rootsite, sorted(allvhosts.configs.keys())))
> - root_url = allvhosts.configs[rootsite].rooturl
> - else:
> - root_url = self.getApplicationURL() + '/'
> - return root_url
> -
> -
> class LaunchpadBrowserRequest(BasicLaunchpadRequest, BrowserRequest,
> - NotificationRequest, ErrorReportRequest,
> - LaunchpadBrowserRequestMixin):
> + NotificationRequest, ErrorReportRequest):
> """Integration of launchpad mixin request classes to make an uber
> launchpad request class.
> """
> @@ -1370,7 +1374,7 @@
>
>
> class PublicXMLRPCRequest(BasicLaunchpadRequest, XMLRPCRequest,
> - ErrorReportRequest, LaunchpadBrowserRequestMixin):
> + ErrorReportRequest):
> """Request type for doing public XML-RPC in Launchpad."""
>
> def _createResponse(self):
>
...

review: Approve (code)

[Michael Nelson (michael.nelson)]

On Mon, Sep 27, 2010 at 7:49 PM, Henning Eggers
<email address hidden> wrote:
> Review: Approve code
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Hi Michael,
> thank you for this cool feature. I don't see any problems with your branch
> just a few suggestions some of which we already discussed in IRC

Cheers Henninge, comments below.

>> === modified file 'lib/canonical/launchpad/webapp/servers.py'
>> --- lib/canonical/launchpad/webapp/servers.py 2010-09-22 15:17:39 +0000
>> +++ lib/canonical/launchpad/webapp/servers.py 2010-09-27 15:03:26 +0000
>> @@ -526,7 +526,27 @@
>>      return decoded_qs
>>
>>
>> -class BasicLaunchpadRequest:
>> +class LaunchpadBrowserRequestMixin:
>> +    """A mixin for classes that share some method implementations."""
>
> We already talked about how senseless this comment is ... Can you please
> improve it?

Yep - I didn't write it (just moved it), but have updated it to:

"""Provides methods used for both API and web browser requests."""

>> +    @property
>> +    def is_ajax(self):
>> +        """See `IBasicLaunchpadRequest`."""
>> +        return 'XMLHttpRequest' == self.getHeader('HTTP_X_REQUESTED_WITH')
>
> I was just wondering: How much of a standard is the "X-Requested-With" header?
> Is there a fall-back if browser does not send it?

It is sent by most ajax libraries when sending xhr requests. The above
implementation matches Django's implementation.

>> === modified file 'lib/canonical/launchpad/webapp/tests/test_servers.py'
>> --- lib/canonical/launchpad/webapp/tests/test_servers.py      2010-08-20 20:31:18 +0000
>> +++ lib/canonical/launchpad/webapp/tests/test_servers.py      2010-09-27 15:03:26 +0000
>> @@ -358,6 +358,16 @@
>>              retried_request.response.getHeader('Vary'),
>>              'Cookie, Authorization')
>>
>> +    def test_is_ajax(self):
>
> Please split this test up:
>       def test_is_not_ajax(self):

Done (test_is_ajax_false).

>>      implements(IConversation)
>> +    schema = IDistroSeriesDifferenceForm
>> +    custom_widget('blacklist_options', RadioWidget)
>> +
>> +    @property
>> +    def initial_values(self):
>> +        """Ensure the correct radio button is checked for blacklisting."""
>> +        if self.context.status in (
>> +            DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT,
>> +            DistroSeriesDifferenceStatus.BLACKLISTED_ALWAYS,
>> +            ):
>
> My feel is that multi-line if statements are to be avoided at all costs. ;)
> They just read badly. You can construct the tuple first and use that in the if
> statement.

OK, that's much better.
>> +    def test_show_blacklist_options(self):
>> +        # Blacklist options are shown if requested by an editor via
>> +        # ajax.
>> +        ds_diff = self.factory.makeDistroSeriesDifference()
>> +
>> +        # Without JS, even editors don't see blacklist options.
>> +        with person_logged_in(ds_diff.owner):
>> +            view = create_initialized_view(
>> +                ds_diff, '+listing-distroseries-extra')
>> +        self.assertFalse(view.show_blacklist_options)
>
> Yes, as you mentioned, please split this test up as well.

Done - it's now 3 tests.

>> +    def test_bl...

=== modified file 'lib/canonical/launchpad/icing/style-3-0.css.in'
--- lib/canonical/launchpad/icing/style-3-0.css.in	2010-09-21 16:02:23 +0000
+++ lib/canonical/launchpad/icing/style-3-0.css.in	2010-09-28 07:13:54 +0000
@@ -2419,7 +2419,7 @@
     background-repeat: no-repeat;
     background-position:right center;
     }
-table#packages_list tr.superseded {
+table#packages_list tr.superseded, tr.blacklisted {
     background-color: #eee;
     }
 ul.latest-ppa-updates li:nth-child(odd) {

=== modified file 'lib/canonical/launchpad/webapp/interfaces.py'
--- lib/canonical/launchpad/webapp/interfaces.py	2010-09-12 11:43:36 +0000
+++ lib/canonical/launchpad/webapp/interfaces.py	2010-09-28 07:13:54 +0000
@@ -335,6 +335,10 @@
     query_string_params = Attribute(
         'A dictionary of the query string parameters.')
 
+    is_ajax = Bool(
+        title=_('Is ajax'), required=False, readonly=True,
+        description=_("Indicates whether the request is an XMLHttpRequest."))
+
     def getRootURL(rootsite):
         """Return this request's root URL.
 

=== modified file 'lib/canonical/launchpad/webapp/servers.py'
--- lib/canonical/launchpad/webapp/servers.py	2010-09-22 15:17:39 +0000
+++ lib/canonical/launchpad/webapp/servers.py	2010-09-28 07:13:54 +0000
@@ -526,7 +526,27 @@
     return decoded_qs
 
 
-class BasicLaunchpadRequest:
+class LaunchpadBrowserRequestMixin:
+    """Provides methods used for both API and web browser requests."""
+
+    def getRootURL(self, rootsite):
+        """See IBasicLaunchpadRequest."""
+        if rootsite is not None:
+            assert rootsite in allvhosts.configs, (
+                "rootsite is %s.  Must be in %r." % (
+                    rootsite, sorted(allvhosts.configs.keys())))
+            root_url = allvhosts.configs[rootsite].rooturl
+        else:
+            root_url = self.getApplicationURL() + '/'
+        return root_url
+
+    @property
+    def is_ajax(self):
+        """See `IBasicLaunchpadRequest`."""
+        return 'XMLHttpRequest' == self.getHeader('HTTP_X_REQUESTED_WITH')
+
+
+class BasicLaunchpadRequest(LaunchpadBrowserRequestMixin):
     """Mixin request class to provide stepstogo."""
 
     implements(IBasicLaunchpadRequest)
@@ -581,24 +601,8 @@
         return get_query_string_params(self)
 
 
-class LaunchpadBrowserRequestMixin:
-    """A mixin for classes that share some method implementations."""
-
-    def getRootURL(self, rootsite):
-        """See IBasicLaunchpadRequest."""
-        if rootsite is not None:
-            assert rootsite in allvhosts.configs, (
-                "rootsite is %s.  Must be in %r." % (
-                    rootsite, sorted(allvhosts.configs.keys())))
-            root_url = allvhosts.configs[rootsite].rooturl
-        else:
-            root_url = self.getApplicationURL() + '/'
-        return root_url
-
-
 class LaunchpadBrowserRequest(BasicLaunchpadRequest, BrowserRequest,
-                              NotificationRequest, ErrorReportRequest,
-                              LaunchpadBrowserRequestMixin):
+                              NotificationRequest, ErrorReportRequest):
     """Integration of launchpad mixin request classes to make an uber
     launchpad request class.
     """
@@ -1370,7 +1374,7 @@
 
 
 class PublicXMLRPCRequest(BasicLaunchpadRequest, XMLRPCRequest,
-                          ErrorReportRequest, LaunchpadBrowserRequestMixin):
+                          ErrorReportRequest):
     """Request type for doing public XML-RPC in Launchpad."""
 
     def _createResponse(self):

=== modified file 'lib/canonical/launchpad/webapp/tests/test_servers.py'
--- lib/canonical/launchpad/webapp/tests/test_servers.py	2010-08-20 20:31:18 +0000
+++ lib/canonical/launchpad/webapp/tests/test_servers.py	2010-09-28 07:13:54 +0000
@@ -358,6 +358,20 @@
             retried_request.response.getHeader('Vary'),
             'Cookie, Authorization')
 
+    def test_is_ajax_false(self):
+        """Normal requests do not define HTTP_X_REQUESTED_WITH."""
+        request = LaunchpadBrowserRequest(StringIO.StringIO(''), {})
+
+        self.assertFalse(request.is_ajax)
+
+    def test_is_ajax_true(self):
+        """Requests with HTTP_X_REQUESTED_WITH set are ajax requests."""
+        request = LaunchpadBrowserRequest(StringIO.StringIO(''), {
+            'HTTP_X_REQUESTED_WITH': 'XMLHttpRequest',
+            })
+
+        self.assertTrue(request.is_ajax)
+
 
 class IThingSet(Interface):
     """Marker interface for a set of things."""

=== modified file 'lib/lp/registry/browser/distroseriesdifference.py'
--- lib/lp/registry/browser/distroseriesdifference.py	2010-09-17 09:48:32 +0000
+++ lib/lp/registry/browser/distroseriesdifference.py	2010-09-28 07:13:54 +0000
@@ -8,18 +8,59 @@
     'DistroSeriesDifferenceView',
     ]
 
-from zope.interface import implements
+from zope.app.form.browser.itemswidgets import RadioWidget
+from zope.interface import (
+    implements,
+    Interface,
+    )
+from zope.schema import Choice
+from zope.schema.vocabulary import (
+    SimpleTerm,
+    SimpleVocabulary,
+    )
 
-from canonical.launchpad.webapp.publisher import LaunchpadView
+from canonical.launchpad.webapp import LaunchpadFormView
+from canonical.launchpad.webapp.authorization import check_permission
+from canonical.launchpad.webapp.launchpadform import custom_widget
+from lp.registry.enum import DistroSeriesDifferenceStatus
 from lp.services.comments.interfaces.conversation import (
     IComment,
     IConversation,
     )
 
 
-class DistroSeriesDifferenceView(LaunchpadView):
+class IDistroSeriesDifferenceForm(Interface):
+    """An interface used in the browser only for displaying form elements."""
+    blacklist_options = Choice(vocabulary=SimpleVocabulary((
+        SimpleTerm('NONE', 'NONE', 'No'),
+        SimpleTerm(
+            DistroSeriesDifferenceStatus.BLACKLISTED_ALWAYS,
+            DistroSeriesDifferenceStatus.BLACKLISTED_ALWAYS.name,
+            'All versions'),
+        SimpleTerm(
+            DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT,
+            DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT.name,
+            'This version'),
+        )))
+
+
+class DistroSeriesDifferenceView(LaunchpadFormView):
 
     implements(IConversation)
+    schema = IDistroSeriesDifferenceForm
+    custom_widget('blacklist_options', RadioWidget)
+
+    @property
+    def initial_values(self):
+        """Ensure the correct radio button is checked for blacklisting."""
+        blacklisted_statuses = (
+            DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT,
+            DistroSeriesDifferenceStatus.BLACKLISTED_ALWAYS,
+            )
+        if self.context.status in blacklisted_statuses:
+            return dict(blacklist_options=self.context.status)
+
+        return dict(blacklist_options='NONE')
 
     @property
     def binary_summaries(self):
@@ -40,11 +81,16 @@
     @property
     def comments(self):
         """See `IConversation`."""
-        # Could use a generator here?
         return [
             DistroSeriesDifferenceDisplayComment(comment) for
                 comment in self.context.getComments()]
 
+    @property
+    def show_blacklist_options(self):
+        """Only show the options if an editor requests via JS."""
+        return self.request.is_ajax and check_permission(
+            'launchpad.Edit', self.context)
+
 
 class DistroSeriesDifferenceDisplayComment:
     """Used simply to provide `IComment` for rendering."""

=== modified file 'lib/lp/registry/browser/tests/test_distroseriesdifference_views.py'
--- lib/lp/registry/browser/tests/test_distroseriesdifference_views.py	2010-09-17 09:48:32 +0000
+++ lib/lp/registry/browser/tests/test_distroseriesdifference_views.py	2010-09-28 07:13:54 +0000
@@ -10,6 +10,7 @@
 from BeautifulSoup import BeautifulSoup
 from zope.component import getUtility
 
+from canonical.launchpad.webapp.servers import LaunchpadTestRequest
 from canonical.launchpad.webapp.testing import verifyObject
 from canonical.testing import (
     DatabaseFunctionalLayer,
@@ -18,14 +19,17 @@
 from lp.registry.browser.distroseriesdifference import (
     DistroSeriesDifferenceDisplayComment,
     )
-from lp.registry.enum import DistroSeriesDifferenceType
-from lp.registry.interfaces.distroseriesdifference import IDistroSeriesDifferenceSource
+from lp.registry.enum import (
+    DistroSeriesDifferenceStatus,
+    DistroSeriesDifferenceType,
+    )
+from lp.registry.interfaces.distroseriesdifference import (
+    IDistroSeriesDifferenceSource)
 from lp.services.comments.interfaces.conversation import (
     IComment,
     IConversation,
     )
 from lp.soyuz.enums import PackagePublishingStatus
-from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
 from lp.testing import (
     celebrity_logged_in,
     person_logged_in,
@@ -57,6 +61,8 @@
 
     def addSummaryToDifference(self, distro_series_difference):
         """Helper that adds binaries with summary info to the source pubs."""
+        # Avoid circular import.
+        from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
         distro_series = distro_series_difference.derived_series
         source_package_name_str = (
             distro_series_difference.source_package_name.name)
@@ -124,6 +130,36 @@
         self.assertIs(None, ds_diff.source_pub)
         self.assertIs(None, view.binary_summaries)
 
+    def test_show_blacklist_options_non_ajax(self):
+        # Blacklist options are not shown for non-ajax requests.
+        ds_diff = self.factory.makeDistroSeriesDifference()
+
+        # Without JS, even editors don't see blacklist options.
+        with person_logged_in(ds_diff.owner):
+            view = create_initialized_view(
+                ds_diff, '+listing-distroseries-extra')
+        self.assertFalse(view.show_blacklist_options)
+
+    def test_show_blacklist_options_editor(self):
+        # Blacklist options are shown if requested by an editor via
+        # ajax.
+        ds_diff = self.factory.makeDistroSeriesDifference()
+
+        request = LaunchpadTestRequest(HTTP_X_REQUESTED_WITH='XMLHttpRequest')
+        with person_logged_in(ds_diff.owner):
+            view = create_initialized_view(
+                ds_diff, '+listing-distroseries-extra', request=request)
+            self.assertTrue(view.show_blacklist_options)
+
+    def test_show_blacklist_options_non_editor(self):
+        # Even with a JS request, non-editors do not see the options.
+        ds_diff = self.factory.makeDistroSeriesDifference()
+
+        request = LaunchpadTestRequest(HTTP_X_REQUESTED_WITH='XMLHttpRequest')
+        view = create_initialized_view(
+            ds_diff, '+listing-distroseries-extra', request=request)
+        self.assertFalse(view.show_blacklist_options)
+
 
 class DistroSeriesDifferenceTemplateTestCase(TestCaseWithFactory):
 
@@ -215,3 +251,43 @@
             1, len(soup.findAll('pre', text="I'm working on this.")))
         self.assertEqual(
             1, len(soup.findAll('pre', text="Here's another comment.")))
+
+    def test_blacklist_options(self):
+        # blacklist options are presented to editors.
+        ds_diff = self.factory.makeDistroSeriesDifference()
+
+        with person_logged_in(ds_diff.owner):
+            request = LaunchpadTestRequest(
+                HTTP_X_REQUESTED_WITH='XMLHttpRequest')
+            view = create_initialized_view(
+                ds_diff, '+listing-distroseries-extra', request=request)
+            soup = BeautifulSoup(view())
+
+        self.assertEqual(
+            1, len(soup.findAll('div', {'class': 'blacklist-options'})))
+
+    def test_blacklist_options_initial_values_NONE(self):
+        ds_diff = self.factory.makeDistroSeriesDifference()
+        view = create_initialized_view(ds_diff, '+listing-distroseries-extra')
+
+        # If the difference is not currently blacklisted, 'NONE' is set
+        # as the default value for the field.
+        self.assertEqual('NONE', view.initial_values.get('blacklist_options'))
+
+    def test_blacklist_options_initial_values_CURRENT(self):
+        ds_diff = self.factory.makeDistroSeriesDifference(
+            status=DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT)
+        view = create_initialized_view(ds_diff, '+listing-distroseries-extra')
+
+        self.assertEqual(
+            DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT,
+            view.initial_values.get('blacklist_options'))
+
+    def test_blacklist_options_initial_values_ALWAYS(self):
+        ds_diff = self.factory.makeDistroSeriesDifference(
+            status=DistroSeriesDifferenceStatus.BLACKLISTED_ALWAYS)
+        view = create_initialized_view(ds_diff, '+listing-distroseries-extra')
+
+        self.assertEqual(
+            DistroSeriesDifferenceStatus.BLACKLISTED_ALWAYS,
+            view.initial_values.get('blacklist_options'))

=== modified file 'lib/lp/registry/browser/tests/test_distroseriesdifference_webservice.py'
--- lib/lp/registry/browser/tests/test_distroseriesdifference_webservice.py	2010-09-21 11:17:25 +0000
+++ lib/lp/registry/browser/tests/test_distroseriesdifference_webservice.py	2010-09-28 07:13:54 +0000
@@ -59,3 +59,28 @@
         self.assertEqual(
             DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT,
             ds_diff.status)
+
+    def test_unblacklist_not_public(self):
+        # The unblacklist method is not publically available.
+        ds_diff = self.factory.makeDistroSeriesDifference(
+            status=DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT)
+        ws_diff = ws_object(self.factory.makeLaunchpadService(), ds_diff)
+
+        self.assertRaises(Unauthorized, ws_diff.unblacklist)
+
+    def test_unblacklist(self):
+        # The unblacklist method can be called by people with edit access.
+        ds_diff = self.factory.makeDistroSeriesDifference(
+            status=DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT)
+        ws_diff = ws_object(self.factory.makeLaunchpadService(
+            ds_diff.owner), ds_diff)
+
+        result = ws_diff.unblacklist()
+        transaction.commit()
+
+        utility = getUtility(IDistroSeriesDifferenceSource)
+        ds_diff = utility.getByDistroSeriesAndName(
+            ds_diff.derived_series, ds_diff.source_package_name.name)
+        self.assertEqual(
+            DistroSeriesDifferenceStatus.NEEDS_ATTENTION,
+            ds_diff.status)

=== modified file 'lib/lp/registry/browser/tests/test_series_views.py'
--- lib/lp/registry/browser/tests/test_series_views.py	2010-09-21 08:42:29 +0000
+++ lib/lp/registry/browser/tests/test_series_views.py	2010-09-28 07:13:54 +0000
@@ -73,15 +73,6 @@
             name=derived_name, parent_series=parent)
         return derived_series
 
-    def makeControllerInScopes(self, scopes):
-        """Make a controller that will report it's in the given scopes."""
-        call_log = []
-
-        def scope_cb(scope):
-            call_log.append(scope)
-            return scope in scopes
-        return FeatureController(scope_cb), call_log
-
     def setDerivedSeriesUIFeatureFlag(self):
         # Helper to set the feature flag enabling the derived series ui.
         ignore = getFeatureStore().add(FeatureFlag(

=== modified file 'lib/lp/registry/interfaces/distroseriesdifference.py'
--- lib/lp/registry/interfaces/distroseriesdifference.py	2010-09-20 15:00:11 +0000
+++ lib/lp/registry/interfaces/distroseriesdifference.py	2010-09-28 07:13:54 +0000
@@ -17,10 +17,12 @@
     export_as_webservice_entry,
     export_write_operation,
     exported,
+    operation_parameters,
     )
 from lazr.restful.fields import Reference
 from zope.interface import Interface
 from zope.schema import (
+    Bool,
     Choice,
     Int,
     TextLine,
@@ -135,6 +137,8 @@
     def addComment(owner, comment):
         """Add a comment on this difference."""
 
+    @operation_parameters(
+        all=Bool(title=_("All"), required=False))
     @export_write_operation()
     def blacklist(all=False):
         """Blacklist this version or all versions of this source package.
@@ -143,6 +147,13 @@
             be blacklisted or just the current (default).
         """
 
+    @export_write_operation()
+    def unblacklist():
+        """Removes this difference from the blacklist.
+
+        The status will be updated based on the versions.
+        """
+
 
 class IDistroSeriesDifference(IDistroSeriesDifferencePublic,
                               IDistroSeriesDifferenceEdit):

=== modified file 'lib/lp/registry/javascript/distroseriesdifferences_details.js'
--- lib/lp/registry/javascript/distroseriesdifferences_details.js	2010-09-15 15:05:11 +0000
+++ lib/lp/registry/javascript/distroseriesdifferences_details.js	2010-09-28 07:13:54 +0000
@@ -11,40 +11,130 @@
 
 var namespace = Y.namespace('lp.registry.distroseriesdifferences_details');
 
+/**
+ * Create one Launchpad client that will be used with multiple requests.
+ */
+var lp_client = new LP.client.Launchpad();
+
 /*
  * Setup the expandable rows for each difference.
  *
  * @method setup_expandable_rows
  */
 namespace.setup_expandable_rows = function() {
-    var start_update = function(uri, container) {
+
+    var blacklist_handler = function(e, api_uri, source_name) {
+        // We only want to select the new radio if the update is
+        // successful.
+        e.preventDefault();
+        var blacklist_options_container = this.ancestor('div');
+
+        // Disable all the inputs
+        blacklist_options_container.all('input').set('disabled', 'disabled');
+        e.target.prepend('<img src="/@@/spinner" />');
+
+        var method_name = (e.target.get('value') == 'NONE') ?
+            'unblacklist' : 'blacklist';
+        var blacklist_all = (e.target.get('value') == 'BLACKLISTED_ALWAYS');
+
+        var diff_rows = Y.all('tr.' + source_name);
+
+        var config = {
+            on: {
+                success: function(updated_entry, args) {
+                    // Let the user know this item is now blacklisted.
+                    blacklist_options_container.one('img').remove();
+                    blacklist_options_container.all(
+                        'input').set('disabled', false);
+                    e.target.set('checked', true);
+                    Y.each(diff_rows, function(diff_row) {
+                        var fade_to_gray = new Y.Anim({
+                            node: diff_row,
+                            from: { backgroundColor: '#FFFFFF'},
+                            to: { backgroundColor: '#EEEEEE'}
+                            });
+                        if (method_name == 'unblacklist') {
+                            fade_to_gray.set('reverse', true);
+                            }
+                        fade_to_gray.run();
+                        });
+                },
+                failure: function(id, response) {
+                    blacklist_options_container.one('img').remove();
+                    blacklist_options_container.all(
+                        'input').set('disabled', false);
+                }
+            },
+            parameters: {
+                all: blacklist_all
+            }
+        };
+
+        lp_client.named_post(api_uri, method_name, config);
+
+    };
+
+    /**
+     * Link the click event for these blacklist options to the correct
+     * api uri.
+     *
+     * @param blacklist_options {Node} The node containing the blacklist
+     *                          options.
+     * @param source_name {string} The name of the source to update.
+     */
+    var setup_blacklist_options = function(blacklist_options,
+                                                 source_name) {
+        var api_uri = [
+            LP.client.cache.context.self_link,
+            '+difference',
+            source_name
+            ].join('/')
+        Y.on('click', blacklist_handler, blacklist_options.all('input'),
+             blacklist_options, api_uri, source_name);
+    };
+
+    /**
+     * Get the extra information for this diff to display.
+     *
+     * @param uri {string} The uri for the extra diff info.
+     * @param container {Node} A node which must contain a div with the
+     *                  class 'diff-extra-container' into which the results
+     *                  are inserted.
+     */
+    var get_extra_diff_info = function(uri, container, source_name) {
 
         var in_progress_message = Y.lp.soyuz.base.makeInProgressNode(
             'Fetching difference details ...')
-        container.insert(in_progress_message, 'replace');
+        container.one('div.diff-extra-container').insert(
+            in_progress_message, 'replace');
+
+        var success_cb = function(transaction_id, response, args) {
+            args.container.one('div.diff-extra-container').insert(
+                response.responseText, 'replace');
+            setup_blacklist_options(args.container.one(
+                'div.blacklist-options'), source_name);
+            };
+
+        var failure_cb = function(transaction_id, response, args){
+           var retry_handler = function(e) {
+               e.preventDefault();
+               get_extra_diff_info(args.uri, args.container);
+               };
+           var failure_message = Y.lp.soyuz.base.makeFailureNode(
+               'Failed to fetch difference details.',
+               retry_handler);
+           args.container.insert(failure_message, 'replace');
+
+           var anim = Y.lazr.anim.red_flash({
+                node: args.container
+                });
+           anim.run();
+           };
 
         var config = {
             on: {
-                'success': function(transaction_id, response, args) {
-                    args.container.set(
-                        'innerHTML', response.responseText);
-                        // Change to insert(,'replace)
-                    },
-                'failure': function(transaction_id, response, args){
-                       var retry_handler = function(e) {
-                           e.preventDefault();
-                           start_update(args.uri, args.container);
-                           };
-                       var failure_message = Y.lp.soyuz.base.makeFailureNode(
-                           'Failed to fetch difference details.',
-                           retry_handler);
-                       args.container.insert(failure_message, 'replace');
-
-                       var anim = Y.lazr.anim.red_flash({
-                            node: args.container
-                            });
-                       anim.run();
-                }
+                'success': success_cb,
+                'failure': failure_cb,
             },
             arguments: {
                 'container': container,
@@ -64,18 +154,22 @@
         // Only insert if there isn't already a container row there.
         next_row = row.next();
         if (next_row == null || !next_row.hasClass('diff-extra')) {
-            details_row = Y.Node.create(
-                '<table><tr class="diff-extra unseen"><td colspan="5"></td></tr></table>').one('tr');
+            var source_name = row.one('a.toggle-extra').get('text');
+            var details_row = Y.Node.create([
+                '<table><tr class="diff-extra unseen ' + source_name + '">',
+                '  <td colspan="5">',
+                '    <div class="diff-extra-container"></div>',
+                '  </td></tr></table>'
+                ].join('')).one('tr');
             row.insert(details_row, 'after');
             var uri = toggle.get('href');
-            start_update(uri, details_row.one('td'));
+            get_extra_diff_info(uri, details_row.one('td'), source_name);
         } else {
             details_row = next_row
         }
-
         details_row.toggleClass('unseen');
+    };
 
-    };
     Y.all('table.listing a.toggle-extra').each(function(toggle){
         toggle.addClass('treeCollapsed').addClass('sprite');
         toggle.on("click", expander_handler);
@@ -83,4 +177,4 @@
 
 };
 
-}, "0.1", {"requires": ["io-base", "lp.soyuz.base"]});
+}, "0.1", {"requires": ["io-base", "lp.soyuz.base", "lazr.anim"]});

=== modified file 'lib/lp/registry/model/distroseriesdifference.py'
--- lib/lp/registry/model/distroseriesdifference.py	2010-09-21 11:17:25 +0000
+++ lib/lp/registry/model/distroseriesdifference.py	2010-09-28 07:13:54 +0000
@@ -272,3 +272,8 @@
             self.status = DistroSeriesDifferenceStatus.BLACKLISTED_ALWAYS
         else:
             self.status = DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT
+
+    def unblacklist(self):
+        """See `IDistroSeriesDifference`."""
+        self.status = DistroSeriesDifferenceStatus.NEEDS_ATTENTION
+        self.update()

=== modified file 'lib/lp/registry/templates/distroseries-localdifferences.pt'
--- lib/lp/registry/templates/distroseries-localdifferences.pt	2010-09-16 09:32:35 +0000
+++ lib/lp/registry/templates/distroseries-localdifferences.pt	2010-09-28 07:13:54 +0000
@@ -44,7 +44,8 @@
             <tal:difference repeat="difference differences/batch">
             <tr tal:define="parent_source_pub difference/parent_source_pub;
                             source_pub difference/source_pub;
-                            signer source_pub/sourcepackagerelease/uploader/fmt:link|nothing;">
+                            signer source_pub/sourcepackagerelease/uploader/fmt:link|nothing;"
+                tal:attributes="class parent_source_pub/source_package_name">
               <td><a tal:attributes="href difference/fmt:url" class="toggle-extra"
                      tal:content="parent_source_pub/source_package_name">Foo</a>
               </td>
@@ -52,7 +53,8 @@
                     <tal:replace
                     replace="parent_source_pub/sourcepackagerelease/version"/></a>
               </td>
-              <td><a tal:attributes="href source_pub/sourcepackagerelease/fmt:url">
+              <td><a tal:attributes="href source_pub/sourcepackagerelease/fmt:url"
+                     class="derived-version">
                     <tal:replace
                     replace="source_pub/sourcepackagerelease/version"/></a>
               </td>

=== modified file 'lib/lp/registry/templates/distroseriesdifference-listing-extra.pt'
--- lib/lp/registry/templates/distroseriesdifference-listing-extra.pt	2010-09-17 09:48:32 +0000
+++ lib/lp/registry/templates/distroseriesdifference-listing-extra.pt	2010-09-28 07:13:54 +0000
@@ -3,6 +3,18 @@
   xmlns:metal="http://xml.zope.org/namespaces/metal"
   xmlns:i18n="http://xml.zope.org/namespaces/i18n"
   i18n:domain="launchpad">
+
+  <tal:show_options condition="view/show_blacklist_options">
+  <div class="blacklist-options" style="float:right">
+      <dl>
+        <dt>Blacklisted:</dt>
+        <dd>
+          <form><tal:replace replace="structure view/widgets/blacklist_options" /></form>
+        </dd>
+      </dl>
+  </div>
+  </tal:show_options>
+
   <dl>
     <dt>Binary descriptions:</dt>
       <dd><ul>

=== modified file 'lib/lp/registry/tests/test_distroseriesdifference.py'
--- lib/lp/registry/tests/test_distroseriesdifference.py	2010-09-20 15:00:11 +0000
+++ lib/lp/registry/tests/test_distroseriesdifference.py	2010-09-28 07:13:54 +0000
@@ -369,6 +369,39 @@
             DistroSeriesDifferenceStatus.BLACKLISTED_ALWAYS,
             ds_diff.status)
 
+    def test_unblacklist(self):
+        # Unblacklisting will return to NEEDS_ATTENTION by default.
+        ds_diff = self.factory.makeDistroSeriesDifference(
+            status=DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT)
+
+        with person_logged_in(ds_diff.owner):
+            ds_diff.unblacklist()
+
+        self.assertEqual(
+            DistroSeriesDifferenceStatus.NEEDS_ATTENTION,
+            ds_diff.status)
+
+    def test_unblacklist_resolved(self):
+        # Status is resolved when unblacklisting a now-resolved difference.
+        ds_diff = self.factory.makeDistroSeriesDifference(
+            versions={
+                'derived': '0.9',
+                'parent': '1.0',
+                },
+            status=DistroSeriesDifferenceStatus.BLACKLISTED_ALWAYS)
+        new_derived_pub = self.factory.makeSourcePackagePublishingHistory(
+            sourcepackagename=ds_diff.source_package_name,
+            distroseries=ds_diff.derived_series,
+            status=PackagePublishingStatus.PENDING,
+            version='1.0')
+
+        with person_logged_in(ds_diff.owner):
+            ds_diff.unblacklist()
+
+        self.assertEqual(
+            DistroSeriesDifferenceStatus.RESOLVED,
+            ds_diff.status)
+
     def test_source_package_name_unique_for_derived_series(self):
         # We cannot create two differences for the same derived series
         # for the same package.

=== modified file 'lib/lp/registry/windmill/tests/test_distroseriesdifference_expander.py'
--- lib/lp/registry/windmill/tests/test_distroseriesdifference_expander.py	2010-09-16 14:02:30 +0000
+++ lib/lp/registry/windmill/tests/test_distroseriesdifference_expander.py	2010-09-28 07:13:54 +0000
@@ -1,10 +1,15 @@
 # Copyright 2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
+from zope.component import getUtility
+
 import transaction
 
 from canonical.launchpad.webapp.publisher import canonical_url
 from canonical.launchpad.windmill.testing import constants
+from canonical.launchpad.windmill.testing import lpuser
+from lp.registry.enum import DistroSeriesDifferenceStatus
+from lp.registry.interfaces.distroseriesdifference import IDistroSeriesDifferenceSource
 from lp.registry.windmill.testing import RegistryWindmillLayer
 from lp.services.features.model import FeatureFlag, getFeatureStore
 from lp.testing import WindmillTestCase
@@ -32,11 +37,36 @@
         self.package_diffs_url = (
             canonical_url(self.diff.derived_series) + '/+localpackagediffs')
 
-    def test_diff_extra_details_available(self):
+    def test_diff_extra_details_blacklisting(self):
         """A successful request for the extra info updates the display."""
+        #login_person(self.diff.owner, 'test', self.client)
+        lpuser.FOO_BAR.ensure_login(self.client)
         self.client.open(url=self.package_diffs_url)
         self.client.waits.forPageLoad(timeout=constants.PAGE_LOAD)
         self.client.click(link=u'foo')
         self.client.waits.forElement(
             classname=u'diff-extra', timeout=constants.FOR_ELEMENT)
 
+        self.client.click(id=u'field.blacklist_options.1')
+        self.client.waits.forElementProperty(
+            option=u'enabled', id=u'field.blacklist_options.1')
+
+        # Reload the diff and ensure it's been updated.
+        transaction.commit()
+        diff_source = getUtility(IDistroSeriesDifferenceSource)
+        diff_reloaded = diff_source.getByDistroSeriesAndName(
+            self.diff.derived_series, 'foo')
+        self.assertEqual(
+            DistroSeriesDifferenceStatus.BLACKLISTED_ALWAYS,
+            diff_reloaded.status)
+
+        # Now set it back so that it's not blacklisted.
+        self.client.click(id=u'field.blacklist_options.0')
+        self.client.waits.forElementProperty(
+            option=u'enabled', id=u'field.blacklist_options.0')
+        transaction.commit()
+        diff_reloaded = diff_source.getByDistroSeriesAndName(
+            self.diff.derived_series, 'foo')
+        self.assertEqual(
+            DistroSeriesDifferenceStatus.NEEDS_ATTENTION,
+            diff_reloaded.status)