Hi, thanks for putting so much work into the review.  Everything I
haven't commented on I agree with.

> The UI isn't very nice, but since this is meant for internal use only I can see the value in getting something simple working quickly.

Right, getting something better than raw sql is useful, and perhaps
some experience with it will show what people most want to have
changed.  If you have ideas about what an ideal UI would be like I'd
be happy to hear them, at least for a later branch.

On 27 September 2010 22:17, Jeroen T. Vermeulen <email address hidden> wrote:
> Review: Needs Fixing code
> Hi Martin,
>
  However I don't think this branch is quite ready yet.
>
>
>  1     === added directory 'lib/lp/services/features/browser'
>  2     === added file 'lib/lp/services/features/browser/__init__.py'
>  3     --- lib/lp/services/features/browser/__init__.py        1970-01-01 00:00:00 +0000
>  4     +++ lib/lp/services/features/browser/__init__.py        2010-09-27 08:17:44 +0000
>  5     @@ -0,0 +1,45 @@
>  6     +# Copyright 2010 Canonical Ltd.  This software is licensed under the
>  7     +# GNU Affero General Public License version 3 (see the file LICENSE).
>
> I don't much care for putting code in __init__.py.  I can see that it keeps the imports shorter, but it's unusual enough that it may be easy to miss.

OK.  Can I make it just browser.py, or should I have a submodule with
a named file within it?


>
>
>  8     +
>  9     +"""Feature control view"""
>  10     +
>
> Where's the __all__?
>
>
>  11     +
>  12     +import logging
>  13     +
>  14     +
>  15     +from zope.interface import Interface
>  16     +from zope.schema import (
>  17     +    Text,
>  18     +    )
>  19     +
>  20     +from canonical.launchpad.webapp import (
>  21     +    action,
>  22     +    LaunchpadFormView,
>  23     +    )
>  24     +
>  25     +
>  26     +class IFeatureControlForm(Interface):
>  27     +    """Interface specifically for editing a text form of feature rules"""
>  28     +
>  29     +    def __init__(self, context):
>  30     +        self.context = context
>  31     +
>  32     +    feature_rules = Text(title=u"Feature rules")
>  33     +
>  34     +
>  35     +class FeatureControlView(LaunchpadFormView):
>
> Needs a docstring.
>
>
>  91     === added file 'lib/lp/services/features/browser/tests/test_feature_editor.py'
>  92     --- lib/lp/services/features/browser/tests/test_feature_editor.py       1970-01-01  00:00:00 +0000
>  93     +++ lib/lp/services/features/browser/tests/test_feature_editor.py       2010-09-27 08:17:44 +0000
>
> 130     +class TestFeatureControlPage(BrowserTestCase):
> 131     +
> 132     +    layer = DatabaseFunctionalLayer
> 133     +
> 134     +    def getUserBrowserAsTeamMember(self, url, teams):
> 135     +        """Make a TestBrowser authenticated as a team member.
> 136     +
> 137     +        :param teams: List of teams to add the new user to.
> 138     +        """
>
> This method dithers between being a test setup helper and a test verification helper.  Things end up muddled: when you expect getUserBrowserAsTeamMember to raise an authorization error, you're making an assertion about the "open the page" part but the code makes it look as if you're talking about the "add user to teams" part instead.

Hm, so perhaps (if it's possible) it would be clearer to write something like:

  browser = getUserBrowserAsTeamMember(teams)
  self.assertRaises(Unauthorized, browser.open(url))



>
>
> 139     +        # XXX bug=646563: To make a UserBrowser, you must know the password.  This
> 140     +        # should be separated out into test infrastructure.  -- mbp 20100923
>
> The XXX format as I recall it goes:
>
>    # XXX MartinPool 2010-09-23 bug=646563: To make a UserBrowser, you
>    # etc.

Thanks.

>
>
> 141     +        user = self.factory.makePerson(password='test')
> 142     +        for team in teams:
> 143     +            with person_logged_in(team.teamowner):
> 144     +                team.addMember(user, reviewer=team.teamowner)
> 145     +        return self.getUserBrowser(url, user=user, password='test')
>
> I would suggest not opening "url" here, so that getUserBrowserAsTeamMember can serve as an unmistakable setup helper.  That also leaves you free to assume an admin identity just once (or removeSecurityProxy) in to add the user to all teams.  It's only confusing for a setup method to engage with the security model in too much detail; leave that stuff for the actual actions that you're testing.

OK

>
>
> 147     +    def getFeatureRulesURL(self):
>
> Needs a docstring.
>
>
> 148     +        root = getUtility(ILaunchpadRoot)
> 149     +        url = canonical_url(root, view_name='+feature-rules')
> 150     +        return url
>
> Why the extra line?
>
>
> 152     +    def getFeaturePageBrowserAsAdmin(self):
> 153     +        url = self.getFeatureRulesURL()
> 154     +        admin_team = getUtility(ILaunchpadCelebrities).admin
> 155     +        return self.getUserBrowserAsTeamMember(url, [admin_team])
>
> It looks to me as if BrowserTestCase ought to have a "getAdminBrowser."  Have you considered creating one?

Right, that's bug 646563, but this branch is getting big enough already.

To me it seems overspecific to say "admin browser" in particular.

It seems strange this doesn't already exist, but I can't find it if it does.

>
>
> 157     +    def test_feature_page_default_value(self):
> 158     +        """No rules in the sampledata gives no content in the page"""
> 159     +        browser = self.getFeaturePageBrowserAsAdmin()
> 160     +        textarea = browser.getControl(name="feature_rules")
> 161     +        # and by default, since there are no rules in the sample data, it's
> 162     +        # empty
> 163     +        self.assertThat(textarea.value, Equals(''))
>
> docstring and comment lack punctuation and comment lacks capitalization
>
>
> 165     +    def test_feature_page_from_database(self):
> 166     +        addFeatureFlagRules([
> 167     +            ('default', 'ui.icing', u'3.0', 100),
> 168     +            ('beta_user', 'ui.icing', u'4.0', 300),
> 169     +            ])
> 170     +        browser = self.getFeaturePageBrowserAsAdmin()
> 171     +        textarea = browser.getControl(name="feature_rules")
> 172     +        self.assertThat(textarea.value.replace('\r', ''), Equals("""\
> 173     +ui.icing\tbeta_user\t300\t4.0
> 174     +ui.icing\tdefault\t100\t3.0
> 175     +"""))
>
> Try BrowserTestCase.assertTextMatchesExpressionIgnoreWhitespace instead.  It's not great, so feel free to improve it and help everyone.  (Also, if you find yourself dedenting manually like this in the future, consider using textwrap.dedent).

Are we actually better off ignoring whitespace?  Perhaps so.  If I was
going to change it, I'd change it into a testtools Matcher.


>
>
> 177     +    def test_feature_rules_anonymous_unauthorized(self):
> 178     +        self.assertRaises(Unauthorized,
> 179     +            self.getUserBrowser,
> 180     +            self.getFeatureRulesURL())
>
> This will be tons clearer once "set up a browser for testing" is separate from "try opening the given page with this browser."

Right.

>
>
> 182     +    def test_feature_rules_peon_unauthorized(self):
> 183     +        """Logged in, but not a member of any interesting teams."""
> 184     +        self.assertRaises(Unauthorized,
> 185     +            self.getUserBrowserAsTeamMember,
> 186     +            self.getFeatureRulesURL(),
> 187     +            [])
>
> "Peon," eh?  An inspired naming choice.  I like it.

:) Possible 'plebian' is more accurate.

>
>
> 189     +    def test_feature_page_submit_changes(self):
> 190     +        # XXX: read/write mode not supported yet
> 191     +        return
> 192     +        ## new_value = 'beta_user 10 some_key some value with spaces'
> 193     +        ## browser = self.getFeaturePageBrowser()
> 194     +        ## textarea = browser.getControl(name="feature_rules")
> 195     +        ## textarea.value = new_value
> 196     +        ## browser.getControl(name="submit").click()
> 197     +        ## self.assertThat(textarea.value.replace('\t', ' '),
> 198     +        ##     Equals(new_value))
>
> A forgotten corner of the branch!

Not forgotten, I just wanted to get some review in parallel with
finishing it off.

> If you do activate the commented-out code, please use one of the accepted line-breaking styles for the assertion.  If not, remove this test altogether and file a separate bug.

iow with the first line break immediately after the opening parenthesis.

>
>
> 201     +    # TODO: test that for unauthorized or anonymous users, the page works
> 202     +    # (including showing the active rules) but the textarea is readonly and
> 203     +    # there is no submit button.
>
> It may help prevent accidents if you have a display page for all, plus an edit link for admins.

Mm, I actually ended up deciding to take that course, but the comment
is out of date with it.

Also I need an actual link from the view page to the edit page.

>
>
> 205     +    # TODO: test that you can submit it and it changes the database.
>
> That'd be nice to test, yes.  (Though not vital I guess, given how much this relies on standard functionality).
>
>
> 322     === modified file 'lib/lp/services/features/model.py'
> 323     --- lib/lp/services/features/model.py   2010-08-20 20:31:18 +0000
> 324     +++ lib/lp/services/features/model.py   2010-09-27 08:17:44 +0000
>
> 347     +def addFeatureFlagRules(rule_list):
>
> A standalone function should follow the lower_case_with_underscores naming convention.

stand_alone_function vs objectMethod? omg.

ok.

>
>
> 348     +    """Add rules in to the database; intended for use in testing.
>
> Then shouldn't it live in lp.testing?  Call it makeFeatureFlagRule and it'd fit perfectly in the LaunchpadObjectFactory.

Actually I think for most testing, we want to just change the
featurecontroller, not actually change the database.  And perhaps it
should be a test fixture not on that monolith class?
>
>
> 350     +    :param rule_list: [(scope, flag, value, priority)...]
>
> How do these 4 values relate to the ones in the screenshot?  They seem to be in a different order.

I think the screenshot is out of date.  I want to consistently use that order.

>
>
> 360     === added file 'lib/lp/services/features/rulesource.py'
> 361     --- lib/lp/services/features/rulesource.py      1970-01-01 00:00:00 +0000
> 362     +++ lib/lp/services/features/rulesource.py      2010-09-27 08:17:44 +0000
> 363     @@ -0,0 +1,90 @@
> 364     +# Copyright 2010 Canonical Ltd.  This software is licensed under the
> 365     +# GNU Affero General Public License version 3 (see the file LICENSE).
> 366     +
> 367     +"""Returns rules defining which features are active"""
>
> This looks like a function docstring.  Could you make it applicable to the module as a whole?

Do you mean, rephrase it so it talks about what kind of thing the
module contains, rather than what the class does.

>
> I'm not sure about the design of this module.  Separating out the storage layer is obviously very useful, and it looks as if you could do with a setAllRules in NullFeatureRuleSource as well.  But the testing code and production-usable code live very close together here.  Wouldn't it be more appropriate (and consistent with our existing codebase) to have a functional FeatureRuleSource for production, plus a FakeRuleSource in lp.testing that overrides the two storage methods?

If the consistent pattern is that implementations only for testing use
go into lp.testing then I'll move it there.

>
>
> 387     +class FeatureRuleSource(object):
> 388     +    """Access feature rule sources from the database or elsewhere."""
> 389     +
> 390     +    def getAllRulesAsDict(self):
> 391     +        """Return all rule definitions.
> 392     +
> 393     +        :returns: dict from flag name to an ordered list of
> 394     +            (scope, priority, value)
> 395     +            tuples, where the first matching scope should be used.
>
> I don't understand what "where the first matching scope should be used" means in this context.

I'll clarify that it means: take the first tuple from the returned
list that names an active scope, and use that value.

>
>
> 396     +        """
> 397     +        d = {}
> 398     +        for (flag, scope, priority, value) in self.getAllRulesAsTuples():
>
> This leaves unsaid where getAllRulesAsTuples should come from.  I'd introduce it as a mandatory overridable through a nonfunctional "raise NotImplemented" implementation.
>
>
> 402     +    def getAllRulesAsText(self):
>
> Needs a docstring.
>
>
> 403     +        tr = []
> 404     +        for (flag, scope, priority, value) in self.getAllRulesAsTuples():
> 405     +            tr.append('\t'.join((flag, scope, str(priority), value)))
>
> Would this be more naturally be expressed as a list comprehension?
>
>
> 406     +        tr.append('')
> 407     +        return '\n'.join(tr)
> 408     +
> 409     +    def setAllRulesFromText(self, text_form):
> 410     +        self.setAllRules(self.parseRules(text_form))
>
> Needs a docstring.  And again please introduce setAllRules as an overridable.  That will also produce clearer errors if a test tries to set rules on a NullFeatureRuleSource.
>
>
> 412     +    def parseRules(self, text_form):
>
> Needs a docstring.
>
>
> 413     +        for line in text_form.splitlines():
> 414     +            if line.strip() == '':
> 415     +                continue
> 416     +            flag, scope, priority_str, value = re.split('[ \t]+', line, 3)
> 417     +            yield (flag, scope, int(priority_str), unicode(value))
>
> This seems to be the only place where the human input gets validated before it gets flushed to the database (probably sometime after you leave setAllRulesFromText and the caller has moved on to do something else).
>
> A pilot error would lead to an oops in the best case or lots of inconvenienced users in the worst case.  What if they accidentally enter one field too many, for instance?  What of duplicate lines?  What of near-duplicate lines with different values?
>
> Even if the data is meant to be entered only by careful and usually sober admins, I'd like to see a bit more checking and a lot more testing!
>
>
> 420     +class StormFeatureRuleSource(FeatureRuleSource):
> 421     +    """Access feature rules stored in the database via Storm.
> 422     +    """
> 423     +
> 424     +    def getAllRulesAsTuples(self):
>
> Needs a docstring.  If you introduce the method in FeatureRuleSource, it can just read """See `FeatureRuleSource`."""
>
>
> 425     +        store = getFeatureStore()
> 426     +        rs = (store
> 427     +                .find(FeatureFlag)
> 428     +                .order_by(FeatureFlag.flag, Desc(FeatureFlag.priority)))
> 429     +        for r in rs:
> 430     +            yield str(r.flag), str(r.scope), r.priority, r.value
> 431     +
> 432     +    def setAllRules(self, new_rules):
> 433     +        """Replace all existing rules with a new set.
> 434     +
> 435     +        :param new_rules: List of (name, scope, priority, value) tuples.
> 436     +        """
> 437     +        # XXX: would be slightly better to only update rules as necessary so we keep
> 438     +        # timestamps, and to avoid the direct sql etc -- mbp 20100924
>
> XXX format.
>
>
> 439     +        store = getFeatureStore()
> 440     +        store.execute('delete from featureflag')
>
> We usually upper-case SQL statements.  It's a bit noisy but also keeps them nicely recognizable.
>
>
> 449     +class NullFeatureRuleSource(FeatureRuleSource):
> 450     +    """For use in testing: everything is turned off"""
> 451     +
> 452     +    def getAllRulesAsTuples(self):
> 453     +        return []
>
> It may make more sense to move this into lp.testing.
>
>
> 456     === added file 'lib/lp/services/features/templates/feature-rules-edit.pt'
> 457     --- lib/lp/services/features/templates/feature-rules-edit.pt    1970-01-01 00:00:00 +0000
> 458     +++ lib/lp/services/features/templates/feature-rules-edit.pt    2010-09-27 08:17:44 +0000
> 459     @@ -0,0 +1,29 @@
> 460     +<html
> 461     +  xmlns="http://www.w3.org/1999/xhtml"
> 462     +  xmlns:tal="http://xml.zope.org/namespaces/tal"
> 463     +  xmlns:metal="http://xml.zope.org/namespaces/metal"
> 464     +  xmlns:i18n="http://xml.zope.org/namespaces/i18n"
> 465     +  metal:use-macro="view/macro:page/main_only"
> 466     +  i18n:domain="launchpad"
> 467     +  tal:define="page_title string:features;">
> 468     +
> 469     +<body>
> 470     +
> 471     +<div metal:fill-slot="main">
> 472     +  <div metal:use-macro="context/@@launchpad_form/form">
> 473     +
> 474     +    <p><a href="http://dev.launchpad.net/LEP/FeatureFlags">Feature flag</a>
> 475     +      rules currently active on Launchpad:</p>
> 476     +
> 477     +    <div metal:fill-slot="widgets">
> 478     +
> 479     +       <textarea style="font-family: monospace;"
>
> Is the style attribute the right way to do this?  You could also try to find a single, more succinct CSS class name for this existing rule:
>
> pre.changelog, table.diff,
> .bug-comment, .bug-activity, .codereviewcomment {
>    font-size: 93%;
>    font-family: 'UbuntuBeta Mono', 'Ubuntu Mono', monospace;
>    }
>
> (See lib/canonical/launchpad/icing/style-3-0.css.in).

You're probably right that it should use a class not an inline style.

(I think tweaking the size for what's essentially body text is a bit
strange, since the user has presumably already set the size to
something they like and Launchpad has an annoying habit of using
inconsistent sizes, but that's a separate issue.  Avoiding inline css
can only help.)

>
>
> 490     === added file 'lib/lp/services/features/templates/feature-rules.pt'
> 491     --- lib/lp/services/features/templates/feature-rules.pt 1970-01-01 00:00:00 +0000
> 492     +++ lib/lp/services/features/templates/feature-rules.pt 2010-09-27 08:17:44 +0000
>
> It may be more consistent to rename this template feature-rules-index.pt.
>
>
> 493     @@ -0,0 +1,30 @@
> 494     +<html
> 495     +  xmlns="http://www.w3.org/1999/xhtml"
> 496     +  xmlns:tal="http://xml.zope.org/namespaces/tal"
> 497     +  xmlns:metal="http://xml.zope.org/namespaces/metal"
> 498     +  xmlns:i18n="http://xml.zope.org/namespaces/i18n"
> 499     +  metal:use-macro="view/macro:page/main_only"
> 500     +  i18n:domain="launchpad"
> 501     +  tal:define="page_title string:features;">
> 502     +
> 503     +<body>
> 504     +
> 505     +<div metal:fill-slot="main">
> 506     +
> 507     +  <p><a href="http://dev.launchpad.net/LEP/FeatureFlags">Feature flag</a>
> 508     +    rules currently active on Launchpad:</p>
> 509     +
> 510     +  <div metal:use-macro="context/@@launchpad_form/form">
> 511     +    <div metal:fill-slot="widgets">
> 512     +
> 513     +      <textarea style="font-family: monospace;"
> 514     +       rows="10" columns="80"
> 515     +       readonly="true"
> 516     +       tal:content="view/rules_text" name="feature_rules">
> 517     +      </textarea>
>
> If you've got a separate template for this anyway, I think it'd be much appreciated if you could render this as a table.  It would help expose some basic mistakes, and besides, the listing sorely needs column headers!

ok

>
>
> 525     === modified file 'lib/lp/services/features/tests/test_flags.py'
> 526     --- lib/lp/services/features/tests/test_flags.py        2010-09-12 05:19:38 +0000
> 527     +++ lib/lp/services/features/tests/test_flags.py        2010-09-27 08:17:44 +0000
>
> 562     +test_rules_list = [
> 563     +    (notification_name, 'beta_user', 100, notification_value),
> 564     +    ('ui.icing', 'beta_user', 300, u'4.0'),
> 565     +    ('ui.icing', 'default', 100, u'3.0'),
> 566     +    ]
> 567     +
> 568     +
> 569     +class TestStormFeatureRuleSource(TestCase):
> 570     +
> 571     +    layer = layers.DatabaseFunctionalLayer
> 572     +
> 573     +    def setUp(self):
> 574     +        super(TestStormFeatureRuleSource, self).setUp()
> 575     +        if os.environ.get("STORM_TRACE", None):
> 576     +            from storm.tracer import debug
> 577     +            debug(True)
>
> What is this for?  Can the import be moved to a more conventional place at the top of the file?
>
>
> 578     +        self.source = StormFeatureRuleSource()
> 579     +        self.source.setAllRules(test_rules_list)
>
> Calling setAllRules here saves a line in each of the tests, but obscures the relationship between input and output.
>
> Testing a multiplicity of data in one go is usually a bad idea: it makes for brittle tests that are also hard to fix once they break.  It also discourages testing of error conditions, corner-case inputs (empty file!) and input validation, which is currently seriously underexistent.  Better to do most of your unit-testing with single lines, and only use multiple lines when specifically testing for interactions between lines.
>
>
> 581     +    def test_getAllRulesAsTuples(self):
> 582     +        self.assertEquals(list(self.source.getAllRulesAsTuples()),
> 583     +            test_rules_list)
>
> What exactly are you testing here?  The basic function of getAllRulesAsTuples for non-empty files, or that it preserves the ordering of this particular input?  In the former case, use assertContentEqual to eliminate ordering sensitivity and test separately for ordering.  In the latter case, at least show that feeding in the same ruleset in a different order will produce the same output.

The ordering does matter, and I can add a test that we get it right.

>
> Since you're line-breaking in the middle of an arguments list, could you also break the line after its opening parenthesis?
>
> For assertEquals, we pass the expected value as the first argument and the actual value as the second.

That's a good reason for me to use assertThat so I can't get it wrong.

>
>
> 585     +    def test_getAllRulesAsText(self):
> 586     +        self.assertEquals(self.source.getAllRulesAsText(),
> 587     +            """\
> 588     +%s\tbeta_user\t100\t%s
> 589     +ui.icing\tbeta_user\t300\t4.0
> 590     +ui.icing\tdefault\t100\t3.0
> 591     +""" % (notification_name, notification_value))
>
> This is hard to read.  It would help if you were to construct the expected output in a separate variable.  Also, the expected output should come before the actual output in the arguments list and the first argument shouldn't be on the same line as the method name if later arguments to the same call aren't on the same line.  And again, consider using either the BrowserTestCase helper for whitespace-insensitive comparison or textwrap.dedent (and perhaps the "strip" methods) to fix the irregular indentation.
>
>
> 593     +    def test_setAllRulesFromText(self):
> 594     +        self.source.setAllRulesFromText("""
> 595     +
> 596     +flag1   beta_user   200   alpha
> 597     +flag1   default     100   gamma with spaces
> 598     +flag2   default     0\ton
> 599     +""")
>
> Here I'd definitely recommend dedent and friends.
>
>
> 600     +        self.assertEquals(self.source.getAllRulesAsDict(), {
>
> The expected value should come first.
>
>
> 601     +            'flag1': [
> 602     +                ('beta_user', 200, 'alpha'),
> 603     +                ('default', 100, 'gamma with spaces'), ],
> 604     +            'flag2': [
> 605     +                ('default', 0, 'on'),]})
>
> Neither the dict nor its embedded lists follow our syntax conventions.  There should be a newline before the closing brace or bracket.  This is easier to get right if you store the dict in a variable so that you can pass it by name.

OK, thanks.

-- 
Martin