Merge lp:~brian-murray/launchpad/limited-subscriptions-page into lp:launchpad

Hi Brian,

I like this branch; nice work. Couple of minor changes needed, but
nothing that isn't just nitpicking. I've also posed a question about
performance, but I'm not sure that we can do much about at this point.

You should get a UI review for this branch from one of our sainted UI
reviewers before you land it.

> === modified file 'lib/lp/registry/browser/person.py'
> --- lib/lp/registry/browser/person.py 2010-09-10 12:24:03 +0000
> +++ lib/lp/registry/browser/person.py 2010-09-10 22:30:12 +0000
> @@ -2396,6 +2397,42 @@
> return self.getSearchPageHeading()
>
>
> +class PersonSubscriptionsView(BugTaskSearchListingView):
> + """All the subscriptions for a person."""
> +
> + page_title = 'Subscriptions'
> +
> + def subscribedBugTasks(self):
> + """Return a BatchNavigator for distinct bug tasks to which the
> + person is subscribed."""
> + bugtasks = self.context.searchTasks(None, user=self.user,

bugtasks => bug_tasks.

> + order_by='-date_last_updated',
> + status=(BugTaskStatus.NEW,
> + BugTaskStatus.INCOMPLETE,
> + BugTaskStatus.CONFIRMED,
> + BugTaskStatus.TRIAGED,
> + BugTaskStatus.INPROGRESS,
> + BugTaskStatus.FIXCOMMITTED,
> + BugTaskStatus.FIXRELEASED))

You can outdent all of this by four spaces; we use four-space indents
per level in method calls.

> +
> + sub_bugtasks = []
> + sub_bugs = []
> +
> + for task in bugtasks:
> + if task.bug not in sub_bugs:
> + sub_bugtasks.append(task)
> + sub_bugs.append(task.bug)

Is this loop going to cause performance problems for people with a large
number of direct subscriptions?

> + return BatchNavigator(sub_bugtasks, self.request)
> +
> + def getSubscriptionsPageHeading(self):
> + """The header for the subscriptions page."""
> + return "Subscriptions for %s" % self.context.displayname
> +
> + @property
> + def label(self):
> + return self.getSubscriptionsPageHeading()
> +
> +
> class PersonVouchersView(LaunchpadFormView):
> """Form for displaying and redeeming commercial subscription vouchers."""
>
> === added file 'lib/lp/registry/stories/person/xx-person-subscriptions.txt'
> --- lib/lp/registry/stories/person/xx-person-subscriptions.txt 1970-01-01 00:00:00 +0000
> +++ lib/lp/registry/stories/person/xx-person-subscriptions.txt 2010-09-10 22:30:12 +0000
> @@ -0,0 +1,56 @@
> +Subscriptions View
> +==================
> +
> +Direct bug subscriptions
> +
> +Each person and team has a subscriptions page that lists things to which they
> +are directly subscribed.
> +
> + >>> anon_browser.open('http://launchpad.dev/~ubuntu-team/+subscriptions')
> + >>> print anon_browser.title
> + Subscriptions : “Ubuntu Team” team
> +
> +The bug subscriptions table does not appear for people without any direct bug
> +subscriptions.
> +
> + >>> page_text = extract_text(find_main_content(anon_browser.contents))
> + >>> "does not have any direct bug subscriptions" in page_text
> + Tru...

Hi Brian,

It's great to see that work is being done on this long-awaited feature, but I was expecting to see some mockups for what we expect this page to look like and how it'd work. Something similar to what the Soyuz team is doing with derivative distributions. Do you know if there's a LEP for this? Or maybe just some mockups? I think it's important to work on those (maybe with help from the Design team, as described in https://dev.launchpad.net/UI/Design) for this feature, as the current approach may not scale well once we start adding the other types of items that a person can subscribe to this page.

Regardless of that, here's some feedback on the current implementation.

In the long term I'm sure we want to use ajax to unsubscribe without leaving the page, and given how easy it is not much more work to do so, I think it'd make sense to do that from the start. That would probably make an undo functionality more important, but I think we can live without that for now as that'd complicate things a bit more.

Also, upon clicking the unsubscribe link, you're taken to the bug's +subscribe page, and clicking Cancel there will take you to the bug's +index page rather than to +subscriptions as one would expect. This wouldn't be a problem if we were using ajax.

The page should not include the unsubscribe buttons when looking at somebody else's page, as the only thing you can do is unsubscribe yourself but not the person whose page you're looking at. Either that or make the page not available for other users.

For teams, the unsubscribe link takes you to the bug's +subscribe page but if you're not currently subscribed to that bug, the default option (and the heading) is for you to subscribe yourself rather than to unsubscribe the team. When you are subscribed, the default options is to unsubscribe yourself. This is a bit confusing, but we could avoid this completely by using ajax to unsubscribe directly from +subscriptions.

I think Salgado is review of the UI is very good. This is a much needed feature, but it introduces a lot of UI confusion. This feature needs a feature flag to ensure lpnet users do not see this until it is ready.

This UI will get complicated in the next year because we do need to move blueprint and answers subscriptions into this page. I think AJAX is going to be a requirement by then.

I do not think this can land until some of the obvious UI confusion is addressed.

You can use this mixin so that the view returns the user to the correct page:
    from canonical.launchpad.webapp.launchpadform import ReturnToReferrerMixin
But I think switching to AJAX will be the only way to address the clarity of the operation that the user needs to accomplish--users can be in 50 teams and that will distract the user from his task.

Do not show me remove icon when the page is an edit operations (switching to AJAX will fix this).

Do not show me edit operations for other users and teams I am not a member of.

The story is not a story. It is not written from the perspective of a user who visits Lp to accomplish a task. I expect to see the user arrive at his profile page, choose the link to +subscriptions, then use a remove link to remove the subscription, then he sees the bug is not listed. I suppose we want a similar story for removing a subscription for a user's team. The conditions for not showing the bug listing belong in a test for the view

I have read the story and I have no idea how I will discover the link to this page. I assume we are not publishing the link in pages to prevent users from seeing the link until we are ready. If this is the case you need an XXX in the story explaining why the user does not start on the page that directs the user to look at his subscriptions. The other course is to use a feature flag that hides the link and the page until we are ready.

This is the first branch I've done requiring UI review or that is fulfilling a LEP. The LEP is located at - https://dev.launchpad.net/Research/Bugs/Subscriptions2010.

That looks more like a report from user testing sessions than a LEP. I
was expecting something that would clearly say how the UI should
look/behave. Is that the only document you're using to drive the
implementation of this? Maybe it'd be worth writing a LEP first?

Hi, salgado and sinzui.

We do have a LEP for this feature. This +subscriptions page is one bit of the "Better Subscriptions and Notifications" story we're currently doing development work on. The LEP is at:

https://dev.launchpad.net/LEP/BetterBugSubscriptionsAndNotifications

This is an old LEP now, and I think we've refined the process a bit since writing this one, so maybe some elements are missing that should be there. This is not Brian's fault, so please don't block Brian for this reason. Let me know if you need something there, and I'll add it.

The link Brian provided is the result of the user research we did with mrevell for all of the UI elements within this story.

https://dev.launchpad.net/Research/Bugs/Subscriptions2010

A bit of history here may help with some of your concerns, too. We did try to work with the DUX team on this feature. Francis, Jono, Tom, Graham, and I did a fair amount of coordination work, and in the end we spent two months chasing meetings, which slowed the progress of this feature. We did get valuable feedback from a couple sessions with Alejandra about subscriptions UI elements, and this +subscriptions page is a direct result of that feedback. We also then did two rounds of user testing on these mockups with mrevell. I think we've more than covered ourselves in terms of thinking about the UI before beginning to implement it.

Having said that, I don't think https://dev.launchpad.net/UI/Design should be consider the gospel for UI procedure, at least the initial pre-imp recommendation in its current form. We on the bugs team in particular are trying to experiment with approaches to UI design to achieve quicker turnaround on both producing mockups and receiving feedback. I think a pre-imp with a UI reviewer should be considered the same as for a code reviewer -- recommended, but the designer/programmer can use his/her own discretion. We also cannot block on speaking with the DUX team as my team has already proven this will delay work beyond a reasonable amount of time. Mockups, of course, are a reasonable requirement. Regardless, in this case, we've covered all our bases. ;)

Finally, as to some of your more technical concerns. I have asked Brian to land this page without any Ajax enabled widgets initially. The subscription widget is not easily re-usable, and it would require one or two more branches to get widgets enabled for this page. This is meant to be the very first step in this process for Brian. He has many more branches to go until this work is completed -- adding Ajax widgets, adding sections for every item we support a subscription on, good paginating for these things, performance tuning every step of the way, and finally making a link available to users so they become aware of this page. I would prefer we don't add a link and hide it with a feature flag since you guys are correct that we haven't yet thought about where this link should go. I think there is value in telling a few key stakeholders about this page as we work on it, and this seems simpler to me than feature flagging it, at least at this point in the life of the feature flag system. If we do require this, let's have...

On Mon, 2010-09-20 at 13:52 +0000, Deryck Hodge wrote:
> Hi, salgado and sinzui.
>
> We do have a LEP for this feature. This +subscriptions page is one
> bit of the "Better Subscriptions and Notifications" story we're
> currently doing development work on. The LEP is at:
>
> https://dev.launchpad.net/LEP/BetterBugSubscriptionsAndNotifications
>
> This is an old LEP now, and I think we've refined the process a bit
> since writing this one, so maybe some elements are missing that should
> be there. This is not Brian's fault, so please don't block Brian for
> this reason. Let me know if you need something there, and I'll add
> it.
>
> The link Brian provided is the result of the user research we did with
> mrevell for all of the UI elements within this story.
>
> https://dev.launchpad.net/Research/Bugs/Subscriptions2010
>
> A bit of history here may help with some of your concerns, too. We
> did try to work with the DUX team on this feature. Francis, Jono,
> Tom, Graham, and I did a fair amount of coordination work, and in the
> end we spent two months chasing meetings, which slowed the progress of
> this feature. We did get valuable feedback from a couple sessions
> with Alejandra about subscriptions UI elements, and this
> +subscriptions page is a direct result of that feedback. We also then
> did two rounds of user testing on these mockups with mrevell. I think
> we've more than covered ourselves in terms of thinking about the UI
> before beginning to implement it.

Cool, that's good to know. I asked for a LEP because I'd expect it to
either contain some of this information or at show me that this UI (and
possibly others) had been thought through. By looking at the merge
proposal this was not clear.

>
> Having said that, I don't think https://dev.launchpad.net/UI/Design
> should be consider the gospel for UI procedure, at least the initial
> pre-imp recommendation in its current form. We on the bugs team in
> particular are trying to experiment with approaches to UI design to
> achieve quicker turnaround on both producing mockups and receiving
> feedback. I think a pre-imp with a UI reviewer should be considered
> the same as for a code reviewer -- recommended, but the
> designer/programmer can use his/her own discretion. We also cannot
> block on speaking with the DUX team as my team has already proven this
> will delay work beyond a reasonable amount of time. Mockups, of
> course, are a reasonable requirement. Regardless, in this case, we've
> covered all our bases. ;)

I completely agree.

Someone (I think Curtis) said that UI reviews are more about making sure
people think carefully about UI changes rather than anything else, and
that's what I was trying to do by pointing to the wiki page. Again, the
merge proposal had no indication any of this was done.

>
> Finally, as to some of your more technical concerns. I have asked
> Brian to land this page without any Ajax enabled widgets initially.
> The subscription widget is not easily re-usable, and it would require
> one or two more branches to get widgets enabled for this page. This
> is meant to be the very first step in this process for Brian. He has
> many ...

+1 on not exposing this until we're fairly comfortable with it.

You can enable it via a feature flag to permit user testing as we go:

<a tal:condition="features/bugs.subscriptions_page" href="link to the
page here" />

I am review r11483,

I am seeing some problems as Sample Person when I try to manage my subscriptions
    https://launchpad.dev/~name12/+subscriptions
 * I choose to remove the blackhole subscription. The link to edit it shows a radio button. I cannot unselected it. I can change it.
 * If The cancel link goes to the bug, not +subscriptions

Check my teams (as Sample Person)
    https://launchpad.dev/~hwdb-team/+subscriptions
 * I cannot unselect the radio button for the team. I can of course select myself to deselect the team, but I do not want to subscribe to the bug
 * Again the cancel link the bug, not +subscriptions

As anonymous when I see https://launchpad.dev/~hwdb-team/+subscriptions I read
    HWDB Team does not have any direct bug subscriptions.
^ that is great, I think the text for the bug listing table should make it clear I am seeing "direct bug subscriptions"

I think part of the problem here is that I am reading the story, but it is clearly not a story. A story is from a users perspective and explains his experience performing a task. There are not tasks completed in this story, and when I try to complete what I think Sample Person is doing, I fail. I will reply in about 30 minutes with a revised story I what I think the users' goals are.

This is closer to a proper story and described the goals that Sample Person uses
+subscriptions to accomplish.

Subscriptions View
==================

XXX: bdmurray 2010-09-24 bug=628411 This story is complete until we publish
the link that leads to the +subscriptions page.

XXX: A story is written from a user's perspective. It explains how a user
accomplishes a task and how he know it. Use user names and rules when
telling the story. Stories do not test failure,
permissions, or method functions--that is the domain of unittests.

Any user can view the direct subcriptions that a person or team has to
bugs, blueprints, questions, branches, and merge proposals.

    >>> anon_browser.open('http://launchpad.dev/~ubuntu-team/+subscriptions')
    >>> print anon_browser.title
    Subscriptions : “Ubuntu Team” team

The user can see that the Ubuntu Team does not have an direct bug
subscritions.

    >>> page_text = extract_text(find_main_content(anon_browser.contents))
    >>> "does not have any direct bug subscriptions" in page_text
    True

Any user can see that that Sample Person is subscribed to several bugs.
The bug subscriptions table includes the bug number, title and location.

    >>> anon_browser.open('http://launchpad.dev/~name12/+subscriptions')
    >>> bug_sub_table = find_tag_by_id(
    ... anon_browser.contents, 'bug_subscriptions')
    >>> for tr in bug_sub_table.findAll('tr'):
    ... print extract_text(tr)
    Summary
    In
    13
    Launchpad CSS and JS is not testible
    Launchpad
    5
    Firefox install instructions should be complete
    ...
    4
    Reflow problems with complex page layouts
    Mozilla Firefox
    2
    Blackhole Trash folder
    ...
    9
    Thunderbird crashes
    thunderbird (Ubuntu)

Sample Person can see his see his direct bug subscriptions too, and he can
use the page manage his direct subscriptions. He chooses to view
bug 13, "Launchpad CSS and JS is not testible".

    >>> sample_browser = setupBrowser(auth='Basic <email address hidden>:test')
    >>> sample_browser.open('http://bugs.launchpad.dev/~name12/+subscriptions')
    >>> sample_browser.getLink(id='unsubscribe-subscriber-12').click()
    >>> print sample_browser.title
   Unsubscribe from bug #13

After viewing the +subscribe page Sample Person decides that he want to
reamain subscribed to bug 13. He uses the cancel link to

    >>> sample_browser.getLink('Cancel').click()
    >>> print sample_browser.title
    Subscriptions: Sample Person

He chooses to unsubscribe from bug 2, "Blackhole Trash folder".

    >>> sample_browser.getLink(id='unsubscribe-subscriber-2').click()
    >>> print sample_browser.title
   Unsubscribe from bug #2

Sample Person unselects the checkbox and continues.

    >>> sample_browser.getControl('Unsubscribe me from this bug') = False
    >>> sample_browser.getControl('Continue').click()
    >>> print sample_browser.title
    Subscriptions: Sample Person

Sample Person can see that bug #2 is not listed in his direct bug
subscriptions.

    >>> print extract_text(find_tag_by_id(
    ... sample_browser.contents, 'bug_subscriptions'))
    Summary In
    13 Launchp...

I believe I've addressed all the issues with the review of this branch and feel it is now ready for review again.

Thanks for fixing the things I'd reported, Brian. The only remaining issue I see is the 'Cancel' link on +subscribe which still sends me to the bug's page, but that might be a bug on ReturnToReferrerMixin or just an oversight on your side. If it's the former, it's fine to just file a bug, but if it's the latter it should be trivial to fix.

Salgado, Brian BugTaskView's cancel url is not hooked up the same way that next_url is. I think this is a simple property, or it may be cancel_url = next_url in the view.

Thanks for making the changes Brian. I think this is good to land.

The Continue button shows the same behaviour for me, but what is weird is that they work as we expect in the test.

I couldn't figure out what is going on, but this might be related to the fact that ReturnToReferrerMixin must be mixed into a LaunchpadFormView, but here it was mixed into a LaunchpadView. That mixin works by storing a hidden field (_return_url) in the page's form and mapping that field to a view property with the same name. Here the hidden field is not stored automatically by the mixin as the page does not use launchpad-form.pt (since it's not a LaunchpadFormView), so the hidden field is created manually. The hidden field created manually has a different name, though ('next_url'), so the mixin's _return_url property would always return None. That's consistent with the behaviour I see on the two browsers I've tested (chromium and epiphany), but is not consistent with the behaviour shown by the tests.

There's one thing which I overlooked in my analysis above: ReturnToReferrerMixin._return_url will return self.request.getHeader('referer') when a _return_url is not present in the request. So there must be something that is stripping referrers out of my requests. I'll figure it out; sorry for the false alarm.

Damn it, found what's going on!

The test passes because it accesses the +subscription page on the bugs vhost. It works for me if I use that vhost as well.

If you use the main vhost, it won't work. I think this is a problem because this page will mix content from multiple apps, so it should probably be accessible on the main vhost.