From 55b0e902fb19e6793eb7741b2e193e464b71e456 Mon Sep 17 00:00:00 2001 From: Julian Lam Date: Fri, 24 Jul 2020 14:10:37 -0400 Subject: [PATCH] feat: consolidation of flags to reduce flagspam, #8510 Squashed commit of the following: commit c6d09396208a10c244d7b3d22ffd2d7dd1274d3a Author: Julian Lam Date: Fri Jul 24 13:41:32 2020 -0400 fix: more tests commit 32f9af2a87a81fa62ecca01e71d6f0d5b9d37ba1 Merge: e50907535 4eae927d1 Author: Julian Lam Date: Fri Jul 24 10:53:04 2020 -0400 Merge remote-tracking branch 'origin/master' into singleton-flags commit e50907535109dbdbe8f15c3e2fcdf22d90b1332a Author: Julian Lam Date: Fri Jul 24 10:52:46 2020 -0400 fix: controllers-admin test commit fd5af99e303de48a80b0ccc166eee19175cf232b Author: Julian Lam Date: Fri Jul 17 17:26:55 2020 -0400 fix(tests): dummy commit to trigger travisCI commit c452a6ffcfaef91403de084c4ae16795cb23c60e Author: Julian Lam Date: Fri Jul 17 17:05:09 2020 -0400 fix(openapi): openapi spec changes commit 8089a74e89128141ab1e6f8ff83447114b3b846b Author: Julian Lam Date: Fri Jul 17 15:48:00 2020 -0400 fix: reversing the order of reports for display purposes commit a099892b377333561c72f1ad5b6b20ddb4ce8a96 Author: Julian Lam Date: Fri Jul 17 15:45:44 2020 -0400 refactor: run all flag creation calls in a single batch commit b24999682f9d5a33a08a049749c1f0eb4f00facc Author: Julian Lam Date: Fri Jul 17 15:08:23 2020 -0400 feat: handling multiple reporters per flag, #8510 commit 08c75c020021ada754bf0e39eae77d631b01dee5 Author: Julian Lam Date: Thu Jul 16 20:53:18 2020 -0400 feat: upgrade script for #8510 --- public/language/en-GB/flags.json | 5 +- public/openapi/read.yaml | 107 ++++++++-------- public/src/client/flags/list.js | 5 + src/flags.js | 149 ++++++++++++++++------- src/upgrades/1.14.3/consolidate_flags.js | 46 +++++++ test/controllers-admin.js | 4 +- test/flags.js | 11 +- 7 files changed, 214 insertions(+), 113 deletions(-) create mode 100644 src/upgrades/1.14.3/consolidate_flags.js diff --git a/public/language/en-GB/flags.json b/public/language/en-GB/flags.json index cca477a2966f..1259cf8cbbee 100644 --- a/public/language/en-GB/flags.json +++ b/public/language/en-GB/flags.json @@ -1,8 +1,7 @@ { "state": "State", - "reporter": "Reporter", - "reported-at": "Reported At", - "description": "Description", + "reports": "Reports", + "first-reported": "First Reported", "no-flags": "Hooray! No flags found.", "assignee": "Assignee", "update": "Update", diff --git a/public/openapi/read.yaml b/public/openapi/read.yaml index 3318c0f1cb78..88dc2b27af9a 100644 --- a/public/openapi/read.yaml +++ b/public/openapi/read.yaml @@ -3089,6 +3089,9 @@ paths: type: boolean downvoted: type: boolean + flagId: + type: number + description: The flag identifier, if this particular post has been flagged before "/api/topic/tid/{id}": get: tags: @@ -3735,6 +3738,9 @@ paths: type: boolean display_post_menu: type: boolean + flagId: + type: number + description: The flag identifier, if this particular post has been flagged before category: $ref: components/schemas/CategoryObject.yaml#/CategoryObject tagWhitelist: @@ -4788,6 +4794,9 @@ paths: properties: state: type: string + heat: + type: number + description: The number of reports that make up this flag flagId: type: number type: @@ -4796,34 +4805,8 @@ paths: oneOf: - type: string - type: number - description: - type: string - uid: - type: number - description: A user identifier datetime: type: number - reporter: - type: object - properties: - username: - type: string - description: A friendly name for a given user account - picture: - nullable: true - type: string - icon:bgColor: - type: string - description: A six-character hexadecimal colour code assigned to the user. This - value is used in conjunction with - `icon:text` for the user's auto-generated - icon - example: "#f44336" - icon:text: - type: string - description: A single-letter representation of a username. This is used in the - auto-generated icon given to users without - an avatar labelClass: type: string target_readable: @@ -4886,11 +4869,6 @@ paths: type: string targetId: type: number - description: - type: string - uid: - type: number - description: A user identifier datetime: type: number datetimeISO: @@ -5006,34 +4984,45 @@ paths: `icon:text` for the user's auto-generated icon example: "#f44336" - reporter: - type: object - properties: - username: - type: string - description: A friendly name for a given user account - userslug: - type: string - description: An URL-safe variant of the username (i.e. lower-cased, spaces - removed, etc.) - picture: - nullable: true - reputation: - type: number - uid: - type: number - description: A user identifier - icon:text: - type: string - description: A single-letter representation of a username. This is used in the - auto-generated icon given to users without an - avatar - icon:bgColor: - type: string - description: A six-character hexadecimal colour code assigned to the user. This - value is used in conjunction with `icon:text` for - the user's auto-generated icon - example: "#f44336" + reports: + type: array + items: + type: object + properties: + value: + type: string + timestamp: + type: number + timestampISO: + type: string + reporter: + type: object + properties: + username: + type: string + description: A friendly name for a given user account + userslug: + type: string + description: An URL-safe variant of the username (i.e. lower-cased, spaces + removed, etc.) + picture: + nullable: true + reputation: + type: number + uid: + type: number + description: A user identifier + icon:text: + type: string + description: A single-letter representation of a username. This is used in the + auto-generated icon given to users without an + avatar + icon:bgColor: + type: string + description: A six-character hexadecimal colour code assigned to the user. This + value is used in conjunction with `icon:text` for + the user's auto-generated icon + example: "#f44336" type_path: type: string assignees: diff --git a/public/src/client/flags/list.js b/public/src/client/flags/list.js index 42eac69b11e8..3dcd99b38921 100644 --- a/public/src/client/flags/list.js +++ b/public/src/client/flags/list.js @@ -6,6 +6,11 @@ define('forum/flags/list', ['components', 'Chart'], function (components, Chart) Flags.init = function () { Flags.enableFilterForm(); + components.get('flags/list').on('click', '[data-flag-id]', function () { + var flagId = this.getAttribute('data-flag-id'); + ajaxify.go('flags/' + flagId); + }); + var graphWrapper = $('#flags-daily-wrapper'); var graphFooter = graphWrapper.siblings('.panel-footer'); $('#flags-daily-wrapper').one('shown.bs.collapse', function () { diff --git a/src/flags.js b/src/flags.js index 0dc0a05dcd37..ddea5bf08c40 100644 --- a/src/flags.js +++ b/src/flags.js @@ -84,32 +84,28 @@ Flags.init = async function () { }; Flags.get = async function (flagId) { - const [base, history, notes] = await Promise.all([ + const [base, history, notes, reports] = await Promise.all([ db.getObject('flag:' + flagId), Flags.getHistory(flagId), Flags.getNotes(flagId), + Flags.getReports(flagId), ]); if (!base) { return; } - const [userObj, targetObj] = await Promise.all([ - user.getUserFields(base.uid, ['username', 'userslug', 'picture', 'reputation']), - Flags.getTarget(base.type, base.targetId, 0), - ]); - const flagObj = { state: 'open', assignee: null, ...base, - description: validator.escape(base.description), datetimeISO: utils.toISOString(base.datetime), target_readable: base.type.charAt(0).toUpperCase() + base.type.slice(1) + ' ' + base.targetId, - target: targetObj, + target: await Flags.getTarget(base.type, base.targetId, 0), history: history, notes: notes, - reporter: userObj, + reports: reports, }; + const data = await plugins.fireHook('filter:flags.get', { flag: flagObj, }); @@ -160,24 +156,19 @@ Flags.list = async function (filters, uid) { const pageCount = Math.ceil(flagIds.length / flagsPerPage); flagIds = flagIds.slice((filters.page - 1) * flagsPerPage, filters.page * flagsPerPage); - const flags = await Promise.all(flagIds.map(async (flagId) => { + const reportCounts = await db.sortedSetsCard(flagIds.map(flagId => `flag:${flagId}:reports`)); + + const flags = await Promise.all(flagIds.map(async (flagId, idx) => { let flagObj = await db.getObject('flag:' + flagId); - const userObj = await user.getUserFields(flagObj.uid, ['username', 'picture']); flagObj = { state: 'open', assignee: null, + heat: reportCounts[idx], ...flagObj, - reporter: { - username: userObj.username, - picture: userObj.picture, - 'icon:bgColor': userObj['icon:bgColor'], - 'icon:text': userObj['icon:text'], - }, }; flagObj.labelClass = Flags._constants.state_class[flagObj.state]; return Object.assign(flagObj, { - description: validator.escape(String(flagObj.description)), target_readable: flagObj.type.charAt(0).toUpperCase() + flagObj.type.slice(1) + ' ' + flagObj.targetId, datetimeISO: utils.toISOString(flagObj.datetime), }); @@ -242,6 +233,24 @@ Flags.getNote = async function (flagId, datetime) { return notes[0]; }; +Flags.getFlagIdByTarget = async function (type, id) { + let method; + switch (type) { + case 'post': + method = posts.getPostField; + break; + + case 'user': + method = user.getUserField; + break; + + default: + throw new Error('[[error:invalid-data]]'); + } + + return await method(id, 'flagId'); +}; + async function modifyNotes(notes) { const uids = []; notes = notes.map(function (note) { @@ -277,11 +286,13 @@ Flags.create = async function (type, id, uid, reason, timestamp) { timestamp = Date.now(); doHistoryAppend = true; } - const [flagExists, targetExists,, targetUid, targetCid] = await Promise.all([ + const [flagExists, targetExists,, targetFlagged, targetUid, targetCid] = await Promise.all([ // Sanity checks Flags.exists(type, id, uid), Flags.targetExists(type, id), Flags.canFlag(type, id, uid), + Flags.targetFlagged(type, id), + // Extra data for zset insertion Flags.getTargetUid(type, id), Flags.getTargetCid(type, id), @@ -292,45 +303,91 @@ Flags.create = async function (type, id, uid, reason, timestamp) { throw new Error('[[error:invalid-data]]'); } - const flagId = await db.incrObjectField('global', 'nextFlagId'); + // If the flag already exists, just add the report + if (targetFlagged) { + const flagId = await Flags.getFlagIdByTarget(type, id); + await Promise.all([ + Flags.addReport(flagId, uid, reason, timestamp), + Flags.update(flagId, uid, { state: 'open' }), + ]); - await db.setObject('flag:' + flagId, { - flagId: flagId, - type: type, - targetId: id, - description: reason, - uid: uid, - datetime: timestamp, - }); - await db.sortedSetAdd('flags:datetime', timestamp, flagId); // by time, the default - await db.sortedSetAdd('flags:byReporter:' + uid, timestamp, flagId); // by reporter - await db.sortedSetAdd('flags:byType:' + type, timestamp, flagId); // by flag type - await db.sortedSetAdd('flags:hash', flagId, [type, id, uid].join(':')); // save zset for duplicate checking - await db.sortedSetIncrBy('flags:byTarget', 1, [type, id].join(':')); // by flag target (score is count) - await analytics.increment('flags'); // some fancy analytics + return await Flags.get(flagId); + } + + const flagId = await db.incrObjectField('global', 'nextFlagId'); + const batched = []; + + batched.push( + db.setObject.bind(db, 'flag:' + flagId, { + flagId: flagId, + type: type, + targetId: id, + datetime: timestamp, + }), + Flags.addReport.bind(Flags, flagId, uid, reason, timestamp), + db.sortedSetAdd.bind(db, 'flags:datetime', timestamp, flagId), // by time, the default + db.sortedSetAdd.bind(db, 'flags:byType:' + type, timestamp, flagId), // by flag type + db.sortedSetAdd.bind(db, 'flags:hash', flagId, [type, id, uid].join(':')), // save zset for duplicate checking + db.sortedSetIncrBy.bind(db, 'flags:byTarget', 1, [type, id].join(':')), // by flag target (score is count) + analytics.increment.bind(analytics, 'flags') // some fancy analytics + ); if (targetUid) { - await db.sortedSetAdd('flags:byTargetUid:' + targetUid, timestamp, flagId); // by target uid + batched.push(db.sortedSetAdd.bind(db, 'flags:byTargetUid:' + targetUid, timestamp, flagId)); // by target uid } if (targetCid) { - await db.sortedSetAdd('flags:byCid:' + targetCid, timestamp, flagId); // by target cid + batched.push(db.sortedSetAdd.bind(db, 'flags:byCid:' + targetCid, timestamp, flagId)); // by target cid } if (type === 'post') { - await db.sortedSetAdd('flags:byPid:' + id, timestamp, flagId); // by target pid + batched.push( + db.sortedSetAdd.bind(db, 'flags:byPid:' + id, timestamp, flagId), // by target pid + posts.setPostField.bind(posts, id, 'flagId', flagId) + ); + if (targetUid) { - await user.incrementUserFlagsBy(targetUid, 1); + batched.push(user.incrementUserFlagsBy.bind(user, targetUid, 1)); } + } else if (type === 'user') { + batched.push(user.setUserField.bind(user, id, 'flagId', flagId)); } + // Run all the database calls in one single batched call... + await Promise.all(batched.map(async method => await method())); + if (doHistoryAppend) { - await Flags.update(flagId, uid, { state: 'open' }); + Flags.update(flagId, uid, { state: 'open' }); } return await Flags.get(flagId); }; +Flags.getReports = async function (flagId) { + const [reports, reporterUids] = await Promise.all([ + db.getSortedSetRevRangeWithScores(`flag:${flagId}:reports`, 0, -1), + db.getSortedSetRevRange(`flag:${flagId}:reporters`, 0, -1), + ]); + + await Promise.all(reports.map(async (report, idx) => { + report.timestamp = report.score; + report.timestampISO = new Date(report.score).toISOString(); + delete report.score; + report.reporter = await user.getUserFields(reporterUids[idx], ['username', 'userslug', 'picture', 'reputation']); + })); + + return reports; +}; + +Flags.addReport = async function (flagId, uid, reason, timestamp) { + // adds to reporters/report zsets + await Promise.all([ + db.sortedSetAdd(`flags:byReporter:${uid}`, timestamp, flagId), + db.sortedSetAdd(`flag:${flagId}:reports`, timestamp, reason), + db.sortedSetAdd(`flag:${flagId}:reporters`, timestamp, uid), + ]); +}; + Flags.exists = async function (type, id, uid) { return await db.isSortedSetMember('flags:hash', [type, id, uid].join(':')); }; @@ -386,6 +443,10 @@ Flags.targetExists = async function (type, id) { throw new Error('[[error:invalid-data]]'); }; +Flags.targetFlagged = async function (type, id) { + return await db.sortedSetScore('flags:byTarget', [type, id].join(':')) >= 1; +}; + Flags.getTargetUid = async function (type, id) { if (type === 'post') { return await posts.getPostField(id, 'uid'); @@ -443,7 +504,7 @@ Flags.update = async function (flagId, uid, changeset) { tasks.push(db.sortedSetAdd('flags:byState:' + changeset[prop], now, flagId)); tasks.push(db.sortedSetRemove('flags:byState:' + current[prop], flagId)); if (changeset[prop] === 'resolved' || changeset[prop] === 'rejected') { - tasks.push(notifications.rescind('flag:' + current.type + ':' + current.targetId + ':uid:' + current.uid)); + tasks.push(notifications.rescind('flag:' + current.type + ':' + current.targetId)); } } } else if (prop === 'assignee') { @@ -545,11 +606,11 @@ Flags.notify = async function (flagObj, uid) { notifObj = await notifications.create({ type: 'new-post-flag', - bodyShort: '[[notifications:user_flagged_post_in, ' + flagObj.reporter.username + ', ' + titleEscaped + ']]', + bodyShort: '[[notifications:user_flagged_post_in, ' + flagObj.reports[flagObj.reports.length - 1].reporter.username + ', ' + titleEscaped + ']]', bodyLong: flagObj.description, pid: flagObj.targetId, path: '/flags/' + flagObj.flagId, - nid: 'flag:post:' + flagObj.targetId + ':uid:' + uid, + nid: 'flag:post:' + flagObj.targetId, from: uid, mergeId: 'notifications:user_flagged_post_in|' + flagObj.targetId, topicTitle: title, @@ -558,10 +619,10 @@ Flags.notify = async function (flagObj, uid) { } else if (flagObj.type === 'user') { notifObj = await notifications.create({ type: 'new-user-flag', - bodyShort: '[[notifications:user_flagged_user, ' + flagObj.reporter.username + ', ' + flagObj.target.username + ']]', + bodyShort: '[[notifications:user_flagged_user, ' + flagObj.reports[flagObj.reports.length - 1].reporter.username + ', ' + flagObj.target.username + ']]', bodyLong: flagObj.description, path: '/flags/' + flagObj.flagId, - nid: 'flag:user:' + flagObj.targetId + ':uid:' + uid, + nid: 'flag:user:' + flagObj.targetId, from: uid, mergeId: 'notifications:user_flagged_user|' + flagObj.targetId, }); diff --git a/src/upgrades/1.14.3/consolidate_flags.js b/src/upgrades/1.14.3/consolidate_flags.js new file mode 100644 index 000000000000..49da79084018 --- /dev/null +++ b/src/upgrades/1.14.3/consolidate_flags.js @@ -0,0 +1,46 @@ +'use strict'; + +const db = require('../../database'); +const batch = require('../../batch'); +const posts = require('../../posts'); +const user = require('../../user'); + +module.exports = { + name: 'Consolidate multiple flags reports, going forward', + timestamp: Date.UTC(2020, 6, 16), + method: async function () { + const progress = this.progress; + + let flags = await db.getSortedSetRange('flags:datetime', 0, -1); + flags = flags.map(flagId => `flag:${flagId}`); + flags = await db.getObjectsFields(flags, ['flagId', 'type', 'targetId', 'uid', 'description', 'datetime']); + progress.total = flags.length; + + await batch.processArray(flags, async function (subset) { + progress.incr(subset.length); + + await Promise.all(subset.map(async (flagObj) => { + const methods = []; + switch (flagObj.type) { + case 'post': + methods.push(posts.setPostField.bind(posts, flagObj.targetId, 'flagId', flagObj.flagId)); + break; + + case 'user': + methods.push(user.setUserField.bind(user, flagObj.targetId, 'flagId', flagObj.flagId)); + break; + } + + methods.push( + db.sortedSetAdd.bind(db, `flag:${flagObj.flagId}:reports`, flagObj.datetime, flagObj.description), + db.sortedSetAdd.bind(db, `flag:${flagObj.flagId}:reporters`, flagObj.datetime, flagObj.uid) + ); + + await Promise.all(methods.map(async method => method())); + })); + }, { + progress: progress, + batch: 500, + }); + }, +}; diff --git a/test/controllers-admin.js b/test/controllers-admin.js index 52ab4589864c..1b090de2b184 100644 --- a/test/controllers-admin.js +++ b/test/controllers-admin.js @@ -712,7 +712,9 @@ describe('Admin Controllers', function () { request(nconf.get('url') + '/api/flags/' + flagId, { jar: moderatorJar, json: true }, function (err, res, body) { assert.ifError(err); assert(body); - assert.equal(body.reporter.username, 'regular'); + assert(body.reports); + assert(Array.isArray(body.reports)); + assert.equal(body.reports[0].reporter.username, 'regular'); done(); }); }); diff --git a/test/flags.js b/test/flags.js index 2ecb189e4962..41a7c6864ca9 100644 --- a/test/flags.js +++ b/test/flags.js @@ -48,10 +48,10 @@ describe('Flags', function () { assert.ifError(err); var compare = { flagId: 1, - uid: 1, targetId: 1, type: 'post', - description: 'Test flag', + state: 'open', + target_readable: 'Post 1', }; assert(flagData); for (var key in compare) { @@ -124,11 +124,10 @@ describe('Flags', function () { assert.ifError(err); var compare = { flagId: 1, - uid: 1, targetId: 1, type: 'post', - description: 'Test flag', state: 'open', + target_readable: 'Post 1', }; assert(flagData); for (var key in compare) { @@ -378,14 +377,14 @@ describe('Flags', function () { await sleep(2000); let userNotifs = await User.notifications.getAll(adminUid); - assert(userNotifs.includes('flag:post:' + result.postData.pid + ':uid:' + uid1)); + assert(userNotifs.includes('flag:post:' + result.postData.pid)); await Flags.update(flagId, adminUid, { state: 'resolved', }); userNotifs = await User.notifications.getAll(adminUid); - assert(!userNotifs.includes('flag:post:' + result.postData.pid + ':uid:' + uid1)); + assert(!userNotifs.includes('flag:post:' + result.postData.pid)); }); });