From 69222afac8f8c41d90295b33f0695bbff352851e Mon Sep 17 00:00:00 2001 From: Julien Maulny Date: Fri, 15 Nov 2019 19:05:08 +0100 Subject: [PATCH] Soft delete video comments instead of detroy --- .../comment/video-comment.component.html | 47 ++++++++++++---- .../comment/video-comment.component.scss | 11 +++- .../comment/video-comment.component.ts | 2 +- .../comment/video-comment.model.ts | 16 ++++-- .../comment/video-comments.component.ts | 35 +++--------- server/controllers/activitypub/client.ts | 13 +++-- server/controllers/api/videos/comment.ts | 12 ++-- .../0450-soft-delete-video-comments.ts | 36 ++++++++++++ .../lib/activitypub/process/process-delete.ts | 7 ++- server/lib/video-comment.ts | 9 ++- server/models/account/account.ts | 2 +- server/models/video/video-comment.ts | 33 +++++++++-- server/tests/api/videos/multiple-servers.ts | 56 ++++++++++++++++--- server/tests/api/videos/video-comments.ts | 24 ++++++-- .../activitypub/objects/common-objects.ts | 11 ++++ shared/models/videos/video-comment.model.ts | 2 + 16 files changed, 237 insertions(+), 79 deletions(-) create mode 100644 server/initializers/migrations/0450-soft-delete-video-comments.ts diff --git a/client/src/app/videos/+video-watch/comment/video-comment.component.html b/client/src/app/videos/+video-watch/comment/video-comment.component.html index 60b803206..6ec35d63b 100644 --- a/client/src/app/videos/+video-watch/comment/video-comment.component.html +++ b/client/src/app/videos/+video-watch/comment/video-comment.component.html @@ -1,22 +1,45 @@
- Avatar + Avatar + +
-
Highlighted comment
+ +
Highlighted comment
- -
+ +
-
-
Reply
-
Delete
-
+
+
Reply
+
Delete
+
+ + + + + +
+ This comment has been deleted +
+ { - // Delete the comment in the tree - if (commentToDelete.inReplyToCommentId) { - const thread = this.threadComments[commentToDelete.threadId] - if (!thread) { - console.error(`Cannot find thread ${commentToDelete.threadId} of the comment to delete ${commentToDelete.id}`) - return - } - - this.deleteLocalCommentThread(thread, commentToDelete) - return - } - - // Delete the thread - this.comments = this.comments.filter(c => c.id !== commentToDelete.id) - this.componentPagination.totalItems-- + // Mark the comment as deleted + this.softDeleteComment(commentToDelete) if (this.highlightedThread.id === commentToDelete.id) this.highlightedThread = undefined }, @@ -204,15 +187,11 @@ export class VideoCommentsComponent implements OnInit, OnChanges, OnDestroy { } } - private deleteLocalCommentThread (parentComment: VideoCommentThreadTree, commentToDelete: VideoComment) { - for (const commentChild of parentComment.children) { - if (commentChild.comment.id === commentToDelete.id) { - parentComment.children = parentComment.children.filter(c => c.comment.id !== commentToDelete.id) - return - } - - this.deleteLocalCommentThread(commentChild, commentToDelete) - } + private softDeleteComment (comment: VideoComment) { + comment.isDeleted = true + comment.deletedAt = new Date() + comment.text = '' + comment.account = null } private resetVideo () { diff --git a/server/controllers/activitypub/client.ts b/server/controllers/activitypub/client.ts index 453ced8bf..5ed0435ff 100644 --- a/server/controllers/activitypub/client.ts +++ b/server/controllers/activitypub/client.ts @@ -308,13 +308,16 @@ async function videoCommentController (req: express.Request, res: express.Respon const threadParentComments = await VideoCommentModel.listThreadParentComments(videoComment, undefined) const isPublic = true // Comments are always public - const audience = getAudience(videoComment.Account.Actor, isPublic) + let videoCommentObject = videoComment.toActivityPubObject(threadParentComments) - const videoCommentObject = audiencify(videoComment.toActivityPubObject(threadParentComments), audience) + if (videoComment.Account) { + const audience = getAudience(videoComment.Account.Actor, isPublic) + videoCommentObject = audiencify(videoCommentObject, audience) - if (req.path.endsWith('/activity')) { - const data = buildCreateActivity(videoComment.url, videoComment.Account.Actor, videoCommentObject, audience) - return activityPubResponse(activityPubContextify(data), res) + if (req.path.endsWith('/activity')) { + const data = buildCreateActivity(videoComment.url, videoComment.Account.Actor, videoCommentObject, audience) + return activityPubResponse(activityPubContextify(data), res) + } } return activityPubResponse(activityPubContextify(videoCommentObject), res) diff --git a/server/controllers/api/videos/comment.ts b/server/controllers/api/videos/comment.ts index b2b06b170..5f3fed5c0 100644 --- a/server/controllers/api/videos/comment.ts +++ b/server/controllers/api/videos/comment.ts @@ -1,10 +1,11 @@ import * as express from 'express' +import { cloneDeep } from 'lodash' import { ResultList } from '../../../../shared/models' import { VideoCommentCreate } from '../../../../shared/models/videos/video-comment.model' import { logger } from '../../../helpers/logger' import { getFormattedObjects } from '../../../helpers/utils' import { sequelizeTypescript } from '../../../initializers' -import { buildFormattedCommentTree, createVideoComment } from '../../../lib/video-comment' +import { buildFormattedCommentTree, createVideoComment, markCommentAsDeleted } from '../../../lib/video-comment' import { asyncMiddleware, asyncRetryTransactionMiddleware, @@ -177,19 +178,22 @@ async function addVideoCommentReply (req: express.Request, res: express.Response async function removeVideoComment (req: express.Request, res: express.Response) { const videoCommentInstance = res.locals.videoCommentFull + const videoCommentInstanceBefore = cloneDeep(videoCommentInstance) await sequelizeTypescript.transaction(async t => { - await videoCommentInstance.destroy({ transaction: t }) - if (videoCommentInstance.isOwned() || videoCommentInstance.Video.isOwned()) { await sendDeleteVideoComment(videoCommentInstance, t) } + + markCommentAsDeleted(videoCommentInstance) + + await videoCommentInstance.save() }) auditLogger.delete(getAuditIdFromRes(res), new CommentAuditView(videoCommentInstance.toFormattedJSON())) logger.info('Video comment %d deleted.', videoCommentInstance.id) - Hooks.runAction('action:api.video-comment.deleted', { comment: videoCommentInstance }) + Hooks.runAction('action:api.video-comment.deleted', { comment: videoCommentInstanceBefore }) return res.type('json').status(204).end() } diff --git a/server/initializers/migrations/0450-soft-delete-video-comments.ts b/server/initializers/migrations/0450-soft-delete-video-comments.ts new file mode 100644 index 000000000..bcfb97b56 --- /dev/null +++ b/server/initializers/migrations/0450-soft-delete-video-comments.ts @@ -0,0 +1,36 @@ +import * as Sequelize from 'sequelize' + +async function up (utils: { + transaction: Sequelize.Transaction, + queryInterface: Sequelize.QueryInterface, + sequelize: Sequelize.Sequelize, + db: any +}): Promise { + { + const data = { + type: Sequelize.INTEGER, + allowNull: true, + defaultValue: null + } + + await utils.queryInterface.changeColumn('videoComment', 'accountId', data) + } + + { + const data = { + type: Sequelize.DATE, + allowNull: true, + defaultValue: null + } + await utils.queryInterface.addColumn('videoComment', 'deletedAt', data) + } +} + +function down (options) { + throw new Error('Not implemented.') +} + +export { + up, + down +} diff --git a/server/lib/activitypub/process/process-delete.ts b/server/lib/activitypub/process/process-delete.ts index 79d0e0d79..e76132f91 100644 --- a/server/lib/activitypub/process/process-delete.ts +++ b/server/lib/activitypub/process/process-delete.ts @@ -5,6 +5,7 @@ import { sequelizeTypescript } from '../../../initializers' import { ActorModel } from '../../../models/activitypub/actor' import { VideoModel } from '../../../models/video/video' import { VideoCommentModel } from '../../../models/video/video-comment' +import { markCommentAsDeleted } from '../../video-comment' import { forwardVideoRelatedActivity } from '../send/utils' import { VideoPlaylistModel } from '../../../models/video/video-playlist' import { APProcessorOptions } from '../../../typings/activitypub-processor.model' @@ -128,7 +129,11 @@ function processDeleteVideoComment (byActor: MActorSignature, videoComment: Vide throw new Error(`Account ${byActor.url} does not own video comment ${videoComment.url} or video ${videoComment.Video.url}`) } - await videoComment.destroy({ transaction: t }) + await sequelizeTypescript.transaction(async t => { + markCommentAsDeleted(videoComment) + + await videoComment.save() + }) if (videoComment.Video.isOwned()) { // Don't resend the activity to the sender diff --git a/server/lib/video-comment.ts b/server/lib/video-comment.ts index bb811bd2c..b8074e6d2 100644 --- a/server/lib/video-comment.ts +++ b/server/lib/video-comment.ts @@ -73,9 +73,16 @@ function buildFormattedCommentTree (resultList: ResultList): return thread } +function markCommentAsDeleted (comment: MCommentOwnerVideoReply): void { + comment.text = '' + comment.deletedAt = new Date() + comment.accountId = null +} + // --------------------------------------------------------------------------- export { createVideoComment, - buildFormattedCommentTree + buildFormattedCommentTree, + markCommentAsDeleted } diff --git a/server/models/account/account.ts b/server/models/account/account.ts index ba1094536..a818a5a4d 100644 --- a/server/models/account/account.ts +++ b/server/models/account/account.ts @@ -201,7 +201,7 @@ export class AccountModel extends Model { @HasMany(() => VideoCommentModel, { foreignKey: { - allowNull: false + allowNull: true }, onDelete: 'cascade', hooks: true diff --git a/server/models/video/video-comment.ts b/server/models/video/video-comment.ts index 2e4220434..b44d65138 100644 --- a/server/models/video/video-comment.ts +++ b/server/models/video/video-comment.ts @@ -1,5 +1,5 @@ import { AllowNull, BelongsTo, Column, CreatedAt, DataType, ForeignKey, Is, Model, Scopes, Table, UpdatedAt } from 'sequelize-typescript' -import { ActivityTagObject } from '../../../shared/models/activitypub/objects/common-objects' +import { ActivityTagObject, ActivityTombstoneObject } from '../../../shared/models/activitypub/objects/common-objects' import { VideoCommentObject } from '../../../shared/models/activitypub/objects/video-comment-object' import { VideoComment } from '../../../shared/models/videos/video-comment.model' import { isActivityPubUrlValid } from '../../helpers/custom-validators/activitypub/misc' @@ -122,6 +122,10 @@ export class VideoCommentModel extends Model { @UpdatedAt updatedAt: Date + @AllowNull(true) + @Column(DataType.DATE) + deletedAt: Date + @AllowNull(false) @Is('VideoCommentUrl', value => throwIfNotValid(value, isActivityPubUrlValid, 'url')) @Column(DataType.STRING(CONSTRAINTS_FIELDS.VIDEOS.URL.max)) @@ -177,7 +181,7 @@ export class VideoCommentModel extends Model { @BelongsTo(() => AccountModel, { foreignKey: { - allowNull: false + allowNull: true }, onDelete: 'CASCADE' }) @@ -436,9 +440,17 @@ export class VideoCommentModel extends Model { } isOwned () { + if (!this.Account) { + return false + } + return this.Account.isOwned() } + isDeleted () { + return null !== this.deletedAt + } + extractMentions () { let result: string[] = [] @@ -487,12 +499,25 @@ export class VideoCommentModel extends Model { videoId: this.videoId, createdAt: this.createdAt, updatedAt: this.updatedAt, + deletedAt: this.deletedAt, + isDeleted: this.isDeleted(), totalReplies: this.get('totalReplies') || 0, - account: this.Account.toFormattedJSON() + account: this.Account ? this.Account.toFormattedJSON() : null } as VideoComment } - toActivityPubObject (this: MCommentAP, threadParentComments: MCommentOwner[]): VideoCommentObject { + toActivityPubObject (this: MCommentAP, threadParentComments: MCommentOwner[]): VideoCommentObject | ActivityTombstoneObject { + if (this.isDeleted()) { + return { + id: this.url, + type: 'Tombstone', + formerType: 'Note', + published: this.createdAt.toISOString(), + updated: this.updatedAt.toISOString(), + deleted: this.deletedAt.toISOString() + } + } + let inReplyTo: string // New thread, so in AS we reply to the video if (this.inReplyToCommentId === null) { diff --git a/server/tests/api/videos/multiple-servers.ts b/server/tests/api/videos/multiple-servers.ts index aeda188c2..e7b57ba1f 100644 --- a/server/tests/api/videos/multiple-servers.ts +++ b/server/tests/api/videos/multiple-servers.ts @@ -868,7 +868,7 @@ describe('Test multiple servers', function () { await waitJobs(servers) }) - it('Should not have this comment anymore', async function () { + it('Should have this comment marked as deleted', async function () { for (const server of servers) { const res1 = await getVideoCommentThreads(server.url, videoUUID, 0, 5) const threadId = res1.body.data.find(c => c.text === 'my super first comment').id @@ -880,7 +880,13 @@ describe('Test multiple servers', function () { const firstChild = tree.children[0] expect(firstChild.comment.text).to.equal('my super answer to thread 1') - expect(firstChild.children).to.have.lengthOf(0) + expect(firstChild.children).to.have.lengthOf(1) + + const deletedComment = firstChild.children[0].comment + expect(deletedComment.isDeleted).to.be.true + expect(deletedComment.deletedAt).to.not.be.null + expect(deletedComment.account).to.be.null + expect(deletedComment.text).to.equal('') const secondChild = tree.children[1] expect(secondChild.comment.text).to.equal('my second answer to thread 1') @@ -897,13 +903,13 @@ describe('Test multiple servers', function () { await waitJobs(servers) }) - it('Should have the threads deleted on other servers too', async function () { + it('Should have the threads marked as deleted on other servers too', async function () { for (const server of servers) { const res = await getVideoCommentThreads(server.url, videoUUID, 0, 5) - expect(res.body.total).to.equal(1) + expect(res.body.total).to.equal(2) expect(res.body.data).to.be.an('array') - expect(res.body.data).to.have.lengthOf(1) + expect(res.body.data).to.have.lengthOf(2) { const comment: VideoComment = res.body.data[0] @@ -915,6 +921,20 @@ describe('Test multiple servers', function () { expect(dateIsValid(comment.createdAt as string)).to.be.true expect(dateIsValid(comment.updatedAt as string)).to.be.true } + + { + const deletedComment: VideoComment = res.body.data[1] + expect(deletedComment).to.not.be.undefined + expect(deletedComment.isDeleted).to.be.true + expect(deletedComment.deletedAt).to.not.be.null + expect(deletedComment.text).to.equal('') + expect(deletedComment.inReplyToCommentId).to.be.null + expect(deletedComment.account).to.be.null + expect(deletedComment.totalReplies).to.equal(3) + expect(dateIsValid(deletedComment.createdAt as string)).to.be.true + expect(dateIsValid(deletedComment.updatedAt as string)).to.be.true + expect(dateIsValid(deletedComment.deletedAt as string)).to.be.true + } } }) @@ -926,12 +946,32 @@ describe('Test multiple servers', function () { await waitJobs(servers) }) - it('Should have the threads deleted on other servers too', async function () { + it('Should have the threads marked as deleted on other servers too', async function () { for (const server of servers) { const res = await getVideoCommentThreads(server.url, videoUUID, 0, 5) - expect(res.body.total).to.equal(0) - expect(res.body.data).to.have.lengthOf(0) + expect(res.body.total).to.equal(2) + expect(res.body.data).to.have.lengthOf(2) + + { + const comment: VideoComment = res.body.data[0] + expect(comment.text).to.equal('') + expect(comment.isDeleted).to.be.true + expect(comment.createdAt).to.not.be.null + expect(comment.deletedAt).to.not.be.null + expect(comment.account).to.be.null + expect(comment.totalReplies).to.equal(0) + } + + { + const comment: VideoComment = res.body.data[1] + expect(comment.text).to.equal('') + expect(comment.isDeleted).to.be.true + expect(comment.createdAt).to.not.be.null + expect(comment.deletedAt).to.not.be.null + expect(comment.account).to.be.null + expect(comment.totalReplies).to.equal(3) + } } }) diff --git a/server/tests/api/videos/video-comments.ts b/server/tests/api/videos/video-comments.ts index 82182cc7c..95be14c0e 100644 --- a/server/tests/api/videos/video-comments.ts +++ b/server/tests/api/videos/video-comments.ts @@ -172,7 +172,7 @@ describe('Test video comments', function () { const tree: VideoCommentThreadTree = res.body expect(tree.comment.text).equal('my super first comment') - expect(tree.children).to.have.lengthOf(1) + expect(tree.children).to.have.lengthOf(2) const firstChild = tree.children[0] expect(firstChild.comment.text).to.equal('my super answer to thread 1') @@ -181,20 +181,32 @@ describe('Test video comments', function () { const childOfFirstChild = firstChild.children[0] expect(childOfFirstChild.comment.text).to.equal('my super answer to answer of thread 1') expect(childOfFirstChild.children).to.have.lengthOf(0) + + const deletedChildOfFirstChild = tree.children[1] + expect(deletedChildOfFirstChild.comment.text).to.equal('') + expect(deletedChildOfFirstChild.comment.isDeleted).to.be.true + expect(deletedChildOfFirstChild.comment.deletedAt).to.not.be.null + expect(deletedChildOfFirstChild.comment.account).to.be.null + expect(deletedChildOfFirstChild.children).to.have.lengthOf(0) }) it('Should delete a complete thread', async function () { await deleteVideoComment(server.url, server.accessToken, videoId, threadId) const res = await getVideoCommentThreads(server.url, videoUUID, 0, 5, 'createdAt') - expect(res.body.total).to.equal(2) + expect(res.body.total).to.equal(3) expect(res.body.data).to.be.an('array') - expect(res.body.data).to.have.lengthOf(2) + expect(res.body.data).to.have.lengthOf(3) - expect(res.body.data[0].text).to.equal('super thread 2') - expect(res.body.data[0].totalReplies).to.equal(0) - expect(res.body.data[1].text).to.equal('super thread 3') + expect(res.body.data[0].text).to.equal('') + expect(res.body.data[0].isDeleted).to.be.true + expect(res.body.data[0].deletedAt).to.not.be.null + expect(res.body.data[0].account).to.be.null + expect(res.body.data[0].totalReplies).to.equal(3) + expect(res.body.data[1].text).to.equal('super thread 2') expect(res.body.data[1].totalReplies).to.equal(0) + expect(res.body.data[2].text).to.equal('super thread 3') + expect(res.body.data[2].totalReplies).to.equal(0) }) after(async function () { diff --git a/shared/models/activitypub/objects/common-objects.ts b/shared/models/activitypub/objects/common-objects.ts index 2a6529fed..df287d570 100644 --- a/shared/models/activitypub/objects/common-objects.ts +++ b/shared/models/activitypub/objects/common-objects.ts @@ -89,3 +89,14 @@ export interface ActivityPubAttributedTo { type: 'Group' | 'Person' id: string } + +export interface ActivityTombstoneObject { + '@context'?: any + id: string + type: 'Tombstone' + name?: string + formerType?: string + published: string + updated: string + deleted: string +} diff --git a/shared/models/videos/video-comment.model.ts b/shared/models/videos/video-comment.model.ts index 7ac4024fb..044962633 100644 --- a/shared/models/videos/video-comment.model.ts +++ b/shared/models/videos/video-comment.model.ts @@ -9,6 +9,8 @@ export interface VideoComment { videoId: number createdAt: Date | string updatedAt: Date | string + deletedAt: Date | string + isDeleted: boolean totalReplies: number account: Account }