Page MenuHomePhabricator

Adding a discussion to a user talk page should show the new topic without refresh
Closed, ResolvedPublic3 Estimated Story Points

Description

There is a problem in the workflow to add a new talk topic to any talk page that is not a talk page for the NS_MAIN namespace.

Note: This seems to work fine on a talk page e.g. Talk:Foo but not a user talk page e.g. User talk:Foo or a module talk page e.g. https://en.m.wikipedia.beta.wmflabs.org/wiki/Module talk:Foo#

Reproduction:

  1. Visit a user talk page and tap "add discussion".
  2. Enter some text and press save.
  3. Refresh the page.

Expected

New topics are shown immediately.

Actual

The new topic is added but not shown until refresh.

acceptance criteria

  • There is some logic inside resources/skins.minerva.talk/init.js to reload the page when a talk discussion is added (see inTalkNamespace variable). It doesn't seem to be running in any other namespace than namespace 2. Why?
  • Opportunistic refactoring - instead of using a global event the talk-added-wo-overlay event should be local to the current TalkOverlay.

developer notes

This will require changes in Minerva + MobileFrontend.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 5 2018, 2:05 PM
Niedzielski updated the task description. (Show Details)Jan 5 2018, 2:07 PM
ovasileva triaged this task as Medium priority.Jan 8 2018, 11:43 PM
Restricted Application added a subscriber: TerraCodes. · View Herald TranscriptJan 9 2018, 12:14 AM
Jdlrobson renamed this task from Adding a discussion to a talk page should show the new topic without refresh to Adding a discussion to a user talk page should show the new topic without refresh.Jan 9 2018, 12:35 AM
Jdlrobson updated the task description. (Show Details)
Jdlrobson moved this task from Backlog to Bugs on the MinervaNeue board.Jan 20 2018, 12:34 AM
Jdlrobson set the point value for this task to 3.Feb 6 2018, 5:46 PM
Jdlrobson removed a project: good first task.
Jdrewniak added a comment.EditedFeb 27 2018, 11:43 AM

This looks like it's not just an issue on the user namespace. but on article name spaces as well.

The video below shows me creating a new discussion topic on the article namespace, from the "talk" page instead of the article page, and the same error occurs.
The new topic doesn't show up until the page is refreshed.

Editing or deleting a topic however, does seem to reload the content automatically.

(video link)
https://drive.google.com/file/d/14mKKNmkg1ifbNE56M8HQ00Q-GIPmWIeT/view?usp=sharing

Change 414986 had a related patch set uploaded (by Jdrewniak; owner: Jdrewniak):
[mediawiki/extensions/MobileFrontend@master] Fix talk page refresh after adding content

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

Jdrewniak added a subscriber: Jdlrobson.EditedFeb 27 2018, 12:30 PM

the patch above represents the minimum viable fix. It fixes the bug but the resulting (and intended) behaviour is that the page does a hard refresh automatically, instead of the user having to hit refresh. The new content is not being shown "immediately" after save though. After looking through the code, I think this ideal behaviour can be achieved with some refactoring.

If TalkSectionAddOverlay inherited from EditorOverlayBase instead of TalkOverlayBase, it could take advantage of the onSaveComplete method that reloads the content without a hard refresh (as seen in this video after editing / deleting content) and we wouldn't need the custom logic to do the hard refresh. Also, I see a few FIXME's about using the gateways instead of editor api's directly. @Jdlrobson thoughts?

Change 415272 had a related patch set uploaded (by Jdrewniak; owner: Jdrewniak):
[mediawiki/skins/MinervaNeue@master] Passing pageTitle option to TalkSectionAddOverlay

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

For future reference, I'm explaining the cause of this error.

There's a condition in the code that determines wether or not a user used the talk overlay or came from the talk page. That condition is based on comparing the value of the data-title attribute of the "add discussion" button to the title of the current page. The data-attribute of the button always points to the talk page, whereas the title of the current page could be just the article title (if the user is using the overlay).

The problem was that this.title in TalkSectionAddOverlay contains spaces but mw.config.get( 'wgPageName' ) which gets the title of the current page, contains underscores, so the comparison would always be false.

The fix was to use M.getCurrentPage().title instead of mw.config.get( 'wgPageName' ) for the page title comparison.

Jdlrobson added a subscriber: Jdrewniak.
Jdlrobson removed Jdlrobson as the assignee of this task.Feb 28 2018, 7:36 PM

Change 414986 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Fix talk page refresh after adding content

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

Change 415272 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Passing currentPageTitle option to TalkSectionAddOverlay

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

Can be tested on the beta cluster.

@Jdlrobson, I am still seeing this issue on Beta Cluster. Maybe beta hasn't updated yet. I tried a few talk pages including this one https://en.m.wikipedia.beta.wmflabs.org/wiki/Talk:Sandbox

Looks like this needs a bit more investigation..

Looks like the beta cluster is running old code.
Can you test on reading web staging (word of warning: this is currently configured to use Hindi Wikipedia but that should not impact your testing)

Working for me on Staging

Looks good to me. There's some things in the acceptance criteria I'm not sure I can sign off for. @Jdlrobson - could you take a look?

Jdlrobson claimed this task.Mar 7 2018, 6:04 PM
Jdlrobson added a subscriber: ABorbaWMF.
Jdlrobson removed Jdlrobson as the assignee of this task.Mar 7 2018, 7:45 PM
Jdlrobson added a project: Regression.

This seems to have a caused a regression, that's been picked up by our browser tests - it's now no longer possible to interact with a page which doesn't exist.

Browser tests have been failing for 5 days now. Not sure how we missed that :/

Jdlrobson closed this task as Resolved.Mar 8 2018, 10:49 PM
Jdlrobson claimed this task.

The regression (T189194) was actually caused by another change, so this looks good. Sorry for jumping to conclusions.