Launchpad itself

lib/canonical/launchpad/doc/profiling.txt (+5/-21)
lib/lp/app/configure.zcml (+14/-0)
lib/lp/bugs/browser/bugsubscription.py (+59/-15)
lib/lp/bugs/browser/tests/test_bugsubscription_views.py (+65/-0)
lib/lp/bugs/stories/bugs/xx-bug-personal-subscriptions-advanced-features.txt (+42/-0)
lib/lp/services/features/testing.py (+0/-1)
lib/lp/services/memcache/doc/tales-cache.txt (+2/-11)
lib/lp/services/profile/profile.py (+2/-10)
lib/lp/services/profile/tests.py (+44/-54)

To merge this branch:

bzr merge lp://staging/~gmb/launchpad/include-bnl-bug-651108

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Brad Crittenden (community)	code	2010-10-21	Approve on 2010-10-22
Review via email: mp+39047@code.staging.launchpad.net

Commit message

A feature-flagged bug_notififcation_level field has been added to BugSubscriptionSubscribeSelfView.

Description of the change

This branch adds a feature-flagged bug_notification_level field to BugSubscriptionSubscribeSelf View. This is the first step in allowing users to chose what level their direct subscriptions work at.

== lib/lp/bugs/browser/bugsubscription.py ==

- I've added the bug_notification_level field and feature-flagged it.
- I've updated the subscribe_action() and _handleSubscribe() methods to deal with bug_notification_levels correctly.

== lib/lp/bugs/browser/tests/test_bugsubscription_views.py ==

- I've added a test to show that when the malone.advanced-subscriptions.enabled flag is turned on, the bug_notification_level field appears in the UI.

== lib/lp/bugs/stories/bugs/xx-bug-personal-subscriptions-advanced-features.txt ==

- I've added a unit test to show that when the advanced subscriptions features are turned on the bug_notification_level field is used to create a subscription at the appropriate level.

Revision history for this message

Brad Crittenden (bac) wrote on 2010-10-22:

Hi Graham,

Thanks for this branch. I find it particularly interesting as I've not yet done nor reviewed an feature flag branches.

Please run the super new utilities/update-copyright script before landing. I think it only updates the files you've touched, some of which need it.

At line 144 of browser/bugsubscription.py a local variable is assigned but not used. I wonder why lint doesn't catch it? Speaking of, there are some lint issues that appear to be real.

I find the multiple tests in setUpWidgets to be confusing. Please factor out the test for is_subscribed and nest the remaining tests. It should simplify things a bit.

This comment is confusing/misleading as it implies the user can toggle the features:

183 + # When a user subscribes to a bug using the advanced features on
184 + # the Bug +subscribe page, the bug notification level they...

I think this version is more readable and accurate:

183 + # When a user subscribes to a bug with the advanced features on
184 + # the Bug +subscribe page enabled, the bug notification level they...

It is odd the current diff in the MP is showing a conflict in tales-cache.txt but none exists.

review: Approve (code)

Revision history for this message

Brad Crittenden (bac) wrote on 2010-10-22:

Actually, I need to retract my approval because the diff I am shown in the MP is inaccurate. Merging your branch into devel I see a diff of 898 lines where the diff in the MP only shows 283 lines. So assuming mine is correct, I've only reviewed about a 1/4 of your changes.

review: Needs Information (code)

Revision history for this message

Brad Crittenden (bac) wrote on 2010-10-22:

OK, so I just failed to see the pre-req branch. This aspect of it looks good. You *do* have a conflict in the pre-req branch.

review: Approve (code)

Revision history for this message

Graham Binns (gmb) wrote on 2010-10-22:

On Friday, October 22, 2010, Brad Crittenden <email address hidden> wrote:
> Review: Approve code
> OK, so I just failed to see the pre-req branch. This aspect of it looks good. You *do* have a conflict in the pre-req branch.

Thaks for the review. I'll make the changes you suggest. Note that the
prerequisite branch is just a slight modification of one of mars'S
branches, so I may have to just extract the code I need (the test
helper) in order to be able to land this branch.

--
Graham Binns
http://grahambinns.com

lp://staging/~gmb/launchpad/include-bnl-bug-651108 updated on 2010-11-01

11684. By Graham Binns on 2010-10-22: Review changes for brad.
11685. By Graham Binns on 2010-10-22: Reverted all but the FeatureFixture helper.
11686. By Graham Binns on 2010-10-22: De-linted.
11687. By Graham Binns on 2010-10-22: Slight code tweak.
11688. By Graham Binns on 2010-11-01: Merged mainline.