Launchpad itself

Merge lp://staging/~gary/launchpad/bug-772754-2 into lp://staging/launchpad

bug-772754-2
Merge into devel

Proposed by Gary Poster on 2011-06-03

Status:	Merged
Approved by:	Данило Шеган on 2011-06-06
Approved revision:	no longer in the source branch.
Merged at revision:	13243
Proposed branch:	lp://staging/~gary/launchpad/bug-772754-2
Merge into:	lp://staging/launchpad
Prerequisite:	lp://staging/~gary/launchpad/bug792493
Diff against target:	2182 lines (+1238/-638) 15 files modified lib/canonical/launchpad/icing/style.css (+10/-0) lib/lp/bugs/browser/bug.py (+88/-57) lib/lp/bugs/browser/configure.zcml (+1/-1) lib/lp/bugs/browser/structuralsubscription.py (+14/-21) lib/lp/bugs/browser/tests/test_bug_views.py (+1/-13) lib/lp/bugs/javascript/bug_subscription_portlet.js (+385/-0) lib/lp/bugs/javascript/bugtask_index_portlets.js (+0/-519) lib/lp/bugs/javascript/tests/test_bug_subscription_portlet.html (+42/-0) lib/lp/bugs/javascript/tests/test_bug_subscription_portlet.js (+637/-0) lib/lp/bugs/javascript/tests/test_subscription.js (+4/-2) lib/lp/bugs/model/bug.py (+1/-1) lib/lp/bugs/model/structuralsubscription.py (+4/-5) lib/lp/bugs/stories/bugs/xx-bug-personal-subscriptions.txt (+14/-7) lib/lp/bugs/templates/bug-portlet-subscription.pt (+36/-12) lib/lp/registry/javascript/structural-subscription.js (+1/-0)
To merge this branch:	bzr merge lp://staging/~gary/launchpad/bug-772754-2
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Данило Шеган (community)		2011-06-03	Approve on 2011-06-06
Review via email: mp+63444@code.staging.launchpad.net

Description of the change

This branch implements the subscriptions portlet part of the design from bug 772754. I adjusted the design a bit as I worked on it, but it's close to what I had there.

You can think of this diff in three parts:
(A) rip out the old subscription portlet code, including the bits that integrate with the subscriber portlet;
(B) change the rendering of the subscription portlet to match the new design, with or without JS; and
(C) add new subscription portlet JS code to handle subscriptions.

I'll divide up the diff into those three parts.

(A) rip out the old subscription portlet code

lib/lp/bugs/javascript/bugtask_index_portlets.js
Danilo's changes will make this file disappear!

(B) change the rendering of the subscription portlet

lib/canonical/launchpad/icing/style.css
    (No comment)
lib/lp/bugs/browser/configure.zcml
    We use a new view class so we can specify just what we need.
lib/lp/bugs/browser/bug.py
    Note that we are sharing the new strings with JavaScript by dumping them in the JSONRequestCache.
    Note that user_should_see_mute_link is now an attribute rather than a property. The previous tests remain.
    Note that we use an existing structural subscription function to determine whether the user has any pertinent structural subscriptions. That function had to be modified to return a result set. That function change resulted in changes in these files:
  lib/lp/bugs/model/structuralsubscription.py
  lib/lp/bugs/browser/bugsubscription.py
  lib/lp/bugs/model/bug.py
lib/lp/bugs/browser/tests/test_bug_views.py
    I had to make the test helper a bit more robust because I was using much fewer classes, and sometime none at all.
lib/lp/bugs/stories/bugs/xx-bug-personal-subscriptions.txt
    (No comment)
lib/lp/bugs/templates/bug-portlet-subscription.pt
    (No comment)

lib/lp/bugs/javascript/bugtask_index_subscription_portlet.js
    setup_subscription_link_handlers looks a bit hairy at first glance, but I think the main reason is that it starts by setting up the DOM nodes we'll need. After that it is pretty straightforward; and other than that, it is relatively short.
lib/lp/bugs/javascript/tests/test_bugtask_index_subscription_portlet.html
    (No comment)
lib/lp/bugs/javascript/tests/test_bugtask_index_subscription_portlet.js
    I copied LPClient yet again. We really should put that somewhere else, and better document it, and maybe polish it to make it easier to use/understand. I didn't want to do that now. I also noted the problem in lib/lp/bugs/javascript/tests/test_subscription.js .

Beyond those bits, I had a single crazy whitespace change: lib/lp/registry/javascript/structural-subscription.js .

Thank you!

Gary

Revision history for this message

Данило Шеган (danilo) wrote on 2011-06-06:

Download full text (14.7 KiB)

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

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 '<x/>'
instead of '<x></x>', 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('<span> </span>');
> +}

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/<real bug number>/: 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?

review: Approve

Revision history for this message

Gary Poster (gary) wrote on 2011-06-08:

Download full text (18.7 KiB)

On Jun 6, 2011, at 7:43 AM, Данило Шеган wrote:

> Review: Approve
> 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

Thank you Danilo. Comments inline below.

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

Some existing ones are there. I modified them slightly to look at some of the new attributes.

>
> Also, I ain't sure if you could have used BugViewMixin instead of
> BugView directly. Worth checking, imo.

Yeah, that worked. Done, thank you.

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

We talked about this. Practically, we agreed that "Edit bug mail" would be sufficient for now: for my portlet, it will just be another subscription, and you will not list the user in your portlet for any reason. If people need the duplicate functionality, we can look into it when they file a bug.

On Jun 6, 2011, at 7:43 AM, Данило Шеган wrote:

> Review: Approve
> 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

Thank you Danilo.  Comments inline below.

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

Some existing ones are there.  I modified them slightly to look at some of the new attributes.

> 
> Also, I ain't sure if you could have used BugViewMixin instead of
> BugView directly.  Worth checking, imo.

Yeah, that worked.  Done, thank you.

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

We talked about this.  Practically, we agreed that "Edit bug mail" would be sufficient for now: for my portlet, it will just be another subscription, and you will not list the user in your portlet for any reason.  If people need the duplicate functionality, we can look into it when they file a bug.

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

I made the changes that I believe you were describing.

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

I made this change.

> 
>> +
>> +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? :)

I agree.  I have not filed a bug, though: if we start using it multiple times, we can promote it. I plan to talk about it at the Thunderdome.

> 
> Also, I remember that some tags cause problems when added as '<x/>'
> instead of '<x></x>', 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.

Changed.  I'm not sure it matters when you are building nodes--I believe that benji has reported repeatedly that it is unnecessary in his experiments--but what the heck, it's easy to do.

> 
>> +
>> +/*
>> + * 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.

Done.

> 
>> +function ws() {
>> +    return Y.Node.create('<span> </span>');
>> +}
> 
> Perhaps implement this in terms of tag()? No big deal though.

I thought I couldn't do this--that is, I thought the whitespace would be stripped.  It turns out that it worked just fine in my experiments, so I did as you suggested.

> 
>> +
>> +/*
>> + * 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 :)

:-) deleted.

> 
>> +    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 ;)

Yeah yeah yeah.  :-)  It is supposed to delineate generic helpers from bits that are specific to the module.  I left it for now since all you did was say something sarcastic, but I can omit if you like.  :-)

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

Yeah, good idea.  Done.

> 
> ...
>> +
>> +/*
>> + * 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. :)

Heh.

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

As you know, I started refactoring several times but just kept liking what I did initially.  So I appreciate your perspective, but it didn't work for me this time around.

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

As you said, unnecessary here, though a good practice.  I again started a refactoring, but I didn't like the way it went.  Maybe I'll be inspired by your branches. :-)

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

:-)  As we said, given my current factoring, it needed the local binding of clean_up, or else I would need to pass things around or make a class.

> 
>> +    // 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? :)

Eh.  :-)  Note that I need make_action, which needs clean_up, or else I go back to passing things around, which I just was trying to avoid this time around.

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

As you know, I thought about this quite a bit and tried to engage others.  For now, I think things are better than what you saw before, though we have not moved to radio buttons.

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

I simplified this, but did not extract it.

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

FWIW, I thought about this point specifically, and the tests I have I think do pretty well here.  Your counter point when we talked was that setup could be simpler, which I agree with.  My counterpoint is that I like the fuller integration in the test, but I acknowledge that I am in the minority in valuing/preferring this, and that even I would like some of both.

That said, your position is a well-reasoned whole, of which this is only a part.  I'll continue to consider it, and I'm pretty sure that your position corresponds better with our agreed-upon best practices than mine.

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

Cleaned.

> 
>> + */
> 
> You may want to s/XXX/<real bug number>/: 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. :)

Added a bug, and updated.  Sorry, I was supposed to catch that before you, but yes, the "XXX" was a failsafe. :-)

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

I've had experience with a framework that overfactored things, by wide consensus.  You might be familiar with it. :-)  As such, my opinion is that this is small enough with little enough duplication that duplicating it is OK.  Francis' rule of thumb is three dupes == a problem.  I like that.  So I'm not worried about it now.  If we do consider a solution in the future, I'd like to keep us from having files that are too small: I'd prefer a bug module constants file, for instance.

Thanks again!

Gary

Revision history for this message

Данило Шеган (danilo) wrote on 2011-06-09:

У сре, 08. 06 2011. у 18:24 +0000, Gary Poster пише:
>
> That said, your position is a well-reasoned whole, of which this is
> only a part. I'll continue to consider it, and I'm pretty sure that
> your position corresponds better with our agreed-upon best practices
> than mine.

You must also understand that I am coming from a background where I had
to fight numerous historical Rosetta corner-case tests from pagetests
and doctests, and thus I am a bit over-sensitive in the other
direction :)

Anyway, the branch is now even more good-to-go :)