launchpad-reviewers team mailing list archive

[Benji York <benji@xxxxxxxxxxxxx>]

Benji York has proposed merging lp:~benji/launchpad/bug-669701 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #669701 Feature Flag control gives no feedback on successful application of new rules
  https://bugs.launchpad.net/bugs/669701

For more details, see:
https://code.launchpad.net/~benji/launchpad/bug-669701/+merge/45171

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

Several bits of lint were also fixed on this branch.

-- 
https://code.launchpad.net/~benji/launchpad/bug-669701/+merge/45171
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~benji/launchpad/bug-669701 into lp:launchpad.

=== 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-04 20:34:38 +0000
@@ -17,9 +17,7 @@
     person_logged_in,
     TestCaseWithFactory,
     )
-from lp.testing.matchers import (
-    Contains,
-    )
+from lp.testing.matchers import Contains
 from lp.testing.sampledata import ADMIN_EMAIL
 from lp.testing.views import create_initialized_view
 

=== 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-04 20:34:38 +0000
@@ -10,21 +10,19 @@
     ]
 
 
-import logging 
-
+from difflib import unified_diff
+import logging
 
 from zope.interface import Interface
-from zope.schema import (
-    Text,
-    )
+from zope.schema import Text
+from zope.security.interfaces import Unauthorized
 
-from canonical.launchpad.webapp.authorization import (
-    check_permission,
-    )
+from canonical.launchpad.webapp.authorization import check_permission
 from lp.app.browser.launchpadform import (
     action,
     LaunchpadFormView,
     )
+from lp.app.browser.stringformatter import FormattersAPI
 
 
 class IFeatureControlForm(Interface):
@@ -39,10 +37,19 @@
             u"Rules to control feature flags on Launchpad.  "
             u"On each line: (flag, scope, priority, value), "
             u"whitespace-separated.  Numerically higher "
-            u"priorities match first."
-            ),
-        required=False,
-        )
+            u"priorities match first."),
+        required=False)
+
+
+def diff_rules(rules1, rules2):
+    # Just generate a one-block diff.
+    lines_of_context = 999999
+    diff = unified_diff(
+        rules1.splitlines(),
+        rules2.splitlines(),
+        n=lines_of_context)
+    # The three line header is meaningless here.
+    return list(diff)[3:]
 
 
 class FeatureControlView(LaunchpadFormView):
@@ -55,22 +62,26 @@
     schema = IFeatureControlForm
     page_title = label = 'Feature control'
     field_names = ['feature_rules']
+    diff = None
 
     @action(u"Change", name="change")
     def change_action(self, action, data):
         if not check_permission('launchpad.Admin', self.context):
             raise Unauthorized()
-        rules_text = data.get('feature_rules') or ''
+        original_rules = self.request.features.rule_source.getAllRulesAsText()
+        new_rules = data.get('feature_rules') or ''
         logger = logging.getLogger('lp.services.features')
-        logger.warning("Change feature rules to: %s" % (rules_text,))
-        self.request.features.rule_source.setAllRulesFromText(
-            rules_text)
+        logger.warning("Change feature rules to: %s" % (new_rules,))
+        self.request.features.rule_source.setAllRulesFromText(new_rules)
+        diff = '\n'.join(diff_rules(original_rules, new_rules))
+        self.diff = FormattersAPI(diff).format_diff()
 
     @property
     def initial_values(self):
-        return dict(
-            feature_rules=self.request.features.rule_source.getAllRulesAsText(),
-            )
+        return {
+            'feature_rules':
+                self.request.features.rule_source.getAllRulesAsText(),
+        }
 
     def validate(self, data):
         # Try parsing the rules so we give a clean error: at the moment the

=== 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-04 20:34:38 +0000
@@ -14,13 +14,21 @@
 from canonical.launchpad.webapp import canonical_url
 from canonical.launchpad.webapp.interfaces import ILaunchpadRoot
 from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.services.features.browser.edit import FeatureControlView
 from lp.services.features.rulesource import StormFeatureRuleSource
+from lp.testing.matchers import Contains
+
 from lp.testing import (
     BrowserTestCase,
     person_logged_in,
     )
 
 
+class FauxForm:
+    """The simplest fake form, used for testing."""
+    context = None
+
+
 class TestFeatureControlPage(BrowserTestCase):
 
     layer = DatabaseFunctionalLayer
@@ -103,6 +111,40 @@
                 ('beta_user', 'some_key', 10, 'some value with spaces'),
                 ]))
 
+    def test_change_message(self):
+        """Submitting shows a message that the changes have been applied."""
+        browser = self.getUserBrowserAsAdmin()
+        browser.open(self.getFeatureRulesEditURL())
+        textarea = browser.getControl(name="field.feature_rules")
+        textarea.value = 'beta_user some_key 10 some value with spaces'
+        browser.getControl(name="field.actions.change").click()
+        self.assertThat(
+            browser.contents,
+            Contains('Your changes have been applied'))
+
+    def test_change_diff(self):
+        """Submitting shows a diff of the changes."""
+        browser = self.getUserBrowserAsAdmin()
+        browser.open(self.getFeatureRulesEditURL())
+        browser.getControl(name="field.feature_rules"
+            ).value = 'beta_user some_key 10 some value with spaces'
+        browser.getControl(name="field.actions.change").click()
+        browser.getControl(name="field.feature_rules"
+            ).value = 'beta_user some_key 10 another value with spaces'
+        browser.getControl(name="field.actions.change").click()
+        # The diff is formatted nicely using CSS.
+        self.assertThat(
+            browser.contents,
+            Contains('<td class="diff-added text">'))
+        # Removed rules are displayed as being removed.
+        self.assertThat(
+            browser.contents.replace('\t', ' '),
+            Contains('-beta_user some_key 10 some value with spaces'))
+        # Added rules are displayed as being added.
+        self.assertThat(
+            browser.contents.replace('\t', ' '),
+            Contains('+beta_user some_key 10 another value with spaces'))
+
     def test_feature_page_submit_change_to_empty(self):
         """Correctly handle submitting an empty value."""
         # Zope has the quirk of conflating empty with absent; make sure we
@@ -115,5 +157,14 @@
         browser.getControl(name="field.actions.change").click()
         self.assertThat(
             list(StormFeatureRuleSource().getAllRulesAsTuples()),
-            Equals([
-                ]))
+            Equals([]))
+
+    def test_feature_page_submit_change_when_unauthorized(self):
+        """Correctly handling attempted value changes when not authorized."""
+        # When a change is submitted but the user is unauthorized, an
+        # exception is raised.
+
+        view = FeatureControlView(None, None)
+        self.assertRaises(
+            Unauthorized,
+            view.change_action.success_handler, FauxForm(), None, None)

=== 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-04 20:34:38 +0000
@@ -10,8 +10,12 @@
 <body>
 
 <div metal:fill-slot="main">
-
   <div metal:use-macro="context/@@launchpad_form/form">
+    <div metal:fill-slot="extra_top"
+        tal:condition="view/diff">
+        <p>Your changes have been applied:</p>
+        <tal:diff replace="structure view/diff"/>
+    </div>
   </div>
 </div>
 </body>

=== 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-04 20:34:38 +0000
@@ -11,7 +11,6 @@
 from canonical.testing import layers
 from lp.services.features import (
     getFeatureFlag,
-    model,
     per_thread,
     )
 from lp.services.features.flags import FeatureController
@@ -43,6 +42,7 @@
     def makeControllerInScopes(self, scopes):
         """Make a controller that will report it's in the given scopes."""
         call_log = []
+
         def scope_cb(scope):
             call_log.append(scope)
             return scope in scopes
@@ -204,10 +204,10 @@
         self.assertEquals({
             'flag1': [
                 ('beta_user', 200, 'alpha'),
-                ('default', 100, 'gamma with spaces'), 
+                ('default', 100, 'gamma with spaces'),
                 ],
             'flag2': [
                 ('default', 0, 'on'),
                 ],
-            }, 
+            },
             source.getAllRulesAsDict())