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

In T252558#6328437, @ppelberg wrote:

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

In T252558#6339309, @matmarex wrote:

In T252558#6328437, @ppelberg wrote:

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

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)

In T252558#6350430, @matmarex wrote:

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.

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.

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

In T252558#6352463, @Esanders wrote:

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?

In T252558#6360756, @Esanders wrote:

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

Sure, here are the last two diffs on that page where it can be reproduced:

https://en.wikipedia.beta.wmflabs.org/w/index.php?title=User_talk%3ARYasmeen_%28WMF%29&type=revision&diff=443179&oldid=443176

https://en.wikipedia.beta.wmflabs.org/w/index.php?title=User_talk:RYasmeen_(WMF)&diff=next&oldid=443179

(the changes were reverted due to T259855)

In T252558#6350721, @Ryasmeen wrote:

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

In T252558#6352463, @Esanders wrote:

(…) 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?

In T252558#6350721, @Ryasmeen wrote:

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.

In T252558#6352463, @Esanders wrote:

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.

In T252558#6369656, @matmarex wrote:

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

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.

In T252558#6374443, @matmarex wrote:

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

• @Ryasmeen to test the scenario described in T252558#6350721 on the page Bartosz linked to above

(these changes are reverted again for now)

In T252558#6384420, @matmarex wrote:

(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:

• Test languages
  • Arabic (https://ar.wikipedia.beta.wmflabs.org/)
  • Czech (https://cs.wikipedia.beta.wmflabs.org/)

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

In T252558#6384420, @matmarex wrote:

(these changes are reverted again for now)

And merged again in https://gerrit.wikimedia.org/r/c/mediawiki/extensions/DiscussionTools/+/621240

In T252558#6374443, @matmarex wrote:

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?

In T252558#6425120, @Ryasmeen wrote:

In T252558#6374443, @matmarex wrote:

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:

• https://cs.wikipedia.beta.wmflabs.org/w/index.php?title=Diskuse_s_wikipedistou:RYasmeen_(WMF)&diff=2256&oldid=2255
• https://cs.wikipedia.beta.wmflabs.org/w/index.php?title=Diskuse_s_wikipedistou:RYasmeen_(WMF)&diff=2259&oldid=2258

And I was also able to reproduce it:

• https://cs.wikipedia.beta.wmflabs.org/w/index.php?title=Diskuse:T252558&diff=2262&oldid=2261
• https://en.wikipedia.beta.wmflabs.org/w/index.php?title=Talk:T252558&diff=448708&oldid=448707

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.

In T252558#6425614, @Ryasmeen wrote:

@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 also filed T261788: Improve error message for edit conflict in DiscussionTools, about improving the "Edit conflict." message. Not a blocker either.)

In T252558#6425120, @Ryasmeen wrote:

In T252558#6374443, @matmarex wrote:

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.

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