Hi Gary,
Thanks for working on this: it's a huge improvement over what we used to
have, and very nicely factored. I do have a few suggestions on some
further factoring and a few cleanups, but that's about it.
The only other remark of note that I have is that I find the UI
presentation a bit weird for the overlay.
None of the changes I propose are binding, and I find your branch pretty
good.
review approve
merge approve
> === modified file 'lib/lp/bugs/browser/bug.py'
...
> +from lp.bugs.model.personsubscriptioninfo import PersonSubscriptions
I like it that you are reusing the work that we've already done. Cool :)
> @@ -642,6 +607,79 @@
> return ProxiedLibraryFileAlias(
> attachment.libraryfile, attachment).http_url
>
> +class BugSubscriptionPortletView(BugView):
I might have missed them, but at least a test or two for this view would
be nice to have.
Also, I ain't sure if you could have used BugViewMixin instead of
BugView directly. Worth checking, imo.
...
> +
> + def initialize(self):
...
> + if psi.direct.personal:
> + self.direct_notifications = True
> + direct = psi.direct.personal[0]
> + cache['subscription'] = direct.subscription
> + level = direct.subscription.bug_notification_level
> + if level == BugNotificationLevel.COMMENTS:
> + self.direct_all_notifications = True
> + elif level == BugNotificationLevel.METADATA:
> + self.direct_metadata_notifications = True
> + else:
> + assert level == BugNotificationLevel.LIFECYCLE
> + self.direct_lifecycle_notifications = True
> + self.other_subscription_notifications = bool(
> + has_structural_subscriptions or
> + psi.from_duplicate.count or
> + psi.as_owner.count or
> + psi.as_assignee.count or
> + psi.direct.as_team_member or
> + psi.direct.as_team_admin)
I find this interesting. What happens with eg. LIFECYCLE duplicate
subscription? How are we to show that in the UI? Should my code include
the current user among the 'Other subscribers' if they have a duplicate
subscription?
> === modified file 'lib/lp/bugs/browser/bugsubscription.py'
> --- lib/lp/bugs/browser/bugsubscription.py 2011-06-03 22:36:15 +0000
> +++ lib/lp/bugs/browser/bugsubscription.py 2011-06-03 22:36:29 +0000
> @@ -612,8 +612,8 @@
>
> def initialize(self):
> super(BugSubscriptionListView, self).initialize()
> - subscriptions = get_structural_subscriptions_for_bug(
> - self.context.bug, self.user)
> + subscriptions = list(get_structural_subscriptions_for_bug(
> + self.context.bug, self.user))
> expose_structural_subscription_data_to_js(
> self.context, self.request, self.user, subscriptions)
> subscriptions_info = PersonSubscriptions(
Looking at the code of expose_structural_subscription_data_to_js(), it
should have worked with a ResultSet(): it checks the length with
len(list(subscriptions)) and then calls expose_user_subscriptions_to_js
where it simply iterates over the subscriptions.
Your fix above would be a net performance fix (one instead of two
queries), but I feel it'd be better if we instead passed on the result
set, and used .any() for the check, thus making it even more performant.
Of course, that might be a harder fix (i.e. there might be other places
in the code [especially tests] where indeed a list is passed on), so not
necessary for this branch.
At the very least, I'd prefer if we fixed expose_...() function to do
something like:
if subscriptions is not None:
subscriptions = list(subscriptions)
so it passes the already listified result to both len() below and
expose_user_subscriptions_to_js(). Then your change would again be
unneeded.
> === added file
'lib/lp/bugs/javascript/bugtask_index_subscription_portlet.js'
> --- lib/lp/bugs/javascript/bugtask_index_subscription_portlet.js
1970-01-01 00:00:00 +0000
> +++ lib/lp/bugs/javascript/bugtask_index_subscription_portlet.js
2011-06-03 22:36:29 +0000
> @@ -0,0 +1,419 @@
> +/* Copyright 2011 Canonical Ltd. This software is licensed under the
> + * GNU Affero General Public License version 3 (see the file
LICENSE).
> + *
> + * Provide information and actions on all bug subscriptions a person
holds.
> + *
> + * @module bugs
> + * @submodule bugtask_index_subscription_portlet
> + */
FWIW, I wouldn't mind being less succint in the file/module name.
Something shorter like bug_subscription_portlet.js would be just enough
imho, because it's already in lp/bugs/ and there're no other bug
subscription portlets around (there is a pillar's bugs subscription
portlet [for structural subscriptions], so just "subscription_portlet"
might not be sufficient; also, it's really for a bug and not a bug task,
so we might as well fix that too).
> +
> +YUI.add('lp.bugs.bugtask_index.portlets.subscription', function(Y) {
> +
> +var namespace =
Y.namespace('lp.bugs.bugtask_index.portlets.subscription');
> +
> +// We begin with fairly generic helpers for the main functions.
> +
> +/*
> + * Internal helper to build tags.
> + */
> +function tag(name) {
> + return Y.Node.create('<'+name+'/>');
> +}
I like this, perhaps it should live in lib/lp/app/javascript
somewhere? :)
Also, I remember that some tags cause problems when added as ''
instead of '', so I think it'd be better to default to the latter
version.
For instance, see some discussion on
http://stackoverflow.com/questions/348736/xhtml-is-writing-self-closing-tags-for-elements-not-traditionally-empty-bad-pra
Luckily, with your approach, this is a one line fix for the entire file.
> +
> +/*
> + * Internal helper to build sprite link tags.
> + */
It seems that the "new", predominant jsdoc comment style is started with
"/**": at least that's what I've been told when I copied comment style
from the existing bugtask_index_portlets.js. This applies throughout
the file.
> +function ws() {
> + return Y.Node.create(' ');
> +}
Perhaps implement this in terms of tag()? No big deal though.
> +
> +/*
> + * This returns a launchpad client. It is factored in a way to be a
hook
> + * point for tests.
> + */
> +function lp_client() {
> + //
You seem lost for words here :)
> + if (!Y.Lang.isValue(namespace._lp_client)) {
> + namespace._lp_client = new Y.lp.client.Launchpad();
> + }
> + return namespace._lp_client;
> +}
> +
> +// Now we are ready for code that more directly does what we want
here.
A very informative comment, I must admit ;)
> +
> +var MUTED_CLASS = namespace.MUTED_CLASS = 'unmute';
> +var UNMUTED_CLASS = namespace.UNMUTED_CLASS = 'mute';
> +
> +/*
> + * Update the text and link at the top of the subscription portlet.
> + */
> +function update_subscription_status() {
> + var status = Y.one('#current_user_subscription');
> + var span = status.one('span');
> + var whitespace = status.one('span+span');
> + var link = status.one('a');
> + var mute_link = Y.one('.menu-link-mute_subscription');
> + var is_muted = (Y.Lang.isValue(mute_link) &&
> + mute_link.hasClass(MUTED_CLASS));
> + var messages = LP.cache.notifications_text;
> + if (is_muted) {
> + 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)) {
> + status.appendChild(ws());
> + }
> + link = tag('a')
> + .addClass('menu-link-subscription')
> + .addClass('sprite')
> + .addClass('modify')
> + .addClass('edit');
> + link.set('href', LP.cache.context.web_link +
'/+subscribe')
Could you not use
link = make_link('', 'edit')
.addClass('menu-link-subscription')
here?
You'd still be setting the href twice, but I don't see it as a big deal,
considering you've already made the helper function :)
...
> +
> +/*
> + * Set up the handler for the subscription link.
> + */
> +function setup_subscription_link_handlers() {
Let me start with a first impression: WHOA! One big function you've got
here. :)
Ideally, you'd split the node construction into separate, smaller
functions that are called as needed and return nodes you insert from
this function: I personally prefer functions which do one single thing
[or several small things] that is easy to unit test, and that fit on one
screen (which usually means <40 lines, and this one goes over 200).
> + // First we determine if we should bother to run.
> + var link = Y.one('#current_user_subscription a');
While I like the fact that you are using a CSS class for this, I think
it'd be nicer if you got the entire div hodling the current user
subscription info, and then did
var link = current_user_subscription_node.one('...');
That would have several benefits like improved performance and better
encapsulation (i.e. especially with CSS classes, someone else might
accidentally add an element with this class on an entirely different
part of the page, which makes for hard to find bugs).
Not a problem for this branch in particular, but just my general opinion
on the matter. If you disagree, I'd love to hear your arguments. This
applies to most of the Y.one/Y.all calls, and I tend to never use them.
> + var make_action = function(text, class, method_name, parameters)
{
...
This is a lovely candidate for extraction outside the big function and
separate unit testing. Just saying :)
> + // 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.
> + // The first that we create is the header node. The text
alternates
> + // depending on circumstance, so we set that in the main click
handler.
> + var header = tag('h2')
...
> + var level_node = tag('div')
> + .set('id', 'subscription-levels') // Useful for tests.
> + .setStyle('margin-top', '1em')
> + .append(tag('div')
> + .set('id', 'Discussion')
> + .append(make_action(
> + 'Receive all emails about this bug', 'edit',
> + 'subscribe', {person: LP.links.me, level:
'Discussion'})
> + ).append(tag('span')
> + .set('text',
> + 'You currently receive all emails about this
bug.')
> + .addClass('sprite')
> + )
> + ).append(tag('div')
> + .set('id', 'Details')
> + .append(make_action(
> + 'Receive all emails about this bug except comments',
'edit',
> + 'subscribe', {person: LP.links.me, level: 'Details'})
> + ).append(tag('span')
> + .set('text',
> + 'You currently receive all emails about this bug
'+
> + 'except comments.')
> + .addClass('sprite')
> + )
> + ).append(tag('div')
> + .set('id', 'Lifecycle')
> + .append(make_action(
> + 'Only receive email when this bug is closed', 'edit',
> + 'subscribe', {person: LP.links.me, level:
'Lifecycle'})
> + ).append(tag('span')
> + .set('text',
> + 'You currently only receive email when this bug
is '+
> + 'closed.')
> + .addClass('sprite')
> + )
> + );
(Uhm, separate functions for header and level node? :)
I also find the presentation of this a bit weird: the resulting overlay
choices seem confusing, especially when one is selected. The lack of a
familiar UI concept (such as radio boxes) means that it doesn't strike
me that the regular text is one of the options, and a selected one at
that. If it was at least clearly formatted as a list, I think it'd work
better.
Not sure how you could improve it further while keeping "changing a
level requires two clicks total" (one to pop-up a window, another to
select a level), which I think is a big benefit of your approach (vs.
3/4 clicks we used to require: pop-up overlay, select level, click save;
or pop-up overlay, select update subscription, select level, click
save).
> + // The last node we create is the node that includes the ability
to
> + // unsubscribe. As you'd expect, this is only pertinent if the
user
> + // has an existing direct subscription. We have two variants.
If the
> + // user has subscriptions to this bug other than the direct
subscription,
> + // we show a version that includes a warning, and encourages
muting as
> + // a more effective alternative. Otherwise, we show a simple
unsubscribe
> + // link.
> + var unsubscribe_node;
> + if (LP.cache.other_subscription_notifications) {
> + unsubscribe_node = tag('div')
...
> + } else {
> + unsubscribe_node = tag('div')
...
> + }
I'd like this to be a separate function as well: as long as you are not
using context (like `overlay` variable) in a relatively independent part
of the code, I prefer it to be split out.
FWIW, I do find your code very nicely factored, except that it'd be hard
to test, and thus easy to introduce bugs in the future (eg. if node
creation really requires no access to the overlay variable, by having it
out, will ensure that it doesn't have access to it :).
All else looks great in this JS file.
> === added file
'lib/lp/bugs/javascript/tests/test_bugtask_index_subscription_portlet.js'
...
> +/**
> + * XXX gary 2011-05-26 bug XXX
> + * LPClient is copied three times (see also
test_structural_subscription.js
> + * and test_subscription.js). It should be pushed to a shared test
module.
There seem to be some trailing white-space in the above line.
> + */
You may want to s/XXX//: I suppose you left it like
this as a reminder from reviewer to file a bug. This appears in the
other JS file as well. :)
And I obviously agree: lib/lp/testing is missing a "javascript"
directory :)
...
> +// Notification levels.
> +var DISCUSSION = 'Discussion';
> +var DETAILS = 'Details';
> +var LIFECYCLE = 'Lifecycle';
Noting how we both hard-code these in different parts of JS (I do it in
the JS code and in tests as well), would it make sense to hard-code it
in a single place in eg.
lib/lp/bugs/javascript/bug_notification_level.js, and then use those
variables instead? It wouldn't make much of a difference in grepping
for it, but it'd be at least midly better :)
Not for this branch, because I did it as well, but I wonder if you have
any ideas on the matter?