Page MenuHomePhabricator

Create a low bandwidth reply API using parser.php/modifier.php
Closed, ResolvedPublic

Description

Now that parser & modifier are available in PHP, the user should be able to post replies without ever fetching the full Parsoid DOM:

ApiDiscussionTools, paction=transcludedfrom

  • Fetches Parsoid doc, runs the parser
  • Returns list of comments which can't be replied to due to transclusion

ApiDiscussionToolsEdit, paction=addcomment

  • Receives the comment ID and reply HTML; fetches Parsoid doc, runs the modifier, converts to wikitext and saves the edit

Testing details

LANGUAGES TO TEST

SEQUENCES TO TEST
The following "sequences" should be tested in *each* of the languages listed in the "LANGUAGES TO TEST" section above. Where "tested in" means the language the test comments are written in should be native to the wiki on which the Reply Tool is being tested. This means when testing the tool on https://cs.wikipedia.beta.wmflabs.org, test comments should be written in Czech.

  • Sequence 1
    • Action: Post many comments to a talk page
    • Outcome: ensure they are all posted properly [i]
  • Sequence 2
    • Action: open and close the Reply Tool in many places on a single page
    • Outcome: ensure the tool opens and closes properly.
  • Sequence 3
    • Action: Cause an error when submitting a reply (e.g. abuse filter), then retry after fixing it (see T245333)
    • Outcome: ensure said comment was posted correctly and only once (it's not duplicated)
  • Sequence 4
    • Action: try using the Reply Tool to a comment that has been transcluded from another page;
    • Outcome: ensure the comment you posted with the Reply Tool is posts properly in both places: the page from which the comment you are responding was transcluded and the page on which the comment you are responding to appears.
  • Sequence 5
    • Action: try using the Reply Tool to respond to a comment that has since been deleted
    • Outcome: ensure the appropriate error message is shown
  • Sequence 6
    • Action: Try using the Reply Tool to post multiple replies to the same comment (nearly) simultaneously, while using two different accounts (using incognito mode, or two different browsers or devices)
    • Outcome: ensure all comments have been posted properly [i]

Done

  • The "Testing details" section is accurate and exhaustive
  • All "Sequences" in the "Testing details" section pass/work as expected
  • The API is re-deployed during a backport window on 2-September [ii]

i. "Properly" means they are indented correctly, no extraneous syntax/content has been added to the comment, no syntax/content has been removed from the comment and no other parts of the page have been changed.//
ii. https://wikitech.wikimedia.org/wiki/Deployments#Wednesday,_September_02

Related Objects

Event Timeline

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

Editing Engineering: when y'all finish code with review, can you please populate the newly-created "Testing details" section [i] of the task description with testing instructions @Ryasmeen can follow once this ticket is ready for QA?


i. I've filled in what Ed shared with me in the discussion we had offline about this on 7-July

ppelberg updated the task description. (Show Details)Jul 23 2020, 2:36 AM
matmarex updated the task description. (Show Details)Jul 27 2020, 11:34 PM

Editing Engineering: when y'all finish code with review, can you please populate the newly-created "Testing details" section [i] of the task description with testing instructions @Ryasmeen can follow once this ticket is ready for QA?

I made two changes:

  1. Removed "Sequence 3: click [ Reply ] on many comments on a single page…" – the patch for that isn't merged yet, and it has a separate task: T257305
  2. Instead added "Cause an error when submitting a reply, then retry after fixing it…" – this is a regression test for T245333, the code handling that had to be changed
ppelberg added a comment.EditedJul 27 2020, 11:53 PM

Editing Engineering: when y'all finish code with review, can you please populate the newly-created "Testing details" section [i] of the task description with testing instructions @Ryasmeen can follow once this ticket is ready for QA?

I made two changes:

  1. Removed "Sequence 3: click [ Reply ] on many comments on a single page…" – the patch for that isn't merged yet, and it has a separate task: T257305
  2. Instead added "Cause an error when submitting a reply, then retry after fixing it…" – this is a regression test for T245333, the code handling that had to be changed

Both of the above sounds great – thank you for handling, @matmarex.

Once code review is finished, can you please move this to the Editing QA board's "High Priority" column?

Change 608110 merged by jenkins-bot:
[mediawiki/extensions/DiscussionTools@master] Edit API for replies

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

Change 604419 merged by jenkins-bot:
[mediawiki/extensions/DiscussionTools@master] Create a 'transcludedfrom' API endpoint

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

Change 608934 merged by jenkins-bot:
[mediawiki/extensions/DiscussionTools@master] Use transcluded from API to avoid ever fetching Parsoid DOM in client

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

matmarex edited projects, added Editing QA; removed Patch-For-Review.
matmarex moved this task from Inbox to High Priority on the Editing QA board.

Change 617169 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/DiscussionTools@wmf/1.36.0-wmf.2] Revert new reply API for now (1.36.0-wmf.2 only)

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

We've decided that we want this to go out next week in wmf.3 instead, to give us more time to QA it on Beta.

Scheduled: https://wikitech.wikimedia.org/wiki/Deployments#deploycal-item-20200729T1800

Change 617169 merged by jenkins-bot:
[mediawiki/extensions/DiscussionTools@wmf/1.36.0-wmf.2] Revert new reply API for now (1.36.0-wmf.2 only)

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

Mentioned in SAL (#wikimedia-operations) [2020-07-29T18:13:14Z] <urbanecm@deploy1001> Synchronized php-1.36.0-wmf.2/extensions/DiscussionTools/: 00ecec80d12a34977d55dd09bce0c5a1aab369f9: Revert new reply API for now (T252558) (duration: 01m 06s)

Question RE testing sequences

  • @Esanders: for Scenario 6 [i], what is meant by "rapid succession"? Does this mean @Ryasmeen should use multiple devices to respond to the same comment as close to simultaneously possible? To use one device to post replies to the same comment as quickly as possible in different browser tabs? In the same browser tab? Something else entirely?

i.

  • Sequence 6
    • Action: Try using the Reply Tool to post multiple replies to the same comment in rapid succession
    • Outcome: ensure all comments have been posted properly [i]

I think that actually should be "simultaneously" – I believe that scenario is supposed to check for regressions in our edit conflict handling (T240643). So I'd actually phrase it as:

  • Action: Try using the Reply Tool to post multiple replies to the same comment (nearly) simultaneously, while using two different accounts (using incognito mode, or two different browsers or devices)

I think that actually should be "simultaneously" – I believe that scenario is supposed to check for regressions in our edit conflict handling (T240643). So I'd actually phrase it as:

  • Action: Try using the Reply Tool to post multiple replies to the same comment (nearly) simultaneously, while using two different accounts (using incognito mode, or two different browsers or devices)

Understood, ok. Task description updated. Thank you, Bartosz.

ppelberg updated the task description. (Show Details)Jul 30 2020, 11:19 PM

Checked all the sequences. Sequence 1 to Sequence 5 seem to be working as expected.

Sequence 6 turned out to be a bit inconsistent. It seems that the issue in T240643 came back but in a slightly different way. I am listing those scenarios below:

  1. Logged in User A posts reply to a comment using Chrome and simultaneously another anonymous user posts reply to the same comment using Safari incognito mode.

Observed behavior: The reply from User A gets lost.

  1. Another behavior that I observed is, if the same account posts reply to a comment using two different browsers one of them gets retained (maybe the one that's slightly latest). This is probably expected but still wanted to confirm.

I have not checked using two devices yet. Let me know if you want me to do that as well. I can do Windows-Mac, Mac-Mac combinations.

It's possible but very unlikely to get edit conflicts with this tool. The two requests would need to arrive at the server probably within a second of each other. That said when it does happen, the behaviour you describe in (2) sounds correct as these are the rules for same-user edit conflicts.

I couldn't reproduce the issue you described in (1). Did both users see the comment they just posted highlighted? The first users to complete will see just their comment, while the second users will see two new comments when the page updates.

I have not checked using two devices yet. Let me know if you want me to do that as well. I can do Windows-Mac, Mac-Mac combinations.

I shouldn't think this would make a difference.

Esanders updated the task description. (Show Details)Jul 31 2020, 5:57 PM
Ryasmeen added a comment.EditedJul 31 2020, 9:27 PM

It's possible but very unlikely to get edit conflicts with this tool. The two requests would need to arrive at the server probably within a second of each other. That said when it does happen, the behaviour you describe in (2) sounds correct as these are the rules for same-user edit conflicts.

I couldn't reproduce the issue you described in (1). Did both users see the comment they just posted highlighted? The first users to complete will see just their comment, while the second users will see two new comments when the page updates.

I have not checked using two devices yet. Let me know if you want me to do that as well. I can do Windows-Mac, Mac-Mac combinations.

I shouldn't think this would make a difference.

@Esanders: Yes, so I see that both users initially see their comments being posted and highlighted. But after refreshing the page both the first user (logged-in chrome user) and the second user (anonymous user using Safari incognito) see the second user's comment.

Thanks, can you link to a diff of this happening?

ppelberg reassigned this task from Esanders to Ryasmeen.Aug 4 2020, 6:09 PM
Ryasmeen reassigned this task from Ryasmeen to Esanders.Aug 5 2020, 7:03 PM

(the changes were reverted due to T259855)

  1. Another behavior that I observed is, if the same account posts reply to a comment using two different browsers one of them gets retained (maybe the one that's slightly latest). This is probably expected but still wanted to confirm.

I have not checked using two devices yet. Let me know if you want me to do that as well. I can do Windows-Mac, Mac-Mac combinations.

(…) That said when it does happen, the behaviour you describe in (2) sounds correct as these are the rules for same-user edit conflicts.

We have actually fixed this previously in T246726 – both comments should be retained, even if they were posted by the same user.

The fix is to send a baserevid instead of basetimestamp to the edit API. I noticed that the new reply API sends both of them. I'm not sure if this can cause the issue, but we should probably remove basetimestamp just in case?

  1. Logged in User A posts reply to a comment using Chrome and simultaneously another anonymous user posts reply to the same comment using Safari incognito mode.

Observed behavior: The reply from User A gets lost.

I couldn't reproduce the issue you described in (1). Did both users see the comment they just posted highlighted? The first users to complete will see just their comment, while the second users will see two new comments when the page updates.

I also couldn't reproduce locally.

This is making me think that this problem is caused by querying some data from replica databases instead of the master database. My local setup has only one database, so this wouldn't occur there, and Ed's is probably the same.

In ApiParsoidTrait::getLatestRevision(), when calling getRevisionByTitle(), we should probably pass the IDBAccessObject::READ_LATEST flag? Not sure if any other changes are needed.

This is making me think that this problem is caused by querying some data from replica databases instead of the master database. My local setup has only one database, so this wouldn't occur there, and Ed's is probably the same.

In ApiParsoidTrait::getLatestRevision(), when calling getRevisionByTitle(), we should probably pass the IDBAccessObject::READ_LATEST flag? Not sure if any other changes are needed.

On second thought, I don't think this is the problem, and I don't think we need this change.

I tested locally by changing getLatestRevision() so that it actually doesn't return the latest revision (added $latestRevision = $revisionLookup->getPreviousRevision( $latestRevision );), which should simulate that situation. I still didn't reproduce incorrectly resolved edit conflicts – only correctly resolved ones, or an error message when it couldn't be resolved.

Change 619337 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/DiscussionTools@master] CommentController: Remove remains of client-side edit conflict handling

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

Change 619345 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/DiscussionTools@master] ApiDiscussionToolsEdit: Do not pass 'basetimestamp'

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

Change 619337 merged by jenkins-bot:
[mediawiki/extensions/DiscussionTools@master] CommentController: Remove remains of client-side edit conflict handling

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

Change 619345 merged by jenkins-bot:
[mediawiki/extensions/DiscussionTools@master] ApiDiscussionToolsEdit: Do not pass 'basetimestamp'

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

matmarex removed a project: Patch-For-Review.

I've been testing at https://en.wikipedia.beta.wmflabs.org/wiki/Talk:T252558 and I can't reproduce the problem any more. Hopefully that really fixed it and I didn't just get lucky.

ppelberg added a comment.EditedAug 13 2020, 10:22 PM

I've been testing at https://en.wikipedia.beta.wmflabs.org/wiki/Talk:T252558 and I can't reproduce the problem any more. Hopefully that really fixed it and I didn't just get lucky.

Next step

(these changes are reverted again for now)

(these changes are reverted again for now)

Good call, @matmarex. I'm going to move this to "Blocked..." on our board. Once we get to the bottom of the issues reported in T259855 we can hand this task back over to Rummana to test.

Update to testing instructions
I've added the following to the task description's "Testing details" section:

The reason for us expanding the scope of the Reply Tool's regression testing are as follows:

  • During the team's 25-August retrospective meeting, we came to think it would've been more likely that we would have identified "Issue 1" (described in T259855's task description) before it presented itself on production had the Reply Tool's pre-deployment QA process included the following:
    • Writing and publishing comments in a language that has accented characters
    • Writing and publishing comments in a language that as an RTL orientation
ppelberg updated the task description. (Show Details)Wed, Aug 26, 11:02 PM
ppelberg reassigned this task from Esanders to Ryasmeen.Thu, Aug 27, 6:28 PM
Ryasmeen added a comment.EditedMon, Aug 31, 8:35 PM

I've been testing at https://en.wikipedia.beta.wmflabs.org/wiki/Talk:T252558 and I can't reproduce the problem any more. Hopefully that really fixed it and I didn't just get lucky.

@matmarex: Issue 2 has been resolved yeah. But I am still seeing Issue 1 where the reply from a logged in user is getting lost and the anonymous one is getting retained when posted simultaneously to the same comment.

Actually, I found this happening, when I used two different accounts to reply to the same comment at the same time. So, the second user doesn't have to be an anonymous user.

@matmarex: Also, for sequence 6, I am noticing that, sometimes I am getting the error message "Edit Conflict" for the second user and sometimes, they just get posted one after the other just fine. Is there a specific criteria that will trigger this message?

I've been testing at https://en.wikipedia.beta.wmflabs.org/wiki/Talk:T252558 and I can't reproduce the problem any more. Hopefully that really fixed it and I didn't just get lucky.

@matmarex: Issue 2 has been resolved yeah. But I am still seeing Issue 1 where the reply from a logged in user is getting lost and the anonymous one is getting retained when posted simultaneously to the same comment.

Actually, I found this happening, when I used two different accounts to reply to the same comment at the same time. So, the second user doesn't have to be an anonymous user.

For reference:

And I was also able to reproduce it:

Looks like it might only happen when replying to the very last comment on the page? That's a little crazy, but it was the only way I reproduced it.

@matmarex: Also, for sequence 6, I am noticing that, sometimes I am getting the error message "Edit Conflict" for the second user and sometimes, they just get posted one after the other just fine. Is there a specific criteria that will trigger this message?

This is expected – it just depends on the timing, and database load at the moment (that is, on whether your previous message is already present in replica databases or not).

We previously had some code to automatically retry in this case, although we removed it, because it's difficult to decide how many times and after what delay we should retry, and we have no way to actually force the replication to happen or guarantee that it did, so the error could still occur, just less often. I think this error will be very rarely seen by users in normal usage, only when two comments are posted at nearly exactly the same time, but we could probably add that back to make it even rarer.

I tried reproducing the problem also using the regular good old wikitext editor, just adding a new comment at the end.

The first edit went through: https://en.wikipedia.beta.wmflabs.org/w/index.php?title=Talk:T252558&diff=448711&oldid=448710

The second showed me an extremely weird edit conflict screen:

Note how MediaWiki realized that there was an edit conflict, but it produced the contents of the wrong "Latest revision" (it's actually the revision prior to the one I just saved).

I think this bug is in MediaWiki and not in our new code, and either it has always existed and we've just never noticed it, or it was introduced in the last few months after the last time you thoroughly tested the behavior of edit conflicts.

And in another attempt using the old wikitext editor, it indeed just completely lost the previous comment: https://en.wikipedia.beta.wmflabs.org/w/index.php?title=Talk:T252558&diff=448713&oldid=448712

I filed a separate task about the comments lost due to undetected edit conflicts: T261715: MediaWiki doesn't detect edit conflicts when replica databases don't have the latest edit. I don't think this is a blocker, and I think we should ask the Core Platform Team to have a look at this.

Unrelatedly, I noticed a different issue with the reply API today: T261706: Can't reply to comments on wikis with digit transforms using new reply API (ckb.wp) (it's not reproducible on any of the wikis we tested), and I think this one should be a blocker.

Just to be clear, the reason we think this shouldn't be a blocker is because this issue is unrelated to our tool, and is just as likely to happen when using any other editor. We just happened to find it because we were specifically testing very fast edit conflicts.

I've been testing at https://en.wikipedia.beta.wmflabs.org/wiki/Talk:T252558 and I can't reproduce the problem any more. Hopefully that really fixed it and I didn't just get lucky.

@matmarex: Issue 2 has been resolved yeah. But I am still seeing Issue 1 where the reply from a logged in user is getting lost and the anonymous one is getting retained when posted simultaneously to the same comment.

@Ryasmeen, during today's standup, @matmarex confirmed that the remaining issue you identified above ( "Issue #1") should now be fixed.

Outside of "Issue #1," it seems the only remaining issue we're currently aware of was the one Rummana identified around "Sequence 6" [i].

Considering, Bartosz commented the behavior Rummana observed is expected [ii] AND assuming no other issues surface, it seems all that's left on this task is to re-test "Issue #1." If this is not the case please comment as much.

Next steps

  • Test "Issue #1" on Beta

i. T252558#6425614
ii. T252558#6425694

Indeed.

I've also re-tested it a few times again (https://en.wikipedia.beta.wmflabs.org/w/index.php?title=Talk:T252558&action=history) and I couldn't reproduce the problem any more.

Yes, I re-tested it few times now and it seems Issue#1 is fixed.

ppelberg reassigned this task from Ryasmeen to matmarex.Tue, Sep 1, 11:32 PM

Indeed.

I've also re-tested it a few times again (https://en.wikipedia.beta.wmflabs.org/w/index.php?title=Talk:T252558&action=history) and I couldn't reproduce the problem any more.

Yes, I re-tested it few times now and it seems Issue#1 is fixed.

Excellent. Considering all outstanding issues related to the API have been resolved, I'm assigning this task over to @matmarex to re-deploy the API, along with T261706, during one of tomorrow's backport windows. [i]


i. https://wikitech.wikimedia.org/wiki/Deployments#Wednesday,_September_02

ppelberg updated the task description. (Show Details)Tue, Sep 1, 11:33 PM

Change 623560 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/DiscussionTools@wmf/1.36.0-wmf.6] Re-apply new reply API patches (again)

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

Change 623560 merged by jenkins-bot:
[mediawiki/extensions/DiscussionTools@wmf/1.36.0-wmf.6] Re-apply new reply API patches (again)

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

ppelberg closed this task as Resolved.Sat, Sep 5, 12:51 AM
Restricted Application added a project: User-Ryasmeen. · View Herald TranscriptSat, Sep 5, 12:51 AM