Page previews cannot handle empty responses gracefully - throws error in beta cluster and doesn't display preview (browser tests were failing)
Closed, ResolvedPublic2 Estimated Story Points
Actions

Assigned To

Authored By

	Jdlrobson
	Dec 11 2017, 10:57 PM

Description

When investigating T182465 (the failing browser tests) and proposed fix, I noticed a new issue impacting the "green-ness" of the tests.

Visit https://en.wikipedia.beta.wmflabs.org/wiki/Category:Page_Previews and hover over "Test previews no lead".

In the console this will throw an exception:

load.php?debug=false&lang=en&modules=jquery%2Cmediawiki|mediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:52 jQuery.Deferred exception: Cannot read property 'replace' of undefined TypeError: Cannot read property 'replace' of undefined
    at Object.o [as a] (<anonymous>:124:972)
    at i (<anonymous>:127:857)
    at o (<anonymous>:127:535)
    at Object.<anonymous> (<anonymous>:126:988)
    at mightThrow (https://en.wikipedia.beta.wmflabs.org/w/load.php?debug=false&lang=en&modules=jquery%2Cmediawiki%7Cmediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:49:598)
    at process (https://en.wikipedia.beta.wmflabs.org/w/load.php?debug=false&lang=en&modules=jquery%2Cmediawiki%7Cmediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:50:269) undefined
jQuery.Deferred.exceptionHook @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki|mediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:52
process @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki|mediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:50
setTimeout (async)
(anonymous) @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki|mediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:50
fire @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki|mediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:46
fireWith @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki|mediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:47
fire @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki|mediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:47
fire @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki|mediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:46
fireWith @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki|mediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:47
done @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki|mediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:126
(anonymous) @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki|mediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:129
XMLHttpRequest.send (async)
send @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki|mediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:130
ajax @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki|mediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:125
jQuery.ajax @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki|mediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:143
r @ VM77:126
c @ VM77:126
(anonymous) @ VM77:119
(anonymous) @ VM77:111
dispatch @ VM77:114
(anonymous) @ VM77:119
mightThrow @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki|mediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:49
process @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki|mediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:50
setTimeout (async)
(anonymous) @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki|mediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:50
fire @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki|mediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:46
fireWith @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki|mediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:47
fire @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki|mediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:47
fire @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki|mediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:46
fireWith @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki|mediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:47
deferred.(anonymous function) @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki|mediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:51
(anonymous) @ VM77:146
setTimeout (async)
i @ VM77:146
(anonymous) @ VM77:119
(anonymous) @ VM77:111
(anonymous) @ VM77:115
(anonymous) @ VM77:130
dispatch @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki|mediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:69
elemData.handle @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki|mediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:66

Looking at the code:
https://github.com/wikimedia/mediawiki-extensions-Popups/blob/master/src/gateway/rest.js#L61

it appears to only know how to handle 404s and 200 responses. It should be adjusted.

Although this error is caught and swallowed, it does not display a page preview with "no summary available".

Acceptance criteria

If a response completes successfully but does not have any data (e.g. 204) or does not include the extract_html field it should not throw an exception

Details

	Subject	Repo	Branch	Lines +/-
	Better error handling for unexpected responses	mediawiki/extensions/Popups	master	+28 -4
	Fix summary responses for pages with empty intros	mediawiki/services/mobileapps	master	+1 -1

Customize query in gerrit

Related Objects
Search...

Status	Assigned	Task
Resolved	Dereckson	T68374 Enable Hovercards on se.wikimedia.org (Swedish chapter wiki)
Resolved	Jdlrobson	T70860 [GOAL] Graduate Page Previews feature (Popups extension) out of Beta Feature
Resolved	ovasileva	T154635 [EPIC] Deploy page previews to English and German Wikipedia
Resolved	ovasileva	T182319 Show HTML summaries on all wikis
Resolved	ABorbaWMF	T182321 Show HTML summaries on cswiki
Resolved	None	T169242 Develop Page Content Service for Reading Clients
Resolved	None	T177425 Develop General Layer of PCS
Resolved	• Jhernandez	T177426 Develop structured JSON APIs for general consumption
Resolved	• Mholloway	T177431 Develop a Summary JSON API
Resolved	• mobrovac	T179875 Update RESTBase to get summary content from MCS Summary 1.3 endpoint when development is complete
Resolved	ABorbaWMF	T182639 Page previews cannot handle empty responses gracefully - throws error in beta cluster and doesn't display preview (browser tests were failing)

Event Timeline

Jdlrobson created this task.Dec 11 2017, 10:57 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 11 2017, 10:57 PM

cc @ovasileva @phuedx

Jdlrobson moved this task from Incoming to Upcoming on the Web-Team-Backlog board.Dec 11 2017, 10:58 PM

Jdlrobson updated the task description. (Show Details)

• Mholloway subscribed.Dec 11 2017, 11:00 PM

• Pchelolo mentioned this in T182619: 500 error returned for responses with validly empty bodies.Dec 11 2017, 11:13 PM

So RESTBase actually returns 200 and empty body for pages which MCS returns 204 for. Should we return 204 if there's no body? What's the convention here?

Originally 204 was proposed for redirect pages (which RESTBase never returns content for), so icing them as an indication of an empty summary was somewhat a surprise for me

@Pchelolo I updated the title (the 200/204 is not so much an issue but the behaviour). My main concern is the JS exception that's occurring in the client, which suggests this is not being properly handled in JS. I'll defer to @phuedx about how this is actually supposed to be have, but we need to squash that JS error! :)

Jdlrobson renamed this task from Page previews cannot handle empty responses gracefully - throws error in beta cluster and doesn't display preview to Page previews cannot handle empty responses gracefully - throws error in beta cluster and doesn't display preview (browser tests failing).Dec 11 2017, 11:55 PM

Jdlrobson merged a task: T182465: Popups browser tests failing as new summary endpoint returns 500 on Main Page.

ovasileva triaged this task as High priority.Dec 12 2017, 11:04 AM

ovasileva raised the priority of this task from High to Unbreak Now!.Dec 12 2017, 12:36 PM

ovasileva added a project: Readers-Web-Kanbanana-Board-Old.

Restricted Application added subscribers: Liuxinyu970226, Jay8g, TerraCodes. · View Herald TranscriptDec 12 2017, 12:36 PM

ovasileva moved this task from To Do to Needs Design Review on the Readers-Web-Kanbanana-Board-Old board.Dec 12 2017, 12:36 PM

Additional context for T182639#3830702:

12:34:34 <phuedx> olliv: https://phabricator.wikimedia.org/T182639 is high priority if not ubn!
12:35:15 <phuedx> priority comes from the fact that the service is designed to return empty responses when things don't make sense
12:35:24 <phuedx> i think the likelihood of empty responses is low
12:35:36 <olliv> phuedx: argh, yes, saw that this morning
12:35:47 <phuedx> but it's still a broken ux
12:35:51 <phuedx> with no explanation
12:35:54 <olliv> so it's throwing an error and basically not doing anything, right?
12:35:59 <phuedx> (which i think makes it a ubn! ?)
12:36:00 <phuedx> yes
12:36:00 <olliv> yes, high, let's pull it in
12:36:09 <olliv> I'm okay with UBN

A bug was introduced yesterday causing the 'extract' and 'extract_html' properties not to be returned for pages with empty intros. We didn't catch it because of a bug in one of the node services template test utilities wherein deepEqual(undefined, '') surprisingly evaluates to true.

As best I can tell, the errors above look like they're caused by an expected property being absent, so fixing this should fix the broken test once and for all. There might still be some client-side work around resiliency in this situation.

Change 397815 had a related patch set uploaded (by Mholloway; owner: Mholloway):
[mediawiki/services/mobileapps@master] Fix summary responses for pages with empty intros

https://gerrit.wikimedia.org/r/397815

gerritbot added a project: Patch-For-Review.Dec 12 2017, 1:48 PM

(For further background: this response should no longer be empty after last night's fixes, but I had to purge RESTBase's cached copy this a.m. in order to reflect that.)

https://github.com/wikimedia/service-template-node/issues/94

ovasileva moved this task from Upcoming to QA Tasks / Needs Reproduction on the Web-Team-Backlog board.Dec 12 2017, 3:35 PM

ovasileva moved this task from QA Tasks / Needs Reproduction to 2017-18 Q2 on the Web-Team-Backlog board.

ovasileva moved this task from Backlog to incoming on the Page-Previews board.Dec 12 2017, 3:48 PM

Change 397815 merged by jenkins-bot:
[mediawiki/services/mobileapps@master] Fix summary responses for pages with empty intros

https://gerrit.wikimedia.org/r/397815

Jdlrobson moved this task from Needs Design Review to Doing on the Readers-Web-Kanbanana-Board-Old board.Dec 12 2017, 4:58 PM

MBinder_WMF set the point value for this task to 2.Dec 12 2017, 6:11 PM

IMHO there is a change required in Page Previews codebase to not throw browser exception when the API response is incorrect.

Even services is going to fix the issue on their side, the frontend should be bullet-proof and not fail when API returns something that popups do not understand.

Dropping to normal now as this has been addressed in beta cluster. Better error handling should not be the goal in case that ever happens again!

Jdlrobson renamed this task from Page previews cannot handle empty responses gracefully - throws error in beta cluster and doesn't display preview (browser tests failing) to Page previews cannot handle empty responses gracefully - throws error in beta cluster and doesn't display preview (browser tests were failing).Dec 12 2017, 7:10 PM

Jdlrobson claimed this task.

Jdlrobson removed a project: Patch-For-Review.

Jdlrobson moved this task from Doing to Blocked on Others on the Readers-Web-Kanbanana-Board-Old board.Dec 12 2017, 8:25 PM

Jdlrobson updated the task description. (Show Details)

Jdlrobson moved this task from Blocked on Others to Needs Code Review on the Readers-Web-Kanbanana-Board-Old board.Dec 12 2017, 9:25 PM

Change 397927 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/Popups@master] Better error handling for unexpected responses

https://gerrit.wikimedia.org/r/397927

gerritbot added a project: Patch-For-Review.Dec 12 2017, 9:26 PM

The above patch takes care of 204s and 200s without extracts. Do we also want to cover 500 errors?

In T182639#3832496, @Jdlrobson wrote:

The above patch takes care of 204s and 200s without extracts. Do we also want to cover 500 errors?

Nice! And yes, I think we should cover 5xx errors.

@phuedx: as @Jdlrobson is OOO I'll follow-up on his patch.

pmiazga claimed this task.Dec 13 2017, 6:03 PM

@bmansurov can you also add logic to handle 5xx errors and show the generic preview

pmiazga moved this task from Needs Code Review to Needs More Work on the Readers-Web-Kanbanana-Board-Old board.Dec 13 2017, 6:43 PM

• bmansurov moved this task from Needs More Work to Doing on the Readers-Web-Kanbanana-Board-Old board.Dec 13 2017, 6:57 PM

Talked to @pmiazga on IRC. 5xx are already handled and the front-end won't fail when it encounters these errors. Showing the generic preview would give a false impression that a link may not have a preview, when in case the back-end maybe having a temprorary glitch.

^ /cc @ovasileva

IIRC we won't show a preview in the case of a backend failure (the 5xx errors we're talking about above). @Nirzar: Might it be worth adding a "Something went wrong." preview too (which is out of scope for this task BTW).

^ rEPOP49ce8960c19e: Better error handling for unexpected responses is still being code reviewed.

pmiazga removed a project: Patch-For-Review.Dec 14 2017, 12:21 PM

pmiazga moved this task from Needs Code Review to Needs QA on the Readers-Web-Kanbanana-Board-Old board.

@phuedx, @Nirzar - I think it's worth adding the empty preview here too

phuedx assigned this task to ABorbaWMF.Dec 14 2017, 2:46 PM

I agree

here's an error state for previews using our blank preview design

Spacing of the icon in the mock doesn't match the existing spacing. Compare:

scrot-area-2017-12-14_13:27:20.png (239×482 px, 22 KB)

^ Captured in T182905: Show an informative preview when the backend service responds with an error.

@bmansurov

then the existing spacing needs to be fixed.
zpl.io/bAOdAPx < this was the spec provided for it and If i remember correctly the spacing was correct? was there a change that broke the spacing?

This appears to be fixed on Beta, in that the error is now appearing. It sounds like the spacing may be a separate issue?

https://en.wikipedia.beta.wmflabs.org/wiki/Category:Page_Previews

Screen Shot 2017-12-14 at 11.22.07 AM.png (1×2 px, 1 MB)

Also now works on the PreviewsEmpty/sandbox link

Tried it on the big browsers

I'm okay showing the same empty preview for 500 errors as well

^ The above is meant to reflect that the fact that we shouldn't be deploying the Page Summary API until the major client (Page Previews) has been QA'd.

looks good. Resolving. Other issues to be tracked under T182905: Show an informative preview when the backend service responds with an error

ovasileva updated the task description. (Show Details)Dec 15 2017, 2:25 PM

Liuxinyu970226 unsubscribed.Dec 15 2017, 2:37 PM

Change 397927 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Better error handling for unexpected responses

https://gerrit.wikimedia.org/r/397927

ReleaseTaggerBot added a project: MW-1.31-release-notes (WMF-deploy-2018-01-09 (1.31.0-wmf.16)).Jan 2 2018, 10:00 PM

• bmansurov unsubscribed.Jan 2 2018, 10:25 PM

• JAufrecht mentioned this in T184646: [Phlogiston Readers Web] Several tasks appear as Scope: Unknown in Status Report .Feb 8 2018, 7:16 PM

	F11832639: Screen Shot 2017-12-14 at 11.22.07 AM.png
	Dec 14 2017, 8:43 PM

	F11830829: scrot-area-2017-12-14_13:27:20.png
	Dec 14 2017, 6:29 PM

	F11830183: error-state.png
	Dec 14 2017, 5:57 PM

Page previews cannot handle empty responses gracefully - throws error in beta cluster and doesn't display preview (browser tests were failing)Closed, ResolvedPublic2 Estimated Story PointsActions