Launchpad itself

Merge lp:~benji/launchpad/bug-669701 into lp:launchpad

  1. bug-669701
  2. Merge into devel
Proposed by Benji York
Status: Merged
Approved by: Curtis Hovey
Approved revision: no longer in the source branch.
Merged at revision: 12169
Proposed branch: lp:~benji/launchpad/bug-669701
Merge into: lp:launchpad
Diff against target: 273 lines (+114/-28)
6 files modified
lib/lp/registry/browser/tests/test_projectgroup.py (+1/-3)
lib/lp/services/features/browser/edit.py (+35/-18)
lib/lp/services/features/browser/tests/test_feature_editor.py (+64/-2)
lib/lp/services/features/rulesource.py (+4/-1)
lib/lp/services/features/templates/feature-rules.pt (+7/-1)
lib/lp/services/features/tests/test_flags.py (+3/-3)
To merge this branch: bzr merge lp:~benji/launchpad/bug-669701
Reviewer Review Type Date Requested Status
Edwin Grubbs (community) code Approve
Curtis Hovey (community) ui Approve
Leonard Richardson (community) Approve
Review via email: mp+45171@code.launchpad.net

Commit message

[r=edwin-grubbs,leonardr][ui=sinzui][bug=651852,669701] [r=leonardr][ui=sinzui][bug=669701]

Description of the change

When saving the features flag rules the user is given no indication that
anything actually happened (bug 669701). This branch fixes that by
adding a message and displaying the changes applied in the form of a
diff.

This branch also fixes a bug discovered during development: if a user
who is not authorized to make feature flag changes attempts to do so a
NameError will be raised because of a missing import in untested code.
Both the import and a test were added.

The tests for the modified code can be run with:

    bin/test -c lp.services.features.browser

The feature can be interactively tested by starting a dev Launchpad
instance and logging in as an admin user (<email address hidden>/test)
will work and then interacting with https://launchpad.dev/+feature-rules.

Several bits of lint were also fixed on this branch.

To post a comment you must log in.
Revision history for this message
Leonard Richardson (leonardr) wrote :

r=me with minor changes. Around line 151 you have some line breaks that violate the style guide. diff_rules() should be a method of the view so that it's clear which code it belongs to.

review: Approve
Revision history for this message
Benji York (benji) wrote :

Leonard's suggestions have been integrated into the branch.

Revision history for this message
Curtis Hovey (sinzui) wrote :

We talk about why this does not use class="informational message" like other forms. The reason is because the placing a coloured diff looks bad. We do not want to introduce a blue, yellow, red, and green splotch to a page and risk giving a beloved losa a seizure.

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

The text diff seems nice. It might be a bit nicer if you did a diff against the value read back by getAllRulesAsText(), which will avoid spurious diffs due to text changes that the database doesn't care about.

The NameError you fixed in passing was https://bugs.launchpad.net/launchpad/+bug/651852 -- well done!

Revision history for this message
Benji York (benji) wrote :

> The text diff seems nice. It might be a bit nicer if you did a diff against
> the value read back by getAllRulesAsText(), which will avoid spurious diffs
> due to text changes that the database doesn't care about.

Done.

Revision history for this message
Benji York (benji) wrote :

I made a couple of further changes. One address Martin's suggestion above. The other two address bug 670019 and bug 636193. I'm going to get someone to review the incremental diff (at http://pastebin.ubuntu.com/550823/) before landing.

Revision history for this message
Edwin Grubbs (edwin-grubbs) wrote :

>+ # Why re-fetch the rules here? This way we get them reformatted
>+ # (whitespace normalized) and ordered consistently so the diff is
>+ # minimal.
>+ diff = '\n'.join(self.diff_rules(original_rules,
>+ self.request.features.rule_source.getAllRulesAsText()))

This formatting is hard to read and nonstandard. You could reformat like the following to comply with the style guide, but it would probably be more readable if you put the rules into a variable, so that the join() fits on one line.

        diff = '\n'.join(self.diff_rules(
            original_rules,
            self.request.features.rule_source.getAllRulesAsText()))

Everything else looks good.

review: Approve (code)
Revision history for this message
Benji York (benji) wrote :

> This formatting is hard to read and nonstandard. You could reformat like the
> following to comply with the style guide, but it would probably be more
> readable if you put the rules into a variable, so that the join() fits on one
> line.

Fixed. I added a variable.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/lp/registry/browser/tests/test_projectgroup.py'
--- lib/lp/registry/browser/tests/test_projectgroup.py 2010-12-14 15:20:24 +0000
+++ lib/lp/registry/browser/tests/test_projectgroup.py 2011-01-05 22:51:21 +0000
@@ -17,9 +17,7 @@
17 person_logged_in,17 person_logged_in,
18 TestCaseWithFactory,18 TestCaseWithFactory,
19 )19 )
20from lp.testing.matchers import (20from lp.testing.matchers import Contains
21 Contains,
22 )
23from lp.testing.sampledata import ADMIN_EMAIL21from lp.testing.sampledata import ADMIN_EMAIL
24from lp.testing.views import create_initialized_view22from lp.testing.views import create_initialized_view
2523
2624
=== modified file 'lib/lp/services/features/browser/edit.py'
--- lib/lp/services/features/browser/edit.py 2010-11-23 23:22:27 +0000
+++ lib/lp/services/features/browser/edit.py 2011-01-05 22:51:21 +0000
@@ -10,21 +10,19 @@
10 ]10 ]
1111
1212
13import logging 13from difflib import unified_diff
1414import logging
1515
16from zope.interface import Interface16from zope.interface import Interface
17from zope.schema import (17from zope.schema import Text
18 Text,18from zope.security.interfaces import Unauthorized
19 )
2019
21from canonical.launchpad.webapp.authorization import (20from canonical.launchpad.webapp.authorization import check_permission
22 check_permission,
23 )
24from lp.app.browser.launchpadform import (21from lp.app.browser.launchpadform import (
25 action,22 action,
26 LaunchpadFormView,23 LaunchpadFormView,
27 )24 )
25from lp.app.browser.stringformatter import FormattersAPI
2826
2927
30class IFeatureControlForm(Interface):28class IFeatureControlForm(Interface):
@@ -39,10 +37,8 @@
39 u"Rules to control feature flags on Launchpad. "37 u"Rules to control feature flags on Launchpad. "
40 u"On each line: (flag, scope, priority, value), "38 u"On each line: (flag, scope, priority, value), "
41 u"whitespace-separated. Numerically higher "39 u"whitespace-separated. Numerically higher "
42 u"priorities match first."40 u"priorities match first."),
43 ),41 required=False)
44 required=False,
45 )
4642
4743
48class FeatureControlView(LaunchpadFormView):44class FeatureControlView(LaunchpadFormView):
@@ -55,22 +51,43 @@
55 schema = IFeatureControlForm51 schema = IFeatureControlForm
56 page_title = label = 'Feature control'52 page_title = label = 'Feature control'
57 field_names = ['feature_rules']53 field_names = ['feature_rules']
54 diff = None
55 logger_name = 'lp.services.features'
5856
59 @action(u"Change", name="change")57 @action(u"Change", name="change")
60 def change_action(self, action, data):58 def change_action(self, action, data):
61 if not check_permission('launchpad.Admin', self.context):59 if not check_permission('launchpad.Admin', self.context):
62 raise Unauthorized()60 raise Unauthorized()
61 original_rules = self.request.features.rule_source.getAllRulesAsText()
63 rules_text = data.get('feature_rules') or ''62 rules_text = data.get('feature_rules') or ''
64 logger = logging.getLogger('lp.services.features')63 logger = logging.getLogger(self.logger_name)
65 logger.warning("Change feature rules to: %s" % (rules_text,))64 logger.warning("Change feature rules to: %s" % (rules_text,))
66 self.request.features.rule_source.setAllRulesFromText(65 logger.warning("Previous feature rules were: %s" % (original_rules,))
67 rules_text)66 self.request.features.rule_source.setAllRulesFromText(rules_text)
67 # Why re-fetch the rules here? This way we get them reformatted
68 # (whitespace normalized) and ordered consistently so the diff is
69 # minimal.
70 new_rules = self.request.features.rule_source.getAllRulesAsText()
71 diff = '\n'.join(self.diff_rules(original_rules, new_rules))
72 self.diff = FormattersAPI(diff).format_diff()
73
74 @staticmethod
75 def diff_rules(rules1, rules2):
76 # Just generate a one-block diff.
77 lines_of_context = 999999
78 diff = unified_diff(
79 rules1.splitlines(),
80 rules2.splitlines(),
81 n=lines_of_context)
82 # The three line header is meaningless here.
83 return list(diff)[3:]
6884
69 @property85 @property
70 def initial_values(self):86 def initial_values(self):
71 return dict(87 return {
72 feature_rules=self.request.features.rule_source.getAllRulesAsText(),88 'feature_rules':
73 )89 self.request.features.rule_source.getAllRulesAsText(),
90 }
7491
75 def validate(self, data):92 def validate(self, data):
76 # Try parsing the rules so we give a clean error: at the moment the93 # Try parsing the rules so we give a clean error: at the moment the
7794
=== modified file 'lib/lp/services/features/browser/tests/test_feature_editor.py'
--- lib/lp/services/features/browser/tests/test_feature_editor.py 2010-11-08 14:21:01 +0000
+++ lib/lp/services/features/browser/tests/test_feature_editor.py 2011-01-05 22:51:21 +0000
@@ -14,13 +14,21 @@
14from canonical.launchpad.webapp import canonical_url14from canonical.launchpad.webapp import canonical_url
15from canonical.launchpad.webapp.interfaces import ILaunchpadRoot15from canonical.launchpad.webapp.interfaces import ILaunchpadRoot
16from canonical.testing.layers import DatabaseFunctionalLayer16from canonical.testing.layers import DatabaseFunctionalLayer
17from lp.services.features.browser.edit import FeatureControlView
17from lp.services.features.rulesource import StormFeatureRuleSource18from lp.services.features.rulesource import StormFeatureRuleSource
19from lp.testing.matchers import Contains
20
18from lp.testing import (21from lp.testing import (
19 BrowserTestCase,22 BrowserTestCase,
20 person_logged_in,23 person_logged_in,
21 )24 )
2225
2326
27class FauxForm:
28 """The simplest fake form, used for testing."""
29 context = None
30
31
24class TestFeatureControlPage(BrowserTestCase):32class TestFeatureControlPage(BrowserTestCase):
2533
26 layer = DatabaseFunctionalLayer34 layer = DatabaseFunctionalLayer
@@ -103,6 +111,51 @@
103 ('beta_user', 'some_key', 10, 'some value with spaces'),111 ('beta_user', 'some_key', 10, 'some value with spaces'),
104 ]))112 ]))
105113
114 def test_change_message(self):
115 """Submitting shows a message that the changes have been applied."""
116 browser = self.getUserBrowserAsAdmin()
117 browser.open(self.getFeatureRulesEditURL())
118 textarea = browser.getControl(name="field.feature_rules")
119 textarea.value = 'beta_user some_key 10 some value with spaces'
120 browser.getControl(name="field.actions.change").click()
121 self.assertThat(
122 browser.contents,
123 Contains('Your changes have been applied'))
124
125 def test_change_diff(self):
126 """Submitting shows a diff of the changes."""
127 browser = self.getUserBrowserAsAdmin()
128 browser.open(self.getFeatureRulesEditURL())
129 browser.getControl(name="field.feature_rules").value = (
130 'beta_user some_key 10 some value with spaces')
131 browser.getControl(name="field.actions.change").click()
132 browser.getControl(name="field.feature_rules").value = (
133 'beta_user some_key 10 another value with spaces')
134 browser.getControl(name="field.actions.change").click()
135 # The diff is formatted nicely using CSS.
136 self.assertThat(
137 browser.contents,
138 Contains('<td class="diff-added text">'))
139 # Removed rules are displayed as being removed.
140 self.assertThat(
141 browser.contents.replace('\t', ' '),
142 Contains('-beta_user some_key 10 some value with spaces'))
143 # Added rules are displayed as being added.
144 self.assertThat(
145 browser.contents.replace('\t', ' '),
146 Contains('+beta_user some_key 10 another value with spaces'))
147
148 def test_change_logging_note(self):
149 """When submitting changes the name of the logger is shown."""
150 browser = self.getUserBrowserAsAdmin()
151 browser.open(self.getFeatureRulesEditURL())
152 browser.getControl(name="field.feature_rules").value = (
153 'beta_user some_key 10 some value with spaces')
154 browser.getControl(name="field.actions.change").click()
155 self.assertThat(
156 browser.contents,
157 Contains('logged by the lp.services.features logger'))
158
106 def test_feature_page_submit_change_to_empty(self):159 def test_feature_page_submit_change_to_empty(self):
107 """Correctly handle submitting an empty value."""160 """Correctly handle submitting an empty value."""
108 # Zope has the quirk of conflating empty with absent; make sure we161 # Zope has the quirk of conflating empty with absent; make sure we
@@ -115,5 +168,14 @@
115 browser.getControl(name="field.actions.change").click()168 browser.getControl(name="field.actions.change").click()
116 self.assertThat(169 self.assertThat(
117 list(StormFeatureRuleSource().getAllRulesAsTuples()),170 list(StormFeatureRuleSource().getAllRulesAsTuples()),
118 Equals([171 Equals([]))
119 ]))172
173 def test_feature_page_submit_change_when_unauthorized(self):
174 """Correctly handling attempted value changes when not authorized."""
175 # When a change is submitted but the user is unauthorized, an
176 # exception is raised.
177
178 view = FeatureControlView(None, None)
179 self.assertRaises(
180 Unauthorized,
181 view.change_action.success_handler, FauxForm(), None, None)
120182
=== modified file 'lib/lp/services/features/rulesource.py'
--- lib/lp/services/features/rulesource.py 2010-11-02 17:47:05 +0000
+++ lib/lp/services/features/rulesource.py 2011-01-05 22:51:21 +0000
@@ -93,7 +93,10 @@
93 store = getFeatureStore()93 store = getFeatureStore()
94 rs = (store94 rs = (store
95 .find(FeatureFlag)95 .find(FeatureFlag)
96 .order_by(FeatureFlag.flag, Desc(FeatureFlag.priority)))96 .order_by(
97 FeatureFlag.flag,
98 FeatureFlag.scope,
99 Desc(FeatureFlag.priority)))
97 for r in rs:100 for r in rs:
98 yield Rule(str(r.flag), str(r.scope), r.priority, r.value)101 yield Rule(str(r.flag), str(r.scope), r.priority, r.value)
99102
100103
=== modified file 'lib/lp/services/features/templates/feature-rules.pt'
--- lib/lp/services/features/templates/feature-rules.pt 2010-09-29 05:17:50 +0000
+++ lib/lp/services/features/templates/feature-rules.pt 2011-01-05 22:51:21 +0000
@@ -10,8 +10,14 @@
10<body>10<body>
1111
12<div metal:fill-slot="main">12<div metal:fill-slot="main">
13
14 <div metal:use-macro="context/@@launchpad_form/form">13 <div metal:use-macro="context/@@launchpad_form/form">
14 <div metal:fill-slot="extra_top"
15 tal:condition="view/diff">
16 <p>Your changes have been applied (and before and after values of the
17 rules logged by the <tal:logger replace="view/logger_name"/> logger):
18 </p>
19 <tal:diff replace="structure view/diff"/>
20 </div>
15 </div>21 </div>
16</div>22</div>
17</body>23</body>
1824
=== modified file 'lib/lp/services/features/tests/test_flags.py'
--- lib/lp/services/features/tests/test_flags.py 2010-10-26 15:47:24 +0000
+++ lib/lp/services/features/tests/test_flags.py 2011-01-05 22:51:21 +0000
@@ -11,7 +11,6 @@
11from canonical.testing import layers11from canonical.testing import layers
12from lp.services.features import (12from lp.services.features import (
13 getFeatureFlag,13 getFeatureFlag,
14 model,
15 per_thread,14 per_thread,
16 )15 )
17from lp.services.features.flags import FeatureController16from lp.services.features.flags import FeatureController
@@ -43,6 +42,7 @@
43 def makeControllerInScopes(self, scopes):42 def makeControllerInScopes(self, scopes):
44 """Make a controller that will report it's in the given scopes."""43 """Make a controller that will report it's in the given scopes."""
45 call_log = []44 call_log = []
45
46 def scope_cb(scope):46 def scope_cb(scope):
47 call_log.append(scope)47 call_log.append(scope)
48 return scope in scopes48 return scope in scopes
@@ -204,10 +204,10 @@
204 self.assertEquals({204 self.assertEquals({
205 'flag1': [205 'flag1': [
206 ('beta_user', 200, 'alpha'),206 ('beta_user', 200, 'alpha'),
207 ('default', 100, 'gamma with spaces'), 207 ('default', 100, 'gamma with spaces'),
208 ],208 ],
209 'flag2': [209 'flag2': [
210 ('default', 0, 'on'),210 ('default', 0, 'on'),
211 ],211 ],
212 }, 212 },
213 source.getAllRulesAsDict())213 source.getAllRulesAsDict())