From 63268ebca420ebcb7e651122037cbaaa648c1ccc Mon Sep 17 00:00:00 2001 From: Maciej Ziarkowski Date: Fri, 17 Jul 2020 16:08:44 +0200 Subject: [PATCH 1/2] Allow filtering deletions on edit history page --- .../api/controllers/editHistoryController.ts | 5 +- .../api/dataAccess/__mocks__/editHistory.ts | 15 ++++-- app/src/api/dataAccess/editHistory.ts | 11 +++- .../services/__tests__/editHistory.test.ts | 20 ++++++++ app/src/api/services/editHistory.ts | 11 ++-- app/src/frontend/pages/changes.tsx | 50 +++++++++++++++++-- 6 files changed, 97 insertions(+), 15 deletions(-) diff --git a/app/src/api/controllers/editHistoryController.ts b/app/src/api/controllers/editHistoryController.ts index b959d685..15950558 100644 --- a/app/src/api/controllers/editHistoryController.ts +++ b/app/src/api/controllers/editHistoryController.ts @@ -2,7 +2,7 @@ import express from 'express'; import { ApiParamError, ApiUserError } from '../errors/api'; import { ArgumentError } from '../errors/general'; -import { checkRegexParam, parsePositiveIntParam, processParam } from '../parameters'; +import { checkRegexParam, parsePositiveIntParam, processParam, parseBooleanParam } from '../parameters'; import asyncController from "../routes/asyncController"; import * as editHistoryService from '../services/editHistory'; @@ -12,13 +12,14 @@ const getGlobalEditHistory = asyncController(async (req: express.Request, res: e const afterId: string = processParam(req.query, 'after_id', x => checkRegexParam(x, revisionIdRegex)); const beforeId: string = processParam(req.query, 'before_id', x => checkRegexParam(x, revisionIdRegex)); const count: number = processParam(req.query, 'count', parsePositiveIntParam); + const filterDeletions: boolean = processParam(req.query, 'deletions', parseBooleanParam); if(afterId != undefined && beforeId != undefined) { throw new ApiUserError('Cannot specify both after_id and before_id parameters'); } try { - const result = await editHistoryService.getGlobalEditHistory(beforeId, afterId, count); + const result = await editHistoryService.getGlobalEditHistory(beforeId, afterId, count, filterDeletions); res.send(result); } catch(error) { if(error instanceof ArgumentError && error.argumentName === 'count') { diff --git a/app/src/api/dataAccess/__mocks__/editHistory.ts b/app/src/api/dataAccess/__mocks__/editHistory.ts index b4636864..7c2320b7 100644 --- a/app/src/api/dataAccess/__mocks__/editHistory.ts +++ b/app/src/api/dataAccess/__mocks__/editHistory.ts @@ -17,18 +17,27 @@ mockEditHistory.__setHistory = function(mockHistoryData: EditHistoryEntry[]) { mockData = mockHistoryData.sort(numDesc(x => BigInt(x.revision_id))); }; -mockEditHistory.getHistoryAfterId = function(id: string, count: number): Promise { +function hasDeletions(json) { + return Object.values(json).some(x => x === null || (Array.isArray(x) && x.length === 0)) +} + +mockEditHistory.getHistoryAfterId = function(id: string, count: number, filterDeletions: boolean): Promise { return Promise.resolve( mockData - .filter(x => BigInt(x.revision_id) > BigInt(id)) + .filter(x => BigInt(x.revision_id) > BigInt(id) && (!filterDeletions || hasDeletions(x.forward_patch))) .sort(numAsc(x => BigInt(x.revision_id))) .slice(0, count) .sort(numDesc(x => BigInt(x.revision_id))) ); }; -mockEditHistory.getHistoryBeforeId = function(id: string, count: number): Promise { +mockEditHistory.getHistoryBeforeId = function(id: string, count: number, filterDeletions: boolean): Promise { let filteredData = id == undefined ? mockData : mockData.filter(x => BigInt(x.revision_id) < BigInt(id)); + + if(filterDeletions) { + filteredData = filteredData.filter(x => hasDeletions(x.forward_patch)); + } + return Promise.resolve( filteredData .slice(0, count) diff --git a/app/src/api/dataAccess/editHistory.ts b/app/src/api/dataAccess/editHistory.ts index c500f293..215c3bb4 100644 --- a/app/src/api/dataAccess/editHistory.ts +++ b/app/src/api/dataAccess/editHistory.ts @@ -13,7 +13,11 @@ const baseQuery = ` FROM logs JOIN users ON logs.user_id = users.user_id`; -export function getHistoryAfterId(id: string, count: number): Promise { +const deletionCondition = ` + EXISTS (SELECT * from jsonb_each(forward_patch) WHERE value IN ('null', '[]')) +`; + +export function getHistoryAfterId(id: string, count: number, filterDeletions: boolean = false): Promise { /** * SQL with lower time bound specified (records after ID). * The outer SELECT is so that final results are sorted by descending ID @@ -25,6 +29,7 @@ export function getHistoryAfterId(id: string, count: number): Promise $1 + ${filterDeletions ? 'AND ' + deletionCondition : ''} ORDER BY revision_id ASC LIMIT $2 ) AS result_asc ORDER BY revision_id DESC`, @@ -35,12 +40,13 @@ export function getHistoryAfterId(id: string, count: number): Promise { +export function getHistoryBeforeId(id: string, count: number, filterDeletions: boolean = false): Promise { try { if(id == undefined) { return db.any(` ${baseQuery} + ${filterDeletions ? 'WHERE' + deletionCondition : ''} ORDER BY revision_id DESC LIMIT $1 `, [count]); @@ -50,6 +56,7 @@ export function getHistoryBeforeId(id: string, count: number): Promise { }); }); + + describe('Filtering deletions', () => { + + it('Should return only records with deletions when requested', async () => { + const mockData = generateHistory(10); + mockData[0].forward_patch = { 'test_field': null}; + mockData[0].reverse_patch = { 'test_field': 'test'}; + + mockData[3].forward_patch = { 'test_field': null}; + mockData[3].reverse_patch = { 'test_field': 'test'}; + + mockData[4].forward_patch = { 'test_field': 'test2'}; + mockData[4].reverse_patch = { 'test_field': 'test1'}; + mockedEditHistoryData.__setHistory(mockData); + + const result = await getGlobalEditHistory(null, null, 10, true); + + expect(result.history.map(x => x.revision_id)).toEqual(['103', '100']); + }); + }); }); diff --git a/app/src/api/services/editHistory.ts b/app/src/api/services/editHistory.ts index 81b4c899..b6d166f2 100644 --- a/app/src/api/services/editHistory.ts +++ b/app/src/api/services/editHistory.ts @@ -3,7 +3,12 @@ import { decBigInt, incBigInt } from '../../helpers'; import { getHistoryAfterId, getHistoryBeforeId, getIdNewerThan, getIdOlderThan } from '../dataAccess/editHistory'; import { ArgumentError } from '../errors/general'; -async function getGlobalEditHistory(beforeId?: string, afterId?: string, count: number = 100) { +async function getGlobalEditHistory( + beforeId?: string, + afterId?: string, + count: number = 100, + filterDeletions: boolean = false +) { if(count <= 0) throw new ArgumentError('cannot request less than 1 history record', 'count'); if(count > 100) count = 100; @@ -11,9 +16,9 @@ async function getGlobalEditHistory(beforeId?: string, afterId?: string, count: let editHistoryRecords: EditHistoryEntry[]; if(afterId != undefined) { - editHistoryRecords = await getHistoryAfterId(afterId, count); + editHistoryRecords = await getHistoryAfterId(afterId, count, filterDeletions); } else { - editHistoryRecords = await getHistoryBeforeId(beforeId, count); + editHistoryRecords = await getHistoryBeforeId(beforeId, count, filterDeletions); } const currentBatchMaxId = editHistoryRecords[0]?.revision_id ?? decBigInt(beforeId); diff --git a/app/src/frontend/pages/changes.tsx b/app/src/frontend/pages/changes.tsx index f12aceb3..850be59a 100644 --- a/app/src/frontend/pages/changes.tsx +++ b/app/src/frontend/pages/changes.tsx @@ -17,8 +17,26 @@ interface PagingInfo { const recordsPerPage = 20; +function getNavigationUrl(options: { + afterId?: string, + beforeId?: string, + deletions?: string +}) { + let query = '?'; + let params = []; + if(options == undefined) return query; + + if(options.afterId != undefined) params.push(`after_id=${options.afterId}`); + if(options.beforeId != undefined) params.push(`before_id=${options.beforeId}`); + if(options.deletions != undefined) params.push(`deletions=${options.deletions}`); + + return query + params.join('&'); +} + const ChangesPage = (props: RouteComponentProps) => { - const { after_id, before_id } = parse(props.location.search); + let { after_id, before_id, deletions } = parse(props.location.search); + + if(deletions instanceof Array) deletions = deletions[0]; const [history, setHistory] = useState(); const [paging, setPaging] = useState(); @@ -37,6 +55,11 @@ const ChangesPage = (props: RouteComponentProps) => { if (before_id) { url = `${url}&before_id=${before_id}`; } + + if(deletions) { + url = `${url}&deletions=${deletions}`; + } + try { const {history, paging, error} = await apiGet(url); @@ -60,16 +83,30 @@ const ChangesPage = (props: RouteComponentProps) => {

Global edit history

- { paging?.id_for_newer_query && - Show more recent changes + Show more recent changes } { (after_id || before_id) && - Show latest changes + Show latest changes }
    + { + deletions === 'true' && + + } { error && @@ -97,7 +134,10 @@ const ChangesPage = (props: RouteComponentProps) => {
{ paging?.id_for_older_query && - Show older changes + Show older changes }
From 07cdae8899af6b170dfa09edc67350c34c4f1e96 Mon Sep 17 00:00:00 2001 From: Maciej Ziarkowski Date: Fri, 17 Jul 2020 16:27:40 +0200 Subject: [PATCH 2/2] Only detect deletions where attribute set to null For fields where an empty value is represented by null, it is easy to detect deletions. For other fields e.g. arrays, more complex logic needs to be developed later --- app/src/api/dataAccess/__mocks__/editHistory.ts | 2 +- app/src/api/dataAccess/editHistory.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/src/api/dataAccess/__mocks__/editHistory.ts b/app/src/api/dataAccess/__mocks__/editHistory.ts index 7c2320b7..735e71c1 100644 --- a/app/src/api/dataAccess/__mocks__/editHistory.ts +++ b/app/src/api/dataAccess/__mocks__/editHistory.ts @@ -18,7 +18,7 @@ mockEditHistory.__setHistory = function(mockHistoryData: EditHistoryEntry[]) { }; function hasDeletions(json) { - return Object.values(json).some(x => x === null || (Array.isArray(x) && x.length === 0)) + return Object.values(json).some(x => x === null); } mockEditHistory.getHistoryAfterId = function(id: string, count: number, filterDeletions: boolean): Promise { diff --git a/app/src/api/dataAccess/editHistory.ts b/app/src/api/dataAccess/editHistory.ts index 215c3bb4..eba1cfa1 100644 --- a/app/src/api/dataAccess/editHistory.ts +++ b/app/src/api/dataAccess/editHistory.ts @@ -14,7 +14,7 @@ const baseQuery = ` JOIN users ON logs.user_id = users.user_id`; const deletionCondition = ` - EXISTS (SELECT * from jsonb_each(forward_patch) WHERE value IN ('null', '[]')) + EXISTS (SELECT * from jsonb_each(forward_patch) WHERE value = 'null' ) `; export function getHistoryAfterId(id: string, count: number, filterDeletions: boolean = false): Promise {