Page MenuHomePhabricator

Comment lost when attempting to publish to a closed conversation
Closed, ResolvedPublic

Description

This task is about an issue where a comment someone had drafted using the Reply Tool was lost when they attempted to publish said comment to a page/conversation that was "closed" in the time between they started drafted said comment and when they clicked the Reply button.

@ProcrastinatingReader reported this issue at en.wiki here: https://w.wiki/mzx.

Behavior

  1. Open the Reply Tool and start drafting a comment with it
  2. Using a different account, "close" the section in which the comment you were replying to in step "1." belongs, using a template like Template:Archive
  3. Switch back to the account you were using in step "1."
  4. Click the "Reply" button to post the comment you drafted in step "1."

Actual

  1. ❗️The page refreshes and the comment you drafted in step "1." and attempted to publish in step "4." is lost. //See: diff=989736218.

Expected

  1. ✅Show a dialog that contains the following message and call to action:
    • Message: Your comment could not be published to the most recent version of the page. To see the latest changes, copy your drafted comment and then use your browser to reload the page. [i]
    • Call to action: Okay
  2. ✅Once they click Okay and reload the page, show the existing Reload site? Changes you made may not be saved. dialog.
  3. ✅Once they click the Reload call to action in the Reload site? Changes you made may not be saved. dialog, reload the page, scroll them to the section they were drafting their comment in, but do not open the Reply Tool or retain the comment they drafted.

Open questions

  • What should happen when someone attempts to post a comment in a section that is closed in the time between they started drafting their comment and when they click the Tool's Reply button? See: "Step 6." in ===Behavior above.

Testing instrucitons

  • @ppelberg to ask @ProcrastinatingReader to re-try the workflow they went through that led them to experience this issue.

Done

  • "Expected" behavior is implemented

Event Timeline

I can imagine this happening if the conversation is "closed" by wrapping it in some template. Normally, you should get an error message when trying to reply (https://www.mediawiki.org/wiki/Help:DiscussionTools/Why_can%27t_I_reply_to_this_comment%3F#Wrapper_templates). Maybe we don't check that scenario if it happens while you have the reply widget already open.

Change 641818 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/DiscussionTools@master] Catch when no changes are actually saved when posting a comment

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

(I was able to reproduce that locally)

Nice find, @matmarex.

@matmarex, a question for you:

  • As this patch is currently written, would it be accurate for me to assume that when someone attempts to post a comment in a section that is closed in the time between they started drafting their comment and when they click the Tool's Reply button, they will see an error message informing them of the below?
    • The comment they have drafted cannot be published
    • The reason why the comment they have drafted cannot be published
    • What they can do to publish the comment they have drafted

Note: this bit of code [i] is leading me to think the above.


i.

"discussiontools-error-comment-not-saved": "Error message. Parameter: $1 – text of the 'Edit'/'Edit source' tab",
JTannerWMF added a subscriber: JTannerWMF.

The error message will need to be revised considering there is no case when a comment drafted with the reply tool can be saved under these conditions.

I've changed the proposed error message to:

Your comment could not be saved. Please reload the page to see the latest changes, or use the full page editor by clicking "Edit"/"Edit source".

@ppelberg I don't want to refer specifically to a discussion section being closed in the message, because we're not really sure if that's the only workflow that can cause this problem (it's just the only one we've seen). I'd rather have a vague error message than an incorrect one. In case closing the discussion is what really happened, it should be clear enough to the user after reloading the page.

Actually, suggesting to "use the full page editor" might also not be a good idea (if the discussion is closed), so after thinking a bit more, I'm proposing the following:

Your comment could not be saved in the latest version of the page. Please reload the page to see the latest changes.

Change 641818 merged by jenkins-bot:
[mediawiki/extensions/DiscussionTools@master] Catch when no changes are actually saved when posting a comment

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

I've changed the proposed error message to:

Your comment could not be saved. Please reload the page to see the latest changes, or use the full page editor by clicking "Edit"/"Edit source".

@ppelberg I don't want to refer specifically to a discussion section being closed in the message, because we're not really sure if that's the only workflow that can cause this problem (it's just the only one we've seen). I'd rather have a vague error message than an incorrect one.

Understood and agreed.

Actually, suggesting to "use the full page editor" might also not be a good idea (if the discussion is closed)...

Good call.

...so after thinking a bit more, I'm proposing the following:

Your comment could not be saved in the latest version of the page. Please reload the page to see the latest changes.

Before commenting on the language you posted above, a question: In what – if any – cases will someone lose the comment they started drafted after reloading the page, as this message is currently instructing people to do?

I ask this question with the following in mind:

  1. The Reply Tool seems to retain drafted comments after reloading the browser tab in which said comment was drafted in.
    • Steps: open Reply Tool > draft a comment > reload the page > click Reload when the following dialog appears: Reload site? Changes you made may not be saved. > notice the Reply Tool opens with the content you drafted shown inside.
  2. T241404 and T218663 are still open.
    • These tickets being open led me to wonder whether the software handles someone reloading the page (which seems to be supported) differently from someone closing the browser tab (which does not seem to be supported).

They might lose their drafted comment, because the reply widget will probably refuse to open for that comment to which they can no longer reply, but is that still a problem in this scenario? And what do you think we should change to improve this?

Yes, it’s a problem, losing a comment drafted for probably quite a long time, without any chance getting it back, is very frustrating. Even if it should not be posted in an already-closed section, it may be posted somewhere else (probably rephrased), or a senior user may even decide to reopen the section using the full page editor and post their comment anyway. But they need the text to be posted for this.

The best idea I can come up with is a note in that dialog warning the users that the reload may lead to the comment not loaded back, so they should copy the text to the clipboard if they think they’ll need it. Maybe even have a button to copy the text to the clipboard using JavaScript.

They might lose their drafted comment, because the reply widget will probably refuse to open for that comment to which they can no longer reply, but is that still a problem in this scenario? And what do you think we should change to improve this?

Let's do a blend of what you proposed in T268069#6733162, what @Tacsipacsi suggested in T268069#6733997, and what @Whatamidoing-WMF suggested offline and implement the below. I've also added this behavior to the task description.

BEHAVIOR
When someone attempts to publish a comment in a section that's been closed in the time between they started to draft their comment...

  1. Show a dialog that contains the following message and call to action:
    • Message: Your comment could not be published to the most recent version of the page. To see the latest changes, copy your drafted comment and then use your browser to reload the page. [i]
    • Call to action: Okay
  2. Once they click Okay and reload the page, show the existing Reload site? Changes you made may not be saved. dialog.
  3. Once they click the Reload call to action in the `Reload site? Changes you made may not be saved. dialog, reload the page, scroll them to the section they were drafting their comment in, but do not open the Reply Tool or retain the comment they drafted.

i. I think it's okay not to be explicit about the risk of someone's comment being lost at this moment because this information will be communicated to them in "Step 2."

I don't think this needs to be a dialog? I would just present the error message as normal (in a red box above the reply widget). This is not that different from any other error condition, and I don't want to maintain and test two separate versions of error handling forever.

Change 658481 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/DiscussionTools@master] Improve error message used when no changes are saved

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

Change 658482 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/DiscussionTools@master] Don't show error message popups when failing to restore auto-save

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

Here's a recording of the full experience.

Change 658481 merged by jenkins-bot:
[mediawiki/extensions/DiscussionTools@master] Improve error message used when no changes are saved

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

Change 658482 merged by jenkins-bot:
[mediawiki/extensions/DiscussionTools@master] Don't show error message popups when failing to restore auto-save

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

need to review it

I don't think this needs to be a dialog? I would just present the error message as normal (in a red box above the reply widget). This is not that different from any other error condition, and I don't want to maintain and test two separate versions of error handling forever.

Nice improvements. And I appreciate you posting the screencast [i]; it makes seeing what's been implemented a breeze.


i. T268069#6776141