Skip to content

Fixes #3187 - Update Python dependencies#3191

Merged
miketaylr merged 5 commits intomasterfrom
issues/3187/1
Feb 11, 2020
Merged

Fixes #3187 - Update Python dependencies#3191
miketaylr merged 5 commits intomasterfrom
issues/3187/1

Conversation

@miketaylr
Copy link
Copy Markdown
Member

Let's not land this before landing the current big batch of changes.

self.assertEqual(get_browser_name(FIREFOX_MOBILE_UA_OLD),
'firefox mobile')
self.assertEqual(get_browser_name(FIREFOX_TABLET_UA),
'firefox mobile (tablet)')
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably figure out why this changed, and if we care.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FIREFOX_TABLET_UA = 'Mozilla/5.0 (Android 4.4; Tablet; rv:41.0) Gecko/41.0 Firefox/41.0'  # noqa

which is definitely a tablet. I'm not sure why we get firefox mobile (tablet) instead of just firefox tablet but that's probably a separate issue.

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, str):
# 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"

ok

        return get_browser(user_agent_string).rsplit(' ', 1)[0].lower()

Let's go see what get_browser returns.

def get_browser(user_agent_string=None):
"""Return browser name family and version.
It will pre-populate the bug reporting form.
"""
if user_agent_string and isinstance(user_agent_string, str):
ua_dict = user_agent_parser.Parse(user_agent_string)
ua = ua_dict.get('user_agent')
name = get_name(ua)
# if browser is unknown, we don't need further details
if name == "Unknown":
return "Unknown"
version = get_version_string(ua)
# Check for tablet devices
if ua_dict.get('device').get('model') == 'Tablet':
model = '(Tablet) '
else:
model = ''
rv = '{0} {1}{2}'.format(name, model, version).rstrip()
return rv
return "Unknown"

  • UA parser: 0.8
{
    'user_agent': {
        'family': 'Firefox Mobile', 
        'major': '41', 
        'minor': '0', 
        'patch': None
    }, 
    'os': {
        'family': 'Android', 
        'major': '4', 
        'minor': '4', 
        'patch': None, 
        'patch_minor': None
        }, 
    'device': {
        'family': 'Generic Tablet', 
        'brand': 'Generic', 
        'model': 'Tablet'
        }, 
    'string': 'Mozilla/5.0 (Android 4.4; Tablet; rv:41.0) Gecko/41.0 Firefox/41.0'
    }
  • UA parser: 0.9
{
    'user_agent': {
        'family': 'Firefox Mobile', 
        'major': '41', 
        'minor': '0', 
        'patch': None
        }, 
    'os': {
        'family': 'Android', 
        'major': '4', 
        'minor': '4', 
        'patch': None, 
        'patch_minor': None
        }, 
    'device': {
        'family': 'rv:41.0', 
        'brand': 'Generic_Android', 
        'model': 'rv:41.0'
        }, 
    'string': 'Mozilla/5.0 (Android 4.4; Tablet; rv:41.0) Gecko/41.0 Firefox/41.0'
    }

Yoohoo… that device is wow… Probably a bug in user_agent_parser.Parse(user_agent_string)

We could freeze to 0.8 then file a bug on the project. and when they fix it we can upgrade.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/ua-parser/uap-python/issues, I'll check today if i can do a pull request on ua_parser

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

# This is for dev purpose
-r requirements.txt
mock==3.0.5
mock==4.0.1
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mock needs to be removed.
see #3157

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

coooool 👍

Copy link
Copy Markdown
Member

@karlcow karlcow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@miketaylr a couple of things to adjust.

Thanks for doing the updates.

self.assertEqual(get_browser_name(FIREFOX_MOBILE_UA_OLD),
'firefox mobile')
self.assertEqual(get_browser_name(FIREFOX_TABLET_UA),
'firefox mobile (tablet)')
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FIREFOX_TABLET_UA = 'Mozilla/5.0 (Android 4.4; Tablet; rv:41.0) Gecko/41.0 Firefox/41.0'  # noqa

which is definitely a tablet. I'm not sure why we get firefox mobile (tablet) instead of just firefox tablet but that's probably a separate issue.

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, str):
# 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"

ok

        return get_browser(user_agent_string).rsplit(' ', 1)[0].lower()

Let's go see what get_browser returns.

def get_browser(user_agent_string=None):
"""Return browser name family and version.
It will pre-populate the bug reporting form.
"""
if user_agent_string and isinstance(user_agent_string, str):
ua_dict = user_agent_parser.Parse(user_agent_string)
ua = ua_dict.get('user_agent')
name = get_name(ua)
# if browser is unknown, we don't need further details
if name == "Unknown":
return "Unknown"
version = get_version_string(ua)
# Check for tablet devices
if ua_dict.get('device').get('model') == 'Tablet':
model = '(Tablet) '
else:
model = ''
rv = '{0} {1}{2}'.format(name, model, version).rstrip()
return rv
return "Unknown"

  • UA parser: 0.8
{
    'user_agent': {
        'family': 'Firefox Mobile', 
        'major': '41', 
        'minor': '0', 
        'patch': None
    }, 
    'os': {
        'family': 'Android', 
        'major': '4', 
        'minor': '4', 
        'patch': None, 
        'patch_minor': None
        }, 
    'device': {
        'family': 'Generic Tablet', 
        'brand': 'Generic', 
        'model': 'Tablet'
        }, 
    'string': 'Mozilla/5.0 (Android 4.4; Tablet; rv:41.0) Gecko/41.0 Firefox/41.0'
    }
  • UA parser: 0.9
{
    'user_agent': {
        'family': 'Firefox Mobile', 
        'major': '41', 
        'minor': '0', 
        'patch': None
        }, 
    'os': {
        'family': 'Android', 
        'major': '4', 
        'minor': '4', 
        'patch': None, 
        'patch_minor': None
        }, 
    'device': {
        'family': 'rv:41.0', 
        'brand': 'Generic_Android', 
        'model': 'rv:41.0'
        }, 
    'string': 'Mozilla/5.0 (Android 4.4; Tablet; rv:41.0) Gecko/41.0 Firefox/41.0'
    }

Yoohoo… that device is wow… Probably a bug in user_agent_parser.Parse(user_agent_string)

We could freeze to 0.8 then file a bug on the project. and when they fix it we can upgrade.

self.assertEqual(get_browser_name(FIREFOX_MOBILE_UA_OLD),
'firefox mobile')
self.assertEqual(get_browser_name(FIREFOX_TABLET_UA),
'firefox mobile (tablet)')
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/ua-parser/uap-python/issues, I'll check today if i can do a pull request on ua_parser

self.assertEqual(get_browser_name(FIREFOX_MOBILE_UA_OLD),
'firefox mobile')
self.assertEqual(get_browser_name(FIREFOX_TABLET_UA),
'firefox mobile (tablet)')
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@miketaylr
Copy link
Copy Markdown
Member Author

miketaylr commented Feb 10, 2020

I guess we would probably want to freeze the uap module, so to not introduce a regression.
(ah yes, I see you already wrote this :))

Thanks for filing a bug, @karlcow

@miketaylr
Copy link
Copy Markdown
Member Author

Ah, I forgot to pip uninstall mock that's why tests were passing for me locally.

@karlcow
Copy link
Copy Markdown
Member

karlcow commented Feb 10, 2020

I guess we would probably want to freeze the uap module, so to not introduce a regression.

yes. :)

Copy link
Copy Markdown
Member

@karlcow karlcow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @miketaylr

@karlcow
Copy link
Copy Markdown
Member

karlcow commented Feb 11, 2020

I guess we would probably want to freeze the uap module, so to not introduce a regression.
(ah yes, I see you already wrote this :))

Thanks for filing a bug, @karlcow

ua-parser/uap-core@v0.6.4...master Probably changes relative to kaiOS

@miketaylr miketaylr marked this pull request as ready for review February 11, 2020 22:13
@miketaylr
Copy link
Copy Markdown
Member Author

OK, ready to land now that v24.0.0 is shipping to production.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants