Page MenuHomePhabricator

Empty previews not appearing on beta cluster
Closed, ResolvedPublic

Description

After the switch to the new endpoint and html previews, empty previews are no longer appearing in the beta cluster

steps to reproduce:

  1. go to: https://en.wikipedia.beta.wmflabs.org/wiki/Category:Page_Previews
  2. hover on "previewsempty/sandbox"
  3. expected behavior:
    • empty preview should appear
  4. observed behavior:
    • nothing happens

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 11 2017, 3:27 PM
ovasileva triaged this task as High priority.Dec 11 2017, 3:28 PM

note: it is appearing when hovering over "previewsredirect/sandbox"

Pages with empty intros are currently returning 204s. Will fix momentarily.

(PS: To expedite fixes would you mind tagging me on any issues you find? Thanks!)

will do @Mholloway, and thank you!

Change 397566 had a related patch set uploaded (by Mholloway; owner: Mholloway):
[mediawiki/services/mobileapps@master] Summary: Send empty summary (not 204) for empty page

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

I notice there's also currently no popup for PreviewsPopupsList/sandbox. Is that expected, or should there be a popup with "Looks like there isn't a preview for this page"?

bearND added subscribers: phuedx, Jdlrobson, bearND.EditedDec 11 2017, 4:20 PM

This is a divergence from the spec at https://www.mediawiki.org/wiki/User:Phuedx_(WMF)/Reading/Web/Page_Preview_API#For_a_page_that_redirects_to_another_page.

The Location HTTP header must be set to the URL that will get the intro for the target page.
RESTBase handles redirects transparently to the underlying service (see T176517#3634838).
The Page Preview API must respond with 204 No Content.
The response body must be empty.

Due to the divergence from the spec we should get at least a positive confirmation from @phuedx or @Jdlrobson about this change and, if appropriate, an update to the spec.

@Mholloway - I had the same question in https://phabricator.wikimedia.org/T182399#3828370. Based on the requirements, the preview should include the list itself, but I'm not sure if something changed here mid-development.

This is a divergence from the spec at https://www.mediawiki.org/wiki/User:Phuedx_(WMF)/Reading/Web/Page_Preview_API#For_a_page_that_redirects_to_another_page.

The Location HTTP header must be set to the URL that will get the intro for the target page.
RESTBase handles redirects transparently to the underlying service (see T176517#3634838).
The Page Preview API must respond with 204 No Content.
The response body must be empty.

Due to the divergence from the spec we should get at least a positive confirmation from @phuedx or @Jdlrobson about this change and, if appropriate, an update to the spec.

I don't think the new patch is actually a divergence, but rather a correction. The patch to introduce 204s for redirect pages was overbroad and erroneously ended up returning 204s for pages with empty lead sections and intros as well. This fixes that.

Ok, makes sense. For some reason I was thinking more about redirects here.

Change 397566 merged by jenkins-bot:
[mediawiki/services/mobileapps@master] Summary: Send empty extracts (not 204) for an empty page

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

What's happening is https://en.wikipedia.beta.wmflabs.org/api/rest_v1/page/summary/PreviewsEmpty%2Fsandbox is returning a 500 ( "HyperSwitch error: type:"server_error#empty_response") rather than a 204.

Side note: How should the client be behaving in this situation? It feels like doing nothing is most appropriate here.

Side note: How should the client be behaving in this situation? It feels like doing nothing is most appropriate here.

We should be displaying the empty preview

What's happening is https://en.wikipedia.beta.wmflabs.org/api/rest_v1/page/summary/PreviewsEmpty%2Fsandbox is returning a 500 ( "HyperSwitch error: type:"server_error#empty_response") rather than a 204.
Side note: How should the client be behaving in this situation? It feels like doing nothing is most appropriate here.

As mentioned at T182399#3828530 RESTBase is transforming the 204s from MCS into 500s when proxying the response back to the client. It's kind of moot since the client shouldn't really be seeing 204s from this endpoint either (the only valid 204 case in MCS being receiving a redirect title, which Shouldn't Happen because RESTBase resolves redirects first).

Mholloway closed this task as Resolved.Dec 13 2017, 12:10 AM