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

VERIFIED FIXED in Firefox 49

Status

()

Firefox
Pocket
VERIFIED FIXED
2 years ago
a year ago

People

(Reporter: pauly, Assigned: mixedpuppy)

Tracking

46 Branch
Firefox 49
Points:
---

Firefox Tracking Flags

(firefox45 affected, firefox46- wontfix, firefox47 wontfix, firefox48- wontfix, firefox49 verified)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

2 years ago
Created attachment 8737221 [details]
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

2 years ago
[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?
tracking-firefox46: --- → ?
Flags: needinfo?(paul.silaghi)
(Assignee)

Updated

2 years ago
No longer blocks: 1215694
(Reporter)

Comment 2

2 years ago
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.
Flags: needinfo?(paul.silaghi)
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?
status-firefox46: affected → wontfix
tracking-firefox46: ? → -
tracking-firefox48: --- → ?
Flags: needinfo?(mixedpuppy)
Flags: needinfo?(mconnor)

Comment 5

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

I guess I made a mistake for this mechanism, sorry.
Attachment #8740941 - Attachment is obsolete: true
Attachment #8740941 - Flags: review?(jaws)
(Assignee)

Comment 6

2 years ago
Created attachment 8744461 [details] [diff] [review]
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.
Assignee: nobody → mixedpuppy
Flags: needinfo?(mixedpuppy)
Flags: needinfo?(mconnor)
Attachment #8744461 - Flags: review+
(Assignee)

Comment 7

2 years ago
Nate, if the server does not receive a title, the title remains "undefined".  Not sure where to report that.
Flags: needinfo?(nate)
(Assignee)

Updated

2 years ago
Attachment #8744461 - Flags: review?(jaws)
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."
Attachment #8744461 - Flags: review?(jaws) → review+
(Assignee)

Comment 10

2 years ago
Created attachment 8745011 [details] [diff] [review]
fix typeof check

review comments addressed
Attachment #8744461 - Attachment is obsolete: true
Attachment #8745011 - Flags: review+

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/14952428292c
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48

Updated

2 years ago
Target Milestone: Firefox 48 → Firefox 49
(Reporter)

Comment 12

2 years ago
Verified fixed FX 49.0a1 (2016-04-28) Win 7, Ubuntu 14.04, OS X 10.9.5.
Status: RESOLVED → VERIFIED
status-firefox49: fixed → verified
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.
status-firefox47: affected → wontfix
tracking-firefox48: ? → +
(Assignee)

Comment 14

2 years ago
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.
ok, not tracking anymore then, thanks
status-firefox48: affected → wontfix
tracking-firefox48: + → -
(Assignee)

Updated

a year ago
Flags: needinfo?(nate)
You need to log in before you can comment on or make changes to this bug.