-----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):
>
> === 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):
> + """True if the HTTP_X_REQUESTED_WITH is set appropriately."""
> + request = LaunchpadBrowserRequest(StringIO.StringIO(''), {})
> + self.assertFalse(request.is_ajax)
> +
def test_is_ajax(self):
> + 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-27 15:03:26 +0000
> @@ -8,18 +8,58 @@
> '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."""
> + 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.
> + return dict(blacklist_options=self.context.status)
> +
> + return dict(blacklist_options='NONE')
>
> @property
> def binary_summaries(self):
> @@ -40,11 +80,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-27 15:03:26 +0000
> @@ -8,8 +8,10 @@
> __metaclass__ = type
>
> from BeautifulSoup import BeautifulSoup
> +from zope.app.form.browser.itemswidgets import RadioWidget
> from zope.component import getUtility
>
> +from canonical.launchpad.webapp.servers import LaunchpadTestRequest
> from canonical.launchpad.webapp.testing import verifyObject
> from canonical.testing import (
> DatabaseFunctionalLayer,
> @@ -19,7 +21,8 @@
> DistroSeriesDifferenceDisplayComment,
> )
> from lp.registry.enum import DistroSeriesDifferenceType
> -from lp.registry.interfaces.distroseriesdifference import IDistroSeriesDifferenceSource
> +from lp.registry.interfaces.distroseriesdifference import (
> + IDistroSeriesDifferenceSource)
> from lp.services.comments.interfaces.conversation import (
> IComment,
> IConversation,
> @@ -124,6 +127,29 @@
> self.assertIs(None, ds_diff.source_pub)
> self.assertIs(None, view.binary_summaries)
>
> + 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.
> +
> + # With a JS request, editors do see blacklist options.
> + 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)
> +
Here, too.
> + # Even with a JS request, non-editors do not see the options.
> + view = create_initialized_view(
> + ds_diff, '+listing-distroseries-extra', request=request)
> + self.assertFalse(view.show_blacklist_options)
> +
>
> class DistroSeriesDifferenceTemplateTestCase(TestCaseWithFactory):
>
> @@ -215,3 +241,42 @@
> 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(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.
> +
> + from lp.registry.enum import DistroSeriesDifferenceStatus
> + 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'))
> +
And here ... ;-)
> + 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'
> === modified file 'lib/lp/registry/browser/tests/test_series_views.py'
> === modified file 'lib/lp/registry/interfaces/distroseriesdifference.py'
> === 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('
');
> +
> + 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
> + 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(
> - '').one('tr');
> + var source_name = row.one('a.toggle-extra').get('text');
> + var details_row = Y.Node.create([
> + ''
> + ].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.
> 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 +178,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'
> === modified file 'lib/lp/registry/templates/distroseries-localdifferences.pt'
> === modified file 'lib/lp/registry/templates/distroseriesdifference-listing-extra.pt'
> === modified file 'lib/lp/registry/tests/test_distroseriesdifference.py'
> === modified file 'lib/lp/registry/windmill/tests/test_distroseriesdifference_expander.py'
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
iEYEARECAAYFAkyg2E0ACgkQBT3oW1L17ihbvgCZAUXgtA1vsI1EpjMjDtraCWX5
dtEAn1uJyPlSS3dwCRloPmXPNojvvnbI
=o1Ep
-----END PGP SIGNATURE-----