Draft not cleared properly when empty edit of post is saved

(Angus McLeod) #1

Continuing the discussion from Topic Ratings Plugin.

Original report in that topic was slightly incorrect, so here’s a proper bug report.

Steps to reproduce:

  1. Edit a post.
  2. Trigger a draft to be saved (e.g. add a space or two and wait for the draft to be saved)
  3. Save the reply without any changes (e.g. remove the space(s)).
  4. Refresh the page.
  5. The composer will re-open when the page refreshes.

The method triggering the composer reopening in this scenario is here.

(Jeff Atwood) #2

What would be the fix? Don’t save drafts that are only spaces? Not really following the suggestion here…

(Angus McLeod) #3

I think the issue is that when the saved reply is the same as the originalText, the draft with the change (i.e. with the extra space) is not removed, which causes this composer.open function to be triggered (as the draft is not empty) when the topic-from-params route is loaded again.

This is an educated guess though, as I wasn’t quite able to track down (or, at least, understand) the exact source of the ‘draft’ appearing in topic-from-params route in this scenario.

If I’m correct, I would suggest this is technically a bug, albeit a minor one. Having the composer reopen on refresh after saving an edit with no changes is probably not expected behavior. Of course, triggering a draft to be saved, and then not making any changes when actually saving the edit would be rare.

It is a bit more relevant if you’re adding extra content to a post, like in the Topic Ratings Plugin, as the likelihood in which posts will be edited without changing the reply itself increases. Given this, I’ve tried to get to the bottom of the issue, but haven’t had much luck so far. I’m going to give it another shot soon, but thought I’d file this in the meantime.

(Angus McLeod) #4

What would be the fix? Don’t save drafts that are only spaces? Not really following the suggestion here…

To answer your question, I think the fix would be to ensure all drafts made in the process of editing are removed, even if the saved edit is exactly the same as the original post.

(Mittineague) #5

If it is decided to not change the post in any way, isn’t that what “cancel” is for?

(Angus McLeod) #6

Yes. But the 'Save edit" button still works in this scenario currently. One way to fix this would be to just disable the “Save edit” button when no changes are made. Personally, I would prefer the clearing of the drafts, as that would have the ancillary effect of fixing my problem with the Topic Ratings plugin. But I’m biased :slight_smile:. Albeit, I would also suggest that leaving unnecessary drafts lying around is probably not for the best.

I’ll see if I can fix this myself tomorrow…

(Angus McLeod) #7

I think this is what’s happening here.

After an edit (any edit), unlike after a new post, the destroyDraft method is never called following the resolution of the save promise in the composer controller. See here.

The reason why this causes the composer to reopen if the saved edit was the same as the original post is because the should_revise? method in the PostRevisor returns false (as the post has not changed), meaning that the draft sequence is not incremented.

In contrast, if the edit changes the post the draft sequence is incremented in the PostRevisor. When the draft sequence is incremented, drafts with an older sequence are removed by this sql call (I think).

The topic draft is always the latest in the draft sequence. So when the route is refreshed the uncleared draft is the latest in the sequence if the edit contained no changes, whereas the new draft sequence is the latest if the edit contained changes.

There are two ways to address this:

  1. Always clear the draft on the resolution of the save promise in the compose controller, regardless of whether it was a saved post or edit, e.g. have a single self.destroyDraft() outside of any conditional; or

  2. Always increment the draft sequence in the PostRevisor, regardless of whether the edit contains changes to the post or not, e.g. move the advance_draft_sequence method call prior to the should_revise? method call.

I’m not entirely sure which is the best route to take as I assume there were originally reasons for restricting the clearing of the drafts in the save promise and for the structure of the PostRevisor. However, there doesn’t seem to me be a a priori reason for treating the drafts of posts different from the drafts of edits, and the sql call in the self.next! method in DraftSequence seems like more of a backup, rather than a solution. It would seem to be more consistent to handle the clearing of drafts of both edits and new posts the same way.

So, I suggest the move here is ‘1’:

(Jeff Atwood) #8

You should get feedback from @sam first as he worked a lot on this.

(Angus McLeod) #9

@sam New PR created with the simplest version of approach 2:

(Alan Tan) #10

@angus I see the PR has been merged. Has this been fixed?

(Angus McLeod) #11

@tgxworld Yes it has. This is resolved.

(Alan Tan) #12

Thank you! :slight_smile:

(Alan Tan) #13