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

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.

<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

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.

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.

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.

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

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

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