1425496 - Add Recently Pocketed Items to Highlights

Status: RESOLVED FIXED

(Firefox :: New Tab Page, enhancement, P2)

Description

[Ed Lee :Mardak]

This is related/blocked on weighted highlights, though alternatively we could just treat pocketed items as regular bookmarks, and just insert them into the highlights section.

Note that this will not create a new section.

If you are not logged into Pocket, you just won't see any pocket items.

Comment 1

[Andrei Oprea [:andreio]]

> If you are not logged into Pocket, you just won't see any pocket items.
Brian was saying that even though you are not logged in to Pocket you can still add to pocket via the icon in the URL bar. The items are added to a temp account that is merged when you log in (or create an account).
Recent pocket items and bookmarks should make up the same pool of cards (max 3) and they should be sorted chronologically.

Comment 2

[Ursula Sarracini (:ursula)]

Attachment: Bug 1425496 - Add Recently Pocketed Items to Highlights

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

Comment 3

[Ursula Sarracini (:ursula)]

Comment on attachment 8947162 [details]
Bug 1425496 - Add Recently Pocketed Items to Highlights

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

Comment 4

[Code Review Bot [:reviewbot]]

Comment on attachment 8947162 [details]
Bug 1425496 - Add Recently Pocketed Items to Highlights

https://reviewboard.mozilla.org/r/216932/#review222780


Static analysis found 2 defects in this patch.
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/modules/NewTabUtils.jsm:996
(Diff revision 1)
> +          resolve(data);
> +        },
> +        error(error) {
> +          reject(error);
> +        }
> +      })

Error: Missing semicolon. [eslint: semi]

::: toolkit/modules/tests/xpcshell/test_NewTabUtils.js:550
(Diff revision 1)
> +});
> +
> +add_task(async function getHighlightsWithPocketFailure() {
> +  await setUpActivityStreamTest();
> +
> +  NewTabUtils.activityStreamProvider.fetchSavedPocketItems = function () {

Error: Unexpected space before function parentheses. [eslint: space-before-function-paren]

Comment 5

[Code Review Bot [:reviewbot]]

Comment on attachment 8947162 [details]
Bug 1425496 - Add Recently Pocketed Items to Highlights

https://reviewboard.mozilla.org/r/216932/#review222782


Static analysis found 1 defect in this patch.
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/modules/tests/xpcshell/test_NewTabUtils.js:550
(Diff revision 2)
> +});
> +
> +add_task(async function getHighlightsWithPocketFailure() {
> +  await setUpActivityStreamTest();
> +
> +  NewTabUtils.activityStreamProvider.fetchSavedPocketItems = function () {

Error: Unexpected space before function parentheses. [eslint: space-before-function-paren]

Comment 6

[Ursula Sarracini (:ursula)]

Comment on attachment 8947162 [details]
Bug 1425496 - Add Recently Pocketed Items to Highlights

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

Comment 7

[Code Review Bot [:reviewbot]]

Comment on attachment 8947162 [details]
Bug 1425496 - Add Recently Pocketed Items to Highlights

https://reviewboard.mozilla.org/r/216932/#review222788


Static analysis found 1 defect in this patch.
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/modules/NewTabUtils.jsm:37
(Diff revision 3)
>                      .createInstance(Ci.nsIScriptableUnicodeConverter);
>    converter.charset = "utf8";
>    return converter;
>  });
>  
> +XPCOMUtils.defineLazyModuleGetter(this, "pktApi",

Error: Please use ChromeUtils.defineModuleGetter instead of XPCOMUtils.defineLazyModuleGetter [eslint: mozilla/use-chromeutils-import]

Comment 8

[Ursula Sarracini (:ursula)]

Comment on attachment 8947162 [details]
Bug 1425496 - Add Recently Pocketed Items to Highlights

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/216932/diff/3-4/

Comment 9

[Ursula Sarracini (:ursula)]

Comment on attachment 8947162 [details]
Bug 1425496 - Add Recently Pocketed Items to Highlights

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/216932/diff/4-5/

Comment 10

[Ed Lee :Mardak]

Comment on attachment 8947162 [details]
Bug 1425496 - Add Recently Pocketed Items to Highlights

https://reviewboard.mozilla.org/r/216932/#review222820

I think there needs to be a bit more discussion with design on how we expect to use the options and deduping, etc.

::: toolkit/modules/NewTabUtils.jsm:1004
(Diff revision 5)
> +
> +  /**
> +   * Get the most recently Pocket-ed items from a user's Pocket list.
> +   *
> +   * @param {Object} aOptions
> +   *   {string} detailType: Amount of information about each item to be returned.

I don't quite understand what's expected of `detailType` but we probably want to prefix it with "pocket" to not confuse it with other highlight options.

::: toolkit/modules/NewTabUtils.jsm:1028
(Diff revision 5)
> +     * url, preview image, title, description, and the unique item_id
> +     * necessary for Pocket to identify the item
> +     */
> +    let keys = Array.from(Object.keys(data.list));
> +    let items = [];
> +    for (let item of keys) {

What is the format of `data.list`? It looks like an object with keys that match item_id? `Object.keys` already returns an array, so `Array.from` isn't needed. Actually, maybe just
```js
return Object.values(data.list).map(item => ({
  description: item.excerpt,
  …
});
```

::: toolkit/modules/NewTabUtils.jsm:1037
(Diff revision 5)
> +        resolved_title: title,
> +        resolved_url: url,
> +        item_id,
> +      } = data.list[item];
> +      items.push({
> +        type: "savedToPocket",

I think just plain "pocket" would be good enough?

::: toolkit/modules/NewTabUtils.jsm:1328
(Diff revision 5)
> +  *          The url of the pocket story that was blocked
> +  */
> +  removeFromPocketList(aItemId, aUrl) {
> +    pktApi.deleteItem(aItemId, {
> +      success() {
> +        Services.obs.notifyObservers(null, "newtab-linkBlocked", aUrl);

Is this what we want? The existing notification is for when BlockedLinks gets updated, but that state isn't changing here.

I'm assuming this is for handling the equivalent of "Remove Bookmark". But looking at SAVE_TO_POCKET, it does `Pocket.savePage()` from `PlacesFeed.jsm`. Maybe we should just directly call something from there instead of putting it here?

Additionally, the taking a url and notifying with it seems a bit odd. If we do handle it directly from PlacesFeed, perhaps we would just pass in a callback directly?

::: toolkit/modules/NewTabUtils.jsm:1395
(Diff revision 5)
>     *
>     * @return {Promise} Returns a promise with the array of links as the payload
>     */
>    async getHighlights(aOptions = {}) {
>      aOptions.numItems = aOptions.numItems || ACTIVITY_STREAM_DEFAULT_LIMIT;
> +    aOptions.pocketCount = aOptions.pocketCount || ACTIVITY_STREAM_POCKET_LIMIT;

The `excludePocket` seems unnecessary if we set the default for `pocketCount` to 0. This means none of the existing callers will get pocket, and activity stream can pass in 10 or whatever value we want from the add-on.

Although… we probably do want to keep the various `exclude*` for when we have sub-options for Highlights turning on/off each type. But then again, we could just have HighlightsFeed pass in `pocketCount: 0` (or omit)…

What's the expected use of `pocketCount`? I guess it's somewhat similar to `bookmarkSecondsAgo` but we want a number instead of time here?

Or alternatively, we would also honor `bookmarkSecondsAgo` for pocket items too? E.g., get the most recent 10 and additionally limit to within `bookmarkSecondsAgo`???

::: toolkit/modules/NewTabUtils.jsm:1414
(Diff revision 5)
>      if (aOptions.numItems - results.length > 0 && !aOptions.excludeHistory) {
>        // Use the same numItems as bookmarks above in case we remove duplicates
>        const history = await ActivityStreamProvider.getRecentHistory(aOptions);
>  
>        // Only include a url once in the result preferring the bookmark
>        const bookmarkUrls = new Set(results.map(({url}) => url));

The existing behavior deduped bookmarks and history preferring the bookmark. The new behavior has no deduping between bookmarks and pocket, and similarly prefers bookmark+pocket over a same history item.

Should pocket be preferred over bookmarks over history?

Comment 11

[Ed Lee :Mardak]

uiwanted: Do pocket items need to dedupe against bookmarks? Treated as bookmarks, they'll be preferred over history. Bookmarks don't dedupe against each other already.

How do we determine recent? Some number cutoff? Last 3 days like bookmarks? Use the same 3 days as bookmarks?

A pocket item should have a "Remove from Pocket"-type meatball menu item? Should it also have a Bookmark item if not bookmarked?

Comment 12

[Ursula Sarracini (:ursula)]

Comment on attachment 8947162 [details]
Bug 1425496 - Add Recently Pocketed Items to Highlights

https://reviewboard.mozilla.org/r/216932/#review222820

> I don't quite understand what's expected of `detailType` but we probably want to prefix it with "pocket" to not confuse it with other highlight options.

so the detailType is either "complete" or "simple" and it's the amount of information about the item. We need to use "complete" because the preview_image_url doesn't get returned to us with the "simple" call. Api spec: https://getpocket.com/developer/docs/v3/retrieve

> What is the format of `data.list`? It looks like an object with keys that match item_id? `Object.keys` already returns an array, so `Array.from` isn't needed. Actually, maybe just
> ```js
> return Object.values(data.list).map(item => ({
>   description: item.excerpt,
>   …
> });
> ```

It looks like this: 
```
"list" : {
  "1234" : {
    "description": "..." 
    ...
  }
}
```

So yes, we can do what you suggested up there ^^ :)

> I think just plain "pocket" would be good enough?

Sure, I was originally worried it would conflict with the trending pockets ones but I think those are referred to as "Trending" now so we should be good

> Is this what we want? The existing notification is for when BlockedLinks gets updated, but that state isn't changing here.
> 
> I'm assuming this is for handling the equivalent of "Remove Bookmark". But looking at SAVE_TO_POCKET, it does `Pocket.savePage()` from `PlacesFeed.jsm`. Maybe we should just directly call something from there instead of putting it here?
> 
> Additionally, the taking a url and notifying with it seems a bit odd. If we do handle it directly from PlacesFeed, perhaps we would just pass in a callback directly?

I did want to do something similar to Pocket.savePage but there doesn't exist anything in Pocket.jsm which removes the page... it comes from pktApi. I thought this was a nice easy way to not introduce more actions and just handle it as a block on activity stream's side. All AS needs to know is to update the UI and refresh the feed, which we get from this notification. I was hesistant about this solution, so I'm open to other ideas about this

> The `excludePocket` seems unnecessary if we set the default for `pocketCount` to 0. This means none of the existing callers will get pocket, and activity stream can pass in 10 or whatever value we want from the add-on.
> 
> Although… we probably do want to keep the various `exclude*` for when we have sub-options for Highlights turning on/off each type. But then again, we could just have HighlightsFeed pass in `pocketCount: 0` (or omit)…
> 
> What's the expected use of `pocketCount`? I guess it's somewhat similar to `bookmarkSecondsAgo` but we want a number instead of time here?
> 
> Or alternatively, we would also honor `bookmarkSecondsAgo` for pocket items too? E.g., get the most recent 10 and additionally limit to within `bookmarkSecondsAgo`???

My understanding is that even if we send request with the parameter count: 0, the API will still fetch, and just the response will be different. So in testing we would still be making network requests even with a count of 0. I talked to csadilek and he agreed that that would still happen. I like the exludePocket because it's clear, and it keeps all the highlight types consistent which may help us in the future. It also saves us a network request every 5 minutes or whatever

> The existing behavior deduped bookmarks and history preferring the bookmark. The new behavior has no deduping between bookmarks and pocket, and similarly prefers bookmark+pocket over a same history item.
> 
> Should pocket be preferred over bookmarks over history?

As Aaron mentioned, we're just going to show both for now until weighted highlights can take care of this

Comment 13

[Ed Lee :Mardak]

uifeedback: Don't dedupe bookmarks / pocket stuff now, so treat as separate items. This makes it so only the bookmark card has bookmarkguid to unbookmark, and only the pocket card has item_id to remove pocket.

We want the same recency as bookmarks, so 3 days. If we can't get timestamps for individual pocket items, file a bug against pocket to get it added, and for now put all recent pocket items between bookmarks and history. Ideally bookmarks and pocket items are sorted together by timestamp and placed before history.

All cards including pocket and bookmarked stuff should have both add/remove pocket/bookmark.

Comment 14

[Ed Lee :Mardak]

(In reply to Ursula Sarracini (:ursula) from comment #12)
> I did want to do something similar to Pocket.savePage but there doesn't
> exist anything in Pocket.jsm which removes the page... it comes from pktApi.
Blocking a url and un-pocketing should be two separate actions anyway ?

> I thought this was a nice easy way to not introduce more actions and just
> handle it as a block on activity stream's side.
Oh that reminds me, the other highlight types go through `_processHighlights` which remove blocked links. I guess we should `_processHighlights(results, options, "pocket")`?

> My understanding is that even if we send request with the parameter count:
> 0, the API will still fetch, and just the response will be different.
Given that recency is matching with bookmark's 3 days ago, we shouldn't need to expose a `pocketCount` anymore. We can still pass in a hardcoded value similar to a hardcoded "complete". Although arguably `count` for the request could just be `numItems` from the options ?

> It also saves us a network request every 5 minutes
Just to be clear, for those who don't have `excludePocket`, would it trigger a network request every time we update? Or for users who aren't logged in, it'll short circuit?

Something else I just noticed: do we need to do something special to short circuit `_addFavicons` as we don't want to try getting favicons for every pocket item?

Comment 15

[Ursula Sarracini (:ursula)]

(In reply to Ed Lee :Mardak from comment #14)
> Blocking a url and un-pocketing should be two separate actions anyway ?

What do you think of using the Services.obs.notifyObserver but with a different action name, "newtab-pocketLinkBlocked"? And we can process the event in PlacesFeed.jsm. On AS's side we'll still have an at.REMOVE_FROM_POCKET_LIST action that gets used, but this way we don't have to import pktApi into PlacesFeed.jsm if we already have it in NewTabUtils

> Given that recency is matching with bookmark's 3 days ago, we shouldn't need
> to expose a `pocketCount` anymore. We can still pass in a hardcoded value
> similar to a hardcoded "complete". Although arguably `count` for the request
> could just be `numItems` from the options ?

Yup using numItems for count is good. The api suggests we use a count to cap it at some number, but numItems in combination with last 3 days worth should be good.

> Just to be clear, for those who don't have `excludePocket`, would it trigger
> a network request every time we update? Or for users who aren't logged in,
> it'll short circuit?

My understanding is that even if you're not logged in, you can still save stuff to Pocket... so that means that yes, for those who don't have 'excludePocket' it would trigger a request. 

> Something else I just noticed: do we need to do something special to short
> circuit `_addFavicons` as we don't want to try getting favicons for every
> pocket item?

I thought we only add favicons if we explicitly ask for them using aOptions.withFavicons - which we don't use for highlights. We can filter out the type === "pocket" links before attempting to get favicons to be safe?

Comment 16

[Christian Sadilek [:csadilek]]

Comment on attachment 8947162 [details]
Bug 1425496 - Add Recently Pocketed Items to Highlights

https://reviewboard.mozilla.org/r/216932/#review223078

::: toolkit/modules/NewTabUtils.jsm:1008
(Diff revision 5)
> +   * @param {Object} aOptions
> +   *   {string} detailType: Amount of information about each item to be returned.
> +   *   {int}  pocketCount: Maximum number of Pocket items to return.
> +   */
> +  async getRecentlyPocketed(aOptions) {
> +    const requestData = {

It might make sense to add the "since" parameter i.e. since: this.pocketLastUpdated. This way we'd fetch options.pocketCount items the first time and then only what was added since then.

Comment 17

[Christian Sadilek [:csadilek]]

Comment on attachment 8947162 [details]
Bug 1425496 - Add Recently Pocketed Items to Highlights

https://reviewboard.mozilla.org/r/216932/#review223082

::: toolkit/modules/NewTabUtils.jsm:987
(Diff revision 5)
>  
>    /**
> +   * Helper function which makes the call to the Pocket API to fetch the user's
> +   * saved Pocket items.
> +   */
> +  fetchSavedPocketItems(requestData) {

Just adding, as discussed, that we shouldn't trigger a fetch when the user is not logged in. We can use pktApi.isUserLoggedIn..

Comment 18

[Ursula Sarracini (:ursula)]

Comment on attachment 8947162 [details]
Bug 1425496 - Add Recently Pocketed Items to Highlights

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/216932/diff/5-6/

Comment 19

[Ed Lee :Mardak]

Comment on attachment 8947162 [details]
Bug 1425496 - Add Recently Pocketed Items to Highlights

https://reviewboard.mozilla.org/r/216930/#review223118

Who do we need to talk to about hitting the endpoint? The docs best practices says "After retrieving the list, you should store the current time (which is provided along with the list response) and pass that in the next request for the list. This way the server only needs to return a small set (changes since that time) instead of the user's entire list every time." which would require us to keep state in NewTabUtils.

::: toolkit/modules/NewTabUtils.jsm:69
(Diff revision 6)
>  
>  // Some default seconds ago for Activity Stream recent requests
>  const ACTIVITY_STREAM_DEFAULT_RECENT = 5 * 24 * 60 * 60;
>  
> +// Max age of pocket stories to fetch - 3 days old
> +const ACTIVITY_STREAM_POCKET_AGE = 3 * 24 * 60 * 60;

I don't thing we need to define a new thing. Let's just reuse ACTIVITY_STREAM_DEFAULT_RECENT and bookmarkSecondsAgo

::: toolkit/modules/NewTabUtils.jsm:963
(Diff revision 6)
>    _addFavicons(aLinks) {
>      // Each link in the array needs a favicon for it's page - so we fire off a
>      // promise for each link to compute the favicon data and attach it back to
>      // the original link object. We must wait until all favicons for the array
> -    // of links are computed before returning
> -    return Promise.all(aLinks.map(link => new Promise(async resolve => {
> +    // of links are computed before returning. Never collect favicons for pocket items
> +    return Promise.all(aLinks.filter(link => link.type !== "pocket").map(link => new Promise(async resolve => {

I don't think this is what we want..? This will *remove* pocket items from the result if the option withFavicons is set. Probably should short circuit to return the original link if it's "pocket"

::: toolkit/modules/NewTabUtils.jsm:1014
(Diff revision 6)
> +   */
> +  async getRecentlyPocketed(aOptions) {
> +    const requestData = {
> +      detailType: "complete",
> +      count: aOptions.numItems,
> +      since: ACTIVITY_STREAM_POCKET_AGE

We need to do some math to calculate a timestamp based on now(). And probably allow customizing with bookmarkSecondsAgo..?


Although... I just noticed that we aren't setting bookmarkSecondsAgo from HighlightsFeed. I believe it was originally intended to have Highlights do 3 days or something sooner than 3 days to avoid the default bookmarks, but we ended up avoiding the defaults by checking for visits / frecency.

Would we want to differently look back at 1 day of bookmarks vs 3 days of pocket? Mmmm.. I think reusing the same option should be fine.

.... Or if we're not actually customizing it from activity stream, maybe we get rid of bookmarkSecondsAgo and fix up ACTIVITY_STREAM_DEFAULT_RECENT to be 3 days (oops!? I think it's supposed to be 3 days not 5…)

::: toolkit/modules/NewTabUtils.jsm:1030
(Diff revision 6)
> +    }
> +    /* Extract relevant parts needed to show this card as a highlight:
> +     * url, preview image, title, description, and the unique item_id
> +     * necessary for Pocket to identify the item
> +     */
> +    let items = Object.values(data.list).filter(item => item.status === "0").map(item => ({

Moving the `filter` to a separate line for a comment line explaining the === "0" check would help

::: toolkit/modules/NewTabUtils.jsm:1035
(Diff revision 6)
> +    let items = Object.values(data.list).filter(item => item.status === "0").map(item => ({
> +      description: item.excerpt,
> +      preview_image_url: item.has_image === "1" && item.image.src,
> +      title: item.resolved_title,
> +      url: item.resolved_url,
> +      type: "pocket",

This `type` will be added by `_processHighlights`, so unnecessary here.

::: toolkit/modules/tests/xpcshell/test_NewTabUtils.js:397
(Diff revision 6)
>    });
>  
>    await setUpActivityStreamTest();
>  
>    let provider = NewTabUtils.activityStreamLinks;
> -  let links = await provider.getHighlights();
> +  let links = await provider.getHighlights({excludePocket: true});

Do we actually need to excludePocket for each of these now with the loggedIn check?

Comment 20

[Ursula Sarracini (:ursula)]

Comment on attachment 8947162 [details]
Bug 1425496 - Add Recently Pocketed Items to Highlights

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/216932/diff/6-7/

Comment 21

[Ed Lee :Mardak]

Comment on attachment 8947162 [details]
Bug 1425496 - Add Recently Pocketed Items to Highlights

https://reviewboard.mozilla.org/r/216932/#review223428

::: toolkit/modules/NewTabUtils.jsm:963
(Diff revision 7)
>      // the original link object. We must wait until all favicons for the array
>      // of links are computed before returning
>      return Promise.all(aLinks.map(link => new Promise(async resolve => {
> +      // Never add favicon data for pocket items
> +      if (link.type === "pocket") {
> +        resolve(link);

We need to return early here

Comment 22

[Ursula Sarracini (:ursula)]

Comment on attachment 8947162 [details]
Bug 1425496 - Add Recently Pocketed Items to Highlights

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/216932/diff/7-8/

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/903dd768cc44
Add Recently Pocketed Items to Highlights r=Mardak

Comment 24

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/903dd768cc44

Comment 25

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/activity-stream/pull/3966

Comment 26

[[github robot]]

Commits pushed to master at https://github.com/mozilla/activity-stream

https://github.com/mozilla/activity-stream/commit/eb648c0e8328f291f73f356725cd8933b2d8e486
chore(mc): Backport changes from Bug 1425496

https://github.com/mozilla/activity-stream/commit/340a2faf55e12f52f719414a78f8058c9fd79718
Merge pull request #3966 from sarracini/backport_bug_1425496

chore(mc): Backport changes from Bug 1425496