Page MenuHomePhabricator

How should the apps handle Parsoid non-editable and pseudo-sections (section IDs -1 and -2)?
Closed, ResolvedPublic

Description

Summary
A user on English Wikipedia is receiving an error message when trying to edit a particular page. First reported here: https://en.wikipedia.org/wiki/Wikipedia:Village_pump_(technical)#Technical_glitch

Description
From the user, "While clicking edit at any deletion discussion at WP:DSI, the message We're sorry. The Wikipedia app has experienced an error and was terminated. Would you like to Start over or Quit? is displayed."

I've asked the user for more details on the talk page and will update the task once I know more.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Dbrant subscribed.

@CKoerner_WMF Thanks for relaying here.
The crash should be fixed in the latest version of the app. (Please make sure the user has the latest version)
However, the real issue is that the app is not yet really intended for viewing or editing pages outside of the main namespace. Part of the problem is actually rooted in Parsoid and/or the Content Service, which doesn't render certain pages outside the main namespace correctly. Although the app no longer crashes, it will still not allow editing of this type of page.

In this particular case, if we look at the Content Service output for this page, we see that it assigns an incorrect section id (-1) to all sections after the first two:
https://en.wikipedia.org/api/rest_v1/page/mobile-sections-lead/Wikipedia:WikiProject_Deletion_sorting%2FIndia

I'm tagging the Content Service folks, in case they have any additional input (or in case it turns out that this issue can be easily resolved).

Dbrant renamed this task from Android app crashes when editing pages at WP:DSI on English Wikipedia to Content service assigns incorrect ids to sections in certain pages..Feb 14 2018, 9:39 PM

@Dbrant The -1 or -2 can theoretically happen on any page. Practically I have only observed that on non-main namespaces (talk or project are other namespaces that come to mind), though. This comes from Parsoid[1]. IIUC those section ids mean that the section is not editable, mainly due to some template transclusion of a page with some HTML tags happening. If you want we could make a patch in MCS to change the values to something else. The problem is figuring out what that something else should be.

/me puts on my brainstorming hat on: We could use the previous section id value. Although this has the side effect that positive ids would be not unique anymore. Not sure if that would cause other problems.

[1] https://www.mediawiki.org/wiki/Parsing/Notes/Section_Wrapping#Plan_of_Record:_Implementation_proposal

Adding to that, I think for now the Android app should treat sections with negative section ids as uneditable, and wouldn't even show the pencil icon for these. On this article, editing the "India" section in the Android app works but none of its subheadings.

Ideally, Parsoid would give us the page title of the transcluded page used to create the section as well. Then we could expose it in mobile-sections and the app could use that info to edit the transcluded page directly. I've created T187507 to explore this possibility from a Parsoid perspective.
In addition to editing the correct page or instead of offering the pencil icon on the transcluding page the app could opt to show a special icon there which would open the transcluded page.

Mholloway renamed this task from Content service assigns incorrect ids to sections in certain pages. to How should the apps handle Parsoid non-editable and pseudo-sections (section IDs -1 and -2)?.Mar 22 2018, 5:15 PM

TL;DR the apps shouldn't consider sections with IDs -1 or -2 editable and should not display them in UI such as the table of contents. Does that resolve this issue or is further action needed?

@Mholloway The part I'm not sure about is that this seems to be implicit. -1 and -2 does not clearly mean "non-editible" without knowledge of Parsoid's/MCS's business logic

It would be much more clear to clients if sections were actually marked as "non-editable". In Mobile Sections a JSON property in the section would work. In PCS I think, the metadata endpoint would be the right place for this information.

What do you think?

I guess we could add an editable: false to the toc entry in the metadata for these.

I guess we could add an editable: false to the toc entry in the metadata for these.

That seems like the most natural semantics, but might be suboptimal for clients with JS and other weakly typed languages. The natural thing would be to check for if (section.editable) but that would be falsy in both cases (either undefined or false). Maybe it would be better to set non-editable: true as needed.

That being said... do clients have any use for -1 and -2 sections, or are they just for Parsoid's internal bookkeeping? I can't think of any. There might be a good argument that they should just be filtered out as needed in MCS/PCS before sending along to clients.

The only reason for their existence I can come up with right now is to have the entry in the ToC and be able to jump to that section. In general I think the app just should add the edit button if the section number is non-negative.

Change 427453 had a related patch set uploaded (by Mholloway; owner: Mholloway):
[mediawiki/services/mobileapps@master] Flag non-editable sections

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

@Mholloway The dash in the non-editable makes it a bit harder to deal with. Are there any alternate names you like?

@Mholloway The dash in the non-editable makes it a bit harder to deal with. Are there any alternate names you like?

Yeah, a bit annoying. Maybe uneditable or noedit?

All of these solutions are fine with me…

As far as the clients needing them… I don't know. Is there any reason they need an id? I know that ToCs use fragments to jump to sections - so I don't think ids are used for that… but maybe in Android?

Do we know whether the are -1/-2 because we expect clients to sort the sections based on section id? ¯\_(ツ)_/¯

FYI I moved this to the kanban board to do column, but you don't have to do this before other work, just wanted to resolve the open question

Change 427453 merged by jenkins-bot:
[mediawiki/services/mobileapps@master] Flag non-editable sections with 'noedit: true'

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

LGoto triaged this task as Medium priority.Apr 18 2018, 8:17 PM

Android uses the section id for determining which section is being edited. We'll be happy to see a noedit parameter to prevent editing of sections that are transcluded or otherwise weird.

Change 427555 had a related patch set uploaded (by Mholloway; owner: Mholloway):
[apps/android/wikipedia@master] Don't add edit button to 'noedit' sections

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

Change 427555 merged by jenkins-bot:
[apps/android/wikipedia@master] Don't add edit button to 'noedit' sections

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

We should either keep this open for Android unless the Android team has a task for the app work for this.

The merged patch above removes the edit button for non-editable sections. @Dbrant I'll leave it to you to close, then, if that resolves this issue.

The merged patch above removes the edit button for non-editable sections

Interesting. I see the edit button for the sub-sections of the "India" section of the WP:DSI page. Tapping on them has no effect, though. Are the edit buttons being shown incorrectly? This is on version 2.7.235-alpha-2018-06-22 of the app.

The merged patch above removes the edit button for non-editable sections

Interesting. I see the edit button for the sub-sections of the "India" section of the WP:DSI page. Tapping on them has no effect, though. Are the edit buttons being shown incorrectly? This is on version 2.7.235-alpha-2018-06-22 of the app.

Yes, thanks for spotting, this appears to have regressed.

Jhernandez claimed this task.
Jhernandez subscribed.

Android seems to have this in tracking and there is a task (T198088) specifically for the regression so I'm closing this one.