Merge lp:~thumper/launchpad/recipe-inline-edit-recipe-text into lp:launchpad

[Tim Penhey]

[r=leonardr][bug=673530,708265] Allow inline editing of the recipe text.

This branch adds the ability to edit the recipe text on the
main recipe page.

As part of this I've tweaked the styles so we don't get the
horrible overlap at the top of the multiline editors. I
believe this was missed during the yui 3.1 -> 3.2 transition
as the yui- prefix became yui3-.

lib/lp/code/errors.py
 - annotated the RecipeParseError to be a 400 Bad Request error.
 - also for the NoSuchBranch and PrivateBranchRecipe errors as
   they can also be raised when setting recipe text
 - pulled the webservice_error up to the base class instead of
   having it in each of the three derived classes.

lib/lp/code/interfaces/sourcepackagerecipe.py
 - change the setRecipeText from being exported as a writable
   operatioln to be a mutator for the recipe_text attribute.
   I talked with Leonard about the stack of adapters here.
   The operate from bottom to top.

lib/lp/code/model/sourcepackagerecipe.py
 - in order to get the full error returned to the client, we
   need to call the "expose" method from lazr.restful. I
   chatted with Leonard about this one too as it seemed a
   little strange. There was some reason at some stage, but
   he couldn't recall the underlying reason at the time I
   was asking. We may perhaps want to re-evaluate the need
   for this method at some time.

[Leonard Richardson (leonardr)]

I don't understand why you got rid of the CSS styles for edit-description and edit-commit-message. Did you already generalize those widgets, and you're just cleaning up the CSS?

Your annotations create a situation where setRecipeText is published in version 1.0 of the web service but absent in version devel, and in which you can PATCH ISourcePackageRecipe['recipe_text'] in devel but not in 1.0. I'd like to see some tests of that using launchpadlib. Your windmill test only tests that you can PATCH ISourcePackageRecipe['recipe_test'] in devel.

I don't think this is that big a deal, but if you care enough to distinguish between 1.0 and devel in the first place, you should test the different versions. Let me know if you need help with the tests. I'll start you out by saying that you can us launchpadlib_for("test", person, version="1.0") to get a launchpadlib that operates against 1.0.

review: Needs Fixing

[Tim Penhey (thumper)]

As discussed over mumble and IRC, the CSS styles were changed to
be based more on the classes rather than the IDs.

Tests have been added, as you're aware.

I've also bumped the lazr.restful version here.

[Leonard Richardson (leonardr)]

<leonardr> thumper, i don't think test_recipe_text tests anything? to test that you can edit the recipe text, you would have to call lp_save()
<thumper> do we have to call lp_save for mutators?
<leonardr> thumper: you don't have to call lp_save() when you invoke a named operation
<leonardr> but the purpose of a mutator is to hide a named operation so that it looks like normal field access
<thumper> leonardr: but you do for mutators?
<leonardr> and in that case you do need to call lp_save()
* thumper adds a lp_save()

review: Approve

=== modified file 'lib/canonical/launchpad/icing/style-3-0.css.in'
--- lib/canonical/launchpad/icing/style-3-0.css.in	2011-02-14 00:58:22 +0000
+++ lib/canonical/launchpad/icing/style-3-0.css.in	2011-02-17 22:48:58 +0000
@@ -947,18 +947,18 @@
     top: -3px;
     margin-left: 0.5em;
     }
-.yui-ieditor {
+.yui3-ieditor {
     padding-right: 288px;
     }
-.lazr-multiline-edit .yui-ieditor {
+h1 .yui3-ieditor-errors {
+    font-size: 0.5em;
+    }
+.lazr-multiline-edit .yui3-ieditor {
     padding-right: 0;
     }
 .lazr-multiline-edit textarea {
     max-width: inherit;
     }
-.lazr-multiline-edit .yui-ieditor-input {
-    padding-right: 5px !important;
-    }
 .widget-hd.js-action {
     /* The js-action class is also used for non-links, for example, with
        expand/collapse sections. */
@@ -1825,8 +1825,8 @@
     font-size: 93%;
     margin: 1em 0;
     }
-div#edit-description .yui-ieditor-input,
-div#edit-commit_message .yui-ieditor-input {
+
+.yui3-ieditor-multiline .yui3-ieditor-input {
     top: 0;
     }
 
 
=== modified file 'lib/lp/bugs/interfaces/bugsupervisor.py'
--- lib/lp/bugs/interfaces/bugsupervisor.py	2010-08-20 20:31:18 +0000
+++ lib/lp/bugs/interfaces/bugsupervisor.py	2011-02-17 22:48:58 +0000
@@ -36,9 +36,6 @@
             "all bugs and will receive email about all activity on all bugs "
             "for this project, so that should be a factor in your decision.  "
             "The bug supervisor will also have access to all private bugs."),
-
-
-
         required=False, vocabulary='ValidPersonOrTeam', readonly=True))
 
     @mutator_for(bug_supervisor)
 
=== modified file 'lib/lp/code/browser/sourcepackagerecipe.py'
--- lib/lp/code/browser/sourcepackagerecipe.py	2011-02-15 11:31:56 +0000
+++ lib/lp/code/browser/sourcepackagerecipe.py	2011-02-17 22:48:58 +0000
@@ -72,6 +72,7 @@
 from lp.app.browser.lazrjs import (
     BooleanChoiceWidget,
     InlineEditPickerWidget,
+    TextAreaEditorWidget,
     )
 from lp.app.browser.tales import format_link
 from lp.app.widgets.itemswidgets import (
@@ -264,6 +265,12 @@
             step_title='Select a PPA')
 
     @property
+    def recipe_text_widget(self):
+        """The recipe text as widget HTML."""
+        recipe_text = ISourcePackageRecipe['recipe_text']
+        return TextAreaEditorWidget(self.context, recipe_text, title="")
+
+    @property
     def daily_build_widget(self):
         return BooleanChoiceWidget(
             self.context, ISourcePackageRecipe['build_daily'],
 
=== modified file 'lib/lp/code/errors.py'
--- lib/lp/code/errors.py	2011-01-21 21:12:58 +0000
+++ lib/lp/code/errors.py	2011-02-17 22:48:58 +0000
@@ -42,10 +42,17 @@
 
 import httplib
 
-from lazr.restful.declarations import webservice_error
+from bzrlib.plugins.builder.recipe import RecipeParseError
+from lazr.restful.declarations import (
+    error_status,
+    webservice_error,
+    )
 
 from lp.app.errors import NameLookupFailed
 
+# Annotate the RecipeParseError's with a 400 webservice status.
+error_status(400)(RecipeParseError)
+
 
 class BadBranchMergeProposalSearchContext(Exception):
     """The context is not valid for a branch merge proposal search."""
@@ -204,11 +211,15 @@
 class NoSuchBranch(NameLookupFailed):
     """Raised when we try to load a branch that does not exist."""
 
+    webservice_error(400)
+
     _message_prefix = "No such branch"
 
 
 class PrivateBranchRecipe(Exception):
 
+    webservice_error(400)
+
     def __init__(self, branch):
         message = (
             'Recipe may not refer to private branch: %s' %
@@ -265,6 +276,8 @@
 class TooNewRecipeFormat(Exception):
     """The format of the recipe supplied was too new."""
 
+    webservice_error(400)
+
     def __init__(self, supplied_format, newest_supported):
         super(TooNewRecipeFormat, self).__init__()
         self.supplied_format = supplied_format
@@ -273,6 +286,8 @@
 
 class RecipeBuildException(Exception):
 
+    webservice_error(400)
+
     def __init__(self, recipe, distroseries, template):
         self.recipe = recipe
         self.distroseries = distroseries
@@ -283,8 +298,6 @@
 class TooManyBuilds(RecipeBuildException):
     """A build was requested that exceeded the quota."""
 
-    webservice_error(400)
-
     def __init__(self, recipe, distroseries):
         RecipeBuildException.__init__(
             self, recipe, distroseries,
@@ -295,8 +308,6 @@
 class BuildAlreadyPending(RecipeBuildException):
     """A build was requested when an identical build was already pending."""
 
-    webservice_error(400)
-
     def __init__(self, recipe, distroseries):
         RecipeBuildException.__init__(
             self, recipe, distroseries,
@@ -306,8 +317,6 @@
 class BuildNotAllowedForDistro(RecipeBuildException):
     """A build was requested against an unsupported distroseries."""
 
-    webservice_error(400)
-
     def __init__(self, recipe, distroseries):
         RecipeBuildException.__init__(
             self, recipe, distroseries,
 
=== modified file 'lib/lp/code/interfaces/sourcepackagerecipe.py'
--- lib/lp/code/interfaces/sourcepackagerecipe.py	2011-02-14 01:48:57 +0000
+++ lib/lp/code/interfaces/sourcepackagerecipe.py	2011-02-17 22:48:58 +0000
@@ -25,13 +25,17 @@
     export_as_webservice_entry,
     export_write_operation,
     exported,
+    mutator_for,
+    operation_for_version,
     operation_parameters,
+    operation_removed_in_version,
     REQUEST_USER,
     )
 from lazr.restful.fields import (
     CollectionField,
     Reference,
     )
+from lazr.restful.interface import copy_field
 from zope.interface import (
     Attribute,
     Interface,
@@ -97,7 +101,7 @@
             required=True, readonly=True,
             vocabulary='ValidPersonOrTeam'))
 
-    recipe_text = exported(Text())
+    recipe_text = exported(Text(readonly=True))
 
     def isOverQuota(requester, distroseries):
         """True if the recipe/requester/distroseries combo is >= quota.
@@ -135,7 +139,11 @@
 class ISourcePackageRecipeEdit(Interface):
     """ISourcePackageRecipe methods that require launchpad.Edit permission."""
 
-    @operation_parameters(recipe_text=Text())
+    @mutator_for(ISourcePackageRecipeView['recipe_text'])
+    @operation_for_version("devel")
+    @operation_parameters(
+        recipe_text=copy_field(
+            ISourcePackageRecipeView['recipe_text']))
     @export_write_operation()
     def setRecipeText(recipe_text):
         """Set the text of the recipe."""
 
=== modified file 'lib/lp/code/model/sourcepackagerecipe.py'
--- lib/lp/code/model/sourcepackagerecipe.py	2011-02-17 04:58:44 +0000
+++ lib/lp/code/model/sourcepackagerecipe.py	2011-02-17 22:48:58 +0000
@@ -15,7 +15,10 @@
     datetime,
     timedelta,
     )
+
+from bzrlib.plugins.builder.recipe import RecipeParseError
 from lazr.delegates import delegates
+from lazr.restful.error import expose
 from pytz import utc
 from storm.expr import (
     And,
@@ -50,7 +53,10 @@
 from lp.code.errors import (
     BuildAlreadyPending,
     BuildNotAllowedForDistro,
+    NoSuchBranch,
+    PrivateBranchRecipe,
     TooManyBuilds,
+    TooNewRecipeFormat,
     )
 from lp.code.interfaces.sourcepackagerecipe import (
     ISourcePackageRecipe,
@@ -160,8 +166,13 @@
         return self._recipe_data.base_branch
 
     def setRecipeText(self, recipe_text):
-        parsed = SourcePackageRecipeData.getParsedRecipe(recipe_text)
-        self._recipe_data.setRecipe(parsed)
+        try:
+            parsed = SourcePackageRecipeData.getParsedRecipe(recipe_text)
+            self._recipe_data.setRecipe(parsed)
+        except (RecipeParseError, NoSuchBranch, PrivateBranchRecipe,
+                TooNewRecipeFormat) as e:
+            expose(e)
+            raise
 
     @property
     def recipe_text(self):
 
=== modified file 'lib/lp/code/model/tests/test_sourcepackagerecipe.py'
--- lib/lp/code/model/tests/test_sourcepackagerecipe.py	2011-02-17 10:10:29 +0000
+++ lib/lp/code/model/tests/test_sourcepackagerecipe.py	2011-02-17 22:48:58 +0000
@@ -616,11 +616,40 @@
         self.assertEqual([build], list(recipe.getBuilds()))
         self.assertEqual([], list(recipe.getPendingBuilds()))
 
+    def test_setRecipeText_private_base_branch(self):
+        source_package_recipe = self.factory.makeSourcePackageRecipe()
+        with person_logged_in(source_package_recipe.owner):
+            branch = self.factory.makeAnyBranch(
+                private=True, owner=source_package_recipe.owner)
+            recipe_text = self.factory.makeRecipeText(branch)
+            e = self.assertRaises(
+                PrivateBranchRecipe, source_package_recipe.setRecipeText,
+                recipe_text)
+            self.assertEqual(
+                'Recipe may not refer to private branch: %s' %
+                branch.bzr_identity, str(e))
+
+    def test_setRecipeText_private_referenced_branch(self):
+        source_package_recipe = self.factory.makeSourcePackageRecipe()
+        with person_logged_in(source_package_recipe.owner):
+            base_branch = self.factory.makeAnyBranch(
+                owner=source_package_recipe.owner)
+            referenced_branch = self.factory.makeAnyBranch(
+                private=True, owner=source_package_recipe.owner)
+            recipe_text = self.factory.makeRecipeText(
+                base_branch, referenced_branch)
+            e = self.assertRaises(
+                PrivateBranchRecipe, source_package_recipe.setRecipeText,
+                recipe_text)
+            self.assertEqual(
+                'Recipe may not refer to private branch: %s' %
+                referenced_branch.bzr_identity, str(e))
+
     def test_getBuilds_ignores_disabled_archive(self):
         # Builds into a disabled archive aren't returned.
         archive = self.factory.makeArchive()
         recipe = self.factory.makeSourcePackageRecipe()
-        build = self.factory.makeSourcePackageRecipeBuild(
+        self.factory.makeSourcePackageRecipeBuild(
             recipe=recipe, archive=archive)
         with person_logged_in(archive.owner):
             archive.disable()
@@ -825,7 +854,7 @@
         branch = self.factory.makeBranch()
         return MINIMAL_RECIPE_TEXT % branch.bzr_identity
 
-    def makeRecipe(self, user=None, owner=None, recipe_text=None):
+    def makeRecipe(self, user=None, owner=None, recipe_text=None, version='devel'):
         # rockstar 21 Jul 2010 - This function does more commits than I'd
         # like, but it's the result of the fact that the webservice runs in a
         # separate thread so doesn't get the database updates without those
@@ -839,8 +868,9 @@
             recipe_text = self.makeRecipeText()
         db_archive = self.factory.makeArchive(owner=owner, name="recipe-ppa")
         transaction.commit()
-        launchpad = launchpadlib_for('test', user,
-                service_root=self.layer.appserver_root_url('api'))
+        launchpad = launchpadlib_for(
+            'test', user, version=version,
+            service_root=self.layer.appserver_root_url('api'))
         login(ANONYMOUS)
         distroseries = ws_object(launchpad, db_distroseries)
         ws_owner = ws_object(launchpad, owner)
@@ -871,38 +901,21 @@
     def test_recipe_text(self):
         recipe_text2 = self.makeRecipeText()
         recipe = self.makeRecipe()[0]
+        recipe.recipe_text = recipe_text2
+        recipe.lp_save()
+        self.assertEqual(recipe_text2, recipe.recipe_text)
+
+    def test_recipe_text_setRecipeText_not_in_devel(self):
+        recipe = self.makeRecipe()[0]
+        method = getattr(recipe, 'setRecipeText', None)
+        self.assertIs(None, method)
+
+    def test_recipe_text_setRecipeText_in_one_zero(self):
+        recipe_text2 = self.makeRecipeText()
+        recipe = self.makeRecipe(version='1.0')[0]
         recipe.setRecipeText(recipe_text=recipe_text2)
         self.assertEqual(recipe_text2, recipe.recipe_text)
 
-    def test_setRecipeText_private_base_branch(self):
-        source_package_recipe = self.factory.makeSourcePackageRecipe()
-        with person_logged_in(source_package_recipe.owner):
-            branch = self.factory.makeAnyBranch(
-                private=True, owner=source_package_recipe.owner)
-            recipe_text = self.factory.makeRecipeText(branch)
-            e = self.assertRaises(
-                PrivateBranchRecipe, source_package_recipe.setRecipeText,
-                recipe_text)
-            self.assertEqual(
-                'Recipe may not refer to private branch: %s' %
-                branch.bzr_identity, str(e))
-
-    def test_setRecipeText_private_referenced_branch(self):
-        source_package_recipe = self.factory.makeSourcePackageRecipe()
-        with person_logged_in(source_package_recipe.owner):
-            base_branch = self.factory.makeAnyBranch(
-                owner=source_package_recipe.owner)
-            referenced_branch = self.factory.makeAnyBranch(
-                private=True, owner=source_package_recipe.owner)
-            recipe_text = self.factory.makeRecipeText(
-                base_branch, referenced_branch)
-            e = self.assertRaises(
-                PrivateBranchRecipe, source_package_recipe.setRecipeText,
-                recipe_text)
-            self.assertEqual(
-                'Recipe may not refer to private branch: %s' %
-                referenced_branch.bzr_identity, str(e))
-
     def test_getRecipe(self):
         """Person.getRecipe returns the named recipe."""
         recipe, user = self.makeRecipe()[:-1]
 
=== modified file 'lib/lp/code/templates/sourcepackagerecipe-index.pt'
--- lib/lp/code/templates/sourcepackagerecipe-index.pt	2011-02-11 01:50:20 +0000
+++ lib/lp/code/templates/sourcepackagerecipe-index.pt	2011-02-17 22:48:58 +0000
@@ -12,6 +12,10 @@
     .binary-build .indent {
       padding-left: 2em;
     }
+    #edit-recipe_text {
+      font-family: "UbuntuBeta Mono","Ubuntu Mono",monospace;
+      margin-top: -15px;
+    }
   </style>
 </metal:block>
 
@@ -164,7 +168,7 @@
       </div>
       <div class='portlet'>
         <h2>Recipe contents</h2>
-        <pre tal:content="context/recipe_text" />
+        <tal:widget replace="structure view/recipe_text_widget"/>
       </div>
     </div>
   </div>
 
=== modified file 'lib/lp/code/windmill/tests/test_recipe_index.py'
--- lib/lp/code/windmill/tests/test_recipe_index.py	2011-02-14 00:58:45 +0000
+++ lib/lp/code/windmill/tests/test_recipe_index.py	2011-02-17 22:48:58 +0000
@@ -44,3 +44,29 @@
         freshly_fetched_recipe = Store.of(recipe).find(
             SourcePackageRecipe, SourcePackageRecipe.id == recipe.id).one()
         self.assertTrue(freshly_fetched_recipe.build_daily)
+
+    def test_inline_recipe_text_errors(self):
+        eric = self.factory.makePerson(
+            name="eric", displayname="Eric the Viking", password="test",
+            email="[e-mail]")
+        recipe = self.factory.makeSourcePackageRecipe(owner=eric)
+        recipe_text = recipe.recipe_text + 'merge WTF?'
+        transaction.commit()
+
+        client, start_url = self.getClientFor(recipe, user=eric)
+        client.click(
+            jquery=u'("div#edit-recipe_text a.yui3-editable_text-trigger")[0]')
+        client.waits.forElement(
+            jquery=u'("div#edit-recipe_text textarea.yui3-ieditor-input")',
+            timeout=FOR_ELEMENT)
+        client.type(
+            text=recipe_text,
+            jquery=u'("div#edit-recipe_text textarea.yui3-ieditor-input")[0]')
+        client.click(
+            jquery=u'("div#edit-recipe_text button.yui3-ieditor-submit_button")[0]')
+        client.waits.forElement(
+            jquery=u'("div#edit-recipe_text textarea.yui3-ieditor-errors")',
+            timeout=FOR_ELEMENT)
+        client.asserts.assertTextIn(
+            jquery=u'("div#edit-recipe_text textarea.yui3-ieditor-errors")[0]',
+            validator=u'End of line while looking for the branch url.')
 
=== modified file 'versions.cfg'
--- versions.cfg	2011-02-11 04:25:27 +0000
+++ versions.cfg	2011-02-17 22:48:58 +0000
@@ -33,7 +33,7 @@
 lazr.delegates = 1.2.0
 lazr.enum = 1.1.2
 lazr.lifecycle = 1.1
-lazr.restful = 0.16.0-r171
+lazr.restful = 0.16.1
 lazr.restfulclient = 0.11.2
 lazr.smtptest = 1.1
 lazr.testing = 0.1.1