Skip to content
This repository has been archived by the owner on Apr 3, 2019. It is now read-only.

fix(devices): handle new user agent string from Sync client lib #1909

Merged
merged 1 commit into from
May 25, 2017

Conversation

philbooth
Copy link
Contributor

@philbooth philbooth commented May 24, 2017

Fixes #1889.

Adds handling for user agent strings from the new Sync mobile library.

I was torn between adding a third distinct regex to lib/userAgent, thereby creating an ugly chain of if-then-else-if ugliness, or consolidating all of our debt regexes into a single ultra-regex. Here you can see the ultra-regex approach in all it's glory: it starts off okay but then somewhere round the middle becomes completely impenetrable, in the customary style of all great regexes.

I'm happy to revert to separate expressions and explicit conditions if people find this one too unreadable.

There is one requirement from the linked issue that is not addressed here. Quoting from it:

  • Form factor (on Android, "Mobile" or "Tablet"; on iOS, device name instead)
    ...

Which can be parsed to the initial device name:

<Application-name>, <OS> <form factor>

When we synthesize the device name, I'm not including the form factor as it stands. This is because we coerce the form factor to either 'mobile' or 'tablet' and have logic in the content server that hangs off that distinction. So the raw form factor from the user agent string, e.g. iPhone 6S, is not available to us at the moment.

We could add an extra column to the devices table, say uaFormFactor, but that's a bigger change involving other repos and probably not worth blocking this PR on. Even without that part of the device name in place, this PR succeeds at the main aim of parsing the new user agent string. I'll open a separate issue for incorporating form factor in to the device name.

@mozilla/fxa-devs r?

/cc @mcomella

@philbooth
Copy link
Contributor Author

I've opened #1910 to cover incorporating the form factor into the synthesized device name.

// $3 = application version
// $4 = form factor
// $5 = OS version
// $6 = application name
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes me pine for named groups in JavaScript's regexp 😭

const SYNC_IOS_USER_AGENT = /^Firefox-iOS-FxA\/([^\sb]+)b\S+ \(.+; iPhone OS (\S+)\)/
const ua = require('node-uap')

const MOBILE_OS_FAMILIES = new Set([
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 for Set

@philbooth
Copy link
Contributor Author

Thanks @seanmonstar!

@philbooth philbooth merged commit 009428e into master May 25, 2017
@philbooth philbooth deleted the phil/issue-1889 branch May 25, 2017 07:10
@rfk rfk added this to the FxA-0: quality milestone Jun 1, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants