Skip to content
This repository has been archived by the owner on Feb 29, 2020. It is now read-only.

feat(pocket): Use geo and locale to determine when to enable Pocket #3021

Merged
merged 1 commit into from
Jul 31, 2017

Conversation

Mardak
Copy link
Member

@Mardak Mardak commented Jul 30, 2017

Fix #3004. r?@k88hudson This cleans up AS.jsm a little bit to not have a separate SECTIONS and just inlines it to the rest of FEEDS_DATA. Also sorts things alphabetically.

The main core change is that prefs can have a valueFunc that will compute a value based on geo/locale. If geo isn't available, it'll observe the pref and update the pref config as well as updating the default.

This should also greatly simplify things for #2776.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 86.407% when pulling f071f67 on Mardak:gh3004-dynamic into 8c7a4d9 on mozilla:master.

stories_endpoint: "https://getpocket.com/v3/firefox/global-recs?consumer_key=$apiKey&locale_lang=$locale",
stories_referrer: "https://getpocket.com/recommendations",
survey_link: "https://www.surveymonkey.com/r/newtabffx",
topics_endpoint: "https://getpocket.com/v3/firefox/trending-topics?consumer_key=$apiKey&locale_lang=$locale"
Copy link
Member Author

@Mardak Mardak Jul 30, 2017

Choose a reason for hiding this comment

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

I suppose with valueFunc, at some point we could just insert args.locale directly into these endpoints instead of having TopStoriesFeed separately get the locale and insert it from #2991. @csadilek

Theoretically valueFunc could be called when locale changes too (it doesn't do it in this PR), but that also means TopStoriesFeed wouldn't need to watch for locale changes as its prefs will just update.

And if Pocket wants a client-provided geo (in addition to its own server computed geo from IP address), this would be a natural place to insert it too. E.g., this would support detecting, Firefox says it's a CA/Canada geo, but the current IP address is in San Francisco, so maybe this user is traveling.

});

// Configure default Activity Stream prefs with a plain `value` or a `valueFunc`
// that computes a value. A `value_local_dev` is used for development defaults.
Copy link
Member Author

Choose a reason for hiding this comment

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

k88: I would probably name this getValue, but up to you
Sure.

Copy link
Member Author

@Mardak Mardak left a comment

Choose a reason for hiding this comment

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

k88: R+ with nits, looks good to me, just a couple of naming-related comments. The prefs stuff could use some updated documentation, and possibly some refactoring for clarity in the future

@Mardak Mardak merged commit f52f501 into mozilla:master Jul 31, 2017
@Mardak Mardak deleted the gh3004-dynamic branch July 31, 2017 18:14
@coveralls
Copy link

Coverage Status

Coverage remained the same at 86.407% when pulling 574fe01 on Mardak:gh3004-dynamic into 535f5d7 on mozilla:master.

@as-pine-proxy
Copy link
Collaborator

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants