* Add new choice assigned_at to option merge_order (#268)

Previously the option merge_order had two choices created_at and updated_at.
This is problematic in large projects as both options does not allow
the maintainer to control the order of merge requests.

This commit adds a new choice assigned_at to option merge_order as
maintainers in large project often expect the order of the merge requests
related to order in which the merge requests did got assigned to the
marge-bot user.

Signed-off-by: Konrad Weihmann <konrad.weihmann@mbition.io>

README.md

@@ -97,7 +97,8 @@ optional arguments:
                         Marge-bot pushes effectively don't change approval status.
                            [env var: MARGE_IMPERSONATE_APPROVERS] (default: False)
   --merge-order         The order you want marge to merge its requests.
-                        As of earliest merge request creation time (created_at) or update time (updated_at)
+                        As of earliest merge request creation time (created_at), update time (updated_at)
+                        or assigned to 'marge-bot' user time (assigned_at)
                           [env var: MARGE_MERGE_ORDER] (default: created_at)
   --approval-reset-timeout APPROVAL_RESET_TIMEOUT
                         How long to wait for approvals to reset after pushing.

marge/app.py

@@ -147,8 +147,8 @@ def regexp(str_regex):
     parser.add_argument(
         '--merge-order',
         default='created_at',
-        choices=('created_at', 'updated_at'),
-        help='Order marge merges assigned requests. created_at (default) or updated_at.\n',
+        choices=('created_at', 'updated_at', 'assigned_at'),
+        help='Order marge merges assigned requests. created_at (default), updated_at or assigned_at.\n',
     )
     parser.add_argument(
         '--approval-reset-timeout',

marge/bot.py

@@ -102,7 +102,7 @@ def _get_merge_requests(self, project, project_name):
         log.info('Fetching merge requests assigned to me in %s...', project_name)
         my_merge_requests = MergeRequest.fetch_all_open_for_user(
             project_id=project.id,
-            user_id=self.user.id,
+            user=self.user,
             api=self._api,
             merge_order=self._config.merge_order,
         )

marge/merge_request.py

@@ -1,5 +1,6 @@
 import logging as log
 import time
+import datetime
 
 from . import gitlab
 from .approvals import Approvals
@@ -34,17 +35,41 @@ def fetch_by_iid(cls, project_id, merge_request_iid, api):
         return merge_request
 
     @classmethod
-    def fetch_all_open_for_user(cls, project_id, user_id, api, merge_order):
+    def fetch_assigned_at(cls, user, api, merge_request):
+        assigned_at = 0
+        all_discussions = api.collect_all_pages(
+            GET('/projects/{project_id}/merge_requests/{merge_requests_id}/discussions'.format(
+                project_id=merge_request.get('project_id'),
+                merge_requests_id=merge_request.get('iid')
+            )))
+        match_body = 'assigned to @{username}'.format(username=user.username)
+        for discussion in all_discussions:
+            for note in discussion.get('notes'):
+                if match_body in note.get('body'):
+                    assigned = datetime.datetime.strptime(
+                        note.get('created_at')[0:19], "%Y-%m-%dT%H:%M:%S"
+                        ).timestamp()
+                    if assigned > assigned_at:
+                        assigned_at = assigned
+        return assigned_at
+
+    @classmethod
+    def fetch_all_open_for_user(cls, project_id, user, api, merge_order):
+        request_merge_order = 'created_at' if merge_order == 'assigned_at' else merge_order
+
         all_merge_request_infos = api.collect_all_pages(GET(
             '/projects/{project_id}/merge_requests'.format(project_id=project_id),
-            {'state': 'opened', 'order_by': merge_order, 'sort': 'asc'},
+            {'state': 'opened', 'order_by': request_merge_order, 'sort': 'asc'},
         ))
         my_merge_request_infos = [
             mri for mri in all_merge_request_infos
-            if ((mri.get('assignee', {}) or {}).get('id') == user_id) or
-               (user_id in [assignee.get('id') for assignee in (mri.get('assignees', []) or [])])
+            if ((mri.get('assignee', {}) or {}).get('id') == user.id) or
+               (user.id in [assignee.get('id') for assignee in (mri.get('assignees', []) or [])])
         ]
 
+        if merge_order == 'assigned_at':
+            my_merge_request_infos.sort(key=lambda mri: cls.fetch_assigned_at(user, api, mri))
+
         return [cls(api, merge_request_info) for merge_request_info in my_merge_request_infos]
 
     @property

tests/test_app.py

@@ -228,12 +228,18 @@ def test_git_reference_repo():
             assert bot.config.git_reference_repo == '/foo/reference_repo'
 
 
-def test_merge_order():
+def test_merge_order_updated():
     with env(MARGE_AUTH_TOKEN="NON-ADMIN-TOKEN", MARGE_SSH_KEY="KEY", MARGE_GITLAB_URL='http://foo.com'):
         with main("--merge-order='updated_at'") as bot:
             assert bot.config.merge_order == 'updated_at'
 
 
+def test_merge_order_assigned():
+    with env(MARGE_AUTH_TOKEN="NON-ADMIN-TOKEN", MARGE_SSH_KEY="KEY", MARGE_GITLAB_URL='http://foo.com'):
+        with main("--merge-order='assigned_at'") as bot:
+            assert bot.config.merge_order == 'assigned_at'
+
+
 # FIXME: I'd reallly prefer this to be a doctest, but adding --doctest-modules
 # seems to seriously mess up the test run
 def test_time_interval():

tests/test_merge_request.py

@@ -4,6 +4,9 @@
 
 from marge.gitlab import Api, GET, POST, PUT, Version
 from marge.merge_request import MergeRequest, MergeRequestRebaseFailed
+import marge.user
+
+from tests.test_user import INFO as USER_INFO
 
 _MARGE_ID = 77
 
@@ -24,6 +27,14 @@
     'work_in_progress': False,
 }
 
+DISCUSSION = {
+    'id': 'aabbcc0044',
+    'notes': [
+        {'id': 12, "body": "assigned to @john_smith", "created_at": "2020-08-04T06:56:11.854Z"},
+        {'id': 13, "body": "assigned to @john_smith", "created_at": "2020-08-18T06:52:58.093Z"}
+    ],
+}
+
 
 # pylint: disable=attribute-defined-outside-init
 class TestMergeRequest:
@@ -192,16 +203,31 @@ def test_accept_merge_when_pipeline_succeeds(self):
     def test_fetch_all_opened_for_me(self):
         api = self.api
         mr1, mr_not_me, mr2 = INFO, dict(INFO, assignees=[{'id': _MARGE_ID+1}], id=679), dict(INFO, id=678)
+        user = marge.user.User(api=None, info=dict(USER_INFO, id=_MARGE_ID))
         api.collect_all_pages = Mock(return_value=[mr1, mr_not_me, mr2])
         result = MergeRequest.fetch_all_open_for_user(
-            1234, user_id=_MARGE_ID, api=api, merge_order='created_at'
+            1234, user=user, api=api, merge_order='created_at'
         )
         api.collect_all_pages.assert_called_once_with(GET(
             '/projects/1234/merge_requests',
             {'state': 'opened', 'order_by': 'created_at', 'sort': 'asc'},
         ))
         assert [mr.info for mr in result] == [mr1, mr2]
 
+    def test_fetch_assigned_at(self):
+        api = self.api
+        dis1, dis2 = DISCUSSION, dict(DISCUSSION, id=679)
+        mr1 = INFO
+        user = marge.user.User(api=None, info=dict(USER_INFO, id=_MARGE_ID))
+        api.collect_all_pages = Mock(return_value=[dis1, dis2])
+        result = MergeRequest.fetch_assigned_at(
+            user=user, api=api, merge_request=mr1
+        )
+        api.collect_all_pages.assert_called_once_with(GET(
+            '/projects/1234/merge_requests/54/discussions',
+        ))
+        assert result == 1597733578
+
     def _load(self, json):
         old_mock = self.api.call
         self.api.call = Mock(return_value=json)