Merge lp:~chad.smith/lp2kanban/handle-multitask-with-no-linked-branches into lp:lp2kanban

[Chad Smith]

Fix a corner case in lp2kanban where bug cards with multiple tasks were being left in CODING status, because get_bug_status didn't account for bugs fixed as trivials (without linked branches).

It also fixes our ignore series tasks logic to avoid tracking series task target status in favor of tracking only the trunk target status.

This branch handles cases specifically like this one:
Specific to bugs like: https://bugs.launchpad.net/landscape/+bug/1565839

In the above bug we now ignore task status of Fix Released and track only the trunk status of Fix Committed. This now pushes the card to the deploy lanes (QA::TODO), properly handling the fact that the bug has no branches linked because it was "trivialed".

To test:

# Either make this project
make configs
make credentials
make
make check
./bin/py ./src/lp2kanban/bugs2cards.py -c configs/sync.ini -b "Landscape 2016"

# Or Hijack jenkins kanban sync job and point the Repository URL to lp:~chad.smith/lp2kanban/handle-multitask-with-no-linked-branches instead of lp:lp2kanban

https://ci.lscape.net/job/kanban-sync/configure

# Or stop the jenkins kanban-sync job and patch the existing checkout (which is refreshed each run anyway)
ssh <email address hidden>; sudo -u jenkins bash; cd /var/lib/jenkins/workspaces/kanban-sync; bzr merge lp:~chad.smith/lp2kanban/handle-multitask-with-no-linked-branches; ./bin/py -c configs/sync.ini -b "Landscape 2016"

[Free Ekanayaka (free.ekanayaka)]

LGTM, +1

review: Approve

[Simon Poirier (simpoir)]

+1 looks good. I was about to complain about PEP8 but I noticed the whole repo is already a mess in that regard.

review: Approve

=== modified file 'src/lp2kanban/bugs2cards.py'
--- src/lp2kanban/bugs2cards.py	2016-04-13 16:06:23 +0000
+++ src/lp2kanban/bugs2cards.py	2016-04-15 17:27:22 +0000
@@ -10,6 +10,7 @@
     )
 from lp2kanban.kanban import (
     BRANCH_MP_REGEX,
+    RESOURCE_TYPE_LINK_SERIES,
     LeankitKanban,
     LeankitTaskBoard,
     Record,
@@ -451,7 +452,7 @@
     # status of None means card needs not to be moved.
     status = None
     branch_card_status = None
-    if branch_info:
+    if branch_info.status:
         if branch_info.status == 'In Progress':
             branch_card_status = CardStatus.CODING
         elif branch_info.status == 'In Review':
@@ -466,7 +467,7 @@
         status = CardStatus.NEW
     elif bug_status in IN_PROGRESS_BUG_STATUSES:
         status = CardStatus.CODING
-        if branch_info.status == 'Merged':
+        if branch_card_status in (None, CardStatus.LANDING):
             if bug_status == 'Fix Committed':
                 if 'qa-needstesting' in bug_tags:
                     status = CardStatus.QA
@@ -475,10 +476,11 @@
                         status = CardStatus.DOWNTIME_DEPLOY
                     else:
                         status = CardStatus.DEPLOY
-            else:
+            elif branch_card_status is not None:
                 status = CardStatus.LANDING
         elif branch_card_status:
             return branch_card_status
+            
     elif bug_status in DONE_FIX_BUG_STATUSES:
         status = CardStatus.DONE_FIX
     elif bug_status in DONE_NOFIX_BUG_STATUSES:
@@ -492,9 +494,15 @@
     assignees = []
     tasks = lp_bug.bug_tasks_collection
     for task in tasks:
+        # Ignore bug task status for series targets
+        if (task.target.resource_type_link == RESOURCE_TYPE_LINK_SERIES and
+            task.target.project in all_projects):
+            continue  # Skip series task status for our projects
+        if (task.assignee is not None and
+            task.assignee.name not in all_users):
+            continue  # Skip non-users tasks
         if (task.target in all_projects or
-            (task.assignee is not None and
-             task.assignee.name in all_users)):
+            task.assignee is not None):
             matches = True
             if (lowest_status is None or
                 (ORDERED_BUG_STATUSES.index(task.status) <

=== modified file 'src/lp2kanban/kanban.py'
--- src/lp2kanban/kanban.py	2016-03-15 11:37:52 +0000
+++ src/lp2kanban/kanban.py	2016-04-15 17:27:22 +0000
@@ -16,6 +16,8 @@
     r'(?P<branch>~(?P<owner>[-.\w]*)(/[-.\w]+){2,4})'
     '(/\+merge/\d+)?/?$')
 
+RESOURCE_TYPE_LINK_SERIES = "https://api.launchpad.net/devel/#project_series"
+
 
 class Record(dict):
     """A little dict subclass that adds attribute access to values."""

=== modified file 'src/lp2kanban/tests/test_bugs2cards.py'
--- src/lp2kanban/tests/test_bugs2cards.py	2016-04-13 16:06:23 +0000
+++ src/lp2kanban/tests/test_bugs2cards.py	2016-04-15 17:27:22 +0000
@@ -33,6 +33,7 @@
     sync_board
     )
 from lp2kanban.kanban import (
+    RESOURCE_TYPE_LINK_SERIES,
     LeankitCard,
     Record,
     )
@@ -290,6 +291,13 @@
         self.saved = True
 
 
+class FauxTarget:
+
+    def __init__(self, resource_type_link="http://api/project", project=None):
+        self.resource_type_link = resource_type_link
+        self.project = project
+   
+
 class FauxBugTask:
 
     def __init__(self, bug_id, target='faux-project', tags=None,
@@ -307,7 +315,8 @@
         # For a bug on an unrelated project and not assigned to any
         # of the relevant users, it is not considered to match at all.
         bug = FauxBug()
-        bug.addTask('project')
+        project = FauxTarget(resource_type_link="project", project="project1")
+        bug.addTask(project)
         matches, status, assignees, mp_url, mp_type = (
             get_bug_status(bug, ['launchpad'], []))
         self.assertFalse(matches)
@@ -319,9 +328,12 @@
         # we care about, it is returned as matching with proper status.
         bug = FauxBug()
         user = Record(name='user')
-        bug.addTask('project', status='In Progress', assignee=user)
+        project = FauxTarget(resource_type_link="project", project="project1")
+        other_project = FauxTarget(
+            resource_type_link="project", project="launchpad")
+        bug.addTask(project, status='In Progress', assignee=user)
         matches, status, assignees, mp_url, mp_type = (
-            get_bug_status(bug, ['launchpad'], ['user']))
+            get_bug_status(bug, [other_project], ['user']))
         self.assertTrue(matches)
         self.assertEqual(CardStatus.CODING, status)
         self.assertEqual([user], assignees)
@@ -331,14 +343,17 @@
         # as matching with a status that reflects the project task ignoring
         # non-project tasks.
         bug = FauxBug()
-        nonuser = Record(name='nonuser')
-        bug.addTask('launchpad', status='In Progress', assignee=nonuser)
-        bug.addTask('launchpad/16.03', status='Fix Released', assignee=nonuser)
+        user = Record(name='user')
+        project = FauxTarget(resource_type_link="project", project="project")
+        series = FauxTarget(
+            resource_type_link=RESOURCE_TYPE_LINK_SERIES, project=project)
+        bug.addTask(project, status='In Progress', assignee=user)
+        bug.addTask(series, status='Fix Released', assignee=user)
         matches, status, assignees, mp_url, mp_type = (
-            get_bug_status(bug, ['launchpad'], []))
+            get_bug_status(bug, [project], ['user']))
         self.assertTrue(matches)
         self.assertEqual(CardStatus.CODING, status)
-        self.assertEqual([nonuser], assignees)
+        self.assertEqual([user], assignees)
 
     def test_get_bug_status_multi_targets(self):
         # For a bug with multiple tasks, only some of which are on
@@ -347,10 +362,13 @@
         bug = FauxBug()
         nonuser = Record(name='nonuser')
         user = Record(name='user')
-        bug.addTask('project', status='In Progress', assignee=nonuser)
-        bug.addTask('launchpad', status='Fix Released', assignee=user)
+        project = FauxTarget(resource_type_link="project", project="project")
+        other_project = FauxTarget(
+            resource_type_link="project", project="launchpad")
+        bug.addTask(project, status='In Progress', assignee=nonuser)
+        bug.addTask(other_project, status='Fix Released', assignee=user)
         matches, status, assignees, mp_url, mp_type = (
-            get_bug_status(bug, ['launchpad'], []))
+            get_bug_status(bug, [project], ["user"]))
         self.assertTrue(matches)
         self.assertEqual(CardStatus.DONE_FIX, status)
         self.assertEqual([user], assignees)
@@ -361,10 +379,12 @@
         # "lowest" of all bug task statuses (i.e. 'In Progress' is lower
         # than 'Fix Committed').
         bug = FauxBug()
-        bug.addTask('lazr', status='Fix Released')
-        bug.addTask('launchpad', status='In Progress')
+        project1 = FauxTarget(resource_type_link="project", project="lazr")
+        project2 = FauxTarget(resource_type_link="project", project="launchpad")
+        bug.addTask(project1, status='Fix Released')
+        bug.addTask(project2, status='In Progress')
         matches, status, assignees, mp_url, mp_type = get_bug_status(
-            bug, ['launchpad', 'lazr'], [])
+            bug, [project1, project2], [])
         self.assertTrue(matches)
         # 'Fix Released' on 'lazr' would put the bug status in DONE state,
         # but since there is an 'In Progress' task as well, it is instead
@@ -611,18 +631,19 @@
     def test_get_card_status_new_no_branch(self):
         # For a bug in 'New', 'Triaged' or 'Confirmed' or 'Incomplete' statuses,
         # the branch status is new state.
-        self.assertEqual(
-            CardStatus.NEW,
-            get_card_status('New', [], None))
-        self.assertEqual(
-            CardStatus.NEW,
-            get_card_status('Confirmed', [], None))
-        self.assertEqual(
-            CardStatus.NEW,
-            get_card_status('Triaged', [], None))
-        self.assertEqual(
-            CardStatus.NEW,
-            get_card_status('Incomplete', [], None))
+        no_branch_info = Record(status=None)
+        self.assertEqual(
+            CardStatus.NEW,
+            get_card_status('New', [], no_branch_info))
+        self.assertEqual(
+            CardStatus.NEW,
+            get_card_status('Confirmed', [], no_branch_info))
+        self.assertEqual(
+            CardStatus.NEW,
+            get_card_status('Triaged', [], no_branch_info))
+        self.assertEqual(
+            CardStatus.NEW,
+            get_card_status('Incomplete', [], no_branch_info))
 
     def test_get_card_status_coding_no_bug(self):
         # For a card with no associated bug, an 'In Progress' branch status
@@ -651,15 +672,21 @@
             CardStatus.LANDING, 
             get_card_status(None, [], branch_info))
 
-    def test_get_card_status_coding_no_branch(self):
-        # For a bug in 'In Progress' or 'Fix Committed' status, it is
-        # considered to be in the coding state if there is no branch.
+    def test_get_card_status_coding_in_progress_no_branch(self):
+        # For a bug in 'In Progress' status, it is considered to be in the
+        # coding state if there is no branch.
         branch_info = Record(status=None, target=None)
         self.assertEqual(
             CardStatus.CODING,
             get_card_status('In Progress', [], branch_info))
+
+    def test_get_card_status_deploy_fix_committed_no_branch(self):
+        # For a bug in 'Fix Committed' status, it is considered to be in the
+        # deploy phase when there is no branch linked as the bug is considered
+        # fixed with a trivial.
+        branch_info = Record(status=None, target=None)
         self.assertEqual(
-            CardStatus.CODING,
+            CardStatus.DEPLOY,
             get_card_status('Fix Committed', [], branch_info))
 
     def test_get_card_status_coding_branch(self):
@@ -741,19 +768,25 @@
     def test_get_card_status_done_with_fix(self):
         # For a bug in 'Fix Released' status, it is considered completely done.
         self.assertEqual(
-            CardStatus.DONE_FIX, get_card_status('Fix Released', [], None))
+            CardStatus.DONE_FIX,
+            get_card_status('Fix Released', [], Record(status=None)))
 
     def test_get_card_status_done_with_no_fix(self):
         # For a bug in 'Opinion', 'Won't fix', 'Invalid' or'Expired' statuses,
         # it is considered completely done without a fix
-        self.assertEqual(
-            CardStatus.DONE_NOFIX, get_card_status('Opinion', [], None))
-        self.assertEqual(
-            CardStatus.DONE_NOFIX, get_card_status("Won't Fix", [], None))
-        self.assertEqual(
-            CardStatus.DONE_NOFIX, get_card_status('Invalid', [], None))
-        self.assertEqual(
-            CardStatus.DONE_NOFIX, get_card_status('Expired', [], None))
+        no_branch_info = Record(status=None)
+        self.assertEqual(
+            CardStatus.DONE_NOFIX,
+            get_card_status('Opinion', [], no_branch_info))
+        self.assertEqual(
+            CardStatus.DONE_NOFIX,
+            get_card_status("Won't Fix", [], no_branch_info))
+        self.assertEqual(
+            CardStatus.DONE_NOFIX,
+            get_card_status('Invalid', [], no_branch_info))
+        self.assertEqual(
+            CardStatus.DONE_NOFIX,
+            get_card_status('Expired', [], no_branch_info))
 
 
 class GetCardsForFeatureTest(unittest.TestCase):