Merge lp:~abentley/bzr-pqm/fix-lp-land into lp:bzr-pqm

[Aaron Bentley]

Fix lp-land.

Recent changes to config handling broke lp-land. This breakage was not detected because lp-land was not sufficiently tested.

This branch fixes the breakage by using an implementation of config.Stack as the input to pqm_email.

It increases the test coverage of lp-land to reduce the chance of similar problems in the future.

It changes the behaviour of ConfigStack.get so that it supports environment variables such as BZR_EMAIL and EMAIL.

It extracts get_stacked_config and get_mail_from to increase code re-use.

It moves most functionality out of Submitter.__init__ into the new from_cmdline factory method, to make testing easier.

It extracts Submitter.make_submission_email to simplify testing.

[lp:~abentley/bzr-pqm/fix-lp-land]

89. By Aaron Bentley on 2012-02-02

Cleanup.

[Jelmer Vernooij (jelmer)]

Not much to add, nice cleanup!

review: Approve

=== modified file '__init__.py'
--- __init__.py	2011-12-19 11:04:03 +0000
+++ __init__.py	2012-02-02 17:34:20 +0000
@@ -150,11 +150,9 @@
     def run(self, location=None, dry_run=False, testfix=False,
             no_qa=False, incremental=False, rollback=None):
         from bzrlib.plugins.pqm.lpland import Submitter
-        from bzrlib import branch as _mod_branch
         from bzrlib.plugins.pqm.lpland import (
             MissingReviewError, MissingBugsError, MissingBugsIncrementalError)
 
-        branch = _mod_branch.Branch.open_containing('.')[0]
         if dry_run:
             outf = self.outf
         else:
@@ -162,8 +160,9 @@
         if rollback and (no_qa or incremental):
             print "--rollback option used. Ignoring --no-qa and --incremental."
         try:
-            submitter = Submitter(branch, location, testfix, no_qa,
-                incremental, rollback=rollback).run(outf)
+            submitter = Submitter.from_cmdline(
+                location, testfix, no_qa, incremental,
+                rollback=rollback).run(outf)
         except MissingReviewError:
             raise BzrCommandError(
                 "Cannot land branches that haven't got approved code "

=== modified file 'lpland.py'
--- lpland.py	2011-12-16 20:21:06 +0000
+++ lpland.py	2012-02-02 17:34:20 +0000
@@ -198,39 +198,57 @@
 
 
 class Submitter(object):
+    """Class that submits a to PQM from a merge proposal."""
 
-    def __init__(self, branch, location, testfix=False, no_qa=False,
+    def __init__(self, branch, mp, testfix=False, no_qa=False,
                  incremental=False, rollback=None):
         self.branch = branch
+        self.mp = mp
         self.testfix = testfix
         self.no_qa = no_qa
         self.incremental = incremental
         self.rollback = rollback
-        self.config = self.branch.get_config()
-        self.mail_from = self.config.get_user_option('pqm_user_email')
-        if not self.mail_from:
-            self.mail_from = self.config.username()
-        self.lander = LaunchpadBranchLander.load()
-        self.location = location
-
-    def submission(self, mp):
+        self.config = pqm_submit.get_stacked_config(self.branch, None)
+        self.mail_from = pqm_submit.get_mail_from(self.config)
+
+    @classmethod
+    def from_cmdline(cls, location, testfix, no_qa, incremental, rollback):
+        """Factory that returns a Submitter from commandline arguments."""
+        from bzrlib import branch as _mod_branch
+        branch = _mod_branch.Branch.open_containing('.')[0]
+        lander = LaunchpadBranchLander.load()
+        if location is None:
+            mp = lander.get_merge_proposal_from_branch(branch)
+        else:
+            mp = lander.load_merge_proposal(location)
+        return cls(branch, mp, testfix, no_qa, incremental, rollback)
+
+    def submission(self):
+        """Create a PQMSubmission for this Submitter."""
         submission = pqm_submit.PQMSubmission(self.branch,
-            mp.source_branch, mp.target_branch, '')
+            self.mp.source_branch, self.mp.target_branch, '')
         return submission
 
     @staticmethod
     def check_submission(submission):
+        """Ensure the Submission is reasonable."""
         submission.check_tree()
         submission.check_public_branch()
 
-    def set_message(self, submission, mp):
-        pqm_command = ''.join(submission.to_lines())
-        commit_message = mp.commit_message or ''
-        start_message = mp.get_commit_message(commit_message, self.testfix,
-            self.no_qa, self.incremental, rollback=self.rollback)
+    def set_message(self, submission):
+        """Set the message of the Submission.
+
+        This starts with automatic generation from merge proposal and branch
+        properties, then allows the user to edit the values.
+        """
+        commit_message = self.mp.commit_message or ''
+        start_message = self.mp.get_commit_message(commit_message,
+            self.testfix, self.no_qa, self.incremental,
+            rollback=self.rollback)
         prompt = ('Proposed commit message:\n%s\nEdit before sending'
                   % start_message)
         if commit_message == '' or ui.ui_factory.get_boolean(prompt):
+            pqm_command = ''.join(submission.to_lines())
             unicode_message = msgeditor.edit_commit_message(
                 'pqm command:\n%s' % pqm_command,
                 start_message=start_message).rstrip('\n')
@@ -239,18 +257,17 @@
             message = start_message
         submission.message = message
 
-    def run(self, outf):
-        if self.location is None:
-            mp = self.lander.get_merge_proposal_from_branch(self.branch)
-        else:
-            mp = self.lander.load_merge_proposal(self.location)
-        submission = self.submission(mp)
-        self.check_submission(submission)
-        self.set_message(submission, mp)
-        mail_to = pqm_submit.pqm_email(self.config, mp.target_branch)
+    def make_submission_email(self, submission, sign=True):
+        mail_to = pqm_submit.pqm_email(self.config, self.mp.target_branch)
         if not mail_to:
             raise pqm_submit.NoPQMSubmissionAddress(self.branch)
-        email = submission.to_email(self.mail_from, mail_to)
+        return submission.to_email(self.mail_from, mail_to, sign=sign)
+
+    def run(self, outf, sign=True):
+        submission = self.submission()
+        self.check_submission(submission)
+        self.set_message(submission)
+        email = self.make_submission_email(submission, sign=sign)
         if outf is not None:
             outf.write(email.as_string())
         else:

=== modified file 'pqm_submit.py'
--- pqm_submit.py	2012-01-20 15:32:43 +0000
+++ pqm_submit.py	2012-02-02 17:34:20 +0000
@@ -219,7 +219,16 @@
                 return value
         return None
 
-    get = _get_user_option
+    def get(self, option_name):
+        """Return an option exactly as bzrlib.config.Stack would.
+
+        Since Stack allows the environment to override 'email', this uses the
+        same logic.
+        """
+        if option_name == 'email':
+            return self.username()
+        else:
+            return self._get_user_option(option_name)
 
     def _get_user_id(self):
         for source in self._sources:
@@ -245,9 +254,20 @@
     return mail_to.encode('utf8') # same here
 
 
-def submit(branch, message, dry_run=False, public_location=None,
-           submit_location=None, tree=None, ignore_local=False):
-    """Submit the given branch to the pqm."""
+def get_stacked_config(branch=None, public_location=None):
+    """Return the relevant stacked config.
+
+    If the branch is supplied, a branch stacked config is returned.
+    Otherwise, if the public location is supplied, a stacked location config
+    is returned.
+    Otherwise, a global config is returned.
+
+    For bzr versions earlier than 2.5, pqm_submit.StackedConfig is used.  For
+    later versions, the standard stacked config is used.
+
+    :param branch: The branch to retrieve the config for.
+    :param public_location: The public location to retrieve the config for.
+    """
     if bzrlib_version < (2, 5):
         config = StackedConfig()
         if branch:
@@ -263,16 +283,31 @@
             config = _mod_config.LocationStack(public_location)
         else:
             config = _mod_config.GlobalStack()
-
+    return config
+
+
+def get_mail_from(config):
+    """Return the email id that the email is from.
+
+    :param config: The config to use for determining the from address.
+    """
+    mail_from = config.get('pqm_user_email')
+    if not mail_from:
+        mail_from = config.get('email')
+    mail_from = mail_from.encode('utf8') # Make sure this isn't unicode
+    return mail_from
+
+
+def submit(branch, message, dry_run=False, public_location=None,
+           submit_location=None, tree=None, ignore_local=False):
+    """Submit the given branch to the pqm."""
+    config = get_stacked_config(branch, public_location)
     submission = PQMSubmission(
         source_branch=branch, public_location=public_location, message=message,
         submit_location=submit_location,
         tree=tree)
 
-    mail_from = config.get('pqm_user_email')
-    if not mail_from:
-        mail_from = config.get('email')
-    mail_from = mail_from.encode('utf8') # Make sure this isn't unicode
+    mail_from = get_mail_from(config)
     mail_to = pqm_email(config, submit_location)
     if not mail_to:
         raise NoPQMSubmissionAddress(branch)

=== modified file 'tests/test_lpland.py'
--- tests/test_lpland.py	2011-02-16 00:01:50 +0000
+++ tests/test_lpland.py	2012-02-02 17:34:20 +0000
@@ -5,17 +5,27 @@
 
 __metaclass__ = type
 
+from cStringIO import StringIO
+from textwrap import dedent
 import unittest
 
-from launchpadlib.uris import (
-    DEV_SERVICE_ROOT, EDGE_SERVICE_ROOT, LPNET_SERVICE_ROOT,
-    STAGING_SERVICE_ROOT)
-
+from bzrlib import (
+    errors,
+    ui,
+)
 from bzrlib.plugins.pqm.lpland import (
-    get_bugs_clause, get_reviewer_clause,
-    get_reviewer_handle, get_testfix_clause, get_qa_clause,
-    MissingReviewError, MissingBugsError, MissingBugsIncrementalError,
-    MergeProposal)
+    get_bugs_clause,
+    get_reviewer_clause,
+    get_reviewer_handle,
+    get_testfix_clause,
+    get_qa_clause,
+    MissingReviewError,
+    MissingBugsError,
+    MissingBugsIncrementalError,
+    MergeProposal,
+    Submitter,
+)
+from bzrlib.tests import TestCaseWithTransport
 
 from fakemethod import FakeMethod
 
@@ -36,7 +46,7 @@
     Only used for the purposes of testing.
     """
 
-    def __init__(self, name, irc_handles):
+    def __init__(self, name='jrandom', irc_handles=()):
         self.name = name
         self.irc_nicknames = list(irc_handles)
 
@@ -52,6 +62,30 @@
         self.network = network
 
 
+class FakeBranch:
+
+    def __init__(self, location):
+        self.location = location
+        self.linked_bugs = [FakeBug(5)]
+
+    def composePublicURL(self, scheme):
+        return self.location
+
+
+class FakeComment:
+
+    def __init__(self):
+        self.vote = 'Approve'
+
+
+class FakeVote:
+
+    def __init__(self):
+        self.comment = FakeComment()
+        self.review_type = None
+        self.reviewer = FakePerson()
+
+
 class FakeLPMergeProposal:
     """Fake launchpadlib MergeProposal object.
 
@@ -60,6 +94,10 @@
 
     def __init__(self, root=None):
         self._root = root
+        self.source_branch = FakeBranch('lp_source')
+        self.target_branch = FakeBranch('lp_target')
+        self.commit_message = 'Message1'
+        self.votes = [FakeVote()]
 
 
 class TestBugsClaused(unittest.TestCase):
@@ -361,3 +399,75 @@
         # If the merge proposal hasn't been approved by anyone, we cannot
         # generate a valid clause.
         self.assertRaises(MissingReviewError, self.get_reviewer_clause, {})
+
+
+class TestSubmitter(TestCaseWithTransport):
+
+    def make_submitter(self):
+        """Return a Submitter for testing."""
+        b = self.make_branch('source')
+        b.get_config().set_user_option('pqm_email', 'PQM <[e-mail]>')
+        lp_source = self.make_branch('lp_source')
+        mp = MergeProposal(FakeLPMergeProposal())
+        return Submitter(b, mp)
+
+    def test_submission(self):
+        """Creating a submission produces the expected result."""
+        submitter = self.make_submitter()
+        submission = submitter.submission()
+        self.assertEqual(submitter.branch, submission.source_branch)
+        self.assertEqual('lp_source', submission.public_location)
+        self.assertEqual('lp_target', submission.submit_location)
+        self.assertEqual('', submission.message)
+
+    def test_check_submission(self):
+        """Checking the submission returns in the comment case."""
+        submitter = self.make_submitter()
+        submitter.check_submission(submitter.submission())
+
+    def test_check_submission_public_branch(self):
+        """Checking the submission raises if the public branch is outdated."""
+        submitter = self.make_submitter()
+        tree = submitter.branch.create_checkout('tree', lightweight=True)
+        tree.commit('message')
+        self.assertRaises(errors.PublicBranchOutOfDate,
+                          submitter.check_submission, submitter.submission())
+
+    def test_set_message(self):
+        """Setting the message produces one in the correct format."""
+        submitter = self.make_submitter()
+        submission = submitter.submission()
+        ui.ui_factory = ui.CannedInputUIFactory([False])
+        submitter.set_message(submission)
+        self.assertEqual('[r=jrandom][bug=5] Message1', submission.message)
+
+    def test_make_submission_email(self):
+        """Creating a submission email works."""
+        submitter = self.make_submitter()
+        submission = submitter.submission()
+        submission.message = 'Hello!'
+        target = self.make_branch('lp_target')
+        email = submitter.make_submission_email(submission, sign=False)
+        self.assertEqual('PQM <[e-mail]>', email['To'])
+        self.assertEqual('[e-mail]', email['From'])
+        self.assertEqual('Hello!', email['Subject'])
+        self.assertContainsRe(email['User-Agent'], '^Bazaar')
+        self.assertEqual('star-merge lp_source lp_target\n', email._body)
+
+    def test_run(self):
+        """End-to-end test of Submitter.run."""
+        submitter = self.make_submitter()
+        ui.ui_factory = ui.CannedInputUIFactory([False])
+        outf = StringIO()
+        submitter.run(outf, sign=False)
+        self.assertContainsRe(outf.getvalue(), dedent("""\
+        MIME-Version: 1.0
+        Content-Type: text/plain; charset="us-ascii"
+        Content-Transfer-Encoding: 7bit
+        From: [e-mail]
+        Subject: \[r=jrandom\]\[bug=5\] Message1
+        To: PQM <[e-mail]>
+        User-Agent: Bazaar (.*)
+
+        star-merge lp_source lp_target
+        """))

=== modified file 'tests/test_pqm_submit.py'
--- tests/test_pqm_submit.py	2010-08-06 16:52:30 +0000
+++ tests/test_pqm_submit.py	2012-02-02 17:34:20 +0000
@@ -471,3 +471,11 @@
                          call[1:3])
         self.assertContainsRe(call[3], EMAIL)
 
+
+class TestConfig(TestCaseWithMemoryTransport):
+
+    def test_email_from_environ(self):
+        """The config can get the email from the environ."""
+        branch = self.make_branch('foo')
+        config = pqm_submit.get_stacked_config(branch, None)
+        self.assertEqual('[e-mail]', config.get('email'))