Page MenuHomePhabricator

[Bug] Blue links are broken on the Serbian (Latin) Burek article on iOS
Open, HighPublic

Description

Steps to reproduce

  1. Set language to Serbian (Latin)
    • This is handled via the HTTP header: accept-language: sr-el
  2. Go to the Burek article
  3. Tap on any blue link

Expected behavior

Relevant link is loaded, which means the redirect should be resolved for links that are altered by language variant fallback

How?

The proper way to handle this is to make mobile-html to resolve redirects since parsoid is not supposed to resolve redirects.

Proposal 1

Proposal 2

Current behavior

Error is shown and relevant article is not loaded

Acceptance Criteria

  • link in the page have redirects resolved and doesn't show error when clicked

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

This is happening on the PCS side (or deeper). I tested in Android and it's also broken. In a link callback on that page, say, for the "Osmanskog carstva" link in the intro, this is what I see for the href over the Javascript bridge:

- key : "href"
- value : ./Osmansko_carstvo

The mobile-html link fails when we try to fetch that variant. If we were to fetch the Cyrillic naming of it, it works.

Tagging PI for further investigation.

Notably, on web the link in latin wikipedia is shown to readers as Osmanskog carstva but links to https://sr.wikipedia.org/wiki/%D0%9E%D1%81%D0%BC%D0%B0%D0%BD%D1%81%D0%BA%D0%BE_%D1%86%D0%B0%D1%80%D1%81%D1%82%D0%B2%D0%BE, a non-latin version of the article title.

Is it possible PCS is losing this nuance?

I took a look at the pcs level:

  • Using my local dev mobileapps deployment
  • Visited http://127.0.0.1:8888/sr.wikipedia.org/v1/page/mobile-html/%D0%91%D1%83%D1%80%D0%B5%D0%BA (Burek article)
  • When I click the links in the mobile-html version I get link action events like:
{…}
action: "link"
data: Object { href: "./Сир", text: "сиром", title: "Сир", … }
<prototype>: Object { … }
pcs:1412:22

The same happens on prod:
https://sr.wikipedia.org/api/rest_v1/page/mobile-html/%D0%91%D1%83%D1%80%D0%B5%D0%BA

Isn't this something that should be handled in the client side ?

@Jgiannelos it looks like you are testing Serbian Cyrillic, the bug is when you fetch with an Accept-Language header of "sr-el" for Latin. The link you mention will return as: <a href="./Sir" title="Sir">sirom</a>. Then when we try to fetch that mobile-html link it fails:

https://sr.wikipedia.org/api/rest_v1/page/mobile-html/Sir

The Cyrillic wording works, and on desktop "Sir" automatically redirects to the Cyrillic wording:

https://sr.wikipedia.org/api/rest_v1/page/mobile-html/%D0%A1%D0%B8%D1%80

https://sr.wikipedia.org/wiki/Sir

We have no way on the client-side of knowing what the Cyrillic wording is at this point, so I think we need https://sr.wikipedia.org/api/rest_v1/page/mobile-html/Sir to work, redirect to the Cyrillic wording if necessary, and be sure an Accept-Language header of "sr-el" still converts all of it's characters in the content to the Latin variant.

Got it, thanks @Tsevener i am gonna take a look now.

Also just to be clear, we get those link action events as expected, and are linking to them ourselves client-side. That's fine, it's more that the action events have a link href that the mobile-html endpoint fails on. Here's the event I'm seeing:

{"text": "sirom", "href": "./Sir", "source": <null>, "title": "Sir"}

@Jgiannelos Thanks!

MSantos updated the task description. (Show Details)
MSantos added a subscriber: MSantos.
MSantos added a subscriber: Amire80.

I guess this is a question directed to Language-Team (Language-2022-April-June)

Why can’t I have redirects information from the mwapi but MW still redirects to the right page? The page title in question is generated for a language variant request:
Page title changes with language variation: sr = Сир and sr-el = Sir;

cc/ @Amire80

This isn't really our area of expertise, since it is about MediaWiki-Language-converter which we did not develop and do not maintain. I can say, however, that the API does not return a redirect because it isn't a redirect in the usual sense of the other page existing with #REDIRECT statement. Language converter is doing some kind of equivalency mapping between the converted titles. It seems the behavior which would make sense is that this should be part of the normalization similar to other things such as page -> Page.

Actually, it seems you can get the expected behavior with the converttitles option: https://sr.wikipedia.org/w/index.php?title=%D0%9F%D0%BE%D1%81%D0%B5%D0%B1%D0%BD%D0%BE:API_%D0%BF%D0%B5%D1%81%D0%B0%D0%BA&uselang=en#action=query&format=json&titles=Sir&redirects=1&converttitles=1

Hope this helps.

That's perfect! Thanks, @Nikerabbit! It's exactly what I was looking for.

Change 696025 had a related patch set uploaded (by Vadim Kovalenko; author: Vadim Kovalenko):

[mediawiki/services/mobileapps@master] Blue links are broken on the Serbian (Latin) Burek article on iOS

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

I've uploaded a patch with redirection algorithm. The main logic there is to check the converted: [{ 'from': '...', 'to': '...' }] property from mediawiki api for each request. If this property exists, mobile html will perform redirection to route, specified in converted[0].to property. Link titles remain untouched, the redirect route will recalculate only when the user clicks the link.
cc: @MSantos , @Tsevener

@vadim-kovalenko thanks for the heads up! I've been testing this and the mobile-html endpoint is working well. Since this is getting farther along in the flow I found another couple of 404s that occur once mobile-html successfully loads for the summary and media-list endpoints. Would it be possible to add the redirect logic to these as well? I wasn't sure if you wanted to consider the fix as a part of this task or separately since they are different endpoints.

http://localhost:8888/sr.wikipedia.org/v1/page/summary/Osmansko_carstvo
Accept-Language: sr-el
Response
..."status":404,"type":"missingtitle","detail":{"batchcomplete":true,"query":{"normalized":[{"fromencoded":false,"from":"Osmansko_carstvo","to":"Osmansko carstvo"}],"pages":[{"ns":0,"title":"Osmansko carstvo","missing":true,"contentmodel":"wikitext","pagelanguage":"sr","pagelanguagehtmlcode":"sr","pagelanguagedir":"ltr"}]}}...

http://localhost:8888/sr.wikipedia.org/v1/page/media-list/Osmansko_carstvo
Accept-Language: sr-el
Response
..."status":404,"type":"https://mediawiki.org/wiki/HyperSwitch/errors/not_found","detail":"Page or revision not found."...

@Tsevener thanks for the extra info. I'll create a separate task and we will be fixing them in follow-up patches.

Change 685455 had a related patch set uploaded (by Vadim Kovalenko; author: Vadim Kovalenko):

[mediawiki/services/mobileapps@master] Fix Media list endpoint returning unexpected and out of order item responses

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

Hi @MSantos , @Tsevener !
I've uploaded the patch that solves both this task and T269312
I created resolve redirection method and chained it to these routes:

  • /v1/page/summary/{title}
  • /v1/page/media-list/{title}
  • /v1/page/mobile-html/{title}

Worth mentioning that I perform overriding title parameter of the request - in the previous patchset it was a real redirect invocation with changing URL
Now there is an implicit redirection - for example, article sr.wikipedia.org/v1/page/mobile-html/Burek will redirect to article with title Бурек but URL title param in the search bar will remain Burek.

@vadim-kovalenko thanks! I tested the patch and it's looking good. I see some weirdness with saving for offline for us but I'm pretty sure that's client-side and/or because I'm serving it locally. Do you still have a staging step once the patch is merged, where it's hosted in mobileapps.wmflabs.org but not yet in production? I'd like to test there too before this goes live. Tagging @MattCleinman and @Dbrant in case they want to confirm anything with this on the Android side.

I also didn't dig too deep into T269312, but I found something that may be a bug, I'll comment in the patch about it.

Change 685455 merged by jenkins-bot:

[mediawiki/services/mobileapps@master] Fix Media list endpoint returning unexpected and out of order item responses

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

Update: I'm going to highlight a few bugs I found client-side for us. I need to do some more digging on the problem and how we'd handle the fix, because initial digging looked like there might be data duplication issues in the app that I wasn't eager to introduce and mitigate later. Thanks for holding the deployment of this fix for us!

Bugs revolve around saving articles. I tested with Burek and tapping its link sirom in the first paragraph (which links to http://mobileapps.wmflabs.org/sr.wikipedia.org/v1/page/mobile-html/Sir)

  1. Saving on Sirarticle doesn’t update the selected state on save button properly.
  2. Visiting Sir from Burek after saving both triggers the “article queued to be downloaded” label again on the Sir article in the Saved tab
  3. In offline mode, visiting Sir from Burek errors out

To reflect reality, I'll move this to the blocked column on our board. I restored the changes in the beta cluster so you can keep testing.

Hi @MSantos , @Tsevener !
I've uploaded the patch that solves both this task and T269312
I created resolve redirection method and chained it to these routes:

  • /v1/page/summary/{title}
  • /v1/page/media-list/{title}
  • /v1/page/mobile-html/{title}

Worth mentioning that I perform overriding title parameter of the request - in the previous patchset it was a real redirect invocation with changing URL
Now there is an implicit redirection - for example, article sr.wikipedia.org/v1/page/mobile-html/Burek will redirect to article with title Бурек but URL title param in the search bar will remain Burek.

Hi @vadim-kovalenko

Thanks for fixing the issue as it also affects the Android app side.

Would it be possible to also resolve the /v1/page/related/{title} because the /v1/page/mobile-html will load it for its related pages.

Change 703749 had a related patch set uploaded (by Vadim Kovalenko; author: Vadim Kovalenko):

[mediawiki/services/mobileapps@master] Fix Media list endpoint returning unexpected and out of order item responses

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

Hi @cooltey
I've added title redirection for /related pages in the patch above along with other changes that have been implemented in the reverted patch
To check 'read more' section locally I had a need to deploy a RESTBase

Steps to reproduce locally:

Adjust mobileapps and restbase by taking changes from these commits provided by @Jgiannelos :

Run restbase: node server.js -c config.fullstack.test.yaml

Run mobileapps:
In root folder - npm start
In /pagelib folder - npm run dev:debug

Optional: enable CORS in browser

Go to arbitrary article, for example http://127.0.0.1:7233/en.wikipedia.beta.wmflabs.org/v1/page/mobile-html/Dog?footer=true
footer=true url parameter needs to make 'read more' section visible
Set domain variable to en.wikipedia.org here.

At the bottom of the article there should be a read more section (see the shot).

readmore-article.png (302×888 px, 56 KB)

@MSantos , @Jgiannelos if you have any suggestions on how to improve localhost experience to testing this section (and similar sections), please share with me.

Also, I wasn't able to test API for the /related articles of sr wiki
https://sr.wikipedia.org/api/rest_v1/page/related/Сир - Example on prod works
http://127.0.0.1:7233/sr.wikipedia.beta.wmflabs.org/v1/page/related/Сир - But this one not fount
http://127.0.0.1:7233/en.wikipedia.beta.wmflabs.org/v1/page/related/Dog - 'en' wiki articles on local environment works, however

cc: @Tsevener

Update - I've finished investigating the iOS UI bugs I was seeing. They are existing bugs that are related to any redirect, so it's not specifically something introduced with this patch (though it will be more prevalent on Serbian Latin links). I spun off https://phabricator.wikimedia.org/T285992 to work against for the client-side fixes. This patch deployment does not need to wait on T285992's fix. Going to unassign myself from this one since it encompasses the server-side fixes.

@cooltey good catch, iOS needs the /v1/page/related/{title} redirect too. @vadim-kovalenko I wasn't able to see the read more section locally from your instructions, but I may have missed a step somewhere (and @cooltey may have better luck), I can try again tomorrow. I'm not familiar with how RESTBase interacts here, would it be easy to deploy the fix to the http://mobileapps.wmflabs.org staging and we test the /related fix there?

@Tsevener I expect to see these changes working in the beta cluster once this patch with reverted changes and other fixes is merged. You will no need to setup RESTBase locally then. But for local testing, I had to use it.

Thanks for the fix, @vadim-kovalenko. By checking the patch above, I have noticed that you are making an extra API call from api.php to get the redirected title to resolve the issue, which makes sense to me.

I am also curious about that is it possible to solve this issue in the /page/related API as well? The /page/related/ will be used in the Explore feed (Because you read) as well. Should we create a ticket for it? @Pchelolo

@Tsevener Is there any update regarding this task? Does my assistance is needed in the current stage?

@vadim-kovalenko so sorry for dropping this. Everything looks good to me except I've been unable to test the page/related/{title} fix locally, but I'm happy to test again once it's in http://mobileapps.wmflabs.org (it would be a very quick test). Let me know if that's doable, thanks!

Change 696025 abandoned by MSantos:

[mediawiki/services/mobileapps@master] Blue links are broken on the Serbian (Latin) Burek article on iOS

Reason:

Abandon in favor of Iacb82601c222d8b0fe1023b29d1bc8d108929897

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

Change 703749 merged by jenkins-bot:

[mediawiki/services/mobileapps@master] Fix Media list endpoint returning unexpected and out of order item responses

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

MSantos claimed this task.

I've separated the page/related specs in another task (T289575) since this one is done and deployed, please feel free to re-open in case it's needed.

Potentially there was a regression, this is failing again.

Removing "Language-Team (Language-2021-July-September)" as that's in the past.

I assume no more help needed from us. Do ping again if needed.

Tried to retrieve specific Serbian articles using RESTBase API. For example for Voće (this is serbian latin word)
This article exists in the desktop wiki - https://sr.wikipedia.org/sr-el/Vo%C4%87e
But response from the REST API is not found. Check - https://sr.wikipedia.org/api/rest_v1/page/html/Vo%C4%87e.
My assumption is that mobilapps nor RESTBase know nothing about /sr-el directory path which is utilizing in the desktop wiki. By the way, related tests in the mobiileapps (run ./node_modules/mocha/bin/mocha test/lib/wikiLanguage/wikiLanguage-test.js) and in the RESTBase (run ./node_modules/mocha/bin/mocha test/features/pagecontent/language_variants.js) work fine. I hope this research was useful. Can someone assist of how /sr-el or /sr-cr consumed by the RESTBase?

cc: @MSantos , @Jgiannelos

Tried to retrieve specific Serbian articles using RESTBase API. For example for Voće (this is serbian latin word)
This article exists in the desktop wiki - https://sr.wikipedia.org/sr-el/Vo%C4%87e
But response from the REST API is not found. Check - https://sr.wikipedia.org/api/rest_v1/page/html/Vo%C4%87e.
My assumption is that mobilapps nor RESTBase know nothing about /sr-el directory path which is utilizing in the desktop wiki. By the way, related tests in the mobiileapps (run ./node_modules/mocha/bin/mocha test/lib/wikiLanguage/wikiLanguage-test.js) and in the RESTBase (run ./node_modules/mocha/bin/mocha test/features/pagecontent/language_variants.js) work fine. I hope this research was useful. Can someone assist of how /sr-el or /sr-cr consumed by the RESTBase?

@vadim-kovalenko as suspected, the problem is in how parsoid is handling the title, here is the rest api request that doesn't use restbase as a proxy (although it's public this URL won't be available in the future).

cc/ @cscott and @ssastry for visibility

On a side note, this is not a regression for the media-list fix, but I'm okay with tracking this in this task because of the history context

In this case, the title of the article is actually Воће: https://sr.wikipedia.org/sr/%D0%92%D0%BE%D1%9B%D0%B5

When you have language conversion turned on, MediaWiki will attempt to look up Voće, and /when that fails/, attempt to use language conversion to generate a new title (in this case, conversion to sr-ec yields Воће) and then transparently redirect to that title.

Restbase isn't "wrong" here -- there is no article in the database with the title Воће. But the parsoid rest API should probably be issuing a redirect to the localized title with the same fallback mechanism as in core.