Merge lp:~bac/lp2kanban/pick-newest-mp into lp:~launchpad/lp2kanban/trunk

[Brad Crittenden]

If there are multiple active merge proposals, choose the most recent to link.

In testing I found a bug where a single mp_info was being reused, so the latest added to the list was always the winner.

[Benji York (benji)]

The branch looks great.

I suggest ignoring the two-lines-beween-module-globals rule for
"constants" -- just as we do for imports.

review: Approve (code)

[lp:~bac/lp2kanban/pick-newest-mp]

65. By Brad Crittenden on 2013-01-18

lint

=== modified file 'setup.py'
--- setup.py	2011-07-18 14:25:26 +0000
+++ setup.py	2013-01-18 19:14:20 +0000
@@ -15,6 +15,7 @@
         'argparse',
         'launchpadlib',
         'lazr.uri',
+        'testtools',
         'setuptools',
         ],
     )

=== modified file 'src/lp2kanban/bugs2cards.py'
--- src/lp2kanban/bugs2cards.py	2013-01-16 23:10:55 +0000
+++ src/lp2kanban/bugs2cards.py	2013-01-18 19:14:20 +0000
@@ -93,6 +93,22 @@
             self.kanban_to_lp[board.users[user]] = lp_user
 
 
+ORDERED_STATUSES = {'Rejected': 0,
+                    'Work in progress': 0,
+                    'Needs review': 1,
+                    'Approved': 2,
+                    'Merged': 2,
+                    }
+REVIEW_STATUS = 'In Review'
+TRANSFORM_STATUS = {'Rejected': 'In Progress',
+                    'Work in progress': 'In Progress',
+                    'Needs review': REVIEW_STATUS,
+                    'Approved': 'Approved',
+                    'Merged': 'Merged',
+                    }
+FINAL_STATUSES = ('Approved', 'Merged')
+
+
 def get_branch_info(branches):
     """Returns a bug's status based on the given branches.
 
@@ -107,26 +123,11 @@
         # refuse to guess.
         return info
 
-    ORDERED_STATUSES = {'Rejected': 0,
-                        'Work in progress': 0,
-                        'Needs review': 1,
-                        'Approved': 2,
-                        'Merged': 2,
-                        }
-    REVIEW_STATUS = 'In Review'
-    TRANSFORM_STATUS = {'Rejected': 'In Progress',
-                        'Work in progress': 'In Progress',
-                        'Needs review': REVIEW_STATUS,
-                        'Approved': 'Approved',
-                        'Merged': 'Merged',
-                        }
-    FINAL_STATUSES = ('Approved', 'Merged')
-
     def _get_mp_info():
         mps = []
         for bug_branch in branches:
-            mp_info = Record(rank=None, status=None, mp=None)
             for mp in bug_branch.branch.landing_targets:
+                mp_info = Record(rank=None, status=None, mp=None)
                 status = mp.queue_status
                 mp_info.rank = ORDERED_STATUSES.get(status, 1)
                 mp_info.status = TRANSFORM_STATUS.get(status, REVIEW_STATUS)
@@ -138,7 +139,7 @@
                 mps.append(mp_info)
         if len(mps) == 0:
             return None
-        mps.sort(key=lambda x:x.rank, reverse=True)
+        mps.sort(key=lambda x:(x.rank, x.mp.date_created), reverse=True)
         return mps[0]
 
     def _get_mp_link(mp):

=== modified file 'src/lp2kanban/tests/test_bugs2cards.py'
--- src/lp2kanban/tests/test_bugs2cards.py	2013-01-17 14:29:57 +0000
+++ src/lp2kanban/tests/test_bugs2cards.py	2013-01-18 19:14:20 +0000
@@ -6,6 +6,7 @@
     )
 from StringIO import StringIO
 from textwrap import dedent
+import datetime
 import unittest
 
 from lp2kanban.bugs2cards import (
@@ -24,6 +25,7 @@
     FauxCard,
     )
 
+
 class ConfigReadingTest(unittest.TestCase):
 
     def test_parse_config_no_globals(self):
@@ -101,11 +103,12 @@
 
 
 class FauxMP:
-    def __init__(self, queue_status, target_name, link=None, description=None):
+    def __init__(self, queue_status, target_name, link=None, description=None, date_created=None):
         self.queue_status = queue_status
         self.target_branch = Record(name=target_name)
         self.web_link = link
         self.description = description or ''
+        self.date_created = date_created
 
 
 class BranchInfoTests(unittest.TestCase):
@@ -185,6 +188,36 @@
                                    ])]
         result = get_branch_info(branches)
         self.assertEqual(result['status'], 'Approved')
+        self.assertEqual(result['target'], 'approved-branch')
+
+    def test_first_highest_ranked_returned(self):
+        # If there are multiple merge proposals of the same highest rank, the
+        # newest one wins.
+        branches = [FauxBugBranch(
+            [FauxMP('In Review', 'devel'),
+             FauxMP('Approved', 'first'),
+             FauxMP('Approved', 'second'),
+             FauxMP('Rejected', 'bad-branch'),
+             ])]
+        result = get_branch_info(branches)
+        self.assertEqual(result['status'], 'Approved')
+        self.assertEqual(result['target'], 'first')
+
+    def test_newest_highest_ranked_returned(self):
+        # If there are multiple merge proposals of the same highest rank, but
+        # not in final states, the newest one wins.
+        now = datetime.datetime.now()
+        yesterday = now - datetime.timedelta(days=1)
+        lastweek = now - datetime.timedelta(days=7)
+        branches = [FauxBugBranch(
+            [FauxMP('Needs review', 'lastweek', date_created=lastweek),
+             FauxMP('Needs review', 'now', date_created=now),
+             FauxMP('Needs review', 'yesterday', date_created=yesterday),
+             FauxMP('Rejected', 'bad-branch'),
+             ])]
+        result = get_branch_info(branches)
+        self.assertEqual(result['status'], 'In Review')
+        self.assertEqual(result['target'], 'now')
 
     def test_first_seen_final_state_returned(self):
         # If there are multiple MPs in a final state, the first one seen wins.