Syncing up with what's in production :-/
authorJude N <juden@pwan.org>
Thu, 21 Dec 2017 14:39:22 +0000 (09:39 -0500)
committerJude N <juden@pwan.org>
Thu, 21 Dec 2017 14:39:22 +0000 (09:39 -0500)
- the webhook doesn't report a new MR if it's already tracking it

.pylintrc
plugins/marge.py
tests/test_marge.py

index 4d85818..0de8463 100644 (file)
--- a/.pylintrc
+++ b/.pylintrc
@@ -59,7 +59,7 @@ confidence=
 # --enable=similarities". If you want to run only the classes checker, but have
 # no Warning level messages displayed, use"--disable=all --enable=classes
 # --disable=W"
-disable=import-star-module-level,old-octal-literal,oct-method,print-statement,unpacking-in-except,parameter-unpacking,backtick,old-raise-syntax,old-ne-operator,long-suffix,dict-view-method,dict-iter-method,metaclass-assignment,next-method-called,raising-string,indexing-exception,raw_input-builtin,long-builtin,file-builtin,execfile-builtin,coerce-builtin,cmp-builtin,buffer-builtin,basestring-builtin,apply-builtin,filter-builtin-not-iterating,using-cmp-argument,useless-suppression,range-builtin-not-iterating,suppressed-message,no-absolute-import,old-division,cmp-method,reload-builtin,zip-builtin-not-iterating,intern-builtin,unichr-builtin,reduce-builtin,standarderror-builtin,unicode-builtin,xrange-builtin,coerce-method,delslice-method,getslice-method,setslice-method,input-builtin,round-builtin,hex-method,nonzero-method,map-builtin-not-iterating,line-too-long,no-name-in-module,input-error,missing-super-argument,no-self-use,too-many-locals,too-many-branches,import-error,locally-disabled,locally-enabled,too-many-ancestors,useless-super-delegation,logging-format-interpolation
+disable=import-star-module-level,old-octal-literal,oct-method,print-statement,unpacking-in-except,parameter-unpacking,backtick,old-raise-syntax,old-ne-operator,long-suffix,dict-view-method,dict-iter-method,metaclass-assignment,next-method-called,raising-string,indexing-exception,raw_input-builtin,long-builtin,file-builtin,execfile-builtin,coerce-builtin,cmp-builtin,buffer-builtin,basestring-builtin,apply-builtin,filter-builtin-not-iterating,using-cmp-argument,useless-suppression,range-builtin-not-iterating,suppressed-message,no-absolute-import,old-division,cmp-method,reload-builtin,zip-builtin-not-iterating,intern-builtin,unichr-builtin,reduce-builtin,standarderror-builtin,unicode-builtin,xrange-builtin,coerce-method,delslice-method,getslice-method,setslice-method,input-builtin,round-builtin,hex-method,nonzero-method,map-builtin-not-iterating,line-too-long,no-name-in-module,input-error,missing-super-argument,no-self-use,too-many-locals,too-many-branches,import-error,locally-disabled,locally-enabled,too-many-ancestors,useless-super-delegation,logging-format-interpolation,too-many-statements
 
 
 [REPORTS]
index 968ef7a..42e42c3 100755 (executable)
@@ -137,23 +137,25 @@ class Marge(BotPlugin, CrontabMixin):
 
             target_project_id = request['object_attributes']['target_project_id']
             iid = request['object_attributes']['iid']
+            mr_id = request['object_attributes']['id']
 
-            msg_template = "New Review: {} has opened a new {}MR: \"{}\"\n{}/merge_requests/{}"
+            msg_template = "Hi there ! {} has opened a new {}MR: \"{}\"\n{}/merge_requests/{}"
             msg = msg_template.format(author_name, wip, title, url, iid)
 
-            for a_room in rooms.split(','):
-                if self.config:
-                    self.send(self.build_identifier(a_room + '@' + self.chatroom_host), msg)
-
             if 'OPEN_MRS' not in self.keys():
                 empty_dict = {}
                 self['OPEN_MRS'] = empty_dict
 
             open_mrs = self['OPEN_MRS']
-            self.log.info("webhook: Saving ({}, {}, {})".format(target_project_id, iid, rooms))
-            open_mrs[(target_project_id, id, rooms)] = True
-            self['OPEN_MRS'] = open_mrs
 
+            if (target_project_id, mr_id, rooms) not in open_mrs:
+                for a_room in rooms.split(','):
+                    if self.config:
+                        self.send(self.build_identifier(a_room + '@' + self.chatroom_host), msg)
+
+                self.log.info("webhook: Saving ({}, {}, {})".format(target_project_id, mr_id, rooms))
+                open_mrs[(target_project_id, mr_id, rooms)] = True
+                self['OPEN_MRS'] = open_mrs
         return "OK"
 
     def mr_status_msg(self, a_mr, author=None):
@@ -162,17 +164,19 @@ class Marge(BotPlugin, CrontabMixin):
         """
         self.log.info("mr_status_msg: a_mr: {}".format(a_mr))
 
+        # Only weed out MRs less than the soak time for the crontab output (where author==None)
         now = datetime.now(timezone.utc)
         creation_time = parser.parse(a_mr['created_at'], tzinfos=tzutc)
-        self.log.info("times: {}, {}, {}".format(creation_time, self.soak_delta, now))
-        if creation_time + self.soak_delta > now:
-            project = a_mr['project']
-            iid = a_mr['iid']
-            soak_hours = self.config['CRONTAB_SOAK_HOURS']
-            info_template = "skipping: MR <{},{}> was opened less than {} hours ago"
-            info_msg = info_template.format(project, iid, soak_hours)
-            self.log.info(info_msg)
-            return None
+        if not author:
+            self.log.info("times: {}, {}, {}".format(creation_time, self.soak_delta, now))
+            if creation_time + self.soak_delta > now:
+                project_id = a_mr['target_project_id']
+                mr_id = a_mr['id']
+                soak_hours = self.config['CRONTAB_SOAK_HOURS']
+                info_template = "skipping: MR <{},{}> was opened less than {} hours ago"
+                info_msg = info_template.format(project_id, mr_id, soak_hours)
+                self.log.info(info_msg)
+                return None
 
         str_open_since = deltastr(now - creation_time)
 
@@ -246,19 +250,19 @@ class Marge(BotPlugin, CrontabMixin):
         # Let's walk through the MRs we've seen already:
         open_mrs = self['OPEN_MRS']
 
-        for (project, iid, notify_rooms) in open_mrs:
+        for (project_id, mr_id, notify_rooms) in open_mrs:
 
-            # Lookup the MR from the project/iid
-            a_mr = self.gitlab.getmergerequest(project, iid)
+            # Lookup the MR from the project/id
+            a_mr = self.gitlab.getmergerequest(project_id, mr_id)
 
-            self.log.info("a_mr: {} {} {} {}".format(project, iid, notify_rooms, a_mr['state']))
+            self.log.info("a_mr: {} {} {} {}".format(project_id, mr_id, notify_rooms, a_mr['state']))
 
             # If the MR is no longer open, skip to the next MR,
             # and don't include this MR in the next check
             if 'opened' not in a_mr['state']:
                 continue
             else:
-                still_open_mrs[(project, iid, notify_rooms)] = True
+                still_open_mrs[(project_id, mr_id, notify_rooms)] = True
 
             msg_tuple = self.mr_status_msg(a_mr)
             if msg_tuple is None:
@@ -283,7 +287,7 @@ class Marge(BotPlugin, CrontabMixin):
                 room_msg = "\n".join(msgs)
 
                 if self.config:
-                    msg_template = "These MRs need some attention:{}\n"
+                    msg_template = "These MRs need some attention:\n{}\n"
                     msg_template += "You can get an updated list with the !reviews command."
                     to_room = a_room + '@' + self.config['CHATROOM_HOST']
                     msg = msg_template.format(room_msg)
@@ -330,17 +334,17 @@ class Marge(BotPlugin, CrontabMixin):
         msg = ""
         still_open_mrs = {}
         open_mrs = self['OPEN_MRS']
-        for (project, iid, notify_rooms) in open_mrs:
+        for (project, mr_id, notify_rooms) in open_mrs:
 
-            # Lookup the MR from the project/iid
-            a_mr = self.gitlab.getmergerequest(project, iid)
+            # Lookup the MR from the project/id
+            a_mr = self.gitlab.getmergerequest(project, mr_id)
 
             # If the MR is no longer open, skip to the next MR,
             # and don't include this MR in the next check
             if 'opened' not in a_mr['state']:
                 continue
             else:
-                still_open_mrs[(project, iid, notify_rooms)] = True
+                still_open_mrs[(project, mr_id, notify_rooms)] = True
 
             msg_tuple = self.mr_status_msg(a_mr, author=sender_gitlab_id)
             if msg_tuple is None:
index bb53f9d..8aed0ba 100644 (file)
@@ -38,6 +38,10 @@ class TestMarge(object):
         testbot.push_message("!plugin config Webserver {'HOST': '0.0.0.0', 'PORT':3141, 'SSL': {'certificate': '', 'enabled': False, 'host': '0.0.0.0', 'key': '', 'port': 3142}}")
         testbot.pop_message()
 
+        def mock_get(self, key):
+            return {}
+        monkeypatch.setattr(errbot.storage.StoreMixin, '__getitem__', mock_get, raising=False)
+
         def mock_keys(self):
             return ['OPEN_MRS']
         monkeypatch.setattr(errbot.storage.StoreMixin, 'keys', mock_keys, raising=False)
@@ -46,11 +50,6 @@ class TestMarge(object):
 
     @pytest.fixture
     def margebot_no_reviews(self, margebot, monkeypatch):
-
-        def mock_get(self, key):
-            return {}
-
-        monkeypatch.setattr(errbot.storage.StoreMixin, '__getitem__', mock_get, raising=False)
         return margebot
 
     @pytest.fixture
@@ -151,6 +150,7 @@ class TestMarge(object):
         def mock_getmergerequest(self, project, iid):
             return {}
         monkeypatch.setattr(gitlab, 'getuser', mock_getmergerequest)
+        return gitlab
 
     @pytest.fixture
     def gitlab_one_review(self, gitlab, monkeypatch):
@@ -163,6 +163,7 @@ class TestMarge(object):
                     'web_url': 'http://gitlab.example.com/sample/mr/2001',
                     'work_in_progress': False}
         monkeypatch.setattr(gitlab, 'getmergerequest', mock_getmergerequest)
+        return gitlab
 
     # ============================================================================
 
@@ -211,13 +212,14 @@ class TestMarge(object):
                                   'title': 'title',
                                   'author_id': 'author_id',
                                   'target_project_id': 'project_id',
+                                  'id': 'id',
                                   'iid': 'iid'},
                               'project': {
                                   'homepage': 'url'}})
         margebot.push_message("!webhook test /margebot/room1,room2 " + request)
-        assert 'New Review: author_id username has opened a new MR: \"title\"\nurl/merge_requests/iid' in margebot.pop_message()
+        assert 'Hi there ! author_id username has opened a new MR: \"title\"\nurl/merge_requests/iid' in margebot.pop_message()
         margebot.push_message('!reviews')
-        assert 'New Review: author_id username has opened a new MR: \"title\"\nurl/merge_requests/iid' in margebot.pop_message()
+        assert 'Hi there ! author_id username has opened a new MR: \"title\"\nurl/merge_requests/iid' in margebot.pop_message()
 
     def test_gitlab_hook_wip(self, margebot, gitlab):
         request = json.dumps({'object_kind': 'merge_request',
@@ -227,13 +229,14 @@ class TestMarge(object):
                                   'title': 'title',
                                   'author_id': 'author_id',
                                   'target_project_id': 'project_id',
+                                  'id': 'id',
                                   'iid': 'iid'},
                               'project': {
                                   'homepage': 'url'}})
         margebot.push_message("!webhook test /margebot/room1,room2 " + request)
-        assert 'New Review: author_id username has opened a new WIP MR: \"title\"\nurl/merge_requests/iid' in margebot.pop_message()
+        assert 'Hi there ! author_id username has opened a new WIP MR: \"title\"\nurl/merge_requests/iid' in margebot.pop_message()
         margebot.push_message('!reviews')
-        assert 'New Review: author_id username has opened a new WIP MR: \"title\"\nurl/merge_requests/iid' in margebot.pop_message()
+        assert 'Hi there ! author_id username has opened a new WIP MR: \"title\"\nurl/merge_requests/iid' in margebot.pop_message()
 
     def test_gitlab_hook_unexpected_user(self, margebot, gitlab):
         request = json.dumps({'object_kind': 'merge_request',
@@ -243,16 +246,17 @@ class TestMarge(object):
                                   'title': 'title',
                                   'author_id': '(missing)',
                                   'target_project_id': 'project_id',
+                                  'id': 'id',
                                   'iid': 'iid'},
                               'project': {
                                   'homepage': 'url'}})
 
         margebot.push_message("!webhook test /margebot/room1,room2 " + request)
-        assert 'New Review: (missing) has opened a new MR: \"title\"\nurl/merge_requests/iid' in margebot.pop_message()
+        assert 'Hi there ! (missing) has opened a new MR: \"title\"\nurl/merge_requests/iid' in margebot.pop_message()
         margebot.push_message('!reviews')
-        assert 'New Review: (missing) has opened a new MR: \"title\"\nurl/merge_requests/iid' in margebot.pop_message()
+        assert 'Hi there ! (missing) has opened a new MR: \"title\"\nurl/merge_requests/iid' in margebot.pop_message()
 
-    def test_gitlab_hook_unexpected_object_kind(self, margebot_no_reviews, gitlab, caplog):
+    def test_gitlab_hook_unexpected_object_kind(self, margebot, gitlab, caplog):
         request = json.dumps({'object_kind': 'not_merge_request',
                               'object_attributes': {
                                   'state': 'opened',
@@ -260,19 +264,20 @@ class TestMarge(object):
                                   'title': 'title',
                                   'author_id': 'author_id',
                                   'target_project_id': 'project_id',
+                                  'id': 'id',
                                   'iid': 'iid'},
                               'project': {
                                   'homepage': 'url'}})
 
-        margebot_no_reviews.push_message("!webhook test /margebot/room1,room2 " + request)
-        margebot_no_reviews.pop_message()
-        margebot_no_reviews.push_message('!reviews')
-        assert 'Hi gbin: I found no open MRs for you.' in margebot_no_reviews.pop_message()
+        margebot.push_message("!webhook test /margebot/room1,room2 " + request)
+        margebot.pop_message()
+        margebot.push_message('!reviews')
+        assert 'Hi gbin: I found no open MRs for you.' in margebot.pop_message()
         assert 'unexpecting object_kind: not_merge_request' in caplog.text  # Has to be at end of method
 
-    def test_nothing_to_review(self, margebot_no_reviews, gitlab_no_reviews):
-        margebot_no_reviews.push_message('!reviews')
-        assert 'Hi gbin: I found no open MRs for you.' in margebot_no_reviews.pop_message()
+    def test_nothing_to_review(self, margebot, gitlab_no_reviews):
+        margebot.push_message('!reviews')
+        assert 'Hi gbin: I found no open MRs for you.' in margebot.pop_message()
 
     def test_get_one_open_mr(self, margebot_one_review, gitlab_one_review):
         margebot_one_review.push_message('!reviews')