Merge lp:~bac/launchpad/904335-milestone-edit into lp:launchpad

[Brad Crittenden]

[r=sinzui] [r=gary_poster,jcsackett,sinzui][bug=904335] Add milestone tags.

Add the tags field to the milestone +edit page.

If there is a cleaner way to insert the tags field into the form I'm
open for suggestions.

Tests:
bin/test -vv lp.registry.browser.tests.test_milestone TestMilestoneEditView

No lint.

[Curtis Hovey (sinzui)]

Thank you for this improvement. I do not see an issue with how you add tags to the list. There are a few lines that of code that I see repeat:
+ self.field_names = self._field_names[:]
+ # Insert the tags field before the summary.
+ summary_index = self.field_names.index('summary')
+ self.field_names[summary_index:summary_index] = [tag_entry.__name__]

^ You might consider a private method or helper function so that there is one right way to add the field.

review: Approve (code)

[Curtis Hovey (sinzui)]

Oh, and you might find that this syntax is easier to read in a few months after your forgot what you were doing:
self.field_names.insert(summary_index, tag_entry.__name__)

=== modified file 'lib/lp/registry/browser/milestone.py'
--- lib/lp/registry/browser/milestone.py	2012-01-16 00:27:33 +0000
+++ lib/lp/registry/browser/milestone.py	2012-01-16 00:27:34 +0000
@@ -447,7 +447,8 @@
         # Make an instance attribute to avoid mutating the class attribute.
         self.field_names = self.field_names[:]
         # Insert the tags field before the summary.
-        self.field_names[3:3] = [tag_entry.__name__]
+        summary_index = self.field_names.index('summary')
+        self.field_names.insert(summary_index, tag_entry.__name__)
 
     @action(_('Register Milestone'), name='register')
     def register_action(self, action, data):
@@ -459,7 +460,7 @@
             summary=data.get('summary'))
         tags = data.get('tags')
         if tags:
-            milestone.setTags(tags.split(), self.user)
+            milestone.setTags(tags.lower().split(), self.user)
         self.next_url = canonical_url(self.context)
 
     @property
@@ -492,7 +493,7 @@
         return canonical_url(self.context)
 
     @property
-    def field_names(self):
+    def _field_names(self):
         """See `LaunchpadFormView`.
 
         There are two series fields, one for for product milestones and the
@@ -508,6 +509,30 @@
             names.append('productseries')
         return names
 
+    @property
+    def initial_values(self):
+        tags = self.context.getTags()
+        tagstring = ' '.join(tags)
+        return dict(tags=tagstring)
+
+    def extendFields(self):
+        """See `LaunchpadFormView`.
+
+        Add a text-entry widget for milestone tags since there is not property
+        on the interface.
+        """
+        tag_entry = TextLine(
+            __name__='tags',
+            title=u'Tags',
+            required=False)
+        self.form_fields += form.Fields(
+            tag_entry, render_context=self.render_context)
+        # Make an instance attribute to avoid mutating the class attribute.
+        self.field_names = self._field_names[:]
+        # Insert the tags field before the summary.
+        summary_index = self.field_names.index('summary')
+        self.field_names.insert(summary_index, tag_entry.__name__)
+
     def setUpFields(self):
         """See `LaunchpadFormView`.
 
@@ -532,7 +557,9 @@
     @action(_('Update'), name='update')
     def update_action(self, action, data):
         """Update the milestone."""
+        tags = data.pop('tags') or u''
         self.updateContextFromData(data)
+        self.context.setTags(tags.lower().split(), self.user)
         self.next_url = canonical_url(self.context)
 
 

=== modified file 'lib/lp/registry/browser/tests/milestone-views.txt'
--- lib/lp/registry/browser/tests/milestone-views.txt	2012-01-16 00:27:33 +0000
+++ lib/lp/registry/browser/tests/milestone-views.txt	2012-01-16 00:27:34 +0000
@@ -347,7 +347,7 @@
     Modify milestone details
 
     >>> view.field_names
-    ['name', 'code_name', 'active', 'dateexpected', 'summary',
+    ['name', 'code_name', 'active', 'dateexpected', 'tags', 'summary',
      'productseries']
 
     >>> print view.cancel_url
@@ -378,6 +378,7 @@
     ...     'field.summary': 'a summary',
     ...     'field.active': 'False',
     ...     'field.productseries': '1',
+    ...     'field.tags': '',
     ...     'field.actions.update': 'Update',
     ...     }
     >>> view = create_initialized_view(milestone, '+edit', form=form)
@@ -406,6 +407,7 @@
     ...     'field.summary': 'a summary',
     ...     'field.active': 'True',
     ...     'field.productseries': '1',
+    ...     'field.tags': '',
     ...     'field.actions.update': 'Update',
     ...     }
     >>> view = create_initialized_view(milestone, '+edit', form=form)
@@ -443,7 +445,7 @@
     >>> milestone = hoary_series.newMilestone('alpha')
     >>> view = create_initialized_view(milestone, '+edit')
     >>> view.field_names
-    ['name', 'code_name', 'active', 'dateexpected', 'summary', 'distroseries']
+    ['name', 'code_name', 'active', 'dateexpected', 'tags', 'summary', 'distroseries']
 
 The distroseries milestone can be updated too.
 
@@ -454,6 +456,7 @@
     ...     'field.summary': 'a summary',
     ...     'field.active': 'False',
     ...     'field.distroseries': '5',
+    ...     'field.tags': '',
     ...     'field.actions.update': 'Update',
     ...     }
     >>> view = create_initialized_view(milestone, '+edit', form=form)

=== modified file 'lib/lp/registry/browser/tests/test_milestone.py'
--- lib/lp/registry/browser/tests/test_milestone.py	2012-01-16 00:27:33 +0000
+++ lib/lp/registry/browser/tests/test_milestone.py	2012-01-16 00:27:34 +0000
@@ -45,8 +45,8 @@
         self.product = self.factory.makeProduct()
         self.series = (
             self.factory.makeProductSeries(product=self.product))
-        owner = self.product.owner
-        login_person(owner)
+        self.owner = self.product.owner
+        login_person(self.owner)
 
     def test_add_milestone(self):
         form = {
@@ -95,7 +95,51 @@
             self.series, '+addmilestone', form=form)
         self.assertEqual([], view.errors)
         expected = sorted(tags.split())
-        self.assertEqual(expected, list(self.product.milestones[0].getTags()))
+        self.assertEqual(expected, self.product.milestones[0].getTags())
+
+
+class TestMilestoneEditView(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        TestCaseWithFactory.setUp(self)
+        self.product = self.factory.makeProduct()
+        self.milestone = self.factory.makeMilestone(
+            name='orig-name', product=self.product)
+        self.owner = self.product.owner
+        login_person(self.owner)
+
+    def test_edit_milestone_with_tags(self):
+        orig_tags = u'b a c'
+        self.milestone.setTags(orig_tags.split(), self.owner)
+        new_tags = u'z a B'
+        form = {
+            'field.name': 'new-name',
+            'field.tags': new_tags,
+            'field.actions.update': 'Update',
+            }
+        view = create_initialized_view(
+            self.milestone, '+edit', form=form)
+        self.assertEqual([], view.errors)
+        self.assertEqual('new-name', self.milestone.name)
+        expected = sorted(new_tags.lower().split())
+        self.assertEqual(expected, self.milestone.getTags())
+
+    def test_edit_milestone_clear_tags(self):
+        orig_tags = u'b a c'
+        self.milestone.setTags(orig_tags.split(), self.owner)
+        form = {
+            'field.name': 'new-name',
+            'field.tags': '',
+            'field.actions.update': 'Update',
+            }
+        view = create_initialized_view(
+            self.milestone, '+edit', form=form)
+        self.assertEqual([], view.errors)
+        self.assertEqual('new-name', self.milestone.name)
+        expected = []
+        self.assertEqual(expected, self.milestone.getTags())
 
 
 class TestMilestoneMemcache(MemcacheTestCase):
@@ -475,7 +519,7 @@
     def _make_form(self, tags):
         return {
             u'field.actions.search': u'Search',
-            u'field.tags': u' '.join(tags)
+            u'field.tags': u' '.join(tags),
             }
 
     def _url_tail(self, url, separator='/'):
@@ -508,7 +552,7 @@
         self.assertEqual(1, len(view.errors))
         self.assertEqual('tags', view.errors[0].field_name)
 
-    def test_buktask_query_count(self):
+    def test_bugtask_query_count(self):
         # Ensure that a correct number of queries is executed for
         # bugtasks retrieval.
         bugtask_count = 10

=== modified file 'lib/lp/registry/javascript/milestoneoverlay.js'
--- lib/lp/registry/javascript/milestoneoverlay.js	2012-01-16 00:27:33 +0000
+++ lib/lp/registry/javascript/milestoneoverlay.js	2012-01-16 00:27:34 +0000
@@ -66,7 +66,11 @@
                             error_handler.getFailureHandler()(
                                 ioid, response, args);
                         },
-                        failure: error_handler_nested.getFailureHandler()
+                        failure: function(ioid, response, args) {
+                            module.last_error = response;
+                            error_handler_nested.getFailureHandler()
+                                (ioid, response, args);
+                            }
                         }
                 };
             lp_client.io_provider.io(milestone.get('self_link'), config);
@@ -93,6 +97,7 @@
                 on: {
                     success: finish_new_milestone,
                     failure: function(ioid, response, args) {
+                        module.last_error = response;
                         delete_new_milestone(
                             milestone, ioid, response, args);
                     }
@@ -105,7 +110,11 @@
             parameters: parameters,
             on: {
                 success: set_milestone_tags,
-                failure: error_handler.getFailureHandler()
+                failure: function(ioid, response, args) {
+                    module.last_error = response;
+                    error_handler.getFailureHandler()
+                        (ioid, response, args);
+                    }
                 }
             });
     };

=== modified file 'lib/lp/registry/javascript/tests/test_milestone_creation.js'
--- lib/lp/registry/javascript/tests/test_milestone_creation.js	2012-01-16 00:27:33 +0000
+++ lib/lp/registry/javascript/tests/test_milestone_creation.js	2012-01-16 00:27:34 +0000
@@ -50,6 +50,7 @@
     test_configure: function() {
         // Ensure configuring the module works as it will be needed in
         // subsequent tests.
+        var client = new Y.lp.client.Launchpad({sync: true});
         var config = {
             milestone_form_uri: 'a',
             series_uri: 'b',
@@ -62,7 +63,6 @@
         // Setup the fixture, retrieving the objects we need for the test.
         var data = serverfixture.setup(this, 'setup');
         var client = new Y.lp.client.Launchpad({sync: true});
-        var config = makeTestConfig();
         var product = new Y.lp.client.Entry(
             client, data.product, data.product.self_link);
         var product_name = product.get('name');
@@ -95,10 +95,12 @@
         };
 
         // Test the creation of the new milestone.
+        the_module.last_error = null;
         the_module.save_new_milestone(params);
+        Y.Assert.isNull(the_module.last_error,
+            "last_error is: " + the_module.last_error);
 
         // Verify the milestone was created.
-        var client = new Y.lp.client.Launchpad({sync: true});
         var product = new Y.lp.client.Entry(
             client, data.product, data.product.self_link);
         config = makeTestConfig({parameters: {name: milestone_name}});
@@ -106,7 +108,8 @@
         product.named_get('getMilestone', config);
         Y.Assert.isTrue(config.successful, 'Getting milestone failed');
         var milestone = config.result;
-        Y.Assert.isInstanceOf(Y.lp.client.Entry, milestone);
+        Y.Assert.isInstanceOf(
+            Y.lp.client.Entry, milestone, 'Milestone is not an Entry');
         Y.Assert.areEqual(milestone_name, milestone.get('name'));
         Y.Assert.areEqual(code_name, milestone.get('code_name'));
         // Ensure no tags are created.
@@ -144,7 +147,10 @@
         };
 
         // Test the creation of the new milestone.
+        the_module.last_error = null;
         the_module.save_new_milestone(params);
+        Y.Assert.isNull(the_module.last_error,
+            "last_error is: " + the_module.last_error);
 
         // Verify the milestone was created.
         var product = new Y.lp.client.Entry(
@@ -154,7 +160,8 @@
         product.named_get('getMilestone', config);
         Y.Assert.isTrue(config.successful, 'Getting milestone failed');
         var milestone = config.result;
-        Y.Assert.isInstanceOf(Y.lp.client.Entry, milestone);
+        Y.Assert.isInstanceOf(
+            Y.lp.client.Entry, milestone, 'Milestone is not an Entry');
         Y.Assert.areEqual(milestone_name, milestone.get('name'));
         Y.Assert.areEqual(code_name, milestone.get('code_name'));
         // Ensure the tags are created.

=== modified file 'lib/lp/registry/model/milestone.py'
--- lib/lp/registry/model/milestone.py	2012-01-16 00:27:33 +0000
+++ lib/lp/registry/model/milestone.py	2012-01-16 00:27:34 +0000
@@ -301,22 +301,21 @@
         else:
             store.find(
                 MilestoneTag, MilestoneTag.milestone_id == self.id).remove()
-        store.commit()
 
     def getTagsData(self):
         """See IMilestone."""
         # Prevent circular references.
         from lp.registry.model.milestonetag import MilestoneTag
         store = Store.of(self)
-        return list(store.find(
+        return store.find(
             MilestoneTag, MilestoneTag.milestone_id == self.id
-            ).order_by(MilestoneTag.tag))
+            ).order_by(MilestoneTag.tag)
 
     def getTags(self):
         """See IMilestone."""
         # Prevent circular references.
         from lp.registry.model.milestonetag import MilestoneTag
-        return self.getTagsData().values(MilestoneTag.tag)
+        return list(self.getTagsData().values(MilestoneTag.tag))
 
 
 class MilestoneSet:

=== modified file 'lib/lp/registry/tests/test_milestonetag.py'
--- lib/lp/registry/tests/test_milestonetag.py	2012-01-16 00:27:33 +0000
+++ lib/lp/registry/tests/test_milestonetag.py	2012-01-16 00:27:34 +0000
@@ -5,8 +5,8 @@
 
 __metaclass__ = type
 
+import datetime
 import transaction
-import datetime
 
 from lp.testing.layers import (
     AppServerLayer,
@@ -20,7 +20,6 @@
     person_logged_in,
     TestCaseWithFactory,
     WebServiceTestCase,
-    ws_object,
     )
 
 
@@ -225,6 +224,7 @@
     def test_get_tags(self):
         tags = [u'zeta', u'alpha', u'beta']
         self.milestone.setTags(tags, self.owner)
+        transaction.commit()
         self.assertEqual(sorted(tags), self.ws_milestone.getTags())
 
     def test_set_tags_initial(self):