Merge pull request #3472 from /issues/3467/1

Fixes #3467 - Comment the reason why an issue was accepted but closed

tests/unit/test_webhook.py

@@ -411,32 +411,6 @@ def test_repo_scope_unknown(self):
         actual = helpers.repo_scope(url)
         self.assertEqual(expected, actual)
 
-    def test_prepare_incomplete_issue(self):
-        """Test we prepare the right payload for the incomplete issue."""
-        expected = {'body': '<p>Thanks for the report. Unfortunately without any\ndetail about the issue you experienced, we cannot help with this bug.\nPlease leave a comment with more detail, or file a new report and we will\ngladly investigate this further.</p>',  # noqa
-                    'milestone': 7,
-                    'state': 'closed',
-                    'title': 'Test incomplete title!'}
-        actual = helpers.prepare_incomplete_issue('Test incomplete title!')
-        self.assertEqual(type(actual), dict)
-        self.assertEqual(actual, expected)
-
-    def test_prepare_invalid_issue(self):
-        """Test we prepare the right payload for the invalid issue."""
-        expected = {'body': '<p>Thanks for the report, but this is not a Compatibility\nissue.</p><p>For this project we try to focus our effort on layouts, features\nor content that works as expected in one browser but not in another.\nClosing the issue as Invalid.</p>',  # noqa
-                    'milestone': 8,
-                    'state': 'closed',
-                    'title': 'Test invalid title!'}
-        actual = helpers.prepare_invalid_issue('Test invalid title!')
-        self.assertEqual(type(actual), dict)
-        self.assertEqual(actual, expected)
-
-    def test_prepare_issue_no_title(self):
-        """Test we raise for missing title arguements."""
-        with pytest.raises(ValueError) as excinfo:
-            helpers.prepare_invalid_issue()
-            helpers.prepare_incomplete_issue()
-
     def test_prepare_rejected_issue(self):
         """Test we prepare the right payload for the rejected issue."""
         expected = {'body': "<p>The content of this issue doesn't meet our\n"

tests/unit/test_webhook_model.py

@@ -114,6 +114,25 @@ def test_comment_public_uri(mock_mr):
     assert str(issue.number) in uri
 
 
+@patch('webcompat.webhooks.model.make_request')
+def test_comment_closed_reason(mock_mr):
+    """Test comment API request that is sent to GitHub."""
+    mock_mr.return_value.status_code == 200
+    json_event, signature = event_data('private_issue_opened.json')
+    payload = json.loads(json_event)
+    issue = WebHookIssue.from_dict(payload)
+    reasons = ['invalid', 'incomplete']
+    for reason in reasons:
+        issue.comment_closed_reason(reason)
+    method, uri, data = mock_mr.call_args[0]
+    # make sure we sent a post with the right data to GitHub
+    assert method == 'post'
+    assert reason in data['body'].lower()
+    assert str(issue.get_public_issue_number()) in uri
+    with pytest.raises(ValueError):
+        issue.comment_closed_reason('boring garbage')
+
+
 @patch('webcompat.webhooks.model.make_request')
 def test_moderate_public_issue(mock_mr):
     """Test issue state and API request that is sent to GitHub."""
@@ -211,6 +230,54 @@ def test_prepare_accepted_issue(mock_priority):
     assert expected == actual
 
 
+@patch('webcompat.webhooks.helpers.extract_priority_label')
+def test_prepare_accepted_issue(mock_priority):
+    """Test the payload preparation for accepted: invalid moderated issues.
+    """
+    mock_priority.return_value = 'priority-critical'
+    json_event, signature = event_data('private_milestone_accepted.json')
+    payload = json.loads(json_event)
+    issue = WebHookIssue.from_dict(payload)
+    actual = issue.prepare_accepted_issue('invalid')
+    expected = {
+        'body': '<!-- @browser: Firefox 55.0 -->\n'
+        '<!-- @ua_header: Mozilla/5.0 (X11; Linux x86_64; rv:55.0) '
+        'Gecko/20100101 Firefox/55.0 -->\n'
+        '<!-- @reported_with: web -->\n'
+        '<!-- @public_url: '
+        'https://github.com/webcompat/webcompat-tests/issues/1  -->\n'
+        '\n'
+        '**URL**: https://www.netflix.com/',
+        'labels': ['browser-firefox', 'priority-critical', 'engine-gecko'],
+        'title': 'www.netflix.com - test private issue accepted',
+        'milestone': 8, 'state': 'closed'}
+    assert expected == actual
+
+
+@patch('webcompat.webhooks.helpers.extract_priority_label')
+def test_prepare_accepted_issue(mock_priority):
+    """Test the payload preparation for accepted: invalid moderated issues.
+    """
+    mock_priority.return_value = 'priority-critical'
+    json_event, signature = event_data('private_milestone_accepted.json')
+    payload = json.loads(json_event)
+    issue = WebHookIssue.from_dict(payload)
+    actual = issue.prepare_accepted_issue('incomplete')
+    expected = {
+        'body': '<!-- @browser: Firefox 55.0 -->\n'
+        '<!-- @ua_header: Mozilla/5.0 (X11; Linux x86_64; rv:55.0) '
+        'Gecko/20100101 Firefox/55.0 -->\n'
+        '<!-- @reported_with: web -->\n'
+        '<!-- @public_url: '
+        'https://github.com/webcompat/webcompat-tests/issues/1  -->\n'
+        '\n'
+        '**URL**: https://www.netflix.com/',
+        'labels': ['browser-firefox', 'priority-critical', 'engine-gecko'],
+        'title': 'www.netflix.com - test private issue accepted',
+        'milestone': 7, 'state': 'closed'}
+    assert expected == actual
+
+
 @patch('webcompat.webhooks.model.make_request')
 def test_process_issue_action_scenarios(mock_mr):
     """Test we are getting the right response for each scenario."""

webcompat/issues.py

@@ -30,7 +30,7 @@
 INCOMPLETE_BODY = '''<p>Thanks for the report. Unfortunately without any
 detail about the issue you experienced, we cannot help with this bug.
 Please leave a comment with more detail, or file a new report and we will
-gladly investigate this further.</p>'''
+gladly investigate this further. Closing the issue as Incomplete.</p>'''
 ONGOING_TITLE = 'In the moderation queue.'
 ONGOING_BODY = '''<p>This issue has been put in the moderation queue. A human
 will review if the message meets our current
@@ -45,7 +45,7 @@
 guidelines. Its original content has been deleted.</p>'''
 
 
-def moderation_template(choice='ongoing', title=None):
+def moderation_template(choice='ongoing'):
     """Gets the placeholder data to send for unmoderated issues.
 
     The moderation is for now these types:
@@ -60,14 +60,10 @@ def moderation_template(choice='ongoing', title=None):
         title = REJECTED_TITLE
         body = REJECTED_BODY
     elif choice == 'invalid':
-        if not title:
-            raise ValueError("A title must be passed in for invalid issues")
-        title = title
+        title = ''
         body = INVALID_BODY
     elif choice == 'incomplete':
-        if not title:
-            raise ValueError("A title must be passed in for incomplete issues")
-        title = title
+        title = ''
         body = INCOMPLETE_BODY
     else:
         title = ONGOING_TITLE

webcompat/webhooks/helpers.py

@@ -209,44 +209,6 @@ def msg_log(msg, issue_number):
     log.info(msg)
 
 
-def prepare_incomplete_issue(title=None):
-    """Create the payload for the incomplete moderated issue.
-
-    When the issue has been moderated as "accepted:incomplete",
-    we need to change a couple of things in the public space
-
-    - change body
-    - close the issue
-    - remove the action-needsmoderation label
-    - change the milestone to invalid
-    """
-    # Extract the relevant information
-    incomplete_id = app.config['STATUSES']['incomplete']['id']
-    payload_request = moderation_template('incomplete', title)
-    payload_request['state'] = 'closed'
-    payload_request['milestone'] = incomplete_id
-    return payload_request
-
-
-def prepare_invalid_issue(title=None):
-    """Create the payload for the invalid moderated issue.
-
-    When the issue has been moderated as "accepted:invalid",
-    we need to change a couple of things in the public space
-
-    - change body
-    - close the issue
-    - remove the action-needsmoderation label
-    - change the milestone to invalid
-    """
-    # Extract the relevant information
-    invalid_id = app.config['STATUSES']['invalid']['id']
-    payload_request = moderation_template('invalid', title)
-    payload_request['state'] = 'closed'
-    payload_request['milestone'] = invalid_id
-    return payload_request
-
-
 def prepare_rejected_issue():
     """Create the payload for the rejected moderated issue.
 

webcompat/webhooks/model.py

@@ -21,10 +21,9 @@
 from webcompat.webhooks.helpers import make_request
 from webcompat.webhooks.helpers import msg_log
 from webcompat.webhooks.helpers import oops
-from webcompat.webhooks.helpers import prepare_incomplete_issue
-from webcompat.webhooks.helpers import prepare_invalid_issue
 from webcompat.webhooks.helpers import prepare_rejected_issue
 from webcompat.webhooks.helpers import repo_scope
+from webcompat.issues import moderation_template
 
 PUBLIC_REPO = app.config['ISSUES_REPO_URI']
 PRIVATE_REPO = app.config['PRIVATE_REPO_URI']
@@ -84,6 +83,17 @@ def close_private_issue(self):
         else:
             self.state = 'closed'
 
+    def comment_closed_reason(self, reason):
+        """Publish a comment on the public issue about why it was closed."""
+        if reason in ['invalid', 'incomplete']:
+            comment = moderation_template(reason).get('body')
+        else:
+            raise ValueError("reason must be one of invalid or incomplete")
+        payload = {'body': comment}
+        issue_number = self.get_public_issue_number()
+        path = f'repos/{PUBLIC_REPO}/{issue_number}/comments'
+        make_request('post', path, payload)
+
     def comment_public_uri(self):
         """Publish a comment on the private issue with the public uri."""
         comment = self.prepare_public_comment()
@@ -114,7 +124,7 @@ def moderate_private_issue(self):
         path = f'repos/{PUBLIC_REPO}/{public_number}'
         make_request('patch', path, payload_request)
 
-    def prepare_accepted_issue(self):
+    def prepare_accepted_issue(self, milestone=None):
         """Create the payload for the accepted moderated issue.
 
         When the issue has been moderated as accepted,
@@ -136,6 +146,10 @@ def prepare_accepted_issue(self):
             'title': self.title,
             'body': self.body
         }
+        if milestone:
+            milestone_id = app.config['STATUSES'][f'{milestone}']['id']
+            payload_request['milestone'] = milestone_id
+            payload_request['state'] = 'closed'
         return payload_request
 
     def prepare_public_comment(self):
@@ -154,9 +168,9 @@ def close_public_issue(self, reason='rejected'):
         'rejected' (default)
         """
         if reason == 'incomplete':
-            payload_request = prepare_incomplete_issue(self.title)
+            payload_request = self.prepare_accepted_issue('incomplete')
         elif reason == 'invalid':
-            payload_request = prepare_invalid_issue(self.title)
+            payload_request = self.prepare_accepted_issue('invalid')
         else:
             payload_request = prepare_rejected_issue()
         public_number = self.get_public_issue_number()
@@ -262,7 +276,9 @@ def process_issue_action(self):
                     self.number)
                 return oops()
             else:
-                # we didn't get exceptions, so it's safe to close it
+                # we didn't get exceptions, so it's safe to comment why
+                # it was closed as incomplete, and close it.
+                self.comment_closed_reason(reason='incomplete')
                 self.close_private_issue()
                 return make_response('Moderated issue closed as incomplete',
                                      200)
@@ -279,7 +295,9 @@ def process_issue_action(self):
                     self.number)
                 return oops()
             else:
-                # we didn't get exceptions, so it's safe to close it
+                # we didn't get exceptions, so it's safe to comment why
+                # it was closed as invalid, and close it.
+                self.comment_closed_reason(reason='invalid')
                 self.close_private_issue()
                 return make_response('Moderated issue closed as invalid', 200)
         elif (scope == 'private' and self.action == 'closed' and