Seeking input on implementing 'Ability to hide specified post revisions'


(Dave McClure) #1

Continuing the discussion from Ability to hide or delete old revisions on selected posts.

I’ve started to investigate what the best approach would be to implement this feature.

On the front end, I’m assuming that it’d be best to add a button within the edit_history modal which would allow authorized users (probably staff only) to delete a given post revision.

On the backend, things look a little trickier at the moment.

Currently, the post_revision model has these fields:

  • id
  • post_id
  • user_id
  • modifications
  • created_at
  • updated_at

There are a few possible approaches:

  • add a ‘deleted’ flag (this seems to best fit the pattern for handling other deletions like posts)
  • just wipe the record entirely
  • nuke the contents of ‘modifications’

I’m assuming approach #1 would be the right way to go.

OK, so then I started looking more closely at what gets stored in the ‘modifications’ field.

It looks like its a hash in the form:

{ raw: ["old raw text", "new raw text"], cooked: ["old html", "new html"] }

So things get tricky in the case where 2 edits have been made.

  • rev 1: some content
  • rev 2: some content I want to hide
  • rev 3: some new content

In this case, you end up with two post_revision records with “some content i want to hide”.

  • rev 1: raw: ['some content', 'some content I want to hide']
  • rev 2: raw: ['some content I want to hide', 'some new content']

It seems a little odd that both sides of the edit are saved on each revision, but I can understand why it was done that way.

So, my current thinking is that it’d make the most sense to do the following:

  1. add a deleted flag to the post_revision model
  2. the old content of the deleted revision would be hidden from users
  3. have the model itself handle filtering out content deleted revisions

So in the case above, one if you delete an rev2, the model would return:

$ rev.current("raw")
# some new content
$ rev.previous("raw")
# some content 

The other approach is to refactor this model so that it stores each revision separately…

There’s no rush to get this done and I’m open to any input here.


(Zane Beckman) #2

In this case, I think it would be preferable to go for option 2, “just wipe the record entirely”.

If there is sensitive information that should be hidden, why should it be stored in the database?

Additionally, it simplifies the process considerably.

becomes

You could add a flag to the modified revision specifying that a revision has been deleted from the history.


(Dave McClure) #3

Thanks @fysics. I think that would be pretty similar to what I was initially thinking - only the collapsing of the two revisions into one would be done on write instead of read the way you suggested.

@sam and @zogstrip - from the history, looks like the two of you are the most familiar with this code. Let me know if either of you have any strong opinions about the approach here when you have the chance.

(I realize you may be busy with the team meeting)


(Sam Saffron) #4

I think we best start at step 0 here.

  • What problem are we solving?
  • What example use cases do you want to cover?
  • Does this need to hide from public or hide from public + moderators or hide from public + moderators + admins ?

(Dave McClure) #5

The main concern I’ve seen raised is that the old revisions are still visible to the public.

This feature would be synonymous with deleting a post, so I think it would be OK for the deleted revision to remain visible to moderators. (If that’s a concern it should also be addressed for deleting posts).


(Dave McClure) #6

This is done now.

I ended up just adding a hidden flag for post revisions and changing the model to only look at the ‘previous’ side of the changes array for each modification before its serialized.


(Régis Hanol) #7