1261376 - 'Undefined' item in the pocket list after 'saving link to pocket' on a Google search result

Status: VERIFIED FIXED

(Firefox :: Pocket, defect)

Description

[Paul Silaghi, QA [:pauly]]

Attachment: save link to pocket - undefined.png

[Affected versions]:
- 46b6

[Affected platforms]:
- all

[Steps to reproduce]:
1. Start FX with a new profile
2. Sign in to Pocket
3. Open google.com and search for "wikipedia"
4. Right click on a search result and click 'Save link to Pocket'
5. Open the Pocket list

[Expected result]:
- "Wikipedia" item should be displayed in the pocket list

[Actual result]:
- "Undefined" item in the pocket list

[Regression range]:
- reproduced on FX 45, 48.0a1 (2016-03-31)

Comment 1

[:Gijs (he/him)]

[Tracking Requested - why for this release]:
Serious functionality issue

Paul, why is this blocking bug 1215694, which didn't land for 45? Did you try to use mozregression to figure out when this broke, or if it's even a regression?

Comment 2

[Paul Silaghi, QA [:pauly]]

Pocked moved to a system add-on in bug 1215694, which landed on FX 46. So, I though all the new bugs should block bug 1215694. The 45 flag is set so people to know this is not a recent regression caused by the Pocket moving.
This is repro on FX 38, when Pocket landed first time, so it's not a regression at all.

Comment 3

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Too late for 46 as we're heading into beta 11 now. We could still take a patch for 47. 
Does this have something to do with the redirect link from a search result? I assume this isn't true for saving all links. 
Mike, Shane, is this a concern for release?

Comment 4

[YF (Yang)]

Attachment: Add missing title parameter

Ref: https://dxr.mozilla.org/mozilla-central/search?q=savePage+path%3Apocket&redirect=false&case=true

Since https://hg.mozilla.org/mozilla-central/rev/d5a378327e88#l7.125 [Bug 1162713]

Comment 5

[YF (Yang)]

Comment on attachment 8740941 [details] [diff] [review]
Add missing title parameter

I guess I made a mistake for this mechanism, sorry.

Comment 6

[Shane Caraveo (:mixedpuppy)]

Attachment: fix typeof check

This issue comes with two flavours.  One, a failed typeof check in the addon.  Two, the server side probably does the same since we're not sending the title when the typeof check is fixed.

Addon fix is simple, self reviewing.  

Will ni? Nate about a fix on their end.

Comment 7

[Shane Caraveo (:mixedpuppy)]

Nate, if the server does not receive a title, the title remains "undefined".  Not sure where to report that.

Comment 8

[(Away until May 24) Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8744461 [details] [diff] [review]
fix typeof check

Review of attachment 8744461 [details] [diff] [review]:
-----------------------------------------------------------------

Please use MozReview in the future as it allows me to expand the diff without having to pull up the file separately.

::: browser/extensions/pocket/content/pktApi.jsm
@@ +341,5 @@
>              since: since ? since : 0
>          };
>  
>          var title = options.title;
> +        if (typeof title !== "undefined") {

This should just be simplified to:
if (options.title) {
  sendData.title = options.title;
}

and the `title` variable can be removed.

Also, can you update the javadoc for this function to fix the grammar for the `options` param? It currently says, "Can provide an title and have a success and error callbacks" which sounds very awkward. Please replace it with "Can provide a string-based title, a `success` callback and an `error` callback."

Comment 9

[Shane Caraveo (:mixedpuppy)]

https://hg.mozilla.org/integration/fx-team/rev/14952428292c90aa00c28fdfb1f1e79bd8257ce2
Bug 1261376 fix typeof check in pocket code, r=jaws

Comment 10

[Shane Caraveo (:mixedpuppy)]

Attachment: fix typeof check

review comments addressed

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/14952428292c

Comment 12

[Paul Silaghi, QA [:pauly]]

Verified fixed FX 49.0a1 (2016-04-28) Win 7, Ubuntu 14.04, OS X 10.9.5.

Comment 13

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Long standing minor issue for pocket but we can track for 48 to make sure it lands there.
Shane, want to request uplift to aurora? This seems minor enough that I'm wontfixing it for 47 beta unless you really want it to make it into 47.

Comment 14

[Shane Caraveo (:mixedpuppy)]

I'm not sure it's worth uplifting.  While this patch is "correct" from the client side, the server side needs to be fixed regardless.

Comment 15

[Sylvestre Ledru [:Sylvestre]]

ok, not tracking anymore then, thanks