Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #1030. More robust handling for UA parsing methods. #1032

Merged
merged 9 commits into from
May 2, 2016
Merged

Fixes #1030. More robust handling for UA parsing methods. #1032

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e376f1f
Issue #1030. Make get_browser and get_browser_name tests more robust.
Apr 29, 2016
8bd0a46
Issue #1030. Be sure to pass in browser_name when showing form errors.
Apr 29, 2016
cbeff0f
Issue #1030. Rewrite get_browser_name.
Apr 29, 2016
b8105b9
Issue #1030. Rewrite get_os.
Apr 29, 2016
43e1198
Issue #1030. Rewrite get_browser.
Apr 29, 2016
d066fc6
Issue #1030. Be more paranoid and make sure we have a string before p…
Apr 29, 2016
850098b
Issue #1030. Update docstrings for tests.
Apr 29, 2016
554b6cf
Issue #1030. Add Fennec UA string with Android version to tests.
May 2, 2016
ea6d265
Issue #1030. Address review nits.
May 2, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
105 changes: 74 additions & 31 deletions tests/test_helpers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,14 @@

import webcompat
from webcompat.helpers import format_link_header
from webcompat.helpers import get_browser_name
from webcompat.helpers import get_browser
from webcompat.helpers import get_os
from webcompat.helpers import normalize_api_params
from webcompat.helpers import parse_link_header
from webcompat.helpers import rewrite_and_sanitize_link
from webcompat.helpers import rewrite_links
from webcompat.helpers import sanitize_link
from webcompat.helpers import get_browser_name
from webcompat.helpers import get_browser


ACCESS_TOKEN_LINK = '<https://api.github.com/repositories/17839063/issues?per_page=50&page=3&access_token=12345>; rel="next", <https://api.github.com/repositories/17839063/issues?access_token=12345&per_page=50&page=4>; rel="last", <https://api.github.com/repositories/17839063/issues?per_page=50&access_token=12345&page=1>; rel="first", <https://api.github.com/repositories/17839063/issues?per_page=50&page=1&access_token=12345>; rel="prev"' # nopep8
Expand All @@ -30,12 +31,15 @@
REWRITTEN_ISSUES_LINK_HEADER = '</api/issues?per_page=50&page=3>; rel="next", </api/issues?per_page=50&page=4>; rel="last", </api/issues?per_page=50&page=1>; rel="first", </api/issues?per_page=50&page=1>; rel="prev"' # nopep8
REWRITTEN_SEARCH_LINK_HEADER = '</api/issues/search?q=taco&page=2>; rel="next", </api/issues/search?q=taco&page=26>; rel="last"' # nopep8
PARSED_LINKED_HEADERS = [{'link': 'https://api.github.com/repositories/17839063/issues?per_page=50&page=3', 'rel': 'next'}, {'link': 'https://api.github.com/repositories/17839063/issues?per_page=50&page=4', 'rel': 'last'}, {'link': 'https://api.github.com/repositories/17839063/issues?per_page=50&page=1', 'rel': 'first'}, {'link': 'https://api.github.com/repositories/17839063/issues?per_page=50&page=1', 'rel': 'prev'}] # nopep8
NON_TABLET_UA = "Mozilla/5.0 (Android; Mobile; rv:40.0) Gecko/40.0 Firefox/40.0" # nopep8
TABLET_UA = "Mozilla/5.0 (Android 4.4; Tablet; rv:41.0) Gecko/41.0 Firefox/41.0" # nopep8
PARSED_NON_TABLET_BROWSER_NAME = "firefox mobile"
PARSED_TABLET_BROWSER_NAME = "firefox mobile tablet"
PARSED_NON_TABLET_BROWSER = "Firefox Mobile 40.0"
PARSED_TABLET_BROWSER = "Firefox Mobile 41.0 (Tablet)"
FIREFOX_UA = 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:48.0) Gecko/20100101 Firefox/48.0' # nopep8
FIREFOX_MOBILE_UA = 'Mozilla/5.0 (Android; Mobile; rv:40.0) Gecko/40.0 Firefox/40.0' # nopep8

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

FIREFOX_TABLET_UA = 'Mozilla/5.0 (Android 4.4; Tablet; rv:41.0) Gecko/41.0 Firefox/41.0' # nopep8
SAFARI_UA = 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11) AppleWebKit/601.1.39 (KHTML, like Gecko) Version/9.0 Safari/601.1.39' # nopep8
SAFARI_MOBILE_UA = 'Mozilla/5.0 (iPhone; CPU iPhone OS 6_1_4 like Mac OS X) AppleWebKit/536.26 (KHTML, like Gecko) Version/6.0 Mobile/10B350 Safari/8536.25' # nopep8
SAFARI_TABLET_UA = 'Mozilla/5.0 (iPad; CPU OS 5_1_1 like Mac OS X) AppleWebKit/534.46 (KHTML, like Gecko) Version/5.1 Mobile/9B206 Safari/7534.48.3' # nopep8
CHROME_UA = 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/52.0.2720.0 Safari/537.36' # nopep8
CHROME_MOBILE_UA = 'Mozilla/5.0 (Linux; Android 4.0.4; Galaxy Nexus Build/IMM76B) AppleWebKit/535.19 (KHTML, like Gecko) Chrome/18.0.1025.133 Mobile Safari/535.19' # nopep8
CHROME_TABLET_UA = 'Mozilla/5.0 (Linux; Android 4.0.4; Galaxy Nexus Build/IMM76B) AppleWebKit/535.19 (KHTML, like Gecko) Chrome/18.0.1025.133 Safari/535.19' # nopep8


class TestHelpers(unittest.TestCase):
Expand Down Expand Up @@ -117,29 +121,68 @@ def test_format_http_link_headers(self):
link_header = GITHUB_ISSUES_LINK_HEADER
self.assertEqual(format_link_header(parsed_headers), link_header)

def test_get_browser_name_Tablet(self):
'''Test Browser name parsing for tablet devices.'''
user_agent = TABLET_UA
parsed_browser_name = PARSED_TABLET_BROWSER_NAME
self.assertEqual(get_browser_name(user_agent), parsed_browser_name)

def test_get_browser_name_Non_Tablet(self):
'''Test Browser name parsing for non-tablet devices.'''
user_agent = NON_TABLET_UA
parsed_browser_name = PARSED_NON_TABLET_BROWSER_NAME
self.assertEqual(get_browser_name(user_agent), parsed_browser_name)

def test_get_browser_Tablet(self):
'''Test Browser parsing for tablet devices.'''
user_agent = TABLET_UA
parsed_browser = PARSED_TABLET_BROWSER
self.assertEqual(get_browser(user_agent), parsed_browser)

def test_get_browser_Non_Tablet(self):
'''Test Browser parsing for non-tablet devices.'''
user_agent = NON_TABLET_UA
parsed_browser = PARSED_NON_TABLET_BROWSER
self.assertEqual(get_browser(user_agent), parsed_browser)
def test_get_browser_name(self):
'''Test browser name parsing via get_browser_name helper method.'''
self.assertEqual(get_browser_name(FIREFOX_UA), 'firefox')
self.assertEqual(get_browser_name(FIREFOX_MOBILE_UA), 'firefox mobile')
self.assertEqual(get_browser_name(FIREFOX_TABLET_UA),
'firefox mobile (tablet)')
self.assertEqual(get_browser_name(SAFARI_UA), 'safari')
self.assertEqual(get_browser_name(SAFARI_MOBILE_UA), 'mobile safari')
self.assertEqual(get_browser_name(SAFARI_TABLET_UA), 'mobile safari')
self.assertEqual(get_browser_name(CHROME_UA), 'chrome')
self.assertEqual(get_browser_name(CHROME_MOBILE_UA), 'chrome mobile')
self.assertEqual(get_browser_name(CHROME_TABLET_UA), 'chrome')
self.assertEqual(get_browser_name(''), 'unknown')
self.assertEqual(get_browser_name(None), 'unknown')
self.assertEqual(get_browser_name(), 'unknown')
self.assertEqual(get_browser_name(u'💀'), 'unknown')
self.assertEqual(get_browser('<script>lol()</script>'), 'Unknown')
self.assertEqual(get_browser(True), 'Unknown')
self.assertEqual(get_browser(False), 'Unknown')
self.assertEqual(get_browser(None), 'Unknown')

def test_get_browser(self):
'''Test browser parsing via get_browser helper method.'''
self.assertEqual(get_browser(FIREFOX_UA), 'Firefox 48.0')
self.assertEqual(get_browser(FIREFOX_MOBILE_UA), 'Firefox Mobile 40.0')
self.assertEqual(get_browser(FIREFOX_TABLET_UA),
'Firefox Mobile (Tablet) 41.0')
self.assertEqual(get_browser(SAFARI_UA), 'Safari 9.0')
self.assertEqual(get_browser(SAFARI_MOBILE_UA), 'Mobile Safari 6.0')
self.assertEqual(get_browser(SAFARI_TABLET_UA), 'Mobile Safari 5.1')
self.assertEqual(get_browser(CHROME_UA), 'Chrome 52.0.2720')
self.assertEqual(get_browser(CHROME_MOBILE_UA),
'Chrome Mobile 18.0.1025')
self.assertEqual(get_browser(CHROME_TABLET_UA), 'Chrome 18.0.1025')
self.assertEqual(get_browser(''), 'Unknown')
self.assertEqual(get_browser(), 'Unknown')
self.assertEqual(get_browser(u'💀'), 'Unknown')
self.assertEqual(get_browser('<script>lol()</script>'), 'Unknown')
self.assertEqual(get_browser(True), 'Unknown')
self.assertEqual(get_browser(False), 'Unknown')
self.assertEqual(get_browser(None), 'Unknown')

def test_get_os(self):
'''Test OS parsing via get_os helper method.'''
self.assertEqual(get_os(FIREFOX_UA), 'Mac OS X 10.11')
self.assertEqual(get_os(FIREFOX_MOBILE_UA), 'Android')
self.assertEqual(get_os(FIREFOX_TABLET_UA), 'Android 4.4')
self.assertEqual(get_os(SAFARI_UA), 'Mac OS X 10.11')
self.assertEqual(get_os(SAFARI_MOBILE_UA), 'iOS 6.1.4')
self.assertEqual(get_os(SAFARI_TABLET_UA), 'iOS 5.1.1')
self.assertEqual(get_os(CHROME_UA), 'Mac OS X 10.11.4')
self.assertEqual(get_os(CHROME_MOBILE_UA),
'Android 4.0.4')
self.assertEqual(get_os(CHROME_TABLET_UA), 'Android 4.0.4')
self.assertEqual(get_os(''), 'Unknown')
self.assertEqual(get_os(), 'Unknown')
self.assertEqual(get_os(u'💀'), 'Unknown')
self.assertEqual(get_os('<script>lol()</script>'), 'Unknown')
self.assertEqual(get_os(True), 'Unknown')
self.assertEqual(get_os(False), 'Unknown')
self.assertEqual(get_os(None), 'Unknown')


if __name__ == '__main__':
unittest.main()
101 changes: 56 additions & 45 deletions webcompat/helpers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,61 +99,72 @@ def get_user_info():
session['avatar_url'] = gh_user.get('avatar_url')


def get_browser_name(user_agent_string):
'''Return just the browser name.

unknown user agents will be reported as "other".
'''
ua_dict = user_agent_parser.Parse(user_agent_string)
name = ua_dict.get('user_agent').get('family').lower()
device = ua_dict.get('device').get('model')
if (name == 'firefox mobile' and
ua_dict.get('os').get('family') == 'Firefox OS'):
name = 'other'
if device == 'Tablet':
name += " " + device.lower()
return name


def get_browser(user_agent_string):
def get_browser(user_agent_string=None):
'''Return browser name family and version.

It will pre-populate the bug reporting form.
'''
ua_dict = user_agent_parser.Parse(user_agent_string)
ua = ua_dict.get('user_agent')
name = ua.get('family')
version = ua.get('major', u'Unknown')
# Add on the minor and patch version numbers if they exist
if version != u'Unknown' and ua.get('minor'):
version = version + "." + ua.get('minor')
if ua.get('patch'):
version = version + "." + ua.get('patch')
else:
version = ''
# Check for tablet devices
if ua_dict.get('device').get('model') == 'Tablet':
model = ' (Tablet)'
else:
model = ''
return '{0} {1}{2}'.format(name, version, model)
if user_agent_string and isinstance(user_agent_string, basestring):
ua_dict = user_agent_parser.Parse(user_agent_string)
ua = ua_dict.get('user_agent')
name = ua.get('family')

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

version = ua.get('major', u'Unknown')
# Add on the minor and patch version numbers if they exist
if version != u'Unknown' and ua.get('minor'):
version = version + "." + ua.get('minor')
if ua.get('patch'):
version = version + "." + ua.get('patch')
else:
version = ''
# Check for tablet devices
if ua_dict.get('device').get('model') == 'Tablet':
model = '(Tablet) '
else:
model = ''
rv = '{0} {1}{2}'.format(name, model, version)
# bizarre UA strings can be parsed like so:
# {'major': None, 'minor': None, 'family': 'Other', 'patch': None}
# but we want to return "Unknown", rather than "Other"
print(rv)

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

if rv.strip().lower() == "other":
return "Unknown"
return rv
return "Unknown"


def get_browser_name(user_agent_string=None):
'''Return just the browser name.

unknown user agents will be reported as "unknown".
'''
if user_agent_string and isinstance(user_agent_string, basestring):
# get_browser will return something like 'Chrome Mobile 47.0'
# we just want 'chrome mobile', i.e., the lowercase name
# w/o the version
return get_browser(user_agent_string).rsplit(' ', 1)[0].lower()
return "unknown"


def get_os(user_agent_string):
def get_os(user_agent_string=None):
'''Return operating system name.

It pre-populates the bug reporting form.
'''
ua_dict = user_agent_parser.Parse(user_agent_string)
os = ua_dict.get('os')
version = os.get('major', u'Unknown')
if version != u'Unknown' and os.get('major'):
version = version + "." + os.get('minor')
if os.get('patch'):
version = version + "." + os.get('patch')
else:
version = ''
return '{0} {1}'.format(os.get('family'), version)
if user_agent_string and isinstance(user_agent_string, basestring):
ua_dict = user_agent_parser.Parse(user_agent_string)
os = ua_dict.get('os')
version = os.get('major', u'Unknown')
if version != u'Unknown' and os.get('major'):

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

version = version + "." + os.get('minor')
if os.get('patch'):
version = version + "." + os.get('patch')
else:
version = ''
rv = '{0} {1}'.format(os.get('family'), version).rstrip()
if rv.strip().lower() == "other":
return "Unknown"
return rv
return "Unknown"


def get_response_headers(response):
Expand Down
4 changes: 3 additions & 1 deletion webcompat/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@ def index():
ua_header = request.headers.get('User-Agent')
bug_form.browser.data = get_browser(ua_header)
bug_form.os.data = get_os(ua_header)
# browser_name is used in topbar.html to show the right add-on link
browser_name = get_browser_name(ua_header)
# GET means you want to file a report.
if request.method == 'GET':
Expand All @@ -151,7 +152,8 @@ def index():

else:
# Validation failed, re-render the form with the errors.
return render_template('index.html', form=bug_form)
return render_template('index.html', form=bug_form,
browser=browser_name)


@app.route('/issues')
Expand Down