1380804 - Integrate Pocket trending stories API

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, enhancement, P1)

Description

[Chenxia Liu [:liuche]]

The API docs for the pocket trending stories can be found on google drive. 

We will use the "/global-recs" endpoint to display global trending stories

For a Pocket item, we'll need url, title, image_src, domain fields.
The results should be cached on disk for at least 3 hours. 

Different clients will require a different number of Pocket stories. The pocket API lets you specify how many stories to fetch.

Make sure the API key is NOT COMMITTED by accident. And for testing only use the development API key. (We can look at how the keys for LeanPlum are added.)

The JSON parsing of the API response should be tested. You can save a copy of the json returned from the API in the test folder and you can use that to test that the json parsing works correctly.

Comment 1

[Chenxia Liu [:liuche]]

Attachment: Bug 1380804: Access Pocket API through loader.

Review commit: https://reviewboard.mozilla.org/r/164166/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/164166/

Comment 2

[Chenxia Liu [:liuche]]

Splitting off bug 1386906 to do the work to add Pocket keys to the build system.

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8893162 [details]
Bug 1380804: Access Pocket API through loader.

https://reviewboard.mozilla.org/r/164166/#review170074

lgtm!

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/topstories/PocketStoriesLoader.java:6
(Diff revision 1)
> +/* -*- Mode: Java; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*-
> + * This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +package org.mozilla.gecko.activitystream.homepanel.topstories;

Add this file to moz.build.

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/topstories/PocketStoriesLoader.java:26
(Diff revision 1)
> +import java.net.HttpURLConnection;
> +import java.net.URI;
> +import java.net.URISyntaxException;
> +import java.util.Locale;
> +
> +public class PocketStoriesLoader extends AsyncTaskLoader<String> {

nit: class comment. please include why you used a loader over other options and what the return value is (a json String).

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/topstories/PocketStoriesLoader.java:39
(Diff revision 1)
> +    // Pocket API params and defaults
> +    private static final String GLOBAL_ENDPOINT = "https://getpocket.com/v3/firefox/global-recs";
> +    private static final String PARAM_APIKEY = "consumer_key";
> +    private static final String APIKEY = "KEY_PLACEHOLDER"; // TODO: Add keys to builders
> +    private static final String PARAM_COUNT = "count";
> +    private static final String DEFAULT_COUNT = "20";

super-nit: I'd prefer this to be an int we convert to a String so we don't accidentally change it to be a non-integer value if we update it.

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/topstories/PocketStoriesLoader.java:42
(Diff revision 1)
> +    private static final String APIKEY = "KEY_PLACEHOLDER"; // TODO: Add keys to builders
> +    private static final String PARAM_COUNT = "count";
> +    private static final String DEFAULT_COUNT = "20";
> +    private static final String PARAM_LOCALE = "locale_lang";
> +
> +    private static final long REFRESH_INTERVAL_MILLIS = 3 * 60 * 60 * 1000; // 3 hours

nit: `TimeUnit.HOURS.toMillis(3);`

Here & below.

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/topstories/PocketStoriesLoader.java:46
(Diff revision 1)
> +
> +    private static final long REFRESH_INTERVAL_MILLIS = 3 * 60 * 60 * 1000; // 3 hours
> +
> +    private static final int BUFFER_SIZE = 2048;
> +    private static final int CONNECT_TIMEOUT = 15000;
> +    private static final int READ_TIMEOUT = 10000;

I wonder if these values are short (e.g. on a slow connection). Maybe we should use something closer to the sync values (30s connection, 2m read): https://dxr.mozilla.org/mozilla-central/rev/52285ea5e54c73d3ed824544cef2ee3f195f05e6/mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/BaseResourceDelegate.java#18

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/topstories/PocketStoriesLoader.java:62
(Diff revision 1)
> +
> +    @Override
> +    protected void onStartLoading() {
> +        // Check timestamp to determine if we have cached stories.
> +        final long previousTime = sharedPreferences.getLong(CACHE_TIMESTAMP_MILLIS_PREFIX + localeLang, 0);
> +        if (System.currentTimeMillis() - previousTime > REFRESH_INTERVAL_MILLIS) {

This won't properly handle a client manually changing their clock times. That's totally fine because this is not a time-sensitive operation, but perhaps we should add a comment to that effect.

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/topstories/PocketStoriesLoader.java:91
(Diff revision 1)
> +                .appendQueryParameter(PARAM_LOCALE, localeLang)
> +                .build();
> +        try {
> +            connection = (HttpURLConnection) ProxySelector.openConnectionWithProxy(new URI(uri.toString()));
> +
> +            connection.setInstanceFollowRedirects(true);

Why do we follow redirects? Add a comment.

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8893162 [details]
Bug 1380804: Access Pocket API through loader.

https://reviewboard.mozilla.org/r/164166/#review170074

Another thought: we'll also need to think about how we should distribute Pocket API keys for local development.

Comment 5

[Chenxia Liu [:liuche]]

Comment on attachment 8893162 [details]
Bug 1380804: Access Pocket API through loader.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164166/diff/1-2/

Comment 6

[Chenxia Liu [:liuche]]

(In reply to Michael Comella (:mcomella) from comment #4)
> Comment on attachment 8893162 [details]
> Bug 1380804: Access Pocket API through loader.
> 
> https://reviewboard.mozilla.org/r/164166/#review170074
> 
> Another thought: we'll also need to think about how we should distribute
> Pocket API keys for local development.

My plan is to turn it off for local builds, and you can update your mozconfig with both a flag and a path to the key. That way, if you don't have the key, we can omit the Pocket sections. Maybe if we have contributors who work on Pocket, we could ask Pocket for specific keys, or just have them monitor them for reasonable use.

Comment 7

[Chenxia Liu [:liuche]]

Comment on attachment 8893162 [details]
Bug 1380804: Access Pocket API through loader.

https://reviewboard.mozilla.org/r/164166/#review170124

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/topstories/PocketStoriesLoader.java:46
(Diff revision 1)
> +
> +    private static final long REFRESH_INTERVAL_MILLIS = 3 * 60 * 60 * 1000; // 3 hours
> +
> +    private static final int BUFFER_SIZE = 2048;
> +    private static final int CONNECT_TIMEOUT = 15000;
> +    private static final int READ_TIMEOUT = 10000;

I grabbed these numbers from the FeedFetcher file, which is a similar use case since we're only fetching a json string of 20 entries, which is at most a few kB.

iirc Sync deals with much much bigger blobs (than 20-item JSON arrays), but we can keep an eye on this.

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/topstories/PocketStoriesLoader.java:91
(Diff revision 1)
> +                .appendQueryParameter(PARAM_LOCALE, localeLang)
> +                .build();
> +        try {
> +            connection = (HttpURLConnection) ProxySelector.openConnectionWithProxy(new URI(uri.toString()));
> +
> +            connection.setInstanceFollowRedirects(true);

Good call, I just lifted this from FeedFetcher, but we don't need to follow redirects at all!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f8ddd8a3b8d8
Access Pocket API through loader. r=mcomella

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Chenxia Liu [:liuche] from comment #6)
> My plan is to turn it off for local builds,

With it turned off, I'm concerned that a dev may change something that seems unrelated but breaks Pocket feeds and it won't be obvious to them. I wonder if we should 1) use dummy data for local builds (using similar code paths sans network request) or 2) write a UI test to ensure Pocket is working as expected.

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/f8ddd8a3b8d8

Comment 11

[Chenxia Liu [:liuche]]

(In reply to Michael Comella (:mcomella) from comment #9)
> (In reply to Chenxia Liu [:liuche] from comment #6)
> > My plan is to turn it off for local builds,
> 
> With it turned off, I'm concerned that a dev may change something that seems
> unrelated but breaks Pocket feeds and it won't be obvious to them. I wonder
> if we should 1) use dummy data for local builds (using similar code paths
> sans network request) or 2) write a UI test to ensure Pocket is working as
> expected.

Good call - I'll add dummy data as part of bug 1378995.