PR#50: Add comments synchronization for Github - sync-to-jira

		`@@ -32,6 +32,8 @@`

		`jira_cache = {}`

		`+ def _comment_format(comment):`
		`+ return "Upstream, %s wrote:\n\n{quote}\n%s\n{quote}" % (comment['author'], comment['body'])`

		`def _get_jira_client(issue, config):`

		`@@ -68,6 +70,21 @@`
		`log.debug("Found %i results for query %r", len(results), query)`
		`return results`

		`+ def _find_comment_in_jira(comment, j_comments):`
		`+ formated_comment = _comment_format(comment)`
		`+ for item in j_comments:`
		`+ if item.raw['body'] == formated_comment:`
		`+ return item`
		`+ return None`
		`+`
		`+`
		`+ def _comment_matching(g_comments, j_comments):`
		`+ return list(`
		`+ filter(lambda x:`
		`+ _find_comment_in_jira(x, j_comments) == None or x['changed'] != None,`
		`+ g_comments`
		`+ )`
		`+ )`

		`def _get_existing_jira_issue(client, issue, config):`
		`""" Get a jira issue by the linked remote issue.`
		`@@ -166,6 +183,8 @@`

		`remote_link = dict(url=issue.url, title=remote_link_title)`
		`_attach_link(client, downstream, remote_link)`
		`+ for comment in issue.comments:`
		`+ client.add_comment(downstream, _comment_format(comment))`
		`return downstream`


		`@@ -222,6 +241,7 @@`

		`def sync_with_jira(issue, config):`
		`log.info("Considering upstream %s, %s", issue.url, issue.title)`
		`+ client = jira.client.JIRA(**config['sync2jira']['jira'])`

		`# Create a client connection for this issue`
		`client = _get_jira_client(issue, config)`
		`@@ -231,6 +251,21 @@`
		`log.info(" Looking for matching downstream issue via new method.")`
		`existing = _get_existing_jira_issue(client, issue, config)`
		`if existing:`
		`+ comments = client.comments(existing)`
		`+ log.info(" Looking for relative complement between downstream comments and upstream")`
		`+ comments_d = _comment_matching(issue.comments, comments)`
		`+ for comment in comments_d:`
		`+ if comment['changed'] == None:`
		`+ comment_body = _comment_format(comment)`
		`+ client.add_comment(existing, comment_body)`
		`+ else:`
		`+ j_comment = find(lambda y: y.raw['body'] == _comment_format(comment), comments)`
		`+ comment_body = "%s wrote: %s" % (comment['author'], comment['changed'])`
		`+ if j_comment == None:`
		`+ client.add_comment(existing, comment_body)`
		`+ else:`
		`+ j_comment.update(body = comment_body)`
		`+ log.info(" Comments synchronization done.")`
		`log.info(" Found existing, matching downstream %r.", existing.key)`
		`return`

sync2jira/intermediary.py

file modified

+18 -1

		`@@ -20,11 +20,12 @@`

		`class Issue(object):`

		`- def __init__(self, source, title, url, upstream, config):`
		`+ def __init__(self, source, title, url, upstream, comments, config):`
		`self.source = source`
		`self._title = title`
		`self.url = url`
		`self.upstream = upstream`
		`+ self.comments = comments`
		`self.downstream = config['sync2jira']['map'][self.source][upstream]`

		`@property`
		`@@ -34,20 +35,36 @@`
		`@classmethod`
		`def from_pagure(cls, upstream, issue, config):`
		`base = config['sync2jira'].get('pagure_url', 'https://pagure.io')`
		`+ comments = []`
		`+ for comment in issue['comments']:`
		`+ comments.append({`
		`+ 'author': comment['user']['name'],`
		`+ 'body': comment['comment'],`
		`+ 'changed': None`
		`+ })`
		`return Issue(`
		`source='pagure',`
		`title=issue['title'],`
		`url=base + '/%s/issue/%i' % (upstream, issue['id']),`
		`upstream=upstream,`
		`config=config,`
		`+ comments=comments,`
		`)`

		`@classmethod`
		`def from_github(cls, upstream, issue, config):`
		`+ comments = []`
		`+ for comment in issue['comments']:`
		`+ comments.append({`
		`+ 'author': comment['user']['login'],`
		`+ 'body': comment['body'],`
		`+ 'changed': comment['changed'] if 'changed' in comment else None`
		`+ })`
		`return Issue(`
		`source='github',`
		`title=issue['title'],`
		`url=issue['html_url'],`
		`+ comments=comments,`
		`upstream=upstream,`
		`config=config,`
		`)`

sync2jira/main.py

file modified

		`@@ -43,10 +43,12 @@`
		`'github.issue.reopened': u.handle_github_message,`
		`# Example: https://apps.fedoraproject.org/datagrepper/id?id=2017-a053e0c2-f514-47d6-8cb2-f7b2858f7052&is_raw=true&size=extra-large`
		`'github.issue.labeled': u.handle_github_message,`
		`+ 'github.issue.comment': u.handle_github_comment,`
		`# Example: https://apps.fedoraproject.org/datagrepper/id?id=2016-d578d8f6-0c4c-493d-9535-4e138a03e197&is_raw=true&size=extra-large`
		`'pagure.issue.new': u.handle_pagure_message,`
		`# Example: https://apps.fedoraproject.org/datagrepper/id?id=2017-c2e81259-8576-41a9-83c6-6db2cbcf67d3&is_raw=true&size=extra-large`
		`'pagure.issue.tag.added': u.handle_pagure_message,`
		`+ 'pagure.issue.comment.added': u.handle_pagure_comment,`
		`}`

sync2jira/upstream.py

file modified

+53 -7

		`@@ -152,22 +152,68 @@`
		`for issue in issues:`
		`yield issue`

		`+ def handle_github_comment(msg, config):`
		`+ owner = msg['msg']['repository']['owner']['login']`
		`+ repo = msg['msg']['repository']['name']`
		`+ upstream = '{owner}/{repo}'.format(owner=owner, repo=repo)`
		`+ mapped_repos = config['sync2jira']['map']['github']`
		`+`
		`+ token = config['sync2jira'].get('github_token')`
		`+ if not token:`
		`+ headers = {}`
		`+ log.warning('No github_token found. We will be rate-limited...')`
		`+ else:`
		`+ headers = {'Authorization': 'token ' + token}`
		`+`
		`+ if upstream not in mapped_repos:`
		`+ log.info("%r not in github map: %r", upstream, mapped_repos.keys())`
		`+ return None`
		`+ issue = msg['msg']['issue']`
		`+`
		`+ comment = msg['msg']['comment']`
		`+ log.info("Fetching comments for: %s", issue['url'])`
		`+ issue['comments'] = _fetch_github_data("%s/comments" % (issue['url']), headers).json()`
		`+ if msg['msg']['action'] == 'edited':`
		`+ for c in issue['comments']:`
		`+ if c['body'] == comment['body']:`
		`+ c['body'] = msg['msg']['changes']['body']['from']`
		`+ c['changed'] = comment['body']`
		`+ return i.Issue.from_github(upstream, issue, config)`
		`+`
		`+ def handle_pagure_comment(msg, config):`
		`+ upstream = msg['msg']['project']['name']`
		`+ ns = msg['msg']['project'].get('namespace') or None`
		`+ if ns:`
		`+ upstream = '{ns}/{upstream}'.format(ns=ns, upstream=upstream)`
		`+ mapped_repos = config['sync2jira']['map']['pagure']`
		`+`
		`+ if upstream not in mapped_repos:`
		`+ log.info("%r not in pagure map: %r", upstream, mapped_repos.keys())`
		`+ return None`
		`+`
		`+ issue = msg['msg']['issue']`
		`+ return i.Issue.from_pagure(upstream, issue, config)`

		`def _get_all_github_issues(url, headers):`
		`""" Pagination utility. Obnoxious. """`
		`link = dict(next=url)`
		`while 'next' in link:`
		`- response = requests.get(link['next'], headers=headers)`
		`- if not bool(response):`
		`- try:`
		`- reason = response.json()`
		`- except Exception:`
		`- reason = response.text`
		`- raise IOError("response: %r %r %r" % (response, reason, response.request.url))`
		`+ response = _fetch_github_data(link['next'], headers)`
		`for issue in response.json():`
		`+ comments = _fetch_github_data(issue['comments_url'], headers)`
		`+ issue['comments'] = comments.json()`
		`yield issue`
		`link = _github_link_field_to_dict(response.headers.get('link', None))`

		`+ def _fetch_github_data(url, headers):`
		`+ response = requests.get(url, headers=headers)`
		`+ if not bool(response):`
		`+ try:`
		`+ reason = response.json()`
		`+ except:`
		`+ reason = response.text`
		`+ raise IOError("response: %r %r %r" % (response, reason, response.request.url))`
		`+ return response`

		`def _github_link_field_to_dict(field):`
		`""" Utility for ripping apart github's Link header field.`

tests/test_downstream.py

file modified

+7 -1

		`@@ -37,6 +37,7 @@`
		`@mock.patch('jira.client.JIRA')`
		`def test_get_existing_newstyle(self, client):`
		`config = self.config.copy()`
		`+`
		`class MockIssue(object):`
		`downstream = {'key': 'value'}`
		`title = 'A title, a title...'`
		`@@ -57,6 +58,7 @@`
		`@mock.patch('jira.client.JIRA')`
		`def test_upgrade_oldstyle_jira_issue(self, client):`
		`config = self.config.copy()`
		`+`
		`class MockIssue(object):`
		`downstream = {'key': 'value'}`
		`title = 'A title, a title...'`
		`@@ -73,7 +75,6 @@`
		`}`
		`client_obj.add_remote_link.assert_called_once_with(downstream.id, remote)`

		`-`
		`@mock.patch('jira.client.JIRA')`
		`def test_create_jira_issue(self, client):`
		`config = self.config.copy()`
		`@@ -90,6 +91,10 @@`
		`}`
		`title = 'A title, a title...'`
		`url = 'http://threebean.org'`
		`+ comments = [{`
		`+ 'author': 'Ralph',`
		`+ 'body': 'Super duper.',`
		`+ }]`

		`config['sync2jira']['testing'] = False`
		`result = d._create_jira_issue(jira.client.JIRA(), MockIssue(), config)`
		`@@ -118,6 +123,7 @@`
		`}`
		`title = 'A title, a title...'`
		`url = 'http://threebean.org'`
		`+ comments = []`

		`config['sync2jira']['testing'] = False`
		`result = d._create_jira_issue(jira.client.JIRA(), MockIssue(), config)`

tests/test_intermediary.py

file modified

+20 -2

		`@@ -24,28 +24,46 @@`
		`}`

		`def test_issue_repr(self):`
		`- issue = i.Issue('pagure', 'title', 'url', 'koji', self.config)`
		`+ issue = i.Issue('pagure', 'title', 'url', 'koji', [], self.config)`
		`eq_(repr(issue), '<Issue url >')`

		`def test_issue_title(self):`
		`- issue = i.Issue('pagure', 'title', 'url', 'koji', self.config)`
		`+ issue = i.Issue('pagure', 'title', 'url', 'koji', [], self.config)`
		`eq_(issue.title, '[koji] title')`

		`def test_issue_from_pagure(self):`
		`upstream_issue = {`
		`'title': 'title',`
		`'id': 21,`
		`+ 'comments': [{`
		`+ 'user': {'name': 'Ralph'},`
		`+ 'comment': 'This is fine.',`
		`+ }],`
		`}`
		`issue = i.Issue.from_pagure('koji', upstream_issue, self.config)`
		`eq_(issue.title, '[koji] title')`
		`eq_(issue.url, 'https://pagure.io/koji/issue/21')`
		`+ eq_(issue.comments, [{`
		`+ 'author': 'Ralph',`
		`+ 'body': 'This is fine.',`
		`+ 'changed': None`
		`+ }])`

		`def test_issue_from_github(self):`
		`upstream_issue = {`
		`'title': 'title',`
		`'html_url': 'Some github url',`
		`+ 'comments': [{`
		`+ 'user': {'login': 'ralphbean'},`
		`+ 'body': 'This is dandy.',`
		`+ }],`
		`}`
		`repo = 'product-definition-center/product-definition-center'`
		`issue = i.Issue.from_github(repo, upstream_issue, self.config)`
		`eq_(issue.title, '[product-definition-center/product-definition-center] title')`
		`eq_(issue.url, 'Some github url')`
		`+ eq_(issue.comments, [{`
		`+ 'author': 'ralphbean',`
		`+ 'body': 'This is dandy.',`
		`+ 'changed': None`
		`+ }])`

tests/test_upstream.py

file modified

+129 -14

		`@@ -1,5 +1,6 @@`
		`import mock`
		`import unittest`
		`+ from nose.tools import eq_`

		`import sync2jira.upstream as u`

		`@@ -59,6 +60,112 @@`
		`'some_repo', issue_dict, self.config)`

		`@mock.patch('sync2jira.upstream.i.Issue.from_github')`
		`+ @mock.patch('sync2jira.upstream._fetch_github_data')`
		`+ def test_handle_github_comment(self, _fetch_github_data, from_github):`
		`+ comment = {`
		`+ 'user': {'login': 'threebean'},`
		`+ 'body': 'This is fine.',`
		`+ }`
		`+ comments = [comment]`
		`+ issue_dict = {`
		`+ 'state': 'Open',`
		`+ 'title': 'title',`
		`+ 'url': 'some_issue_url',`
		`+ }`
		`+ message = {`
		`+ 'msg': {`
		`+ 'action': 'created',`
		`+ 'repository': {`
		`+ 'name': 'repo',`
		`+ 'owner': {`
		`+ 'login': 'org',`
		`+ },`
		`+ },`
		`+ 'issue': issue_dict,`
		`+ 'comment': comment,`
		`+ },`
		`+ }`
		`+ githubResponse = mock.MagicMock()`
		`+ githubResponse.json.return_value = comments`
		`+ _fetch_github_data.return_value = githubResponse`
		`+ u.handle_github_comment(message, self.config)`
		`+`
		`+ from_github.assert_called_once_with(`
		`+ 'org/repo', issue_dict, self.config)`
		`+`
		`+ @mock.patch('sync2jira.upstream.i.Issue.from_github')`
		`+ @mock.patch('sync2jira.upstream._fetch_github_data')`
		`+ def test_handle_github_change_comment(self, _fetch_github_data, from_github):`
		`+ comment = {`
		`+ 'user': {'login': 'threebean'},`
		`+ 'body': 'This is fine.',`
		`+ }`
		`+ comments = [`
		`+ comment,`
		`+ {`
		`+ 'user': {'login': 'threebean'},`
		`+ 'body': 'Some other text',`
		`+ }`
		`+ ]`
		`+ issue_dict = {`
		`+ 'state': 'Open',`
		`+ 'title': 'title',`
		`+ 'url': 'some_issue_url',`
		`+ }`
		`+ message = {`
		`+ 'msg': {`
		`+ 'action': 'edited',`
		`+ 'changes': {`
		`+ 'body': {`
		`+ 'from': 'Some text',`
		`+ },`
		`+ },`
		`+ 'repository': {`
		`+ 'name': 'repo',`
		`+ 'owner': {`
		`+ 'login': 'org',`
		`+ },`
		`+ },`
		`+ 'issue': issue_dict,`
		`+ 'comment': comment,`
		`+ },`
		`+ }`
		`+ githubResponse = mock.MagicMock()`
		`+ githubResponse.json.return_value = comments`
		`+ _fetch_github_data.return_value = githubResponse`
		`+ u.handle_github_comment(message, self.config)`
		`+`
		`+ from_github.assert_called_once_with(`
		`+ 'org/repo', issue_dict, self.config)`
		`+`
		`+`
		`+ @mock.patch('sync2jira.upstream.i.Issue.from_pagure')`
		`+ def test_handle_pagure_comment(self, from_pagure):`
		`+ issue_dict = {`
		`+ 'status': 'Open',`
		`+ 'title': 'title',`
		`+ 'id': 21,`
		`+ 'comments': [{`
		`+ 'user': {'name': 'Ralph'},`
		`+ 'comment': 'This is fine.',`
		`+ }],`
		`+ }`
		`+ message = {`
		`+ 'msg': {`
		`+ 'project': {`
		`+ 'name': 'some_repo',`
		`+ 'owner': {`
		`+ 'login': 'org',`
		`+ },`
		`+ },`
		`+ 'issue': issue_dict,`
		`+ },`
		`+ }`
		`+ u.handle_pagure_comment(message, self.config)`
		`+ from_pagure.assert_called_once_with(`
		`+ 'some_repo', issue_dict, self.config)`
		`+`
		`+ @mock.patch('sync2jira.upstream.i.Issue.from_github')`
		`def test_handle_github_filter_positive(self, from_github):`
		`self.config['sync2jira']['filters'] = {`
		`'github': {`
		`@@ -172,8 +279,9 @@`
		`@mock.patch('sync2jira.upstream.i.Issue.from_pagure')`
		`def test_get_all_pagure_issues(self, from_pagure, requests):`
		`response = mock.MagicMock()`
		`+ mock_issue = {'comments_url': 'comment_url'}`
		`response.json.return_value = {`
		`- 'issues': ['some_issue_dict'],`
		`+ 'issues': [mock_issue],`
		`}`
		`requests.get.return_value = response`

		`@@ -186,27 +294,30 @@`
		`params={'status': 'Open'},`
		`)`
		`from_pagure.assert_called_once_with(`
		`- 'some_repo', 'some_issue_dict', self.config)`
		`+ 'some_repo', mock_issue, self.config)`

		`@mock.patch('sync2jira.upstream.requests')`
		`@mock.patch('sync2jira.upstream.i.Issue.from_github')`
		`def test_get_all_github_issues(self, from_github, requests):`
		`response = mock.MagicMock()`
		`- response.json.return_value = [`
		`- 'some_issue_dict',`
		`- ]`
		`+ mock_issue = {'comments_url': 'comment_url'}`
		`+ response.json.return_value = [mock_issue]`
		`requests.get.return_value = response`

		`generator = u.github_issues('some_repo', self.config)`
		`# Step through that...`
		`list(generator)`

		`- requests.get.assert_called_once_with(`
		`+ requests.get.assert_any_call(`
		`'https://api.github.com/repos/some_repo/issues?state=open',`
		`headers={},`
		`)`
		`+ requests.get.assert_any_call(`
		`+ 'comment_url',`
		`+ headers={},`
		`+ )`
		`from_github.assert_called_once_with(`
		`- 'some_repo', 'some_issue_dict', self.config)`
		`+ 'some_repo', mock_issue, self.config)`

		`@mock.patch('sync2jira.upstream.requests')`
		`@mock.patch('sync2jira.upstream.i.Issue.from_pagure')`
		`@@ -219,8 +330,9 @@`
		`},`
		`}`
		`response = mock.MagicMock()`
		`+ mock_issue = {'comments_url': 'comment_url'}`
		`response.json.return_value = {`
		`- 'issues': ['some_issue_dict'],`
		`+ 'issues': [mock_issue],`
		`}`
		`requests.get.return_value = response`

		`@@ -233,7 +345,7 @@`
		`params={'some_value': 'present'},`
		`)`
		`from_pagure.assert_called_once_with(`
		`- 'some_repo', 'some_issue_dict', self.config)`
		`+ 'some_repo', mock_issue, self.config)`

		`@mock.patch('sync2jira.upstream.requests')`
		`@mock.patch('sync2jira.upstream.i.Issue.from_github')`
		`@@ -246,18 +358,21 @@`
		`},`
		`}`
		`response = mock.MagicMock()`
		`- response.json.return_value = [`
		`- 'some_issue_dict',`
		`- ]`
		`+ mock_issue = {'comments_url': 'comment_url'}`
		`+ response.json.return_value = [mock_issue]`
		`requests.get.return_value = response`

		`generator = u.github_issues('some_repo', self.config)`
		`# Step through that...`
		`list(generator)`

		`- requests.get.assert_called_once_with(`
		`+ requests.get.assert_any_call(`
		`'https://api.github.com/repos/some_repo/issues?some_value=present',`
		`headers={},`
		`)`
		`+ requests.get.assert_any_call(`
		`+ 'comment_url',`
		`+ headers={},`
		`+ )`
		`from_github.assert_called_once_with(`
		`- 'some_repo', 'some_issue_dict', self.config)`
		`+ 'some_repo', mock_issue, self.config)`

no initial comment

ralph commented on line 22 of sync2jira/downstream.py 5 years ago

Oh, my. This is a long line. Bohdan, can you break it into multiple lines to make it readable?

1 new commit added

Split line in _comment_matching

5 years ago

ralph commented 5 years ago

A general comment: Bohdan, can you make this work for both github and pagure upstreams? I'd like to maintain parity/symmetry between them.

biakymet commented 5 years ago

@ralph yes. But I need to test it on something, do you have repository on pagure and component on jira on which I can test it?

ralph commented 5 years ago

Hm, how about this sync-to-jira repo? You could sync it to the FACTORY project in stage jira.

1 new commit added

Added support comment sync for pagure

5 years ago

biakymet commented 5 years ago

@ralph I added comment adding for pagure, but not case if comment will change, because pagure doesn't return previous text of comment. Thus I can't compare it. It gives only id, which is useless.

lucarval commented on line 25 of sync2jira/downstream.py 5 years ago

This should probably use _comment_format.

lucarval commented on line 24 of sync2jira/downstream.py 5 years ago

Personally, I think the multiple use of lambda makes this code kind of hard to read. I wonder if using nested for-loops with if-statements would make it a bit better.

lucarval commented on line 21 of sync2jira/intermediary.py 5 years ago

Let's remove print statements.

lucarval commented on line 19 of sync2jira/main.py 5 years ago

Seems unrelated.

1 new commit added

Made jira and github/pagure comments intersection more understandable, return logging level to previous value and remove print

5 years ago

lucarval commented 5 years ago

This looks reasonable to me. @ralph wdyt?
We probably want test coverage on the new code.

ralph commented on line 60 of sync2jira/downstream.py 5 years ago

What does j stand for?

ralph commented on line 6 of sync2jira/downstream.py 5 years ago

Can we change this to the following please? :bike: :house:

return "Upstream, %s wrote:\n\n{quote}\n%s\n{quote}" % (comment['author'], comment['body'])

ralph commented 5 years ago

This looks reasonable to me. @ralph wdyt?

Agreed. It is looking good. Almost there!

We probably want test coverage on the new code.

Agreed here also - @biakymet, can you add tests to match?

ralph commented on line 65 of sync2jira/downstream.py 5 years ago

What's going on in this line?

ralph commented on line 60 of sync2jira/downstream.py 5 years ago

Also here, it looks like you removed the definition of find without replacing it here.

ralph commented 5 years ago

I fixed a few mocks, got tox passing again, and pushed those commits to a comments-sync branch on the main repo. @biakymet, you should pull from there to update this PR.

There are other issues here that need to be resolved before this can be merged.

New tests should be added specifically covering comments in the upstream, intermediary, and downstream test files.
The use of the find function in downstream.py is definitely broken.
Please update the text format of the comment as requested above.

5 new commits added

Merge branch 'comments-sync' of https://pagure.io/sync-to-jira into comments-sync
Made jira and github/pagure comments intersection more understandable, return logging level to previous value and remove print
Bring mocks in line with latest comment sync changes.
PEP8 futzing.
Remove print statement.

5 years ago

biakymet commented 5 years ago