Support webhook-2fm style messages #281
Conversation
| if 'body' in msg: | ||
| return msg['body'] | ||
| elif 'msg' in msg: | ||
| return msg['msg'] | ||
| else: | ||
| raise KeyError(f"Unrecognized message format with keys {msg.keys()}. Expected either 'msg' or 'body'") |
There was a problem hiding this comment.
Alternatively,
| if 'body' in msg: | |
| return msg['body'] | |
| elif 'msg' in msg: | |
| return msg['msg'] | |
| else: | |
| raise KeyError(f"Unrecognized message format with keys {msg.keys()}. Expected either 'msg' or 'body'") | |
| body = msg.get("body", msg.get("msg")) | |
| if body: | |
| return body | |
| raise KeyError(f"Unrecognized message format with keys {msg.keys()}. Expected either 'msg' or 'body'") |
This avoids duplicate key look-ups, and it also raises an error if the value exists but is empty. (If you don't like this last part, you can change the if test to check for None explicitly to allow it to return an empty body.)
| if suffix == 'github.issue.comment': | ||
| if 'pull_request' in msg['msg']['issue'] and msg['msg']['action'] != 'deleted': | ||
| body = c.extract_message_body(msg) |
There was a problem hiding this comment.
Rather than adding the call to extract_message_body() here, consider changing the function signature to receive the body directly instead of the message wrapper for it. This will save the dict instantiation at line 289 at the possible cost of having to add a call to extract_message_body() in listen() at line 172 (but, since you would be removing the call from here, that would be a net-zero cost and an overall savings, at the cost of an extra line of code change).
| """ | ||
| owner = msg['msg']['repository']['owner']['login'] | ||
| repo = msg['msg']['repository']['name'] | ||
| body = c.extract_message_body(msg) |
There was a problem hiding this comment.
Ditto the previous comment.
| """ | ||
| owner = msg['msg']['repository']['owner']['login'] | ||
| repo = msg['msg']['repository']['name'] | ||
| body = c.extract_message_body(msg) |
There was a problem hiding this comment.
Ditto the previous comment.
| self.new_style_mock_github_message = { | ||
| 'body': { |
There was a problem hiding this comment.
Is new_style_mock_github_message supposed to be identical to old_style_mock_github_message except for the body vs. msg key? If so, I recommend defining the body separately and using that definition to define these two attributes (or to define one attribute using the other), to avoid repeating ourself.
| @mock.patch(PATH + 'Github') | ||
| @mock.patch('sync2jira.intermediary.Issue.from_github') | ||
| def test_handle_github_message_successful(self, | ||
| def test_handle_old_style_github_message_successful(self, | ||
| mock_issue_from_github, | ||
| mock_github): | ||
| """ | ||
| This function tests 'handle_github_message' where everything goes smoothly! | ||
| """ | ||
| # Set up return values | ||
| mock_issue_from_github.return_value = "Successful Call!" | ||
| mock_github.return_value = self.mock_github_client | ||
|
|
||
| # Call function | ||
| response = u.handle_github_message( | ||
| msg=self.old_style_mock_github_message, | ||
| config=self.mock_config | ||
| ) | ||
|
|
||
| # Assert that calls were made correctly | ||
| mock_issue_from_github.assert_called_with('org/repo', | ||
| {'labels': ['custom_tag'], 'number': 'mock_number', | ||
| 'comments': [{'body': 'mock_body', 'name': unittest.mock.ANY, | ||
| 'author': 'mock_username', 'changed': None, | ||
| 'date_created': 'mock_created_at', 'id': 'mock_id'}], | ||
| 'assignees': [{'fullname': 'mock_name'}], | ||
| 'filter1': 'filter1', 'user': | ||
| {'login': 'mock_login', 'fullname': 'mock_name'}, | ||
| 'milestone': 'mock_milestone'}, self.mock_config) | ||
| mock_github.assert_called_with('mock_token', retry=5) | ||
| self.assertEqual('Successful Call!', response) | ||
| self.mock_github_client.get_repo.assert_called_with('org/repo') | ||
| self.mock_github_repo.get_issue.assert_called_with(number='mock_number') | ||
| self.mock_github_issue.get_comments.assert_any_call() | ||
| self.mock_github_client.get_user.assert_called_with('mock_login') | ||
|
|
There was a problem hiding this comment.
Rather than cookie-cuttering this code, I suggest reusing the code in test_handle_new_style_github_message_successful() with each of the old and new style of message, to avoid the code duplication.
https://lists.fedoraproject.org/archives/list/[email protected]/message/UGMBWWAXIM6BHK3RIBWQWCNIUYZ6U5XH/
I found some example messages here (notice the different topic):