Merge lp://staging/~mbp/launchpad/feature-modulus into lp://staging/launchpad

Proposed by Martin Pool
Status: Merged
Approved by: Martin Pool
Approved revision: no longer in the source branch.
Merged at revision: 14510
Proposed branch: lp://staging/~mbp/launchpad/feature-modulus
Merge into: lp://staging/launchpad
Prerequisite: lp://staging/~mbp/launchpad/featurefixture-from-request
Diff against target: 183 lines (+110/-11)
2 files modified
lib/lp/services/features/scopes.py (+48/-10)
lib/lp/services/features/tests/test_scopes.py (+62/-1)
To merge this branch: bzr merge lp://staging/~mbp/launchpad/feature-modulus
Reviewer Review Type Date Requested Status
Gavin Panella (community) Approve
Review via email: mp+84890@code.staging.launchpad.net

This proposal supersedes a proposal from 2011-12-01.

Commit message

[r=allenap][bug=666544] Add a userslice:x,y feature scope, so that 1% of the users can enjoy 80% of the features.

Description of the change

Add a userslice:x,y feature scope, so that 1% of the users can enjoy 80% of the features.

See https://bugs.launchpad.net/launchpad/+bug/666544

To post a comment you must log in.
Revision history for this message
Martin Pool (mbp) wrote : Posted in a previous version of this proposal

This doesn't do anything much towards getting closed loop metrics from it. We could do something either in js or the webapp or both. Probably the webapp should just always log all the active features.

Revision history for this message
Martin Pool (mbp) wrote : Posted in a previous version of this proposal

https://bugs.launchpad.net/launchpad/+bug/666544 (which I didn't have access to when i wrote this offline) suggests that I need to test it for anonymous requests, and secondly that we can do a follow-on thing, or a separate scope, that activates things for a percentage of sessions, which will help for anon users.

Revision history for this message
Gavin Panella (allenap) wrote : Posted in a previous version of this proposal

Very nice :)

[1]

+ def __init__(self, get_person):
+ self._get_person = get_person
+ self._person = None

I don't think the _person attribute is used anywhere.

[2]

+ def test_user_modulus(self):
+ person = self.factory.makePerson()

This is somewhere a test double would work well. Something like:

    class person:
        id = 123

would mean that the test could run without a layer specified (and the
base class would have to be TestCase instead of -WithFactory).

I guess it might be better to leave it as it is though, if new tests
will be added in subsequent branches.

review: Approve
Revision history for this message
Martin Pool (mbp) wrote : Posted in a previous version of this proposal

> Very nice :)
>
>
> [1]
>
> + def __init__(self, get_person):
> + self._get_person = get_person
> + self._person = None
>
> I don't think the _person attribute is used anywhere.

thanks.

>
>
> [2]
>
> + def test_user_modulus(self):
> + person = self.factory.makePerson()
>
> This is somewhere a test double would work well. Something like:
>
> class person:
> id = 123
>
> would mean that the test could run without a layer specified (and the
> base class would have to be TestCase instead of -WithFactory).
>
> I guess it might be better to leave it as it is though, if new tests
> will be added in subsequent branches.

I see what you mean. Probably I should also have a larger test that the scope is correctly detected from the request, by doing a request with a person logged in.

Revision history for this message
Martin Pool (mbp) wrote :

building on https://code.launchpad.net/~mbp/launchpad/featurefixture-from-request/+merge/84887 this now adds a nice realistic integration test (imo). can i get another review?

Revision history for this message
Gavin Panella (allenap) wrote :

Even better :)

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.