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 +Feature flag 475 + rules currently active on Launchpad:
476 + 477 +