Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #2949: Sample image replacement ability #3015

Merged
merged 9 commits into from May 13, 2017

Conversation

evazion
Copy link
Member

@evazion evazion commented May 3, 2017

Fixes #2949. Adds the ability to replace the image file of an existing post.

  • Adds a "Replace Image" link in the options sidebar. Replacement is approvers up.
  • A delayed job deletes the old files 3 days later. This gives time for recovery in case of mistakes.
  • A mod action is logged containing all the old file metadata, to track what exactly changed.

Some corner cases are not handled yet and so are disallowed:

  • Replacing posts that have notes is disallowed. Notes need to be rescaled to the new image dimensions.
  • Replacing posts hosted on S3 is disallowed. There needs to be a way to delete S3 hosted files.
  • Replacing ugoiras is disallowed. The old frame data will need to be replaced.

@BrokenEagle
Copy link
Collaborator

Can this be tested for a while on a test system first. IMO deleting files is huge, and it'd be wise to iron out as many kinks as possible before these commits go live.

@evazion
Copy link
Member Author

evazion commented May 3, 2017

I have it running on http://feat-replace-images.devbooru.ml. login: albert / password: password.

Deleting the old files isn't strictly necessary. Actually I don't think it would hurt to keep them, beyond leaving some loose files on the server and wasting a little disk space. I don't know how albert feels about that though, so I set it to delete the old files in a delayed job three days later. The grace period could be extended if we want.

If there's a mistake, the mod action has a link to the old file still on the server, so it's possible to redownload it before it's deleted. It should also be possible to cancel the delayed job to prevent the deletion, although that would need albert's intervention. It would be possible too to have the delayed job list which files it's going to delete, although I didn't do this.

@Type-kun
Copy link
Collaborator

Type-kun commented May 3, 2017

Hm, the feature itself is pretty nice, but given the amount of samples and the fact that only mods can replace the images, I feel that some sort of request system will soon be necessary as well. Maybe it can be handled using the forums, but it'll probably get clunky.

@kittey
Copy link
Contributor

kittey commented May 3, 2017

While it’s a bit inconsistent to all the other permissions, could sample replacement be allowed for users that are builder+ and have approval right? That way, it wouldn’t be possible to replace samples for regular users and non-approvers.

Currently, we have 38 builders with approval rights out of 604 builders in total.

@BrokenEagle
Copy link
Collaborator

BrokenEagle commented May 3, 2017

I did some initial testing on this. I left my findings on the Danbooru forums.

http://danbooru.donmai.us/forum_posts/130737

@evazion
Copy link
Member Author

evazion commented May 3, 2017

I did some initial testing on this. I left my findings on the Danbooru forums. http://danbooru.donmai.us/forum_posts/130737

I'll respond more fully on the forum. I agree with most of this, but I wanted to keep the first pass simple and see what albert thinks before going too far ahead with it.

@r888888888
Copy link
Collaborator

r888888888 commented May 3, 2017

  • The replacement image will need to be saved to S3 backup somehow.
  • What to do when the higher quality image has already been uploaded?
  • Mod actions get pruned regularly. I think it would be smart to log any actions in a system comment also.
  • I think the deletion grace period should be much more than 3 days. Even 30 days may be appropriate. And an easier interface for aborting a scheduled delete.
  • Or to generalize, provide an easy way to undo a replace action.
  • I think it's worthwhile coming up with a new model to track and centralize replacement actions.
  • A side benefit of a separate model is it would trivialize reporting. I'm not sure how useful it would be but it could definitely highlight abuse or vandalism.

@evazion
Copy link
Member Author

evazion commented May 3, 2017

What to do when the higher quality image has already been uploaded?

I think we'd have to keep doing what we currently do: delete the old sample and move the favorites to the parent (related: #2936).

Going forward, we might want something on the upload page directing people to replace samples so that they don't keep uploading things that should be replaced instead.

Mod actions get pruned regularly. I think it would be smart to log any actions in a system comment also.

IMO mod actions don't need to be pruned as aggressively as they are. We're at id 177063, which is really not that much even if nothing was deleted. At the least, the pruner could only prune the least important things.

I think the deletion grace period should be much more than 3 days. Even 30 days may be appropriate. And an easier interface for aborting a scheduled delete.

30 days is fine by me. What I was thinking was that jobs in the /delayed_jobs listing could have a cancel button for admins. I don't know if that's easy enough, but it could be useful for hung aliases/implications as well.

I think it's worthwhile coming up with a new model to track and centralize replacement actions.

I thought about this and basically I think that flags, appeals, approvals, and disapprovals could possibly be unified into a single post events STI table. These things all have very similar schemas and share a fair amount of duplicated code. Post replacements could be a part of that system, as they'd just be another kind of event in a post's history.

@evazion
Copy link
Member Author

evazion commented May 6, 2017

The replacement image will need to be saved to S3 backup somehow.

#3030 is an attempt at this. It would backup files to S3 immediately on post creation/replacement, instead of in a cronjob. What do you think of that approach?

@r888888888 r888888888 merged commit dc02dcf into danbooru:master May 13, 2017
@r888888888
Copy link
Collaborator

I've added the model in 78b08d8.

@evazion evazion deleted the feat-replace-images branch May 13, 2017 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants