Page MenuHomePhabricator

[Bug] Blue links are broken on the Serbian (Latin) Burek article on iOS and Android
Open, HighPublicBUG REPORT

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

@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.

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.

@cscott Can you point me to where and how that fallabck mechanism is implemented in core?

Tacsipacsi renamed this task from [Bug] Blue links are broken on the Serbian (Latin) Burek article on iOS to [Bug] Blue links are broken on the Serbian (Latin) Burek article on iOS and Android.Jul 7 2022, 4:00 PM
Tacsipacsi removed a project: Patch-For-Review.
Tacsipacsi subscribed.

Received a complaint from a user in TestFlight on this:

"It will not, under ANY circumstances, allow me to enter a page from a page. I tried. Multiple times. It just doesn’t ever work."

fits1024x1024.jpg (712×1 px, 27 KB)

Change 887997 had a related patch set uploaded (by Daniel Kinzler; author: Daniel Kinzler):

[mediawiki/core@master] Apply variant conversion to page bundles

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

For android specifically it looks like the problem is with the summary URLs:
From android studio:

E/org.wikipedia.alpha: org.wikipedia.dataclient.okhttp.HttpStatusException: Code: 404, URL: https://sr.wikipedia.org/api/rest_v1/page/summary/Pekara, title: Not found., detail: Page or revision not found.
        at org.wikipedia.dataclient.okhttp.UnsuccessfulResponseInterceptor.intercept(UnsuccessfulResponseInterceptor.kt:16)
        at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.kt:109)
        at okhttp3.logging.HttpLoggingInterceptor.intercept(HttpLoggingInterceptor.kt:154)
        at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.kt:109)
        at okhttp3.internal.connection.RealCall.getResponseWithInterceptorChain$okhttp(RealCall.kt:201)
        at okhttp3.internal.connection.RealCall$AsyncCall.run(RealCall.kt:517)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1137)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:637)
        at java.lang.Thread.run(Thread.java:1012)

Unfortunately I don't have access to an iOS dev env. @Tsevener can you check if the some issue applies on the ios app too?

Change 887997 merged by jenkins-bot:

[mediawiki/core@master] Apply variant conversion to page bundles

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

Aklapper changed the subtype of this task from "Task" to "Bug Report".Feb 17 2023, 8:26 AM

The patch was applied to page bundles, but it appears the bug is with the summary endpoint? I'm a little confused. AIUI the title redirection should happen in core, and in common for all endpoints, and it's not clear a patch to do that has landed yet.

@cscott Can you point me to where and how that fallabck mechanism is implemented in core?

In core this is handled via LanguageConverter::findVariantLink(), which is invoked early in the web request dispatch, in MediaWiki::parseTitle(), and in the action API endpoint, in ApiPageSet::processTitlesArray(). IMO it should be done in a top-of-stack manner in the REST apis as well.

The patch was applied to page bundles, but it appears the bug is with the summary endpoint? I'm a little confused. AIUI the title redirection should happen in core, and in common for all endpoints, and it's not clear a patch to do that has landed yet.

The problem still exists. The patch related to this ticket fixed a bigger issue of mobile-html not serving content with the right language variant at all.
The assumption was that this would have fixed the problem.

Now the problem is that even though the right language variant is used the URLs when users click a link in an (eg. sr-el) article return 404. This is not summary specific, it applies to all URLs in mobile-html content.

Ideally we should be using MediaWiki::getTitle() exactly to do title lookup in the REST APIs, instead of trying to (partially/incompletely/"mostly") reimplement this.

Change 896409 had a related patch set uploaded (by C. Scott Ananian; author: C. Scott Ananian):

[mediawiki/services/parsoid@master] Parsoid shouldn't Language Convert the href of a wikilink

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

Change 900386 had a related patch set uploaded (by C. Scott Ananian; author: C. Scott Ananian):

[mediawiki/services/parsoid@master] AddRedLinks: Use href attribute, not title, to reconstruct article title

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