Hi Martin, 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. 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. 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. 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. 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. 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? 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). 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." 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. 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! 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. 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. 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. 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. 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. 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? 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? 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. 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 + 468 + 469 + 470 + 471 +
472 +
473 + 474 +

Feature flag 475 + rules currently active on Launchpad:

476 + 477 +
478 + 479 + 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! 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. 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. 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. Jeroen