Page MenuHomePhabricator

Load references via mobileview API while Cite API is still being discussed
Closed, ResolvedPublic2 Estimated Story Points

Description

As a temporary measure (maybe even long term measure) we should use the existing mobileview API instead of the Cite API for the purposes of lazy loading references.

Related Objects

StatusSubtypeAssignedTask
OpenReleaseNone
OpenNone
OpenNone
OpenNone
OpenFeatureNone
OpenNone
Resolveddr0ptp4kt
Duplicate Jhernandez
Duplicatedr0ptp4kt
DeclinedNone
ResolvedJdlrobson
DeclinedNone
DeclinedNone
ResolvedJdlrobson
ResolvedJdlrobson
ResolvedJdlrobson
ResolvedNone
DeclinedNone

Event Timeline

Change 275724 had a related patch set uploaded (by Jdlrobson):
Lazy load references via mobileview API

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

A few problems I've noticed while attempting this approach:

  • I'm not sure if it's an issue that only I am encountering but when I try to add revisionId support to the mobile view API (which would be needed) I'm sometimes getting different output in the API to the page view itself. Possible race condition?
  • The HTML is not ideal for the client - for example . The client would have to do additional parsing to make sense of it and there could be multiple references lists across various sections. We could explore it as a short term solution but it's hacky like a lot of the things we end up doing.
Jdlrobson updated the task description. (Show Details)
Jdlrobson set the point value for this task to 2.
Jdlrobson changed the task status from Stalled to Open.Mar 23 2016, 5:47 PM

Per RFC T130663 we should show the need for this work by doing it using mobile view

@Jdlrobson I think that this work should be blocked by T130551.

I mayn't have explained my objections to your patch well enough. I'd like to schedule a 30 minute design review meeting so that we can talk through my notes and any notes that you've made on the mobile.references module before moving forward /cc @bmansurov @Jhernandez @jhobs

If the point of this exercise is to demonstrate the value of lazily loading references, then we can provision a temporary labs instance, and test the performance of 275724 as best we can without merging it. We should, of course, be cognisant that a labs instance is Not Production™ in our reporting.

I'd love to meet to listen and discuss code. Count me in!

I've addressed Sam's comments and pulled out the CiteGateway work as I'm concerned that complicates what we need unnecessarily.

The work remaining is mostly the controller work. Could someone take ownership of this from me? I really need to focus on reporting out quarterly goals.

@bmansurov, @Jdlrobson: I'll be sure to update T130551 with notes about my reviews as it's brief and there's some implicit assumptions that I should've made clear (and certainly came out during code review).

https://gerrit.wikimedia.org/r/#/c/275724/ is +1ed by Baha now.
@phuedx @jhobs @Jhernandez can you please take a pass on this and help us get this to a +2?

Change 275724 merged by jenkins-bot:
Allow lazy loading references via mobileview API

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

Here is a test URL: http://reading-web-staging.wmflabs.org/wiki/Ref?mobileaction=beta

@Jdlrobson, do you know how to get the ref tag working on the staging environment?

When I log in I get redirected to http://wmflabs.org/wiki/Main_Page so I cannot edit but that page is missing a references tag.

There seems to be something wrong with the staging environment so I don't fully trust this... and I can't ssh into it to fix it.

But visiting http://reading-web-staging.wmflabs.org/wiki/Ref?mobileaction=beta&useformat=mobile causes a request to http://reading-web-staging.wmflabs.org/w/api.php?action=mobileview&format=json&page=Ref&sections=references&prop=text&revision=1521
which returns an empty list. Did we configure it correctly?

I've re-provisioned reading-web-staging-2 and updated the "Setting up a staging environment" page.

… and also enabled the cite role.

Two issues came up in testing that block sign off

  1. T132978 - found by Baha
  2. API returns different results in beta to stable - T133105