Page MenuHomePhabricator

URLs with title query string parameter and additional query string parameters do not redirect to mobile site
Closed, ResolvedPublic3 Story Points

Description

problem statement

When: I hit a desktop link while browsing with a mobile user agent
Expected: User sees Special:MobileDiff
Actual: User sees desktop diff page.

Certain URLs to diffs do not redirect on mobile devices.

Problem

The regex used by puppet seems to be incorrect:
https://github.com/wikimedia/puppet/blob/c88edf93ff096a7e72794e6ec57f83057b3a9f53/modules/varnish/templates/text-frontend.inc.vcl.erb#L28

Code

  • The redirect is handled in MobileContext::redirectMobileEnabledPages
  • The redirect only happens if diff is a provided in the query string parameter
  • The redirect url is determined by SpecialMobileDiff::getMobileUrlFromDesktop();
    • The redirect only applies if both diff and oldid are provided.
    • If only oldid is provided without diff then the revision is shown.

Developer notes

  • This should be fixable by simply updating SpecialMobileDiff::getMobileUrlFromDesktop(); with new test cases and making them green (https://gerrit.wikimedia.org/r/359060 Tests for diffs )
  • A little bit of debugging will be needed to see why the redirect doesn't happen when title query parameter is set but it seems to relate to getStopMobileRedirectCookie

Testing

View https://de.wikipedia.org/w/index.php?title=Skull&diff=164584307 on a mobile device (or emulated Chrome Nexus 5)
Expected: user should be redirected to mobile site.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Dvorapa updated the task description. (Show Details)Dec 28 2016, 9:28 AM

This doesn't make much sense, ultimately the link created and shown to users wouldn't change like that.

This doesn't make much sense, ultimately the link created and shown to users wouldn't change like that.

This is usual behavior of many modern websites. If you find a link to tweet on some website and open it on mobile, it opens in Twitter mobile view. The same works with wiki articles themselves. But, unexpectedly, article diffs don't work that way. Why?

AlexMonk-WMF added a comment.EditedDec 28 2016, 9:06 PM

If you want diff pages to redirect to mobile diffs when viewed on mobile devices, that doesn't seem to have much to do with VE. The VE behaviour here is expected. VE has no control over what is shown to readers after the edit is saved.

Dvorapa added a project: MediaWiki-Redirects.

If you want diff pages to redirect to mobile diffs when viewed on mobile devices, that doesn't seem to have much to do with VE. The VE behaviour here is expected. VE has no control over what is shown to readers after the edit is saved.

Sorry, I created a task by copying a similar one, but forgot to change projects

AlexMonk-WMF renamed this task from Make links to xy.wikipedia automatically redirect to xy.m.wikipedia if user is on a mobile phone to Redirect diff pages to mobile diff on mobile phones.Dec 30 2016, 11:55 AM
phuedx triaged this task as High priority.Jan 5 2017, 10:30 AM
phuedx added subscribers: bmansurov, Jdlrobson, phuedx.

IIRC this should be the case. @bmansurov, @Jdlrobson? I'm prioritising this as High for now but if it's confirmed as a regression then it'll likely be reprioritised as UBN!

I remember seeing something similar some time back.

I'm not sure whether this is a regression.
https://cs.wikipedia.org/w/index.php?title=MyPage&diff=14286854&oldid=14265839 My Page last change may not work but https://cs.wikipedia.org/wiki/MyPage?diff=14286854&oldid=14265839 will redirect

MobileDiff:;getMobileUrlFromDesktop does the right thing for this url as does MobileContext::redirectMobileEnabledPages

The hook onDiffViewHeader should be executing. Something is not happening when the title is passed by query string parameter.
Suggest normal/high.

Jdlrobson lowered the priority of this task from High to Normal.Jan 30 2017, 8:14 PM

So I know why this doesn't work. Special:MobileDiff only allows diffs on a revision compared to the revision before, hence the URL Special:MobileDiff/<revid>

If you are passing both an oldid and a diff you are requesting a diff that this page cannot generate.
Do we want to add support to this page or decline, given that the end goal would be to upstream the single column diff into core and stop creating our own page for this?

Jdlrobson renamed this task from Redirect diff pages to mobile diff on mobile phones to Certain diff URLs do not redirect to mobile site on mobile devices.Apr 17 2017, 4:34 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)May 1 2017, 8:40 PM
Jdlrobson added a project: patch-welcome.
ovasileva updated the task description. (Show Details)Jun 13 2017, 4:31 PM
ovasileva updated the task description. (Show Details)Jun 13 2017, 4:34 PM
ovasileva updated the task description. (Show Details)
ovasileva moved this task from Upcoming to Needs Analysis on the Readers-Web-Backlog board.

@ovasileva to split into two tasks

FWIW I actually merged two tasks into this one (the other task used to be T122409).

Can you hold off spitting this again? IMO this issue is actually relatively trivial to fix if done together.
If you can let me know the issue/problem with estimation that came up during grooming maybe I can shed some more light on this ticket :)

Jdlrobson updated the task description. (Show Details)

@Jdlrobson - go for it, we can discuss during next grooming.

Change 359060 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Tests for diffs

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

Jdlrobson updated the task description. (Show Details)
Jdlrobson moved this task from Needs Analysis to Upcoming on the Readers-Web-Backlog board.

I hope this makes more sense now.. for next backlog grooming meeting :)

Change 359060 abandoned by Jdlrobson:
Tests for diffs

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

Jdlrobson removed a project: Patch-For-Review.
ovasileva set the point value for this task to 3.Jul 5 2017, 4:12 PM
Jdlrobson moved this task from Backlog to Bugs on the MobileFrontend board.Jul 13 2017, 5:54 PM

Change 359060 restored by Jdlrobson:
Tests for diffs

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

This feels like a Varnish problem..
Both URLs in the examples redirect for me locally.

The thing that makes me think varnish is to blame is that:
https://cs.wikipedia.org/w/index.php?title=MyPage&action=info doesn't redirect to the mobile site and https://cs.wikipedia.org/wiki/MyPage?action=info does.

Is it possible we only redirect pages which use the title= query string parameter and nothing else?

What's the correct repo/phabricator ticket for Varnish? I'd like to dig into the redirect code.

Change 359060 abandoned by Jdlrobson:
Tests for diffs

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

What's the correct repo/phabricator ticket for Varnish? I'd like to dig into the redirect code.

https://github.com/wikimedia/puppet/tree/production/modules/varnish/templates

Jdlrobson renamed this task from Certain diff URLs do not redirect to mobile site on mobile devices to URLs with title query string parameter and additional query string parameters do not redirect to mobile site.Jul 24 2017, 6:11 PM
Jdlrobson updated the task description. (Show Details)

Background: Looks like this was introduced by T103592. The regex should probably be relaxed. It's doing more than it should be...

ema moved this task from Triage to Caching on the Traffic board.Jul 27 2017, 10:41 AM

Can you guarantee to support all the URLs with parameters which would get redirected to MobileFrontend?

Can you guarantee to support all the URLs with parameters which would get redirected to MobileFrontend?

Why not? The MobileFrontend extension simply redirects changes the skin for mobile devices. For the 5 pages MobileFrontend replaces (most notably Watchlist), the goal is to have that functionality more closely aligned with (or inside) core so if there are bugs we'll want to know about them and fix them.

Not redirecting on the other hand is confusing and not helpful for anyone. Note: this only impacts users who have followed a link to the desktop site on a mobile, not users who are using the desktop site on a mobile device.

BBlack added a subscriber: BBlack.Jul 31 2017, 4:57 PM

It seems reasonable to relax the regex in question a bit (to allow additional parameters).

Change 368814 had a related patch set uploaded (by BBlack; owner: BBlack):
[operations/puppet@production] VCL mobile redirect: allow other params alongside title=

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

Something like this maybe?

@@ -23,8 +23,13 @@ sub mobile_redirect {
 		&& req.http.Cookie !~ "(stopMobileRedirect=true|mf_useformat=desktop)"
 		&& req.http.User-Agent !~ "(SMART-TV.*SamsungBrowser)"
 		&& (
-			req.url ~ "^/(wiki|(gan|ike|iu|kk|ku|shi|sr|tg|uz|zh)(-[a-z]+)?)[/\?]"
-			|| req.url ~ "^/(w/index\.php)?\?title=[^&]*$"
+			// T103592
+			req.url !~ "\?.*mobileaction=toggle_view_desktop"
+			&& (
+				req.url ~ "^/(wiki|(gan|ike|iu|kk|ku|shi|sr|tg|uz|zh)(-[a-z]+)?)[/\?]"
+				// T154227
+				|| req.url ~ "^/(w/index\.php)?\?.*title=.*$"
+			)
 		)) {
 
 		// Separate regexps for clarity, but multiple regsubs instead of

Change 368814 merged by BBlack:
[operations/puppet@production] VCL mobile redirect: allow other params alongside title=

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

Jdlrobson closed this task as Resolved.
Jdlrobson claimed this task.

Tested on a mobile device. I can toggle to desktop and all urls in description are working beautifully as well as standard ones such as https://en.wikipedia.org/wiki/Peter_Evans_(swimmer)

Restricted Application added a subscriber: jeblad. · View Herald TranscriptAug 25 2017, 4:16 PM