Merge pull request #623 from mz8i/feature/deletions-history

Allow filtering deletions on edit history page

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') {

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<EditHistoryEntry[]> {
+function hasDeletions(json) {
+    return Object.values(json).some(x => x === null);
+}
+
+mockEditHistory.getHistoryAfterId = function(id: string, count: number, filterDeletions: boolean): Promise<EditHistoryEntry[]> {
     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<EditHistoryEntry[]> {
+mockEditHistory.getHistoryBeforeId = function(id: string, count: number, filterDeletions: boolean): Promise<EditHistoryEntry[]> {
     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)

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<EditHistoryEntry[]> {
+const deletionCondition = `
+    EXISTS (SELECT * from jsonb_each(forward_patch) WHERE value = 'null' )
+`;
+
+export function getHistoryAfterId(id: string, count: number, filterDeletions: boolean = false): Promise<EditHistoryEntry[]> {
     /** 
      * 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<EditHistor
             SELECT * FROM (
                 ${baseQuery}
                 WHERE log_id > $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<EditHistor
     }
 }
 
-export function getHistoryBeforeId(id: string, count: number): Promise<EditHistoryEntry[]> {
+export function getHistoryBeforeId(id: string, count: number, filterDeletions: boolean = false): Promise<EditHistoryEntry[]> {
     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<EditHisto
             return db.any(`
                 ${baseQuery}
                 WHERE log_id < $1
+                ${filterDeletions ? 'AND ' + deletionCondition : ''}
                 ORDER BY revision_id DESC
                 LIMIT $2
             `, [id, count]);

app/src/api/services/__tests__/editHistory.test.ts

@@ -173,4 +173,24 @@ describe('getGlobalEditHistory()', () => {
         });
 
     });
+
+    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']);
+        });
+    });
 });

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);

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<EditHistoryEntry[]>();
     const [paging, setPaging] = useState<PagingInfo>();
@@ -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) => {
         <article>
             <section className="main-col">
                 <h1>Global edit history</h1>
-
                 {
                     paging?.id_for_newer_query &&
-                        <Link className='edit-history-link' to={`?after_id=${paging.id_for_newer_query}`}>Show more recent changes</Link>
+                        <Link
+                            className='edit-history-link'
+                            to={getNavigationUrl({
+                                afterId: paging.id_for_newer_query,
+                                deletions: deletions
+                            })}
+                        >Show more recent changes</Link>
                 }
                 {
                     (after_id || before_id) &&
-                        <Link className='edit-history-latest-link' to='?'>Show latest changes</Link>
+                        <Link
+                            className='edit-history-latest-link'
+                            to={getNavigationUrl({
+                                deletions: deletions
+                            })}
+                        >Show latest changes</Link>
                 }
                 <ul className="edit-history-list">
+                    {
+                        deletions === 'true' &&
+                            <InfoBox msg="Showing only changes with at least one deletion" /> 
+                    }
                     {
                         error &&
                             <ErrorBox msg={error}></ErrorBox>
@@ -97,7 +134,10 @@ const ChangesPage = (props: RouteComponentProps) => {
                 </ul>
                 {
                     paging?.id_for_older_query &&
-                        <Link to={`?before_id=${paging.id_for_older_query}`}>Show older changes</Link>
+                        <Link to={getNavigationUrl({
+                            beforeId: paging.id_for_older_query,
+                            deletions: deletions
+                        })}>Show older changes</Link>
                 }
             </section>
         </article>