launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04830
[Merge] lp:~wallyworld/launchpad/bug-subscribed-message-798605 into lp:launchpad
Ian Booth has proposed merging lp:~wallyworld/launchpad/bug-subscribed-message-798605 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #798605 in Launchpad itself: "Marking a bug Private with AJAX should show a "You have been subscribed" notification"
https://bugs.launchpad.net/launchpad/+bug/798605
For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/bug-subscribed-message-798605/+merge/73462
This branch fixes the bug task page so that when the ajax edit widget is used to change the privacy setting:
1. a notification is displayed if the person's subscription details are updated when they are automatically subscribed
2. the subscriber portlet is updated
== Implementation ==
Display notification
Some infrastructure was added to LaunchpadFormView. The initialize() method has code added so that when an ajax request is received, any notifications are extracted from the response object and add them to the response header item X-Lazr-Notifications. There is already client Javascript to process notifications passed in the response header and display them. This Javascript code was updated to allow any existing notifications to be cleared if a "null" or null X-Lazr-Notifications header value is passed.
The mechanism used to pass the user updated privacy settings to the back end was changed from a web services lp_save() to an io call which performs a submit on the existing BugSecrecyEditView. This view was changed to extend LaunchpadFormView rather than LaunchpadEditFormView and only the relevant fields ('private' and 'security_related') are processed. The form's implementation was also cleaned up in terms of how the schema is specified. When a submit action is processed, the ajax render() result is different to the html submit result. This is to provide the required data to update the subscriber portlet. The form works the same as it always did for standard html operation.
Update subscriber portlet
Three different implementations were tried, the last being the one used.
1. Add a view to return new html for the subscriber portlet and update the portlet div.
Bzzzt. Links not wired up correctly.
2. After privacy update is done, make a second ajax call to a new view which returns a json data structure with details used to update the portlet.
Works, but a second ajax call is used.
3. Make the secrecy view used to process the privacy form submit return the json data structure with details used to update the portlet.
This required some refactoring of the BugSubscriptionPortletView to extract into a base BugSubscriptionPortletDetails class the logic to build up the data dict used to pass the required subscriber details to the client so it can be updated. When the bug index view is first loaded, these details are stuffed into the page's lp Javascript cache. When the secrecy form is called, these details are stuffed into a simple dict which is json encoded and sent back to the client.
When the ajax call returns to the client, the data from the json dict is used to update the page cahce with the new subscription details. Then update_subscription_status() is called to render the new portlet details.
== Tests ==
lp.app.browser.tests.base-layout.txt
Add test for stuffing notifications into the response header for LaunchpadFormView
lp.app.javascript.tests.test_lp_client.js
Add test for notification clearing
Add new test to lp.bugs.browser.tests.test_bug_views.TestBugSecrecyViews
test_secrecy_view_ajax_render
Run existing bug privacy view tests:
bin/test -vvc -t test_bug_views -t xx-bug-privacy -t xx-presenting-private-bugs -t xx-bug-activity -t xx-duplicate-of-private-bug
== Lint ==
Linting changed files:
lib/lp/app/browser/launchpadform.py
lib/lp/app/browser/tests/base-layout.txt
lib/lp/app/javascript/client.js
lib/lp/app/javascript/tests/test_lp_client.js
lib/lp/bugs/browser/bug.py
lib/lp/bugs/browser/tests/test_bug_views.py
lib/lp/bugs/javascript/bug_subscription_portlet.js
lib/lp/bugs/javascript/bugtask_index.js
./lib/lp/app/browser/launchpadform.py
119: redefinition of unused 'action' from line 35
243: redefinition of unused 'action' from line 35
--
https://code.launchpad.net/~wallyworld/launchpad/bug-subscribed-message-798605/+merge/73462
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/bug-subscribed-message-798605 into lp:launchpad.
=== modified file 'lib/lp/app/browser/launchpadform.py'
--- lib/lp/app/browser/launchpadform.py 2011-07-15 15:46:51 +0000
+++ lib/lp/app/browser/launchpadform.py 2011-08-31 11:14:20 +0000
@@ -19,6 +19,7 @@
from lazr.lifecycle.event import ObjectModifiedEvent
from lazr.lifecycle.snapshot import Snapshot
+import simplejson
import transaction
from zope.app.form import CustomWidgetFactory
from zope.app.form.browser import (
@@ -47,6 +48,7 @@
IAlwaysSubmittedWidget,
ICheckBoxWidgetLayout,
IMultiLineWidgetLayout,
+ INotificationResponse,
UnsafeFormGetSubmissionError,
)
from canonical.launchpad.webapp.menu import escape
@@ -133,8 +135,23 @@
self.form_result = action.success(data)
if self.next_url:
self.request.response.redirect(self.next_url)
+ if self.request.is_ajax:
+ self._processNotifications(self.request)
self.action_taken = action
+ def _processNotifications(self, request):
+ """Add any notification messages to the response headers."""
+ if not INotificationResponse.providedBy(request.response):
+ return
+ notifications = ([(notification.level, notification.message)
+ for notification in request.response.notifications])
+ if notifications:
+ json_notifications = simplejson.dumps(notifications)
+ else:
+ json_notifications = simplejson.dumps(None)
+ request.response.setHeader(
+ 'X-Lazr-Notifications', json_notifications)
+
def render(self):
"""Return the body of the response.
=== modified file 'lib/lp/app/browser/tests/base-layout.txt'
--- lib/lp/app/browser/tests/base-layout.txt 2011-04-05 17:19:20 +0000
+++ lib/lp/app/browser/tests/base-layout.txt 2011-08-31 11:14:20 +0000
@@ -236,7 +236,7 @@
Notifications
-------------
-Notifications are diplayed between the breadcrumbs and the page content.
+Notifications are displayed between the breadcrumbs and the page content.
>>> request.response.addInfoNotification('I cannot do that Dave.')
>>> view = MainOnlyView(user, request)
@@ -249,3 +249,25 @@
</div>
<div class="top-portlet">
...
+
+For ajax requests to form views, notifications are added to the response
+headers.
+
+ >>> from lp.app.browser.launchpadform import action, LaunchpadFormView
+ >>> from zope.interface import Interface
+ >>> class FormView(LaunchpadFormView):
+ ... """A simple view to test notifications."""
+ ... class schema(Interface):
+ ... """A default schema."""
+ ... @action('Test', name='test')
+ ... def test_action(self, action, data):
+ ... pass
+
+ >>> extra = {'HTTP_X_REQUESTED_WITH': 'XMLHttpRequest'}
+ >>> request = LaunchpadTestRequest(
+ ... method='POST', form={'field.actions.test': 'Test'}, **extra)
+ >>> request.response.addInfoNotification('I cannot do that Dave.')
+ >>> view = FormView(user, request)
+ >>> view.initialize()
+ >>> print request.response.getHeader('X-Lazr-Notifications')
+ [[20, "I cannot do that Dave."]]
=== modified file 'lib/lp/app/javascript/client.js'
--- lib/lp/app/javascript/client.js 2011-08-25 13:20:18 +0000
+++ lib/lp/app/javascript/client.js 2011-08-31 11:14:20 +0000
@@ -327,7 +327,7 @@
var media_type = response.getResponseHeader('Content-Type');
if (media_type.substring(0,16) === 'application/json') {
representation = Y.JSON.parse(response.responseText);
- display_notifications(
+ module.display_notifications(
response.getResponseHeader('X-Lazr-Notifications'));
wrapped = client.wrap_resource(uri, representation);
result = old_on_success(wrapped);
@@ -364,10 +364,14 @@
* Display a list of notifications - error, warning, informational or debug.
* @param notifications An json encoded array of (level, message) tuples.
*/
-var display_notifications = function (notifications) {
+module.display_notifications = function (notifications) {
if (notifications === undefined) {
return;
}
+ if (notifications === 'null' || notifications === null) {
+ module.remove_notifications();
+ return;
+ }
var notifications_by_level = {
'level10': {
=== modified file 'lib/lp/app/javascript/tests/test_lp_client.js'
--- lib/lp/app/javascript/tests/test_lp_client.js 2011-08-24 15:41:23 +0000
+++ lib/lp/app/javascript/tests/test_lp_client.js 2011-08-31 11:14:20 +0000
@@ -378,7 +378,16 @@
this.response.setResponseHeader(
'X-Lazr-Notifications', notifications);
Y.lp.client.wrap_resource_on_success(null, this.response, this.args);
- Y.lp.client.remove_notifications();
+ this.response.setResponseHeader('X-Lazr-Notifications', "null");
+ Y.lp.client.wrap_resource_on_success(null, this.response, this.args);
+ this._checkNoNotificationNode('.debug.message');
+ this._checkNoNotificationNode('.informational.message');
+ notifications = '[ [10, "A debug"], [20, "An info"] ]';
+ this.response.setResponseHeader(
+ 'X-Lazr-Notifications', notifications);
+ Y.lp.client.wrap_resource_on_success(null, this.response, this.args);
+ this.response.setResponseHeader('X-Lazr-Notifications', null);
+ Y.lp.client.wrap_resource_on_success(null, this.response, this.args);
this._checkNoNotificationNode('.debug.message');
this._checkNoNotificationNode('.informational.message');
}
=== modified file 'lib/lp/bugs/browser/bug.py'
--- lib/lp/bugs/browser/bug.py 2011-08-09 18:41:30 +0000
+++ lib/lp/bugs/browser/bug.py 2011-08-31 11:14:20 +0000
@@ -14,6 +14,7 @@
'BugNavigation',
'BugSecrecyEditView',
'BugSetNavigation',
+ 'BugSubscriptionPortletDetails',
'BugTextView',
'BugURL',
'BugView',
@@ -37,8 +38,14 @@
)
from lazr.lifecycle.event import ObjectModifiedEvent
from lazr.lifecycle.snapshot import Snapshot
+from lazr.restful import (
+ EntryResource,
+ ResourceJSONEncoder,
+ )
+from lazr.restful.interface import copy_field
from lazr.restful.interfaces import IJSONRequestCache
import pytz
+from simplejson import dumps
from zope import formlib
from zope.app.form.browser import TextWidget
from zope.component import getUtility
@@ -48,10 +55,7 @@
Interface,
providedBy,
)
-from zope.schema import (
- Bool,
- Choice,
- )
+from zope.schema import Choice
from zope.security.interfaces import Unauthorized
from canonical.launchpad import _
@@ -580,32 +584,10 @@
attachment.libraryfile, attachment).http_url
-class BugSubscriptionPortletView(LaunchpadView, BugViewMixin):
- """View class for the subscription portlet."""
-
- # We want these strings to be available for the template and for the
- # JavaScript.
- notifications_text = {
- 'not_only_other_subscription': _('You are'),
- 'only_other_subscription': _(
- 'You have subscriptions that may cause you to receive '
- 'notifications, but you are'),
- 'direct_all': _('subscribed to all notifications for this bug.'),
- 'direct_metadata': _(
- 'subscribed to all notifications except comments for this bug.'),
- 'direct_lifecycle': _(
- 'subscribed to notifications when this bug is closed or '
- 'reopened.'),
- 'not_direct': _(
- "not directly subscribed to this bug's notifications."),
- 'muted': _(
- 'Your personal email notifications from this bug are muted.'),
- }
-
- def initialize(self):
- """Initialize the view to handle the request."""
- LaunchpadView.initialize(self)
- user = self.user
+class BugSubscriptionPortletDetails:
+ """A mixin used to collate bug subscription details for a view."""
+
+ def extractBugSubscriptionDetails(self, user, bug, cache):
# We are using "direct" to represent both direct and personal
# (not team).
self.direct_notifications = False
@@ -617,11 +599,8 @@
self.any_subscription_notifications = False
self.muted = False
if user is not None:
- bug = self.context
has_structural_subscriptions = not (
get_structural_subscriptions_for_bug(bug, user).is_empty())
- cache = IJSONRequestCache(self.request).objects
- cache['notifications_text'] = self.notifications_text
self.muted = bug.isMuted(user)
psi = PersonSubscriptions(user, bug)
if psi.direct.personal:
@@ -655,6 +634,38 @@
self.any_subscription_notifications or self.muted)
+class BugSubscriptionPortletView(LaunchpadView,
+ BugSubscriptionPortletDetails, BugViewMixin):
+ """View class for the subscription portlet."""
+
+ # We want these strings to be available for the template and for the
+ # JavaScript.
+ notifications_text = {
+ 'not_only_other_subscription': _('You are'),
+ 'only_other_subscription': _(
+ 'You have subscriptions that may cause you to receive '
+ 'notifications, but you are'),
+ 'direct_all': _('subscribed to all notifications for this bug.'),
+ 'direct_metadata': _(
+ 'subscribed to all notifications except comments for this bug.'),
+ 'direct_lifecycle': _(
+ 'subscribed to notifications when this bug is closed or '
+ 'reopened.'),
+ 'not_direct': _(
+ "not directly subscribed to this bug's notifications."),
+ 'muted': _(
+ 'Your personal email notifications from this bug are muted.'),
+ }
+
+ def initialize(self):
+ """Initialize the view to handle the request."""
+ LaunchpadView.initialize(self)
+ cache = IJSONRequestCache(self.request).objects
+ self.extractBugSubscriptionDetails(self.user, self.context, cache)
+ if self.user:
+ cache['notifications_text'] = self.notifications_text
+
+
class BugWithoutContextView:
"""View that redirects to the new bug page.
@@ -801,10 +812,8 @@
self.updateBugFromData(data)
-class BugSecrecyEditView(BugEditViewBase):
- """Page for marking a bug as a private/public."""
-
- field_names = ['private', 'security_related']
+class BugSecrecyEditView(LaunchpadFormView, BugSubscriptionPortletDetails):
+ """Form for marking a bug as a private/public."""
@property
def label(self):
@@ -812,26 +821,18 @@
page_title = label
- def setUpFields(self):
- """Make the read-only version of the form fields writable."""
- private_field = Bool(
- __name__='private',
- title=_("This bug report should be private"),
- required=False,
- description=_("Private bug reports are visible only to "
- "their subscribers."),
- default=False)
- security_related_field = Bool(
- __name__='security_related',
- title=_("This bug is a security vulnerability"),
- required=False, default=False)
+ class schema(Interface):
+ """Schema for editing secrecy info."""
+ private_field = copy_field(IBug['private'], readonly=False)
+ security_related_field = copy_field(
+ IBug['security_related'], readonly=False)
- super(BugSecrecyEditView, self).setUpFields()
- self.form_fields = self.form_fields.omit('private')
- self.form_fields = self.form_fields.omit('security_related')
- self.form_fields = (
- formlib.form.Fields(private_field) +
- formlib.form.Fields(security_related_field))
+ @property
+ def next_url(self):
+ """Return the next URL to call when this call completes."""
+ if not self.request.is_ajax:
+ return canonical_url(self.context)
+ return None
@property
def initial_values(self):
@@ -867,9 +868,18 @@
changed_fields.append('security_related')
notify(ObjectModifiedEvent(
bug, bug_before_modification, changed_fields))
+ if self.request.is_ajax:
+ if private_changed:
+ return self._getSubscriptionDetails()
+ else:
+ return ''
- # Apply other changes.
- self.updateBugFromData(data)
+ def _getSubscriptionDetails(self):
+ cache = dict()
+ self.extractBugSubscriptionDetails(self.user, self.context.bug, cache)
+ return dumps(
+ cache, cls=ResourceJSONEncoder,
+ media_type=EntryResource.JSON_TYPE)
def _handlePrivacyChanged(self, user_will_be_subscribed):
"""Handle the case where the privacy of the bug has been changed.
=== modified file 'lib/lp/bugs/browser/tests/test_bug_views.py'
--- lib/lp/bugs/browser/tests/test_bug_views.py 2011-06-16 13:50:58 +0000
+++ lib/lp/bugs/browser/tests/test_bug_views.py 2011-08-31 11:14:20 +0000
@@ -5,6 +5,7 @@
__metaclass__ = type
+import simplejson
from zope.component import getUtility
from BeautifulSoup import BeautifulSoup
@@ -214,7 +215,8 @@
layer = DatabaseFunctionalLayer
- def createInitializedSecrecyView(self, person=None, bug=None):
+ def createInitializedSecrecyView(self, person=None, bug=None,
+ request=None):
"""Create and return an initialized BugSecrecyView."""
if person is None:
person = self.factory.makePerson()
@@ -226,7 +228,8 @@
'field.private': 'on',
'field.security_related': '',
'field.actions.change': 'Change',
- })
+ },
+ request=request)
return view
def test_notification_shown_if_marking_private_and_not_subscribed(self):
@@ -264,3 +267,33 @@
bug.subscribe(team, person)
view = self.createInitializedSecrecyView(person, bug)
self.assertContentEqual([], view.request.response.notifications)
+
+ def test_secrecy_view_ajax_render(self):
+ # When the bug secrecy view is called from an ajax request, it should
+ # provide a json encoded dict when rendered. The dict contains bug
+ # subscription information resulting from the update to the bug
+ # privacy.
+ person = self.factory.makePerson()
+ bug = self.factory.makeBug()
+ with person_logged_in(person):
+ bug.subscribe(person, person)
+
+ extra = {'HTTP_X_REQUESTED_WITH': 'XMLHttpRequest'}
+ request = LaunchpadTestRequest(
+ method='POST', form={
+ 'field.actions.change': 'Change',
+ 'field.private': 'on',
+ 'field.security_related': 'ff'},
+ **extra)
+ view = self.createInitializedSecrecyView(person, bug, request)
+ cache_data = simplejson.loads(view.render())
+ self.assertFalse(cache_data['other_subscription_notifications'])
+ subscription_data = cache_data['subscription']
+ self.assertEqual(
+ 'http://launchpad.dev/api/devel/bugs/%s' % bug.id,
+ subscription_data['bug_link'])
+ self.assertEqual(
+ 'http://launchpad.dev/api/devel/~%s' % person.name,
+ subscription_data['person_link'])
+ self.assertEqual(
+ 'Discussion', subscription_data['bug_notification_level'])
=== modified file 'lib/lp/bugs/javascript/bug_subscription_portlet.js'
--- lib/lp/bugs/javascript/bug_subscription_portlet.js 2011-08-09 14:18:02 +0000
+++ lib/lp/bugs/javascript/bug_subscription_portlet.js 2011-08-31 11:14:20 +0000
@@ -44,7 +44,6 @@
* point for tests.
*/
function lp_client() {
- //
if (!Y.Lang.isValue(namespace._lp_client)) {
namespace._lp_client = new Y.lp.client.Launchpad();
}
@@ -72,10 +71,10 @@
span.set('text', messages.muted);
if (Y.Lang.isValue(link)) {
link.remove();
- };
+ }
if (Y.Lang.isValue(whitespace)) {
whitespace.remove(); // Tests can be persnickety.
- };
+ }
} else {
if (!Y.Lang.isValue(link)) {
if (!Y.Lang.isValue(whitespace)) {
@@ -86,15 +85,15 @@
.addClass('sprite')
.addClass('modify')
.addClass('edit');
- link.set('href', LP.cache.context.web_link + '/+subscribe')
+ link.set('href', LP.cache.context.web_link + '/+subscribe');
status.appendChild(link);
setup_subscription_link_handlers();
- };
+ }
span.set('text', messages.not_only_other_subscription);
if (Y.Lang.isUndefined(LP.cache.subscription)) {
if (LP.cache.other_subscription_notifications) {
span.set('text', messages.only_other_subscription);
- };
+ }
link.set('text', messages.not_direct);
} else {
switch (LP.cache.subscription.bug_notification_level) {
@@ -111,9 +110,9 @@
Y.error(
'Programmer error: unknown bug notification level: '+
LP.cache.subscription.bug_notification_level);
- };
- };
- };
+ }
+ }
+ }
Y.lp.anim.green_flash({ node: status }).run();
}
namespace.update_subscription_status = update_subscription_status;
@@ -155,7 +154,7 @@
};
handler.clearProgressUI = function () {
link.replaceClass('spinner', current_class);
- }
+ };
var config = {
on: {
success: function(response) {
@@ -200,7 +199,7 @@
overlay.hide();
overlay.destroy();
link.replaceClass('spinner', 'edit');
- }
+ };
// The make_action function is the workhorse for creating all the
// JavaScript action links we need. Given the `text` that the link
// should display, the `class` name to show the appropriate sprite,
@@ -259,7 +258,7 @@
LP.cache.context.bug_link, method_name, config);
});
return result;
- }
+ };
// Now we start building the nodes that will be used within the overlay.
// We reuse these, so we create them once and then the main subscription
// click handler updates and assembles them.
=== modified file 'lib/lp/bugs/javascript/bugtask_index.js'
--- lib/lp/bugs/javascript/bugtask_index.js 2011-08-22 12:13:22 +0000
+++ lib/lp/bugs/javascript/bugtask_index.js 2011-08-31 11:14:20 +0000
@@ -357,12 +357,31 @@
privacy_form_overlay.show();
};
+ var base_url = LP.cache.context.web_link;
+ var submit_url = base_url+"/+secrecy";
+ var qs = Y.lp.client.append_qs('', 'field.actions.change', 'Change');
+ qs = Y.lp.client.append_qs(qs, 'field.private', private_flag?'on':'off');
+ qs = Y.lp.client.append_qs(
+ qs, 'field.security_related', security_related?'on':'off');
var config = {
+ method: "POST",
+ headers: {'Accept': 'application/xhtml'},
+ data: qs,
on: {
- success: function (updated_entry) {
+ success: function (id, response) {
+ if (response.responseText !== '') {
+ var cache_data = Y.JSON.parse(response.responseText);
+ var item;
+ for (item in cache_data) {
+ if (cache_data.hasOwnProperty(item)) {
+ LP.cache[item] = cache_data[item];
+ }
+ }
+ var ns = Y.lp.bugs.bugtask_index.portlets.subscription;
+ ns.update_subscription_status();
+ }
privacy_spinner.setStyle('display', 'none');
privacy_link.setStyle('display', 'inline');
- lp_bug_entry = updated_entry;
var notification = Y.one('.global-notification');
if (private_flag) {
@@ -421,12 +440,14 @@
privacy_div.removeChild(security_message);
}
}
+ Y.lp.client.display_notifications(
+ response.getResponseHeader('X-Lazr-Notifications'));
Y.lp.anim.green_flash({node: privacy_div}).run();
},
failure: error_handler.getFailureHandler()
}
};
- lp_bug_entry.lp_save(config);
+ lp_client.io_provider.io(submit_url, config);
};
/**
@@ -1083,6 +1104,7 @@
"json-parse", "substitute", "widget-position-ext",
"lazr.formoverlay", "lp.anim", "lazr.base",
"lazr.overlay", "lazr.choiceedit", "lp.app.picker",
+ "lp.bugs.bugtask_index.portlets.subscription",
"lp.client", "escape",
"lp.client.plugins", "lp.app.errors",
"lp.app.privacy"]});
