1440768 - Improve saved to pocket stories caching and updating

Status: RESOLVED FIXED

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

Description

[Ursula Sarracini (:ursula)]

Some improvements on the way we fetch/cache saved to pocket stories so that we can poll the server less:

1. We want to check if a user has not had any activity in over a week, and if so, don't even bother hitting the server to retrieve their feed... we assume that user is inactive. This check will happen regularly (either at a regular interval when we expire the cache, or when we expire the cache via pocket related events that happen on the new tab e.g archiving a saved pocket story)

2. Increase polling time from 1 hour to 1 day so we expire cache and force a server hit once a day rather than once an hour (assuming they are logged in/an active user)

3. Invalidate the cache also if the last time we got new pocket stories (which activity stream got via getHighlights) surpasses the last time a user interacted with pocket (via the pref 'latestSince'). This check will happen on every activity stream SYSTEM_TICK event.

Comment 1

[Ursula Sarracini (:ursula)]

Attachment: Bug 1440768 - Improve saved to pocket stories caching and updating

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

Comment 2

[Ed Lee :Mardak]

Comment on attachment 8953638 [details]
Bug 1440768 - Improve saved to pocket stories caching and updating

https://reviewboard.mozilla.org/r/222808/#review228818

Need to fix up the logic for short circuiting fetch if latest is non-existent/0.

::: toolkit/modules/NewTabUtils.jsm:992
(Diff revision 1)
>    /**
>     * Helper function which makes the call to the Pocket API to fetch the user's
>     * saved Pocket items.
>     */
>    fetchSavedPocketItems(requestData) {
> -    if (!pktApi.isUserLoggedIn()) {
> +    const latestSince = (Services.prefs.getStringPref(PREF_POCKET_LATEST_SINCE, 0) * 1000);

This is actually going to cause wrong behavior as a missing latestSince makes it seem like it was active really long ago.

Maybe…
```js
latestSince = getStringPref(…, 0) * 1000 || Infinity;
```

::: toolkit/modules/NewTabUtils.jsm:996
(Diff revision 1)
>    fetchSavedPocketItems(requestData) {
> -    if (!pktApi.isUserLoggedIn()) {
> +    const latestSince = (Services.prefs.getStringPref(PREF_POCKET_LATEST_SINCE, 0) * 1000);
> +    const aWeekAgo = Date.now() - (7 * 24 * 60 * 60 * 1000);
> +
> +    // Do not fetch Pocket items for users that have been inactive for over 1 week or are not logged in
> +    if (!pktApi.isUserLoggedIn() || aWeekAgo > latestSince) {

nit: let's do the same style as your other time check with a `const`:
```js
if (… || (Date.now() - latestSince > POCKET_INACTIVE_TIME))
```

::: toolkit/modules/NewTabUtils.jsm:1465
(Diff revision 1)
>        results.push(...await ActivityStreamProvider.getRecentBookmarks(aOptions));
>      }
>  
>      // Add the Pocket items if we need more and want them
>      if (aOptions.numItems - results.length > 0 && !aOptions.excludePocket) {
> -      // If we do not have saved to Pocket stories already cached, or it has been
> +      const latestSince = Services.prefs.getStringPref(PREF_POCKET_LATEST_SINCE, 0);

nit: this results in a string, but we should just explicitly cast/~~ especially because we do `<` below

::: toolkit/modules/NewTabUtils.jsm:1468
(Diff revision 1)
>      // Add the Pocket items if we need more and want them
>      if (aOptions.numItems - results.length > 0 && !aOptions.excludePocket) {
> -      // If we do not have saved to Pocket stories already cached, or it has been
> -      // more than 1 hour since we last got Pocket stories, invalidate the cache,
> -      // get new stories, and update the timestamp
> -      if (!this._savedPocketStories || (Date.now() - this._pocketLastUpdated > POCKET_UPDATE_TIME)) {
> +      const latestSince = Services.prefs.getStringPref(PREF_POCKET_LATEST_SINCE, 0);
> +      // Invalidate the cache, get new stories, and update timestamps if:
> +      //  1. we do not have saved to Pocket stories already cached OR
> +      //  2. it has been more than 1 day since we last got Pocket stories OR

nit: don't refer to the const's current value as that might change. "it's been too long since…"

Comment 3

[Ursula Sarracini (:ursula)]

Comment on attachment 8953638 [details]
Bug 1440768 - Improve saved to pocket stories caching and updating

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

Comment 4

[Ed Lee :Mardak]

Comment on attachment 8953638 [details]
Bug 1440768 - Improve saved to pocket stories caching and updating

https://reviewboard.mozilla.org/r/222808/#review229292

::: toolkit/modules/NewTabUtils.jsm:993
(Diff revision 2)
>    /**
>     * Helper function which makes the call to the Pocket API to fetch the user's
>     * saved Pocket items.
>     */
>    fetchSavedPocketItems(requestData) {
> -    if (!pktApi.isUserLoggedIn()) {
> +    const latestSince = (parseInt(Services.prefs.getStringPref(PREF_POCKET_LATEST_SINCE, 0), 10) * 1000);

nit: no need for the `parseInt` here as we'll cast it to a number with `*`

::: toolkit/modules/NewTabUtils.jsm:995
(Diff revision 2)
>     * saved Pocket items.
>     */
>    fetchSavedPocketItems(requestData) {
> -    if (!pktApi.isUserLoggedIn()) {
> +    const latestSince = (parseInt(Services.prefs.getStringPref(PREF_POCKET_LATEST_SINCE, 0), 10) * 1000);
> +
> +    // Do not fetch Pocket items for users that have been inactive for over 1 week or are not logged in

nit: this comment still has the const's value in it

::: toolkit/modules/NewTabUtils.jsm:1465
(Diff revision 2)
>        results.push(...await ActivityStreamProvider.getRecentBookmarks(aOptions));
>      }
>  
>      // Add the Pocket items if we need more and want them
>      if (aOptions.numItems - results.length > 0 && !aOptions.excludePocket) {
> -      // If we do not have saved to Pocket stories already cached, or it has been
> +      const latestSince = Services.prefs.getStringPref(PREF_POCKET_LATEST_SINCE, 0);

nit: this is the assignment that is concerning that should be casted: `latestSince = ~~getStringPref(…)`

Basically, making sure this number-looking thing is actually a number when we assign it so that we don't get confused when we do other operations on the variabble later.

Comment 5

[Ed Lee :Mardak]

(In reply to Ed Lee :Mardak from comment #2)
> > +    const latestSince = (Services.prefs.getStringPref(PREF_POCKET_LATEST_SINCE, 0) * 1000);
> This is actually going to cause wrong behavior as a missing latestSince
> makes it seem like it was active really long ago.
> Maybe…
> latestSince = getStringPref(…, 0) * 1000 || Infinity;
Just to wrap up this discussion from IRC, we'll treat a missing latestSince as really long ago as we're assuming the more common behavior of a user logging in (anonymous account) and never pocketing from Firefox (resulting in an updated latestSince) behavior to avoid hitting network vs probably less common pattern of a pocket user who logged via website and never pocketed from the Firefox UI.

Comment 6

[Ursula Sarracini (:ursula)]

Comment on attachment 8953638 [details]
Bug 1440768 - Improve saved to pocket stories caching and updating

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/11a9aa48a220
Improve saved to pocket stories caching and updating r=Mardak

Comment 8

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/11a9aa48a220