Launchpad itself

Merge lp://staging/~danilo/launchpad/bug-772754-other-subscribers-remove-cruft into lp://staging/launchpad

bug-772754-other-subscribers-remove-cruft
Merge into devel

Proposed by Данило Шеган on 2011-06-10

Status:

Merged

Merged at revision:

13243

Proposed branch:

lp://staging/~danilo/launchpad/bug-772754-other-subscribers-remove-cruft

Merge into:

lp://staging/launchpad

Prerequisite:

lp://staging/~danilo/launchpad/bug-772754-other-subscribers-actions

Diff against target:

3223 lines (+122/-2659)

25 files modified

lib/lp/app/javascript/picker.js (+5/-3)
lib/lp/bugs/browser/bug.py (+0/-18)
lib/lp/bugs/browser/bugsubscription.py (+6/-59)
lib/lp/bugs/browser/configure.zcml (+0/-17)
lib/lp/bugs/browser/tests/bug-portlet-subscribers-content.txt (+0/-25)
lib/lp/bugs/browser/tests/bug-subscription-views.txt (+0/-40)
lib/lp/bugs/browser/tests/test_bug_views.py (+0/-11)
lib/lp/bugs/browser/tests/test_bugsubscription.py (+0/-23)
lib/lp/bugs/browser/tests/test_bugsubscription_views.py (+0/-53)
lib/lp/bugs/javascript/bugtask_index.js (+2/-4)
lib/lp/bugs/javascript/bugtask_index_portlets.js (+0/-897)
lib/lp/bugs/javascript/subscriber.js (+0/-390)
lib/lp/bugs/javascript/subscribers_list.js (+0/-66)
lib/lp/bugs/javascript/tests/test_subscriber.html (+0/-104)
lib/lp/bugs/javascript/tests/test_subscriber.js (+0/-288)
lib/lp/bugs/javascript/tests/test_subscribers_list.js (+1/-318)
lib/lp/bugs/stories/bug-privacy/05-set-bug-private-as-admin.txt (+7/-13)
lib/lp/bugs/stories/bugs/bug-add-subscriber.txt (+29/-32)
lib/lp/bugs/stories/bugs/xx-bug-personal-subscriptions.txt (+45/-125)
lib/lp/bugs/templates/bug-portlet-dupe-subscribers-content.pt (+0/-49)
lib/lp/bugs/templates/bug-portlet-subscribers-content.pt (+0/-74)
lib/lp/bugs/templates/bug-portlet-subscribers.pt (+0/-10)
lib/lp/bugs/templates/bug-portlet-subscription.pt (+0/-3)
lib/lp/bugs/templates/bugtask-index.pt (+1/-14)
lib/lp/bugs/tests/bug.py (+26/-23)

To merge this branch:

bzr merge lp://staging/~danilo/launchpad/bug-772754-other-subscribers-remove-cruft

High

Fix Released

Link a bug report

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

Description of the change

= Bug 772754: Other subscribers list, part 7 =

NOTE: Not much review will be needed (this is just removal of all the unneeded bits and pieces). Final step, all the tests are passing, and I'll be landing this branch when all the others have been reviewed.

This is a final part of ongoing work for providing the "other subscribers" list as indicated in mockup https://launchpadlibrarian.net/71552495/all-in-one.png attached to bug 772754 by Gary.

== Tests ==

bin/test -m lp.bugs

== Demo and Q/A ==

N/A

Lint is clean.

Revision history for this message

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

Hi Danilo. I'm using this branch for a UX review, so I'm going to make comments here that can probably be addressed in earlier branches if you like.

It looks good!

In the mockup, "Maybe notified" goes last in the portlet, while in your branch, at least in https://bugs.launchpad.dev/firefox/+bug/1 , it comes before "Notified of all changes". I still think that we should show categories from the most connected to the least--so "Notified of all changes" should come first, followed by METADATA, followed by LIFECYCLE, followed by "Maybe notified". If the change you made from the mockup was intentional, so be it--I won't be around for you to convince me :-) .

In the mockup, there is a lot of text describing what "Maybe notified" means. I do prefer to not have that text around, as you have done, but I am inclined to say that we should have a help link with that information. If you disagree, again, since I'll be absent, I'm a pushover.

I'd prefer the above two changes. This next comment is about a change that you made that I think is for the best. In the mockup, I showed pencil icons for teams that the user could modify. However, as you've noted in the past, we do not let teams change their subscription levels, so including that here would be scope creep. So, +1 on omitting that.

That's it. Thank you!

Revision history for this message

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

Hi Gary, thanks for looking at this branch.

The first one should have already been fixed (there are even tests to prove it, but there was a bug in the ordering logic that allowed it to sometimes be misordered; both tests and code have been improved to catch this case). If it's not in this branch yet, sorry about it (that's one thing I wish pipelines made easier: pushing _all_ branches up).

As for the second, I omitted it on purpose: long text tends to not be read. As far as the help link, I'd like that, but I'll have to get help from Matt (or someone else) on the text, so I am leaving that for a next branch.

As for the third thing, the code is structured in a way that it should be easy to add another "action" (of which unsubscribe is only one option offered atm) which allows one to change the level of the subscription, but since we don't have that for teams yet anyway, I didn't want to implement it here. IOW, adding the icon/action should be easy, making it do the right thing is out of scope.

Revision history for this message

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

Marking this one as approved, it's just a removal of all the unused stuff. All tests are passing in ec2.

review: Approve