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

miketaylr · 2017-11-28T22:04:34Z

Tested with this branch deployed to staging:

with &label=type-stylo in params:

https://staging.webcompat.com/issues/1310

without label param (just to prove things still work):

https://staging.webcompat.com/issues/1311

miketaylr · 2017-11-28T22:05:01Z

r? @karlcow
r? @zoepage (to get more familiar with the python code)

karlcow

Some coding suggestions.
There are missing tests.

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 and extra_label in app.config['EXTRA_LABELS']:


webcompat/form.py

    formdata = {
-        'metadata': get_metadata(('browser', 'ua_header', 'reported_with'),
-                                 form_object),
+        'metadata': get_metadata(metadata_keys, form_object),


webcompat/webhooks/helpers.py

-        labels.append(priority_label)
+    for label in (browser_label, extra_label, priority_label):
+        if label:
+            labels.append(label)


miketaylr · 2017-11-29T03:39:54Z

There are missing tests.

Ha yeah, just came here to comment that I still have tests to write. :)

Thanks for review so far!

karlcow · 2017-11-29T03:50:57Z

huh… I had a full comment about changing the strategy on helpers.py and it's not there. huh.

karlcow

An additional comment I thought I had made in the previous Review. I was sure I did it. I must have forgotten to add… :(

Anyway.

webcompat/webhooks/helpers.py

+    Currently this only handles a single label,
+    because that's all that we set in webcompat.com.
+    """
+    match_list = re.search(r'<!--\s@extra_label:\s([\w-]+)\s-->', body)


miketaylr · 2017-11-29T19:44:14Z

OK, there's a few things I don't love about this, will comment inline. But it works, yay?

tests/test_webhook.py

        self.assertEqual(browser_label, 'browser-firefox')
-        browser_label_none = helpers.extract_browser_label(self.issue_body2)
+        browser_label_none = helpers.extract_browser_label(
+            helpers.extract_metadata(self.issue_body2))


webcompat/webhooks/helpers.py

+    match_list = re.findall(r'<!--\s@(\w+):\s(.+)\s-->', body)
+    # Build a dict but only for the first seen instance of a key
+    # (in case there are duplicates added by the user after)
+    metadata_dict = {}


webcompat/webhooks/helpers.py

+    return metadata_dict
+
+
+def extract_browser_label(metadata_dict):


miketaylr · 2017-11-29T19:53:33Z

r? @karlcow

webcompat/webhooks/helpers.py

+    """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):


karlcow

We are still missing tests extract_metadata.
The code did it by chaining but that's not good for unittests :) If in the future one of the two functions fail, it will be harder to discover which one.

and see more suggestions and trimming code where it's possible.

We are getting closer.

webcompat/webhooks/helpers.py

+    match_list = re.findall(r'<!--\s@(\w+):\s(.+)\s-->', body)
+    # Build a dict but only for the first seen instance of a key
+    # (in case there are duplicates added by the user after)
+    metadata_dict = {}


webcompat/webhooks/helpers.py

+        return extra_label.encode('utf-8')
+    else:
+        return None
+


webcompat/webhooks/helpers.py

+    """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):


tests/test_webhook.py

@@ -133,15 +139,41 @@ def test_fails_on_not_known_action(self):

    def test_extract_browser_label(self):
        """Extract browser label name."""
-        browser_label = helpers.extract_browser_label(self.issue_body)
+        browser_label = helpers.extract_browser_label(
+            helpers.extract_metadata(self.issue_body))


tests/test_webhook.py

        self.assertEqual(browser_label, 'browser-firefox')
-        browser_label_none = helpers.extract_browser_label(self.issue_body2)
+        browser_label_none = helpers.extract_browser_label(
+            helpers.extract_metadata(self.issue_body2))


tests/test_webhook.py

+        self.assertEqual(extra_label, 'type-media')
+        extra_label_none = helpers.extract_extra_label(
+            helpers.extract_metadata(self.issue_body2))
+        self.assertEqual(extra_label_none, None)


This adds an extract_metadata method which the other helpers use.

…tion methods/tests.

miketaylr · 2017-11-30T20:27:38Z

OK, round 3. r? @karlcow

karlcow

ok let's do it.

Thanks for the modifications mike.

karlcow · 2017-11-30T22:06:52Z

There is a check fail because of indentation and line length in test_webhook, I'll fix it in a follow up issue.

karlcow · 2017-11-30T22:11:06Z

Thanks a lot @miketaylr

miketaylr · 2017-11-30T22:52:10Z

There is a check fail because of indentation and line length in test_webhook, I'll fix it in a follow up issue.

(I pushed a commit that fixed this on master... sorry about that)

miketaylr added the in progress label Nov 28, 2017

miketaylr requested review from karlcow and zoepage November 28, 2017 22:04

karlcow requested changes Nov 29, 2017

View reviewed changes

Issue #1938. Pass along GET param label as issue metadata.

d9270d8

miketaylr force-pushed the issues/1938/1 branch from da3823b to 8094eac Compare November 29, 2017 19:43

miketaylr commented Nov 29, 2017

View reviewed changes

webcompat/webhooks/helpers.py Outdated

return metadata_dict

def extract_browser_label(metadata_dict):

This comment was marked as abuse.

Sign in to view

miketaylr commented Nov 29, 2017

View reviewed changes

karlcow requested changes Nov 30, 2017

View reviewed changes

miketaylr added 3 commits November 30, 2017 14:26

Issue #1938. Extract the extra label from issue body metadata.

c516370

This adds an extract_metadata method which the other helpers use.

Issue #1938. Move priority_label methods/tests closer to other extrac…

575c17c

…tion methods/tests.

Issue #1938. Update tests.

9681903

miketaylr force-pushed the issues/1938/1 branch from 8094eac to 9681903 Compare November 30, 2017 20:26

karlcow approved these changes Nov 30, 2017

View reviewed changes

karlcow merged commit 4f36857 into master Nov 30, 2017

karlcow removed the in progress label Nov 30, 2017

zoepage deleted the issues/1938/1 branch November 30, 2017 22:13

zoepage added the status: add to changelog label Nov 30, 2017

miketaylr removed the status: add to changelog label Nov 30, 2017

karlcow mentioned this pull request Dec 1, 2017

Filtering out (or not) the version-less browser name in tagging #1949

Open

		return metadata_dict


		def extract_browser_label(metadata_dict):

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

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

Conversation

miketaylr commented Nov 28, 2017

miketaylr commented Nov 28, 2017

karlcow left a comment

Choose a reason for hiding this comment

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

miketaylr commented Nov 29, 2017

karlcow commented Nov 29, 2017

karlcow left a comment

Choose a reason for hiding this comment

This comment was marked as abuse.

This comment was marked as abuse.

miketaylr commented Nov 29, 2017

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

miketaylr commented Nov 29, 2017

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

karlcow left a comment

Choose a reason for hiding this comment

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

miketaylr commented Nov 30, 2017

karlcow left a comment

Choose a reason for hiding this comment

karlcow commented Nov 30, 2017

karlcow commented Nov 30, 2017

miketaylr commented Nov 30, 2017