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
1=== modified file 'lib/lp/registry/browser/tests/test_projectgroup.py'
2--- lib/lp/registry/browser/tests/test_projectgroup.py 2010-12-14 15:20:24 +0000
3+++ lib/lp/registry/browser/tests/test_projectgroup.py 2011-01-05 22:51:21 +0000
4@@ -17,9 +17,7 @@
5 person_logged_in,
6 TestCaseWithFactory,
7 )
8-from lp.testing.matchers import (
9- Contains,
10- )
11+from lp.testing.matchers import Contains
12 from lp.testing.sampledata import ADMIN_EMAIL
13 from lp.testing.views import create_initialized_view
14
15
16=== modified file 'lib/lp/services/features/browser/edit.py'
17--- lib/lp/services/features/browser/edit.py 2010-11-23 23:22:27 +0000
18+++ lib/lp/services/features/browser/edit.py 2011-01-05 22:51:21 +0000
19@@ -10,21 +10,19 @@
20 ]
21
22
23-import logging
24-
25+from difflib import unified_diff
26+import logging
27
28 from zope.interface import Interface
29-from zope.schema import (
30- Text,
31- )
32+from zope.schema import Text
33+from zope.security.interfaces import Unauthorized
34
35-from canonical.launchpad.webapp.authorization import (
36- check_permission,
37- )
38+from canonical.launchpad.webapp.authorization import check_permission
39 from lp.app.browser.launchpadform import (
40 action,
41 LaunchpadFormView,
42 )
43+from lp.app.browser.stringformatter import FormattersAPI
44
45
46 class IFeatureControlForm(Interface):
47@@ -39,10 +37,8 @@
48 u"Rules to control feature flags on Launchpad. "
49 u"On each line: (flag, scope, priority, value), "
50 u"whitespace-separated. Numerically higher "
51- u"priorities match first."
52- ),
53- required=False,
54- )
55+ u"priorities match first."),
56+ required=False)
57
58
59 class FeatureControlView(LaunchpadFormView):
60@@ -55,22 +51,43 @@
61 schema = IFeatureControlForm
62 page_title = label = 'Feature control'
63 field_names = ['feature_rules']
64+ diff = None
65+ logger_name = 'lp.services.features'
66
67 @action(u"Change", name="change")
68 def change_action(self, action, data):
69 if not check_permission('launchpad.Admin', self.context):
70 raise Unauthorized()
71+ original_rules = self.request.features.rule_source.getAllRulesAsText()
72 rules_text = data.get('feature_rules') or ''
73- logger = logging.getLogger('lp.services.features')
74+ logger = logging.getLogger(self.logger_name)
75 logger.warning("Change feature rules to: %s" % (rules_text,))
76- self.request.features.rule_source.setAllRulesFromText(
77- rules_text)
78+ logger.warning("Previous feature rules were: %s" % (original_rules,))
79+ self.request.features.rule_source.setAllRulesFromText(rules_text)
80+ # Why re-fetch the rules here? This way we get them reformatted
81+ # (whitespace normalized) and ordered consistently so the diff is
82+ # minimal.
83+ new_rules = self.request.features.rule_source.getAllRulesAsText()
84+ diff = '\n'.join(self.diff_rules(original_rules, new_rules))
85+ self.diff = FormattersAPI(diff).format_diff()
86+
87+ @staticmethod
88+ def diff_rules(rules1, rules2):
89+ # Just generate a one-block diff.
90+ lines_of_context = 999999
91+ diff = unified_diff(
92+ rules1.splitlines(),
93+ rules2.splitlines(),
94+ n=lines_of_context)
95+ # The three line header is meaningless here.
96+ return list(diff)[3:]
97
98 @property
99 def initial_values(self):
100- return dict(
101- feature_rules=self.request.features.rule_source.getAllRulesAsText(),
102- )
103+ return {
104+ 'feature_rules':
105+ self.request.features.rule_source.getAllRulesAsText(),
106+ }
107
108 def validate(self, data):
109 # Try parsing the rules so we give a clean error: at the moment the
110
111=== modified file 'lib/lp/services/features/browser/tests/test_feature_editor.py'
112--- lib/lp/services/features/browser/tests/test_feature_editor.py 2010-11-08 14:21:01 +0000
113+++ lib/lp/services/features/browser/tests/test_feature_editor.py 2011-01-05 22:51:21 +0000
114@@ -14,13 +14,21 @@
115 from canonical.launchpad.webapp import canonical_url
116 from canonical.launchpad.webapp.interfaces import ILaunchpadRoot
117 from canonical.testing.layers import DatabaseFunctionalLayer
118+from lp.services.features.browser.edit import FeatureControlView
119 from lp.services.features.rulesource import StormFeatureRuleSource
120+from lp.testing.matchers import Contains
121+
122 from lp.testing import (
123 BrowserTestCase,
124 person_logged_in,
125 )
126
127
128+class FauxForm:
129+ """The simplest fake form, used for testing."""
130+ context = None
131+
132+
133 class TestFeatureControlPage(BrowserTestCase):
134
135 layer = DatabaseFunctionalLayer
136@@ -103,6 +111,51 @@
137 ('beta_user', 'some_key', 10, 'some value with spaces'),
138 ]))
139
140+ def test_change_message(self):
141+ """Submitting shows a message that the changes have been applied."""
142+ browser = self.getUserBrowserAsAdmin()
143+ browser.open(self.getFeatureRulesEditURL())
144+ textarea = browser.getControl(name="field.feature_rules")
145+ textarea.value = 'beta_user some_key 10 some value with spaces'
146+ browser.getControl(name="field.actions.change").click()
147+ self.assertThat(
148+ browser.contents,
149+ Contains('Your changes have been applied'))
150+
151+ def test_change_diff(self):
152+ """Submitting shows a diff of the changes."""
153+ browser = self.getUserBrowserAsAdmin()
154+ browser.open(self.getFeatureRulesEditURL())
155+ browser.getControl(name="field.feature_rules").value = (
156+ 'beta_user some_key 10 some value with spaces')
157+ browser.getControl(name="field.actions.change").click()
158+ browser.getControl(name="field.feature_rules").value = (
159+ 'beta_user some_key 10 another value with spaces')
160+ browser.getControl(name="field.actions.change").click()
161+ # The diff is formatted nicely using CSS.
162+ self.assertThat(
163+ browser.contents,
164+ Contains('<td class="diff-added text">'))
165+ # Removed rules are displayed as being removed.
166+ self.assertThat(
167+ browser.contents.replace('\t', ' '),
168+ Contains('-beta_user some_key 10 some value with spaces'))
169+ # Added rules are displayed as being added.
170+ self.assertThat(
171+ browser.contents.replace('\t', ' '),
172+ Contains('+beta_user some_key 10 another value with spaces'))
173+
174+ def test_change_logging_note(self):
175+ """When submitting changes the name of the logger is shown."""
176+ browser = self.getUserBrowserAsAdmin()
177+ browser.open(self.getFeatureRulesEditURL())
178+ browser.getControl(name="field.feature_rules").value = (
179+ 'beta_user some_key 10 some value with spaces')
180+ browser.getControl(name="field.actions.change").click()
181+ self.assertThat(
182+ browser.contents,
183+ Contains('logged by the lp.services.features logger'))
184+
185 def test_feature_page_submit_change_to_empty(self):
186 """Correctly handle submitting an empty value."""
187 # Zope has the quirk of conflating empty with absent; make sure we
188@@ -115,5 +168,14 @@
189 browser.getControl(name="field.actions.change").click()
190 self.assertThat(
191 list(StormFeatureRuleSource().getAllRulesAsTuples()),
192- Equals([
193- ]))
194+ Equals([]))
195+
196+ def test_feature_page_submit_change_when_unauthorized(self):
197+ """Correctly handling attempted value changes when not authorized."""
198+ # When a change is submitted but the user is unauthorized, an
199+ # exception is raised.
200+
201+ view = FeatureControlView(None, None)
202+ self.assertRaises(
203+ Unauthorized,
204+ view.change_action.success_handler, FauxForm(), None, None)
205
206=== modified file 'lib/lp/services/features/rulesource.py'
207--- lib/lp/services/features/rulesource.py 2010-11-02 17:47:05 +0000
208+++ lib/lp/services/features/rulesource.py 2011-01-05 22:51:21 +0000
209@@ -93,7 +93,10 @@
210 store = getFeatureStore()
211 rs = (store
212 .find(FeatureFlag)
213- .order_by(FeatureFlag.flag, Desc(FeatureFlag.priority)))
214+ .order_by(
215+ FeatureFlag.flag,
216+ FeatureFlag.scope,
217+ Desc(FeatureFlag.priority)))
218 for r in rs:
219 yield Rule(str(r.flag), str(r.scope), r.priority, r.value)
220
221
222=== modified file 'lib/lp/services/features/templates/feature-rules.pt'
223--- lib/lp/services/features/templates/feature-rules.pt 2010-09-29 05:17:50 +0000
224+++ lib/lp/services/features/templates/feature-rules.pt 2011-01-05 22:51:21 +0000
225@@ -10,8 +10,14 @@
226 <body>
227
228 <div metal:fill-slot="main">
229-
230 <div metal:use-macro="context/@@launchpad_form/form">
231+ <div metal:fill-slot="extra_top"
232+ tal:condition="view/diff">
233+ <p>Your changes have been applied (and before and after values of the
234+ rules logged by the <tal:logger replace="view/logger_name"/> logger):
235+ </p>
236+ <tal:diff replace="structure view/diff"/>
237+ </div>
238 </div>
239 </div>
240 </body>
241
242=== modified file 'lib/lp/services/features/tests/test_flags.py'
243--- lib/lp/services/features/tests/test_flags.py 2010-10-26 15:47:24 +0000
244+++ lib/lp/services/features/tests/test_flags.py 2011-01-05 22:51:21 +0000
245@@ -11,7 +11,6 @@
246 from canonical.testing import layers
247 from lp.services.features import (
248 getFeatureFlag,
249- model,
250 per_thread,
251 )
252 from lp.services.features.flags import FeatureController
253@@ -43,6 +42,7 @@
254 def makeControllerInScopes(self, scopes):
255 """Make a controller that will report it's in the given scopes."""
256 call_log = []
257+
258 def scope_cb(scope):
259 call_log.append(scope)
260 return scope in scopes
261@@ -204,10 +204,10 @@
262 self.assertEquals({
263 'flag1': [
264 ('beta_user', 200, 'alpha'),
265- ('default', 100, 'gamma with spaces'),
266+ ('default', 100, 'gamma with spaces'),
267 ],
268 'flag2': [
269 ('default', 0, 'on'),
270 ],
271- },
272+ },
273 source.getAllRulesAsDict())