From 0bc609634c96786703eeb9b27b8d112eb3298425 Mon Sep 17 00:00:00 2001 From: Saumya Pandey Date: Tue, 20 Jul 2021 23:05:29 +0530 Subject: [PATCH] fix: Allow searching of previous document titles (#2326) * Add migrations * Handle previousTitles when titles is updated * Add necessary test cases * Use previous title while searching * Rewrite logic to update previousTitles in beforeSave hook * Update weights * Update test to match new rank order * Add tooltip to inform user on document * Add code comment * Remove previous title tooltip * fix: Remove unused string, add model tests Co-authored-by: Tom Moor --- server/api/documents.test.js | 27 ++++- ...20210716064654-introduce-previousTitles.js | 13 ++ ...10716071454-search-index-previousTitles.js | 33 ++++++ server/models/Document.js | 6 + server/models/Document.test.js | 111 ++++++++++++++++++ 5 files changed, 188 insertions(+), 2 deletions(-) create mode 100644 server/migrations/20210716064654-introduce-previousTitles.js create mode 100644 server/migrations/20210716071454-search-index-previousTitles.js diff --git a/server/api/documents.test.js b/server/api/documents.test.js index 4e0983c85..1f6413a9e 100644 --- a/server/api/documents.test.js +++ b/server/api/documents.test.js @@ -900,6 +900,7 @@ describe("#documents.search", () => { userId: user.id, teamId: user.teamId, }); + const secondResult = await buildDocument({ title: "random text", text: "search term", @@ -907,15 +908,26 @@ describe("#documents.search", () => { teamId: user.teamId, }); + const thirdResult = await buildDocument({ + title: "search term", + text: "random text", + userId: user.id, + teamId: user.teamId, + }); + + thirdResult.title = "change"; + await thirdResult.save(); + const res = await server.post("/api/documents.search", { body: { token: user.getJwtToken(), query: "search term" }, }); const body = await res.json(); expect(res.status).toEqual(200); - expect(body.data.length).toEqual(2); + expect(body.data.length).toEqual(3); expect(body.data[0].document.id).toEqual(firstResult.id); expect(body.data[1].document.id).toEqual(secondResult.id); + expect(body.data[2].document.id).toEqual(thirdResult.id); }); it("should return partial results in ranked order", async () => { @@ -933,15 +945,26 @@ describe("#documents.search", () => { teamId: user.teamId, }); + const thirdResult = await buildDocument({ + title: "search term", + text: "random text", + userId: user.id, + teamId: user.teamId, + }); + + thirdResult.title = "change"; + await thirdResult.save(); + const res = await server.post("/api/documents.search", { body: { token: user.getJwtToken(), query: "sear &" }, }); const body = await res.json(); expect(res.status).toEqual(200); - expect(body.data.length).toEqual(2); + expect(body.data.length).toEqual(3); expect(body.data[0].document.id).toEqual(firstResult.id); expect(body.data[1].document.id).toEqual(secondResult.id); + expect(body.data[2].document.id).toEqual(thirdResult.id); }); it("should strip junk from search term", async () => { diff --git a/server/migrations/20210716064654-introduce-previousTitles.js b/server/migrations/20210716064654-introduce-previousTitles.js new file mode 100644 index 000000000..ca94641b0 --- /dev/null +++ b/server/migrations/20210716064654-introduce-previousTitles.js @@ -0,0 +1,13 @@ +'use strict'; + +module.exports = { + up: async (queryInterface, Sequelize) => { + await queryInterface.addColumn("documents","previousTitles",{ + type: Sequelize.ARRAY(Sequelize.STRING) + }) + }, + + down: async (queryInterface, Sequelize) => { + await queryInterface.removeColumn("documents", "previousTitles"); + } +}; diff --git a/server/migrations/20210716071454-search-index-previousTitles.js b/server/migrations/20210716071454-search-index-previousTitles.js new file mode 100644 index 000000000..6519f8fb5 --- /dev/null +++ b/server/migrations/20210716071454-search-index-previousTitles.js @@ -0,0 +1,33 @@ +'use strict'; + +module.exports = { + up: async (queryInterface, Sequelize) => { + const searchDocument = ` + CREATE OR REPLACE FUNCTION documents_search_trigger() RETURNS trigger AS $$ + begin + new."searchVector" := + setweight(to_tsvector('english', coalesce(new.title, '')),'A') || + setweight(to_tsvector('english', coalesce(array_to_string(new."previousTitles", ' , '),'')),'C') || + setweight(to_tsvector('english', coalesce(new.text, '')), 'B'); + return new; + end + $$ LANGUAGE plpgsql; + `; + await queryInterface.sequelize.query(searchDocument); + }, + + down: async (queryInterface, Sequelize) => { + const searchDocument = ` + CREATE OR REPLACE FUNCTION documents_search_trigger() RETURNS trigger AS $$ + begin + new."searchVector" := + setweight(to_tsvector('english', coalesce(new.title, '')),'A') || + setweight(to_tsvector('english', coalesce(new.text, '')), 'C'); + return new; + end + $$ LANGUAGE plpgsql; + `; + await queryInterface.sequelize.query(searchDocument); + + } +}; diff --git a/server/models/Document.js b/server/models/Document.js index eca88ab0f..472aab664 100644 --- a/server/models/Document.js +++ b/server/models/Document.js @@ -39,6 +39,11 @@ const beforeSave = async (doc) => { // ensure documents have a title doc.title = doc.title || ""; + if (doc.previous("title") && doc.previous("title") !== doc.title) { + if (!doc.previousTitles) doc.previousTitles = []; + doc.previousTitles = uniq(doc.previousTitles.concat(doc.previous("title"))); + } + // add the current user as a collaborator on this doc if (!doc.collaboratorIds) doc.collaboratorIds = []; doc.collaboratorIds = uniq(doc.collaboratorIds.concat(doc.lastModifiedById)); @@ -70,6 +75,7 @@ const Document = sequelize.define( }, }, }, + previousTitles: DataTypes.ARRAY(DataTypes.STRING), version: DataTypes.SMALLINT, template: DataTypes.BOOLEAN, editorVersion: DataTypes.STRING, diff --git a/server/models/Document.test.js b/server/models/Document.test.js index 891107c90..6f24a47b1 100644 --- a/server/models/Document.test.js +++ b/server/models/Document.test.js @@ -221,6 +221,41 @@ describe("#searchForTeam", () => { const { totalCount } = await Document.searchForTeam(team, "test"); expect(totalCount).toBe("2"); }); + + test("should return the document when searched with their previous titles", async () => { + const team = await buildTeam(); + const collection = await buildCollection({ teamId: team.id }); + const document = await buildDocument({ + teamId: team.id, + collectionId: collection.id, + title: "test number 1", + }); + + document.title = "change"; + await document.save(); + + const { totalCount } = await Document.searchForTeam(team, "test number"); + expect(totalCount).toBe("1"); + }); + + test("should not return the document when searched with neither the titles nor the previous titles", async () => { + const team = await buildTeam(); + const collection = await buildCollection({ teamId: team.id }); + const document = await buildDocument({ + teamId: team.id, + collectionId: collection.id, + title: "test number 1", + }); + + document.title = "change"; + await document.save(); + + const { totalCount } = await Document.searchForTeam( + team, + "title doesn't exist" + ); + expect(totalCount).toBe("0"); + }); }); describe("#searchForUser", () => { @@ -273,6 +308,51 @@ describe("#searchForUser", () => { const { totalCount } = await Document.searchForUser(user, "test"); expect(totalCount).toBe("2"); }); + + test("should return the document when searched with their previous titles", async () => { + const team = await buildTeam(); + const user = await buildUser({ teamId: team.id }); + const collection = await buildCollection({ + teamId: team.id, + userId: user.id, + }); + const document = await buildDocument({ + teamId: team.id, + userId: user.id, + collectionId: collection.id, + title: "test number 1", + }); + + document.title = "change"; + await document.save(); + + const { totalCount } = await Document.searchForUser(user, "test number"); + expect(totalCount).toBe("1"); + }); + + test("should not return the document when searched with neither the titles nor the previous titles", async () => { + const team = await buildTeam(); + const user = await buildUser({ teamId: team.id }); + const collection = await buildCollection({ + teamId: team.id, + userId: user.id, + }); + const document = await buildDocument({ + teamId: team.id, + userId: user.id, + collectionId: collection.id, + title: "test number 1", + }); + + document.title = "change"; + await document.save(); + + const { totalCount } = await Document.searchForUser( + user, + "title doesn't exist" + ); + expect(totalCount).toBe("0"); + }); }); describe("#delete", () => { @@ -309,6 +389,37 @@ describe("#delete", () => { }); }); +describe("#save", () => { + test("should have empty previousTitles by default", async () => { + const document = await buildDocument(); + expect(document.previousTitles).toBe(null); + }); + + test("should include previousTitles on save", async () => { + const document = await buildDocument(); + + document.title = "test"; + await document.save(); + + expect(document.previousTitles.length).toBe(1); + }); + + test("should not duplicate previousTitles", async () => { + const document = await buildDocument(); + + document.title = "test"; + await document.save(); + + document.title = "example"; + await document.save(); + + document.title = "test"; + await document.save(); + + expect(document.previousTitles.length).toBe(3); + }); +}); + describe("#findByPk", () => { test("should return document when urlId is correct", async () => { const { document } = await seed();