Merge lp:~mars/launchpad/fix-js-unittests into lp:launchpad/db-devel

[Māris Fogels]

[Māris Fogels (mars)]

Hi,

This branch fixes a number of failures in the JavaScript unit test suite that
resulted from the YUI 3.0.0 upgrade.

I manually ran all of the unit tests under lib/canonical/launchpad/javascript/
to ensure that the features under test still work.

My changes are free of lint. There is one spurious lint error in lp.js.

Maris

[Gavin Panella (allenap)]

Has Y.fail() been renamed as Y.error(), or is it a deprecated synonym? If neither, then I'd be inclined to leave it alone; "fail" more closely matches the terminology used in the Python testing world. Y.error() sounds like it should be used when test set-up fails, not when an assertion fails.

I'm very happy to see that the 'click' simulation has been fixed (re. test_me_too.js). Figuring that one out caused me a very sweary afternoon of pain.

Thanks for doing this :)

review: Approve

[Māris Fogels (mars)]

On Wed, Dec 2, 2009 at 8:54 AM, Gavin Panella <email address hidden> wrote:
> Review: Approve
> Has Y.fail() been renamed as Y.error(), or is it a deprecated synonym? If neither, then I'd be inclined to leave it alone; "fail" more closely matches the terminology used in the Python testing world. Y.error() sounds like it should be used when test set-up fails, not when an assertion fails.

Yes, Y.fail() became Y.error(). Y.fail() is now only in the 'test' module, and has become the standard xUnit "fail()" call.

This caused a subtle bug: my tests were failing when they should have been raising errors. Turns out the code under test was calling Y.fail() itself, so it raised a failure exception instead of a real error.

>
> I'm very happy to see that the 'click' simulation has been fixed (re. test_me_too.js). Figuring that one out caused me a very sweary afternoon of pain.

It took me a while, too. I remembered a comment Bjorn made during the lazr-js sprint about using click because mousedown was unreliable. Turns out it was true!

>
> Thanks for doing this :)
>

My pleasure! Thank you for the review.

Mars

[lp:~mars/launchpad/fix-js-unittests]

8737. By Māris Fogels on 2009-12-07

Merge from db-devel

8738. By Māris Fogels on 2009-12-09

Merged trunk instead of db-devel so we can land on the mainline.

8739. By Māris Fogels on 2009-12-11

Merge from RF. Fixed one conflict.

[Māris Fogels (mars)]

Rejecting my own branch, as it has become stale (because PQM refuses to land it, argh!)

Unmerged revisions

8739. By Māris Fogels on 2009-12-11

Merge from RF. Fixed one conflict.

8738. By Māris Fogels on 2009-12-09

Merged trunk instead of db-devel so we can land on the mainline.

8737. By Māris Fogels on 2009-12-07

Merge from db-devel

8736. By Māris Fogels on 2009-12-02

Fixed a number of JS unit tests that broke during the YUI 3.0.0 upgrade.

=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg	2009-12-02 13:08:18 +0000
+++ database/schema/security.cfg	2009-12-11 13:41:18 +0000
@@ -1216,6 +1216,9 @@
 public.libraryfilecontent               = SELECT, INSERT
 
 # rosetta auto imports
+public.pofile                           = SELECT
+public.potemplate                       = SELECT
+public.translationgroup                 = SELECT
 public.translationimportqueueentry      = SELECT, INSERT, UPDATE
 
 # Closing bugs.
 
=== modified file 'lib/canonical/launchpad/javascript/bugs/filebug-dupefinder.js'
--- lib/canonical/launchpad/javascript/bugs/filebug-dupefinder.js	2009-12-10 13:45:40 +0000
+++ lib/canonical/launchpad/javascript/bugs/filebug-dupefinder.js	2009-12-11 13:41:18 +0000
@@ -56,8 +56,13 @@
 
     // Check that the details_div actually exists and raise an error if
     // we can't find it.
+<<<<<<< TREE
     if (!Y.Lang.isValue(details_div)) {
         Y.fail(
+=======
+    if (!Y.Lang.isValue(details_div)) {
+        Y.error(
+>>>>>>> MERGE-SOURCE
             "Unable to find details div for expander " + expander.get('id'));
     } else {
         return details_div;
 
=== modified file 'lib/canonical/launchpad/javascript/client/client.js'
--- lib/canonical/launchpad/javascript/client/client.js	2009-12-07 12:26:37 +0000
+++ lib/canonical/launchpad/javascript/client/client.js	2009-12-11 13:41:18 +0000
@@ -694,11 +694,11 @@
      */
     initializer: function(config) {
         if (!Y.Lang.isString(config.patch)) {
-            Y.fail("missing config: 'patch' containing the attribute name");
+            Y.error("missing config: 'patch' containing the attribute name");
         }
 
         if (!Y.Lang.isString(config.resource)) {
-            Y.fail("missing config: 'resource' containing the URL to patch");
+            Y.error("missing config: 'resource' containing the URL to patch");
         }
 
         // Save the config object that the user passed in so that we can pass
 
=== modified file 'lib/canonical/launchpad/javascript/lp/comment.js'
--- lib/canonical/launchpad/javascript/lp/comment.js	2009-11-26 19:54:52 +0000
+++ lib/canonical/launchpad/javascript/lp/comment.js	2009-12-11 13:41:18 +0000
@@ -361,7 +361,6 @@
     },
     renderUI: function() {
         CodeReviewComment.superclass.renderUI.apply(this);
-        Y.one('#inline-add-comment').setStyle('display', 'block');
     },
     /**
      * Implementation of Widget.bindUI: Bind events to methods.
 
=== modified file 'lib/canonical/launchpad/javascript/lp/lp.js'
--- lib/canonical/launchpad/javascript/lp/lp.js	2009-12-07 12:26:37 +0000
+++ lib/canonical/launchpad/javascript/lp/lp.js	2009-12-11 13:41:18 +0000
@@ -66,10 +66,10 @@
 
         // If either the wrapper or the icon is null, raise an error.
         if (wrapper_div === null) {
-            Y.fail("Collapsible has no wrapper div.");
+            Y.error("Collapsible has no wrapper div.");
         }
         if (icon === null) {
-            Y.fail("Collapsible has no icon.");
+            Y.error("Collapsible has no icon.");
         }
 
         // Work out the target icon and animation based on the state of
@@ -791,4 +791,3 @@
     tag2.style.display = display;
     return false;
 }
-
 
=== modified file 'lib/canonical/launchpad/javascript/soyuz/lp_dynamic_dom_updater.js'
--- lib/canonical/launchpad/javascript/soyuz/lp_dynamic_dom_updater.js	2009-11-24 16:11:43 +0000
+++ lib/canonical/launchpad/javascript/soyuz/lp_dynamic_dom_updater.js	2009-12-11 13:41:18 +0000
@@ -14,7 +14,7 @@
 var lp = Y.namespace('lp');
 
     /**
-     * The DomUpdater class provides the ability to plugin functionality 
+     * The DomUpdater class provides the ability to plugin functionality
      * to a DOM subtree so that it can update itself when given data in an
      * expected format.
      *
@@ -79,7 +79,7 @@
     Y.lp.DomUpdater = DomUpdater;
 
     /**
-     * The DynamicDomUpdater class provides the ability to plug functionality 
+     * The DynamicDomUpdater class provides the ability to plug functionality
      * into a DOM subtree so that it can update itself using an LP api method.
      *
      * For example:
@@ -94,7 +94,7 @@
      * Once configured, the 'table' dom subtree will now update itself
      * by calling the user defined domUpdateFunction (with a default interval
      * of 6000ms) with the result of the LPs api call.
-     * 
+     *
      * @class DynamicDomUpdater
      * @extends DomUpdater
      * @constructor
@@ -326,7 +326,7 @@
             }
 
             if (actual_interval_updated) {
-                Y.log("Actual poll interval updated to " + 
+                Y.log("Actual poll interval updated to " +
                     this._actual_interval + "ms.");
             }
 
@@ -344,7 +344,7 @@
          * @private
          */
         _handleFailure: function(id, request) {
-            Y.fail("LP.DynamicDomUpdater for " + this.get("host") +
+            Y.error("LP.DynamicDomUpdater for " + this.get("host") +
                    " failed to get dynamic data.");
         }
     });
 
=== modified file 'lib/lp/code/browser/branch.py'
--- lib/lp/code/browser/branch.py	2009-12-11 00:56:16 +0000
+++ lib/lp/code/browser/branch.py	2009-12-11 13:41:18 +0000
@@ -79,9 +79,14 @@
 from lp.code.browser.branchmergeproposal import (
     latest_proposals_for_each_branch)
 from lp.code.enums import (
+<<<<<<< TREE
     BranchLifecycleStatus, BranchType, RevisionControlSystems,
     UICreatableBranchType)
 from lp.code.errors import InvalidBranchMergeProposal
+=======
+    BranchLifecycleStatus, BranchType, UICreatableBranchType)
+from lp.code.errors import InvalidBranchMergeProposal
+>>>>>>> MERGE-SOURCE
 from lp.code.interfaces.branch import (
     BranchCreationForbidden, BranchExists, IBranch,
     user_has_special_branch_access)
 
=== modified file 'lib/lp/code/browser/branchmergeproposal.py'
=== modified file 'lib/lp/code/browser/codereviewcomment.py'
--- lib/lp/code/browser/codereviewcomment.py	2009-10-29 23:51:08 +0000
+++ lib/lp/code/browser/codereviewcomment.py	2009-12-11 13:41:18 +0000
@@ -204,7 +204,7 @@
 
     class MyDropWidget(DropdownWidget):
         "Override the default no-value display name to -Select-."
-        _messageNoValue = '-Select-'
+        _messageNoValue = 'Comment only'
 
     schema = IEditCodeReviewComment
 
@@ -251,10 +251,11 @@
     @action('Save Comment', name='add')
     def add_action(self, action, data):
         """Create the comment..."""
+        vote = data.get('vote')
+        review_type = data.get('review_type')
         comment = self.branch_merge_proposal.createComment(
             self.user, subject=None, content=data['comment'],
-            parent=self.reply_to, vote=data['vote'],
-            review_type=data['review_type'])
+            parent=self.reply_to, vote=vote, review_type=review_type)
 
     @property
     def next_url(self):
 
=== modified file 'lib/lp/code/configure.zcml'
=== modified file 'lib/lp/code/errors.py'
--- lib/lp/code/errors.py	2009-12-07 06:51:42 +0000
+++ lib/lp/code/errors.py	2009-12-11 13:41:18 +0000
@@ -1,3 +1,4 @@
+<<<<<<< TREE
 # Copyright 2009 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
@@ -54,3 +55,53 @@
 
 class WrongBranchMergeProposal(Exception):
     """The comment requested is not associated with this merge proposal."""
+=======
+# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Errors used in the lp/code modules."""
+
+__metaclass__ = type
+__all__ = [
+    'BadBranchMergeProposalSearchContext',
+    'BadStateTransition',
+    'BranchMergeProposalExists',
+    'InvalidBranchMergeProposal',
+    'UserNotBranchReviewer',
+    'WrongBranchMergeProposal',
+]
+
+
+class BadBranchMergeProposalSearchContext(Exception):
+    """The context is not valid for a branch merge proposal search."""
+
+
+class BadStateTransition(Exception):
+    """The user requested a state transition that is not possible."""
+
+
+class InvalidBranchMergeProposal(Exception):
+    """Raised during the creation of a new branch merge proposal.
+
+    The text of the exception is the rule violation.
+    """
+
+
+class BranchMergeProposalExists(InvalidBranchMergeProposal):
+    """Raised if there is already a matching BranchMergeProposal."""
+
+
+class UserNotBranchReviewer(Exception):
+    """The user who attempted to review the merge proposal isn't a reviewer.
+
+    A specific reviewer may be set on a branch.  If a specific reviewer
+    isn't set then any user in the team of the owner of the branch is
+    considered a reviewer.
+    """
+
+
+class WrongBranchMergeProposal(Exception):
+    """The comment requested is not associated with this merge proposal."""
+
+
+>>>>>>> MERGE-SOURCE
 
=== modified file 'lib/lp/code/model/tests/test_branchmergeproposals.py'
=== modified file 'lib/lp/code/templates/branch-import-details.pt'
=== modified file 'lib/lp/code/templates/branchmergeproposal-index.pt'
--- lib/lp/code/templates/branchmergeproposal-index.pt	2009-11-26 23:36:50 +0000
+++ lib/lp/code/templates/branchmergeproposal-index.pt	2009-12-11 13:41:18 +0000
@@ -21,6 +21,24 @@
     #commit-message, #edit-commit-message {
       margin: 1em 0 0 0;
     }
+    #add-comment-form {
+      max-width: 60em;
+      padding-bottom: 3em;
+    }
+    #add-comment-form textarea{
+      width: 100%;
+      max-width: inherit;
+    }
+    #add-comment-form .actions {
+      float: right;
+      margin: 0 -0.5em;
+    }
+    #add-comment-review-fields {
+      margin-top: 1em;
+    }
+    #add-comment-review-fields div {
+      display: inline;
+    }
     /* A page-specific fix for inline text are editing to line up box. */
     #edit-commit-message .yui-ieditor-input { top: 0; }
   </style>
@@ -92,6 +110,12 @@
   </div>
 
   <div class="yui-g">
+    <tal:not-logged-in condition="not: view/user">
+      <div align="center" id="add-comment-login-first">
+        To post a comment you must <a href="+login">log in</a>.
+      </div>
+    </tal:not-logged-in>
+
     <div tal:define="link menu/add_comment"
          tal:condition="link/enabled"
          tal:content="structure link/render">
@@ -101,20 +125,27 @@
     <div id="conversation"
          tal:content="structure view/conversation/@@+render"/>
   </div>
-  <!-- Hide inline commenting if YUI isn't used. -->
-  <div id="inline-add-comment" style="display: none">
-  <tal:comment replace="structure context/@@+comment/++form++" />
-      <div class="actions" id="launchpad-form-actions">
-        <input type="submit" id="field.actions.add" name="field.actions.add" value="Save Comment" class="button" />
-      </div>
-  </div>
-
-  <script type="text/javascript">
-    LPS.use('lp.comment', function(Y) {
-    var comment = new Y.lp.CodeReviewComment();
-    comment.render();
-    })
-  </script>
+
+  <tal:logged-in condition="view/user">
+    <div tal:define="comment_form nocall:context/@@+comment;
+                     dummy comment_form/initialize">
+      <h2 id="add-comment">Add comment</h2>
+      <form action="+comment"
+            method="post"
+            enctype="multipart/form-data"
+            accept-charset="UTF-8"
+            id="add-comment-form">
+        <tal:comment-input replace="structure comment_form/widgets/comment"/>
+        <div id="add-comment-review-fields">
+            Review: <tal:review replace="structure comment_form/widgets/vote"/>
+            Review type: <tal:review replace="structure comment_form/widgets/review_type"/>
+            <div class="actions"
+                 tal:content="structure comment_form/actions/field.actions.add/render" />
+        </div>
+      </form>
+    </div>
+  </tal:logged-in>
+
   <div class="yui-g">
     <div class="yui-u first">
       <div id="source-revisions"
@@ -159,21 +190,25 @@
   string:&lt;script id='codereview-script' type='text/javascript'&gt;" />
   conf = <tal:status-config replace="view/status_config" />
   <!--
-  LPS.use('io-base', 'code.codereview', 'code.branchmergeproposal',
+  LPS.use('io-base', 'code.codereview', 'code.branchmergeproposal', 'lp.comment',
             function(Y) {
 
-
-  if(Y.UA.ie) {
-      return;
-  }
-
-  Y.on('domready', function() {
-      Y.code.codereview.connect_links();
-      Y.code.branchmergeproposal.connect_status(conf);
-  });
-
-  (new Y.codereview.NumberToggle()).render();
-
+    Y.on('load', function() {
+        var logged_in = LP.client.links['me'] !== undefined;
+
+        if (logged_in) {
+            var comment = new Y.lp.CodeReviewComment();
+            comment.render();
+
+            if(Y.UA.ie) {
+                return;
+            }
+
+            Y.code.codereview.connect_links();
+            Y.code.branchmergeproposal.connect_status(conf);
+        }
+        (new Y.codereview.NumberToggle()).render();
+      }, window);
   });
   -->
 <tal:script replace="structure string:&lt;/script&gt;" />
 
=== modified file 'lib/lp/registry/browser/team.py'
--- lib/lp/registry/browser/team.py	2009-12-01 22:09:05 +0000
+++ lib/lp/registry/browser/team.py	2009-12-11 13:41:18 +0000
@@ -901,7 +901,6 @@
         return None
 
 
-
 class ProposedTeamMembersEditView(LaunchpadFormView):
     schema = Interface
     label = 'Proposed team members'
@@ -915,7 +914,10 @@
                 status = TeamMembershipStatus.APPROVED
             elif action == "decline":
                 status = TeamMembershipStatus.DECLINED
-            elif action == "hold":
+            else:
+                # The action is "hold" or no action was specified for this
+                # person, which could happen if the set of proposed members
+                # changed while the form was being processed.
                 continue
 
             self.context.setMembershipData(
 
=== modified file 'lib/lp/translations/tests/test_translationimportqueue.py'
--- lib/lp/translations/tests/test_translationimportqueue.py	2009-11-19 11:24:43 +0000
+++ lib/lp/translations/tests/test_translationimportqueue.py	2009-12-11 13:41:18 +0000
@@ -5,6 +5,7 @@
 
 __metaclass__ = type
 
+import transaction
 import unittest
 
 from zope.component import getUtility
@@ -17,25 +18,27 @@
 from canonical.testing import LaunchpadZopelessLayer
 
 
-class TestTranslationImportQueueEntryStatus(TestCaseWithFactory):
-    """Test handling of the status of a queue entry."""
+class TestCanSetStatusBase(TestCaseWithFactory):
+    """Base for tests that check that canSetStatus works ."""
 
     layer = LaunchpadZopelessLayer
+    dbuser = None
+    entry = None
 
     def setUp(self):
         """Set up context to test in."""
-        super(TestTranslationImportQueueEntryStatus, self).setUp()
+        super(TestCanSetStatusBase, self).setUp()
 
         self.queue = getUtility(ITranslationImportQueue)
         self.rosetta_experts = (
             getUtility(ILaunchpadCelebrities).rosetta_experts)
         self.productseries = self.factory.makeProductSeries()
         self.uploaderperson = self.factory.makePerson()
-        self.potemplate = self.factory.makePOTemplate(
-            productseries=self.productseries)
-        self.entry = self.queue.addOrUpdateEntry(
-            'demo.pot', '#demo', False, self.uploaderperson,
-            productseries=self.productseries, potemplate=self.potemplate)
+
+    def _switch_dbuser(self):
+        if self.dbuser != None:
+            transaction.commit()
+            self.layer.switchDbUser(self.dbuser)
 
     def _assertCanSetStatus(self, user, entry, expected_list):
         # Helper to check for all statuses.
@@ -49,6 +52,7 @@
             RosettaImportStatus.IMPORTED,
             RosettaImportStatus.NEEDS_REVIEW,
         ]
+        self._switch_dbuser()
         # Do *not* use assertContentEqual here, as the order matters.
         self.assertEqual(expected_list,
             [entry.canSetStatus(status, user)
@@ -71,6 +75,7 @@
         # If the entry has no import target set, even Rosetta experts
         # cannot set it to approved.
         self.entry.potemplate = None
+        self.entry.pofile = None
         self._assertCanSetStatus(self.rosetta_experts, self.entry,
             #  A      B     D     F     I    NR
             [False, True, True, True, True, True])
@@ -115,5 +120,66 @@
             [False, False, False, False, False, False])
 
 
+class TestCanSetStatusPOTemplate(TestCanSetStatusBase):
+    """Test canStatus applied to an entry with a POTemplate."""
+
+    def setUp(self):
+        """Create the entry to test on."""
+        super(TestCanSetStatusPOTemplate, self).setUp()
+
+        self.potemplate = self.factory.makePOTemplate(
+            productseries=self.productseries)
+        self.entry = self.queue.addOrUpdateEntry(
+            'demo.pot', '#demo', False, self.uploaderperson,
+            productseries=self.productseries, potemplate=self.potemplate)
+
+
+class TestCanSetStatusPOFile(TestCanSetStatusBase):
+    """Test canStatus applied to an entry with a POFile."""
+
+    def setUp(self):
+        """Create the entry to test on."""
+        super(TestCanSetStatusPOFile, self).setUp()
+
+        self.potemplate = self.factory.makePOTemplate(
+            productseries=self.productseries)
+        self.pofile = self.factory.makePOFile('eo', potemplate=self.potemplate)
+        self.entry = self.queue.addOrUpdateEntry(
+            'demo.po', '#demo', False, self.uploaderperson,
+            productseries=self.productseries, pofile=self.pofile)
+
+
+class TestCanSetStatusPOTemplateWithQueuedUser(TestCanSetStatusPOTemplate):
+    """Test handling of the status of a queue entry with 'queued' db user.
+
+    The archive uploader needs to set (and therefore check) the status of a
+    queue entry. It connects as a different database user and therefore we
+    need to make sure that setStatus stays within this user's permissions.
+    This is the version for POTemplate entries.
+    """
+
+    dbuser = 'queued'
+
+
+class TestCanSetStatusPOFileWithQueuedUser(TestCanSetStatusPOFile):
+    """Test handling of the status of a queue entry with 'queued' db user.
+
+    The archive uploader needs to set (and therefore check) the status of a
+    queue entry. It connects as a different database user and therefore we
+    need to make sure that setStatus stays within this user's permissions.
+    This is the version for POFile entries.
+    """
+
+    dbuser = 'queued'
+
+
 def test_suite():
-    return unittest.TestLoader().loadTestsFromName(__name__)
+    """Add only specific test cases and leave out the base case."""
+    suite = unittest.TestSuite()
+    suite.addTest(unittest.makeSuite(TestCanSetStatusPOTemplate))
+    suite.addTest(unittest.makeSuite(TestCanSetStatusPOFile))
+    suite.addTest(
+        unittest.makeSuite(TestCanSetStatusPOTemplateWithQueuedUser))
+    suite.addTest(unittest.makeSuite(TestCanSetStatusPOFileWithQueuedUser))
+    return suite
+