Merge lp:~brian-murray/launchpad/limited-subscriptions-page into lp:launchpad
| Status: | Merged | ||||
|---|---|---|---|---|---|
| Approved by: | Brian Murray on 2010-09-30 | ||||
| Approved revision: | no longer in the source branch. | ||||
| Merged at revision: | 11669 | ||||
| Proposed branch: | lp:~brian-murray/launchpad/limited-subscriptions-page | ||||
| Merge into: | lp:launchpad | ||||
| Diff against target: |
587 lines (+343/-20) 11 files modified
lib/canonical/launchpad/webapp/launchpadform.py (+1/-1) lib/lp/bugs/browser/bugtask.py (+35/-4) lib/lp/bugs/stories/bug-privacy/40-unsubscribe-from-private-bug.txt (+1/-1) lib/lp/bugs/stories/bugs/xx-bug-personal-subscriptions.txt (+9/-8) lib/lp/bugs/templates/bug-subscription.pt (+5/-2) lib/lp/registry/browser/configure.zcml (+6/-0) lib/lp/registry/browser/person.py (+49/-1) lib/lp/registry/browser/tests/test_person_view.py (+32/-2) lib/lp/registry/stories/person/xx-person-subscriptions.txt (+131/-0) lib/lp/registry/stories/productseries/xx-productseries-delete.txt (+1/-1) lib/lp/registry/templates/person-subscriptions.pt (+73/-0) |
||||
| To merge this branch: | bzr merge lp:~brian-murray/launchpad/limited-subscriptions-page | ||||
| Related bugs: |
|
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Curtis Hovey (community) | ui | 2010-09-14 | Approve on 2010-09-29 |
| Guilherme Salgado (community) | ui* | Approve on 2010-09-29 | |
| Graham Binns (community) | code | 2010-09-10 | Approve on 2010-09-14 |
|
Review via email:
|
|||
Commit Message
Add in the start of a +subscriptions page for managing subscriptions in Launchpad. Currently contains direct bug subscriptions.
Description of the Change
= Summary =
This branch is the start of a +subscriptions page for people and teams that will display information about all items that people are subscribed to in Launchpad and allow them to unsubscribe from those items.
As this is the first revision only direct bug subscriptions have been included and the page is not yet linked to.
== Pre-implementation notes ==
Deryck and I talked about this and decided that it would be worthwhile to show duplicate bugs as these can be harder for people to unsubscribe from these. I've also decided to include Invalid as these can receive noisy traffic.
== Implementation details ==
lib/lp/
add in +subscriptions page
lib/lp/
add in PersonSubscript
subscribedB
fix a typo in getSimpleSearch
lib/lp/
created a new test for the +subscriptions page
lib/lp/
created the +subscriptions page
== Tests ==
bin/test -cvvt xx-person-
== Demo and QA ==
Login in as sample person and go to http://
| Guilherme Salgado (salgado) wrote : | # |
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:/
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.
| Curtis Hovey (sinzui) wrote : | # |
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.
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.
| Brian Murray (brian-murray) wrote : | # |
This is the first branch I've done requiring UI review or that is fulfilling a LEP. The LEP is located at - https:/
| Guilherme Salgado (salgado) wrote : | # |
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?
| Deryck Hodge (deryck) 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:/
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:/
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:/
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...
| Guilherme Salgado (salgado) wrote : | # |
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:/
>
> 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:/
>
> 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:/
> 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 ...
| Robert Collins (lifeless) wrote : | # |
+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=
page here" />
| Curtis Hovey (sinzui) wrote : | # |
I am review r11483,
I am seeing some problems as Sample Person when I try to manage my subscriptions
https:/
* 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:/
* 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:/
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.
| Curtis Hovey (sinzui) wrote : | # |
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://
>>> 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_
>>> "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://
>>> bug_sub_table = find_tag_by_id(
... anon_browser.
>>> for tr in bug_sub_
... 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(
>>> sample_
>>> sample_
>>> print sample_
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_
>>> print sample_
Subscriptions: Sample Person
He chooses to unsubscribe from bug 2, "Blackhole Trash folder".
>>> sample_
>>> print sample_
Unsubscribe from bug #2
Sample Person unselects the checkbox and continues.
>>> sample_
>>> sample_
>>> print sample_
Subscriptions: Sample Person
Sample Person can see that bug #2 is not listed in his direct bug
subscriptions.
>>> print extract_
... sample_
Summary In
13 Launchp...
| Brian Murray (brian-murray) wrote : | # |
I believe I've addressed all the issues with the review of this branch and feel it is now ready for review again.
| Guilherme Salgado (salgado) wrote : | # |
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 ReturnToReferre
| Curtis Hovey (sinzui) wrote : | # |
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.
| Guilherme Salgado (salgado) wrote : | # |
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 ReturnToReferre
| Guilherme Salgado (salgado) wrote : | # |
There's one thing which I overlooked in my analysis above: ReturnToReferre
| Guilherme Salgado (salgado) wrote : | # |
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.

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' registry/ browser/ person. py 2010-09-10 12:24:03 +0000 registry/ browser/ person. py 2010-09-10 22:30:12 +0000 ageHeading( ) ionsView( BugTaskSearchLi stingView) : sks(self) : searchTasks( None, user=self.user,
> --- lib/lp/
> +++ lib/lp/
> @@ -2396,6 +2397,42 @@
> return self.getSearchP
>
>
> +class PersonSubscript
> + """All the subscriptions for a person."""
> +
> + page_title = 'Subscriptions'
> +
> + def subscribedBugTa
> + """Return a BatchNavigator for distinct bug tasks to which the
> + person is subscribed."""
> + bugtasks = self.context.
bugtasks => bug_tasks.
> + order_by= '-date_ last_updated' , (BugTaskStatus. NEW, INCOMPLETE, CONFIRMED, TRIAGED, INPROGRESS, FIXCOMMITTED, FIXRELEASED) )
> + status=
> + BugTaskStatus.
> + BugTaskStatus.
> + BugTaskStatus.
> + BugTaskStatus.
> + BugTaskStatus.
> + BugTaskStatus.
You can outdent all of this by four spaces; we use four-space indents
per level in method calls.
> + append( task) append( task.bug)
> + sub_bugtasks = []
> + sub_bugs = []
> +
> + for task in bugtasks:
> + if task.bug not in sub_bugs:
> + sub_bugtasks.
> + sub_bugs.
Is this loop going to cause performance problems for people with a large
number of direct subscriptions?
> + return BatchNavigator( sub_bugtasks, self.request) sPageHeading( self): displayname ptionsPageHeadi ng() iew(LaunchpadFo rmView) : registry/ stories/ person/ xx-person- subscriptions. txt' registry/ stories/ person/ xx-person- subscriptions. txt 1970-01-01 00:00:00 +0000 registry/ stories/ person/ xx-person- subscriptions. txt 2010-09-10 22:30:12 +0000 launchpad. dev/~ubuntu- team/+subscript ions') text(find_ main_content( anon_browser. contents) )
> +
> + def getSubscription
> + """The header for the subscriptions page."""
> + return "Subscriptions for %s" % self.context.
> +
> + @property
> + def label(self):
> + return self.getSubscri
> +
> +
> class PersonVouchersV
> """Form for displaying and redeeming commercial subscription vouchers."""
>
> === added file 'lib/lp/
> --- lib/lp/
> +++ lib/lp/
> @@ -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://
> + >>> 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_
> + >>> "does not have any direct bug subscriptions" in page_text
> + Tru...