Bug 1398368: Drop malformed Pocket Top Stories. r=liuche draft
authorMichael Comella <michael.l.comella@gmail.com>
Tue, 12 Sep 2017 16:36:21 -0700
changeset 663394 7e395dfdb6016f3cb9973e5642c8377928c8fa64
parent 663134 a73cc4e08bf5a005722c95b43f84ab0c8ff2bc7c
child 731194 3e23157d2aec9dbb3458a9ac7188462564dd28ed
push id79425
push usermichael.l.comella@gmail.com
push dateTue, 12 Sep 2017 23:36:41 +0000
reviewersliuche
bugs1398368
milestone57.0a1
Bug 1398368: Drop malformed Pocket Top Stories. r=liuche I tested this through the unit tests I added. In theory, we could also validate URLs to make sure they're valid but users should see 404s if they're not valid so this seems like unnecessary code. MozReview-Commit-ID: 3XqsMawLabj
mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/StreamRecyclerAdapter.java
mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/topstories/PocketStoriesLoader.java
mobile/android/tests/background/junit4/src/org/mozilla/gecko/activitystream/homepanel/topstories/TestPocketStoriesLoader.java
--- a/mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/StreamRecyclerAdapter.java
+++ b/mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/StreamRecyclerAdapter.java
@@ -50,17 +50,17 @@ public class StreamRecyclerAdapter exten
     private static final String LOGTAG = StringUtils.safeSubstring("Gecko" + StreamRecyclerAdapter.class.getSimpleName(), 0, 23);
 
     private Cursor topSitesCursor;
     private List<RowModel> recyclerViewModel; // List of item types backing this RecyclerView.
     private List<TopStory> topStoriesQueue;
 
     // Content sections available on the Activity Stream page. These may be hidden if the sections are disabled.
     private final RowItemType[] ACTIVITY_STREAM_SECTIONS = { RowItemType.TOP_PANEL, RowItemType.TOP_STORIES_TITLE, RowItemType.HIGHLIGHTS_TITLE };
-    private final int MAX_TOP_STORIES = 3;
+    public static final int MAX_TOP_STORIES = 3;
 
     private HomePager.OnUrlOpenListener onUrlOpenListener;
     private HomePager.OnUrlOpenInBackgroundListener onUrlOpenInBackgroundListener;
 
     private int tiles;
     private int tilesSize;
 
     public enum RowItemType {
--- a/mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/topstories/PocketStoriesLoader.java
+++ b/mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/topstories/PocketStoriesLoader.java
@@ -12,16 +12,17 @@ import android.support.v4.content.AsyncT
 import android.text.TextUtils;
 import android.util.Log;
 
 import org.json.JSONArray;
 import org.json.JSONException;
 import org.json.JSONObject;
 import org.mozilla.gecko.AppConstants;
 import org.mozilla.gecko.Locales;
+import org.mozilla.gecko.activitystream.homepanel.StreamRecyclerAdapter;
 import org.mozilla.gecko.activitystream.homepanel.model.TopStory;
 import org.mozilla.gecko.util.FileUtils;
 import org.mozilla.gecko.util.ProxySelector;
 
 import java.io.BufferedInputStream;
 import java.io.IOException;
 import java.io.InputStream;
 import java.net.HttpURLConnection;
@@ -57,17 +58,17 @@ public class PocketStoriesLoader extends
     private static final String CACHE_TIMESTAMP_MILLIS_PREFIX = "timestampMillis-";
     private static final String STORIES_CACHE_PREFIX = "storiesCache-";
 
     // Pocket API params and defaults
     private static final String GLOBAL_ENDPOINT = "https://getpocket.cdn.mozilla.net/v3/firefox/global-recs";
     private static final String PARAM_APIKEY = "consumer_key";
     private static final String APIKEY = AppConstants.MOZ_POCKET_API_KEY;
     private static final String PARAM_COUNT = "count";
-    private static final int DEFAULT_COUNT = 3;
+    private static final int DEFAULT_COUNT = StreamRecyclerAdapter.MAX_TOP_STORIES + 1; // We have extra so we can hide malformed items.
     private static final String PARAM_LOCALE = "locale_lang";
 
     private static final long REFRESH_INTERVAL_MILLIS = TimeUnit.HOURS.toMillis(1);
 
     private static final int BUFFER_SIZE = 2048;
     private static final int CONNECT_TIMEOUT = (int) TimeUnit.SECONDS.toMillis(15);
     private static final int READ_TIMEOUT = (int) TimeUnit.SECONDS.toMillis(15);
 
@@ -159,29 +160,38 @@ public class PocketStoriesLoader extends
             final JSONObject jsonObject = new JSONObject(jsonResponse);
             JSONArray arr = jsonObject.getJSONArray("list");
             for (int i = 0; i < arr.length(); i++) {
                 try {
                     final JSONObject item = arr.getJSONObject(i);
                     final String title = item.getString("title");
                     final String url = item.getString("dedupe_url");
                     final String imageUrl = item.getString("image_src");
+
+                    // Drop malformed items.
+                    if (TextUtils.isEmpty(title) ||
+                            TextUtils.isEmpty(url) ||
+                            TextUtils.isEmpty(imageUrl)) {
+                        Log.d(LOGTAG, "Dropping malformed Pocket top story.");
+                        continue;
+                    }
+
                     topStories.add(new TopStory(title, url, imageUrl));
                 } catch (JSONException e) {
                     Log.e(LOGTAG, "Couldn't parse fields in Pocket response", e);
                 }
             }
         } catch (JSONException e) {
             Log.e(LOGTAG, "Couldn't load Pocket response", e);
         }
         return topStories;
     }
 
     private static List<TopStory> makePlaceholderStories() {
         final List<TopStory> stories = new LinkedList<>();
         final String TITLE_PREFIX = "Placeholder ";
-        for (int i = 0; i < 3; i++) {
+        for (int i = 0; i < DEFAULT_COUNT; i++) {
             // Urls must be different for bookmark/pinning UI to work properly. Assume this is true for Pocket stories.
             stories.add(new TopStory(TITLE_PREFIX + i, "https://www.mozilla.org/#" + i, null));
         }
         return stories;
     }
 }
--- a/mobile/android/tests/background/junit4/src/org/mozilla/gecko/activitystream/homepanel/topstories/TestPocketStoriesLoader.java
+++ b/mobile/android/tests/background/junit4/src/org/mozilla/gecko/activitystream/homepanel/topstories/TestPocketStoriesLoader.java
@@ -1,16 +1,15 @@
 /* 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;
 
 import junit.framework.Assert;
-
 import org.json.JSONArray;
 import org.json.JSONException;
 import org.json.JSONObject;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.mozilla.gecko.activitystream.homepanel.model.TopStory;
 import org.mozilla.gecko.background.testhelpers.TestRunner;
 
@@ -77,16 +76,49 @@ public class TestPocketStoriesLoader {
     public void testJSONStringToTopStoriesWithMissingTitle() {
         final JSONObject storyItem = makeBasicStoryItem();
         storyItem.remove(KEY_TITLE);
         final String malformedResponseString = makeBasicPocketResponse(new JSONObject[] { storyItem });
         final List<TopStory> stories = PocketStoriesLoader.jsonStringToTopStories(malformedResponseString);
         Assert.assertEquals(0, stories.size()); // Should skip malformed item.
     }
 
+    @Test
+    public void testJSONStringToTopStoriesDropsMalformedTitle() throws Exception {
+        final JSONObject malformedStory = makeBasicStoryItem();
+        malformedStory.put(KEY_TITLE, "");
+        assertJSONStringToTopStoriesDropsMalformedStory(malformedStory);
+    }
+
+    @Test
+    public void testJSONStringToTopStoriesDropsMalformedURL() throws Exception {
+        final JSONObject malformedStory = makeBasicStoryItem();
+        malformedStory.put(KEY_DEDUPE_URL, "");
+        assertJSONStringToTopStoriesDropsMalformedStory(malformedStory);
+    }
+
+    @Test
+    public void testJSONStringToTopStoriesDropsMalformedImageURL() throws Exception {
+        final JSONObject malformedStory = makeBasicStoryItem();
+        malformedStory.put(KEY_IMAGE_SRC, "");
+        assertJSONStringToTopStoriesDropsMalformedStory(malformedStory);
+    }
+
+    private void assertJSONStringToTopStoriesDropsMalformedStory(final JSONObject malformedStory) throws JSONException {
+        final JSONObject expectedStory = makeBasicStoryItem();
+        final String expectedStoryTitle = "expectedItem";
+        expectedStory.put(KEY_TITLE, expectedStoryTitle);
+
+        final String jsonResponse = makeBasicPocketResponse(new JSONObject[] { expectedStory, malformedStory });
+        final List<TopStory> actualTopStories = PocketStoriesLoader.jsonStringToTopStories(jsonResponse);
+
+        Assert.assertEquals("Expected one malformed item to be removed, leaving size 1", 1, actualTopStories.size());
+        Assert.assertEquals(expectedStoryTitle, actualTopStories.get(0).getTitle());
+    }
+
     // Pulled 8/28 Pocket response, with some trimming for content/brevity.
     final String VALID_POCKET_RESPONSE = "{\"status\":1,\"list\":[" +
             "{\"id\":2910,\"url\":\"https:\\/\\/pocket.co\\/pocket-shorted-url\"," +
             "\"dedupe_url\":\"http:\\/\\/www.bbc.co.uk\\/actual-url\"," +
             "\"title\":\"A Title Here\"," +
             "\"excerpt\":\"Middle-aged people are being urged...\"," +
             "\"domain\":\"bbc.co.uk\"," +
             "\"image_src\":\"https:\\/\\/img.cloudfront.net\"," +