Merge lp:~ursinha/bzr-pqm/auto-approve into lp:bzr-pqm

[Ursula Junque]

This branch changes the behavior of the plugin by making it mark the merge proposal status as Approved (queue_status), if it already has any approved code votes. The added method itself doesn't check the mp votes, but the checking is done by other method before this one is called.

This change has been implemented so people won't need to set their mp's to Approved manually.

[Aaron Bentley (abentley)]

This seems like the wrong time to set the proposal to approved. This should ideally be done by the (last) reviewer as part of their review. By the time you lp-land it, it will be set to "Merged" in a few minutes, so I don't think that there's great value to setting it "approved" in the meantime.

This also assumes that the person landing the code is on the review team of the target branch. This is usually the case, but our design for merge proposals is that non-reviewers can land code if such code has been approved by a reviewer.

Doing lp_save as part of set_status_approved seems like a bad pattern, because it means that each change costs a roundtrip, instead of allowing multiple changes to be made in a single roundtrip.

[Martin Pool (mbp)]

On 14 December 2010 03:31, Aaron Bentley <email address hidden> wrote:
> This seems like the wrong time to set the proposal to approved.  This should ideally be done by the (last) reviewer as part of their review.  By the time you lp-land it, it will be set to "Merged" in a few minutes, so I don't think that there's great value to setting it "approved" in the meantime.

I agree.

This seems like a kludge for the fact that through the web ui or mail
ui, people are voting approve but not marking it approved. I think
there are open bugs saying that common action should be made easier.

--
Martin

[Jelmer Vernooij (jelmer)]

review: Abstain

[Jelmer Vernooij (jelmer)]

review: Abstain

[Aaron Bentley (abentley)]

IIRC, this was to be based on a misunderstanding of how to transition from a pqm-based workflow to a Tarmac-based one. In a Tarmac-based workflow, lp-land should no longer be necessary.

review: Disapprove

Unmerged revisions

77. By Ursula Junque on 2010-12-13

adding set_status_approved method to do what it says.

76. By Ursula Junque on 2010-12-13

adding setStatus to FakeLPMergeProposal; adding test to check set_status_approved.

=== modified file 'lpland.py'
--- lpland.py	2010-11-08 16:49:24 +0000
+++ lpland.py	2010-12-13 14:51:45 +0000
@@ -179,6 +179,16 @@
         # XXX: JonathanLange 2009-09-24: No unit tests.
         return branch.composePublicURL(scheme="bzr+ssh")
 
+    def set_status_approved(self):
+        """Set the overall status of the mp to Approved."""
+        try:
+            self._mp.setStatus(status="Approved")
+            self._mp.lp_save()
+        except Exception, e:
+            raise BzrCommandError(
+                "Unable to set the status in the merge proposal.\n"
+                "Got: %s" % e)
+
     def build_commit_message(self, commit_text, testfix=False, no_qa=False,
                              incremental=False, rollback=None):
         """Get the Launchpad-style commit message for a merge proposal."""
@@ -266,6 +276,7 @@
         self.check_submission(submission)
         self.set_message(submission, mp, self.testfix, self.no_qa,
             self.incremental, rollback=self.rollback)
+        self.set_status_approved()
         email = submission.to_email(self.mail_from, self.mail_to)
         if outf is not None:
             outf.write(email.as_string())

=== modified file 'tests/test_lpland.py'
--- tests/test_lpland.py	2010-11-08 16:49:24 +0000
+++ tests/test_lpland.py	2010-12-13 14:51:45 +0000
@@ -61,6 +61,10 @@
     def __init__(self, root=None):
         self._root = root
         self.commit_message = None
+        self.queue_status = None
+
+    def setStatus(self, status):
+        self.queue_status = status
 
     def lp_save(self):
         pass
@@ -319,6 +323,17 @@
         self.assertEqual(self.mp._mp.commit_message, commit_message)
 
 
+class TestSetStatusApproved(unittest.TestCase):
+
+    def setUp(self):
+        self.mp = MergeProposal(FakeLPMergeProposal())
+        self.mp._mp.setStatus(status="Work in progress")
+
+    def test_set_status_approved(self):
+        self.mp.set_status_approved()
+        self.assertEqual(self.mp._mp.queue_status, "Approved")
+
+
 class TestGetReviewerClause(unittest.TestCase):
     """Tests for `get_reviewer_clause`."""