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_blacklist_options_initial_values(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'))
>
> Here.

Also 3 tests now.

>
>> +
>> +        from lp.registry.enum import DistroSeriesDifferenceStatus

And I moved that :)

>> === 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-27 15:03:26 +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 use know this item is now blacklisted.
>
> user

Done.

>
>> +                    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,23 @@
>>          // 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
>>          }
>>
>
> Blank line.

Removed.

Thanks Henning... incremental attached.