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.

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

Add POS Go pattern #47

Merged
merged 3 commits into from
Nov 17, 2022
Merged

Add POS Go pattern #47

merged 3 commits into from
Nov 17, 2022

Conversation

tiannafischer
Copy link
Contributor

Fixes shopify/boot-to-shopify#1509

Description of change

Adding patterns to identify POS Go devices per this ticket.

Why did you choose this approach?

The browser sniffer uses the user agent to identify the browser, OS, and device.

The browser on POS Go uses a custom user agent that can be identified by the model name WSC6X. This model name has been used to identify user agent strings containing WSC6X as the browser on POS Go.

For more context, see this discussion

What should reviewers focus on?

I'm open to suggestions about how things are named. Names will be returned as follows:

  • Browser name: Shopify POS Go
  • OS name: Shopify Retail OS
  • Device name: Shopify POS Go

The browser name was chosen to make sure that it is easy to identify a login on a POS Go device from https://accounts.shopify.com/accounts/.../security. Logins are displayed here as "Browser name on OS name" (e.g., "Shopify POS Go on Shopify Retail OS"

Before you merge

  • The changes covered by automated tests (e.g. unit, E2E)

@@ -45,6 +45,9 @@ class BrowserSniffer
REGEX_MAP = {
:browser => [
[
# Shopify POS Go
/WSC6X/i

Choose a reason for hiding this comment

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

BBPOS is changing the string to WTH1X in the next ROM so we may want to also match on that string to get ahead of the problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

Choose a reason for hiding this comment

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

They are, but we may not be, since analytics relies on this value as is we were considering hardcoding it to stay the same in the user agent...
Don't remember what decision we made, but until we make the change in our UA or at least test that the ROM level change will actually even effect the value we fetch for the UA...
Let's sync on this.

Choose a reason for hiding this comment

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

@LeoKaliazine Not sure if I follow. The UA identified here will be WSC6X and WTH1X, so there should be no impact on Mode (as the changes there are independent from this one)

Choose a reason for hiding this comment

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

Oh sorry, was focused more on the comment than the code, yeah having both works 👍🏻

@tiannafischer tiannafischer merged commit 1302359 into master Nov 17, 2022
@tiannafischer tiannafischer deleted the add-pos-go-pattern branch November 17, 2022 17:09
@shopify-shipit shopify-shipit bot temporarily deployed to rubygems December 6, 2022 23:01 Inactive
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.

None yet

4 participants