Fixes #1938: Add "extra labels" in new issue webhook

tests/test_webhook.py

@@ -50,11 +50,13 @@ def setUp(self):
         <!-- @browser: Firefox 55.0 -->
         <!-- @ua_header: Mozilla/5.0 (what) Gecko/20100101 Firefox/55.0 -->
         <!-- @reported_with: web -->
+        <!-- @extra_label: type-media -->
 
         **URL**: https://www.example.com/
         **Browser / Version**: Firefox 55.0
         <!-- @browser: Chrome 48.0 -->
         """
+
         self.issue_body2 = """
         <!-- @browser: Foobar -->
         """
@@ -131,17 +133,47 @@ def test_fails_on_not_known_action(self):
         self.assertEqual(rv.mimetype, 'text/plain')
         self.assertEqual(rv.data, 'Not an interesting hook')
 
+    def test_extract_metadata(self):
+        expected = {'reported_with': 'web', 'extra_label': 'type-media',
+            'ua_header': 'Mozilla/5.0 (what) Gecko/20100101 Firefox/55.0',
+            'browser': 'Firefox 55.0'}
+        actual = helpers.extract_metadata(self.issue_body)
+        self.assertEqual(expected, actual)
+
     def test_extract_browser_label(self):
         """Extract browser label name."""
-        browser_label = helpers.extract_browser_label(self.issue_body)
-        self.assertEqual(browser_label, 'browser-firefox')
-        browser_label_none = helpers.extract_browser_label(self.issue_body2)
-        self.assertEqual(browser_label_none, None)
-        browser_label_paren = helpers.extract_browser_label(self.issue_body3)
-        self.assertEqual(browser_label_paren, 'browser-firefox-mobile-tablet')
-        browser_label_unicode = helpers.extract_browser_label(self.issue_body4)
-        self.assertEqual(
-            browser_label_unicode, 'browser-firefox-mobile-tablet')
+        metadata_tests = [
+            ({'browser': 'Firefox'}, None),
+            ({'browser': 'Firefox Mobile'}, None),
+            ({'browser': 'Firefox99.0'}, None),
+            ({'browser': 'Firefox (tablet)'}, None),
+            ({'browser': 'Firefox 30.0'}, 'browser-firefox'),
+            ({'browser': 'Firefox Mobile 30.0'}, 'browser-firefox-mobile'),
+            ({'browser': 'Firefox Mobile (Tablet) 88.0'}, 'browser-firefox-mobile-tablet')
+        ]
+        for metadata_dict, expected in metadata_tests:
+            actual = helpers.extract_browser_label(metadata_dict)
+            self.assertEqual(expected, actual)
+
+    def test_extract_extra_label(self):
+        """Extract 'extra' label."""
+        metadata_tests = [
+            ({'extra_label': 'type-media'}, 'type-media'),
+            ({'burgers': 'french fries'}, None)
+        ]
+        for metadata_dict, expected in metadata_tests:
+            actual = helpers.extract_extra_label(metadata_dict)
+            self.assertEqual(expected, actual)
+
+    def test_extract_priority_label(self):
+        """Extract priority label."""
+        with patch('webcompat.db.site_db.query') as db_mock:
+            db_mock.return_value.filter_by.return_value = [
+                Site('google.com', 1, '', 1)]
+            priority_label = helpers.extract_priority_label(self.issue_body3)
+            self.assertEqual(priority_label, 'priority-critical')
+        priority_label_none = helpers.extract_priority_label(self.issue_body)
+        self.assertEqual(priority_label_none, None)
 
     def test_is_github_hook(self):
         """Validation tests for GitHub Webhooks."""
@@ -196,16 +228,6 @@ def test_get_issue_info(self):
         actual = helpers.get_issue_info(payload)
         self.assertDictEqual(expected, actual)
 
-    def test_extract_priority_label(self):
-        """Extract priority label."""
-        with patch('webcompat.db.site_db.query') as db_mock:
-            db_mock.return_value.filter_by.return_value = [
-                Site('google.com', 1, '', 1)]
-            priority_label = helpers.extract_priority_label(self.issue_body3)
-            self.assertEqual(priority_label, 'priority-critical')
-        priority_label_none = helpers.extract_priority_label(self.issue_body)
-        self.assertEqual(priority_label_none, None)
-
 
 if __name__ == '__main__':
     unittest.main()

webcompat/form.py

@@ -210,9 +210,13 @@ def build_formdata(form_object):
     else:
         summary = '{0} - {1}'.format(normalized_url, problem_summary)
 
+    metadata_keys = ['browser', 'ua_header', 'reported_with']
+    extra_label = form_object.get('extra_label', None)
+    if extra_label in app.config['EXTRA_LABELS']:
+        metadata_keys.append('extra_label')
+
     formdata = {
-        'metadata': get_metadata(('browser', 'ua_header', 'reported_with'),
-                                 form_object),
+        'metadata': get_metadata(metadata_keys, form_object),
         'url': form_object.get('url'),
         'browser': form_object.get('browser'),
         'os': form_object.get('os'),
@@ -245,7 +249,4 @@ def build_formdata(form_object):
     # Append "from webcompat.com" message to bottom (for GitHub issue viewers)
     body += u'\n\n{0}'.format(GITHUB_HELP)
     rv = {'title': summary, 'body': body}
-    extra_label = form_object.get('label', None)
-    if extra_label and extra_label in app.config['EXTRA_LABELS']:
-        rv.update({'labels': [extra_label]})
     return rv

webcompat/views.py

@@ -198,7 +198,7 @@ def create_issue():
             return redirect(url_for('index'))
     form['ua_header'] = request.headers.get('User-Agent')
     form['reported_with'] = session.pop('src', 'web')
-    form['label'] = session.pop('label', None)
+    form['extra_label'] = session.pop('label', None)
     # Logging the ip and url for investigation
     log = app.logger
     log.setLevel(logging.INFO)

webcompat/webhooks/helpers.py

@@ -17,20 +17,27 @@
 from webcompat.helpers import proxy_request
 
 
-def extract_browser_label(body):
-    """Parse the labels from the body in comment:
+def extract_metadata(body):
+    """Parse all the hidden comments for issue metadata
 
-    <!-- @browser: value -->.
-    Currently this only handles a single label,
-    because that's all that we set in webcompat.com.
+    <!-- @foo: bar -->.
+    Returns a dict with all such comments as members,
+    with foo, bar as key, value.
     """
-    match_list = re.search(r'<!--\s@(\w+):\s([^\d]+?)\s[\d\.]+\s-->', body)
-    if match_list:
-        # perhaps we do something more interesting depending on
-        # what groups(n)[0] is in the future.
-        # right now, match_list.groups(0) should look like:
-        # ('browser', 'firefox')
-        browser = match_list.groups(0)[1].lower()
+    match_list = re.findall(r'<!--\s@(\w+):\s(.+)\s-->', body)
+    # Reverse the list before creating a dict, because we want to keep
+    # the first instance (in case there are duplicates added by the user after)
+    metadata_dict = dict(reversed(match_list))
+    return metadata_dict
+
+
+def extract_browser_label(metadata_dict):
+    """Return the browser label from metadata_dict."""
+    browser = metadata_dict.get('browser', None)
+    # Only proceed if browser looks like "FooBrowser 99.0"
+    if browser and re.search(r'([^\d]+?)\s[\d\.]+', browser):
+        browser = browser.lower()
+        browser = browser.rsplit(' ', 1)[0]
         browser = browser.encode('utf-8')
         browser = browser.translate(None, '()')
         dash_browser = '-'.join(browser.split())
@@ -39,6 +46,35 @@ def extract_browser_label(body):
         return None
 
 
+def extract_extra_label(metadata_dict):
+    """Return the 'extra' label from metadata_dict"""
+    extra_label = metadata_dict.get('extra_label', None)
+    if extra_label:
+        extra_label = extra_label.lower()
+        extra_label = extra_label.encode('utf-8')
+    return extra_label
+
+
+def extract_priority_label(body):
+    """Parse url from body and query the priority labels."""
+    hostname = domain_name(extract_url(body))
+    if hostname:
+        priorities = ['critical', 'important', 'normal']
+        # Find host_name in DB
+        for site in site_db.query(Site).filter_by(url=hostname):
+            return 'priority-{}'.format(priorities[site.priority - 1])
+        # No host_name in DB, find less-level domain (>2)
+        # If host_name is lv4.lv3.example.com, find lv3.example.com/example.com
+        subparts = hostname.split('.')
+        domains = ['.'.join(subparts[i:])
+                   for i, subpart in enumerate(subparts)
+                   if 0 < i < hostname.count('.')]
+        for domain in domains:
+            for site in site_db.query(Site).filter_by(url=domain):
+                return 'priority-{}'.format(priorities[site.priority - 1])
+    return None
+
+
 def update_issue(payload, issue_number):
     """Does a GitHub PATCH request to set labels and milestone for the issue.
 
@@ -88,26 +124,6 @@ def get_payload_signature(key, payload):
     return mac.hexdigest()
 
 
-def extract_priority_label(body):
-    """Parse url from body and query the priority labels."""
-    hostname = domain_name(extract_url(body))
-    if hostname:
-        priorities = ['critical', 'important', 'normal']
-        # Find host_name in DB
-        for site in site_db.query(Site).filter_by(url=hostname):
-            return 'priority-{}'.format(priorities[site.priority - 1])
-        # No host_name in DB, find less-level domain (>2)
-        # If host_name is lv4.lv3.example.com, find lv3.example.com/example.com
-        subparts = hostname.split('.')
-        domains = ['.'.join(subparts[i:])
-                   for i, subpart in enumerate(subparts)
-                   if 0 < i < hostname.count('.')]
-        for domain in domains:
-            for site in site_db.query(Site).filter_by(url=domain):
-                return 'priority-{}'.format(priorities[site.priority - 1])
-    return None
-
-
 def is_github_hook(request):
     """Validate the github webhook HTTP POST request."""
     if request.headers.get('X-GitHub-Event') is None:
@@ -136,15 +152,14 @@ def new_opened_issue(payload):
     - Priority label
     - Issue milestone
     '''
-    labels = []
     issue_body = payload.get('issue')['body']
     issue_number = payload.get('issue')['number']
-    browser_label = extract_browser_label(issue_body)
+    metadata_dict = extract_metadata_labels(issue_body)
+    browser_label = extract_browser_label(metadata_dict)
+    extra_label = extract_extra_label(metadata_dict)
     priority_label = extract_priority_label(issue_body)
-    if browser_label:
-        labels.append(browser_label)
-    if priority_label:
-        labels.append(priority_label)
+    labels = [label for label in (browser_label, extra_label, priority_label)
+              if label is not None]
     milestone = app.config['STATUSES']['needstriage']['id']
     return update_issue({'labels': labels, 'milestone': milestone},
                         issue_number)