From 98626ebbaf9b53237e52809207a0f22cb7400ec0 Mon Sep 17 00:00:00 2001 From: Renan Filipe Date: Tue, 22 Sep 2020 03:05:42 -0300 Subject: [PATCH] feat: Record search queries (#1554) * Record search queries * feat: add totalCount to the search response * feat: add comments to explain why we use setTimeout --- server/api/documents.js | 11 +- server/api/documents.test.js | 20 +++ server/api/hooks.js | 19 ++- server/api/hooks.test.js | 26 +++- .../20200915010511-create-search-queries.js | 43 ++++++ server/models/Document.js | 144 ++++++++++++------ server/models/Document.test.js | 76 ++++++++- server/models/SearchQuery.js | 42 +++++ server/models/index.js | 3 + 9 files changed, 330 insertions(+), 54 deletions(-) create mode 100644 server/migrations/20200915010511-create-search-queries.js create mode 100644 server/models/SearchQuery.js diff --git a/server/api/documents.js b/server/api/documents.js index f8a37584..6baf3466 100644 --- a/server/api/documents.js +++ b/server/api/documents.js @@ -11,6 +11,7 @@ import { Document, Event, Revision, + SearchQuery, Share, Star, User, @@ -586,7 +587,7 @@ router.post("documents.search", auth(), pagination(), async (ctx) => { ); } - const results = await Document.searchForUser(user, query, { + const { results, totalCount } = await Document.searchForUser(user, query, { includeArchived: includeArchived === "true", includeDrafts: includeDrafts === "true", collaboratorIds, @@ -604,6 +605,14 @@ router.post("documents.search", auth(), pagination(), async (ctx) => { }) ); + SearchQuery.create({ + userId: user.id, + teamId: user.teamId, + source: "app", + query, + results: totalCount, + }); + const policies = presentPolicies(user, documents); ctx.body = { diff --git a/server/api/documents.test.js b/server/api/documents.test.js index 4d079253..e2494145 100644 --- a/server/api/documents.test.js +++ b/server/api/documents.test.js @@ -8,6 +8,7 @@ import { Revision, Backlink, CollectionUser, + SearchQuery, } from "../models"; import { buildShare, @@ -954,6 +955,25 @@ describe("#documents.search", () => { expect(res.status).toEqual(401); expect(body).toMatchSnapshot(); }); + + it("should save search term, hits and source", async (done) => { + const { user } = await seed(); + await server.post("/api/documents.search", { + body: { token: user.getJwtToken(), query: "my term" }, + }); + + // setTimeout is needed here because SearchQuery is saved asynchronously + // in order to not slow down the response time. + setTimeout(async () => { + const searchQuery = await SearchQuery.findAll({ + where: { query: "my term" }, + }); + expect(searchQuery.length).toBe(1); + expect(searchQuery[0].results).toBe(0); + expect(searchQuery[0].source).toBe("app"); + done(); + }, 100); + }); }); describe("#documents.archived", () => { diff --git a/server/api/hooks.js b/server/api/hooks.js index 7dacc255..0616b920 100644 --- a/server/api/hooks.js +++ b/server/api/hooks.js @@ -2,7 +2,14 @@ import Router from "koa-router"; import { escapeRegExp } from "lodash"; import { AuthenticationError, InvalidRequestError } from "../errors"; -import { Authentication, Document, User, Team, Collection } from "../models"; +import { + Authentication, + Document, + User, + Team, + Collection, + SearchQuery, +} from "../models"; import { presentSlackAttachment } from "../presenters"; import * as Slack from "../slack"; const router = new Router(); @@ -146,10 +153,18 @@ router.post("hooks.slack", async (ctx) => { const options = { limit: 5, }; - const results = user + const { results, totalCount } = user ? await Document.searchForUser(user, text, options) : await Document.searchForTeam(team, text, options); + SearchQuery.create({ + userId: user ? user.id : null, + teamId: team.id, + source: "slack", + query: text, + results: totalCount, + }); + if (results.length) { const attachments = []; for (const result of results) { diff --git a/server/api/hooks.test.js b/server/api/hooks.test.js index 7e643b1c..10873f5f 100644 --- a/server/api/hooks.test.js +++ b/server/api/hooks.test.js @@ -1,7 +1,7 @@ /* eslint-disable flowtype/require-valid-file-annotation */ import TestServer from "fetch-test-server"; import app from "../app"; -import { Authentication } from "../models"; +import { Authentication, SearchQuery } from "../models"; import * as Slack from "../slack"; import { buildDocument } from "../test/factories"; import { flushdb, seed } from "../test/support"; @@ -132,6 +132,30 @@ describe("#hooks.slack", () => { ); }); + it("should save search term, hits and source", async (done) => { + const { user, team } = await seed(); + await server.post("/api/hooks.slack", { + body: { + token: process.env.SLACK_VERIFICATION_TOKEN, + user_id: user.serviceId, + team_id: team.slackId, + text: "contains", + }, + }); + + // setTimeout is needed here because SearchQuery is saved asynchronously + // in order to not slow down the response time. + setTimeout(async () => { + const searchQuery = await SearchQuery.findAll({ + where: { query: "contains" }, + }); + expect(searchQuery.length).toBe(1); + expect(searchQuery[0].results).toBe(0); + expect(searchQuery[0].source).toBe("slack"); + done(); + }, 100); + }); + it("should respond with help content for help keyword", async () => { const { user, team } = await seed(); const res = await server.post("/api/hooks.slack", { diff --git a/server/migrations/20200915010511-create-search-queries.js b/server/migrations/20200915010511-create-search-queries.js new file mode 100644 index 00000000..f97abcfa --- /dev/null +++ b/server/migrations/20200915010511-create-search-queries.js @@ -0,0 +1,43 @@ +"use strict"; +module.exports = { + up: async (queryInterface, Sequelize) => { + await queryInterface.createTable("search_queries", { + id: { + allowNull: false, + primaryKey: true, + type: Sequelize.UUID, + }, + userId: { + type: Sequelize.UUID, + references: { + model: "users", + }, + }, + teamId: { + type: Sequelize.UUID, + references: { + model: "teams", + }, + }, + source: { + type: Sequelize.ENUM("slack", "app"), + allowNull: false, + }, + query: { + type: Sequelize.STRING, + allowNull: false, + }, + results: { + type: Sequelize.INTEGER, + allowNull: false, + }, + createdAt: { + allowNull: false, + type: Sequelize.DATE, + }, + }); + }, + down: async (queryInterface, Sequelize) => { + await queryInterface.dropTable("search_queries"); + }, +}; diff --git a/server/models/Document.js b/server/models/Document.js index 472fcdd9..a4932871 100644 --- a/server/models/Document.js +++ b/server/models/Document.js @@ -251,10 +251,13 @@ Document.findByPk = async function (id, options = {}) { } }; -type SearchResult = { - ranking: number, - context: string, - document: Document, +type SearchResponse = { + results: { + ranking: number, + context: string, + document: Document, + }[], + totalCount: number, }; type SearchOptions = { @@ -277,7 +280,7 @@ Document.searchForTeam = async ( team, query, options: SearchOptions = {} -): Promise => { +): Promise => { const limit = options.limit || 15; const offset = options.offset || 0; const wildcardQuery = `${escape(query)}:*`; @@ -285,21 +288,25 @@ Document.searchForTeam = async ( // If the team has access no public collections then shortcircuit the rest of this if (!collectionIds.length) { - return []; + return { results: [], totalCount: 0 }; } // Build the SQL query to get documentIds, ranking, and search term context - const sql = ` + const whereClause = ` + "searchVector" @@ to_tsquery('english', :query) AND + "teamId" = :teamId AND + "collectionId" IN(:collectionIds) AND + "deletedAt" IS NULL AND + "publishedAt" IS NOT NULL + `; + + const selectSql = ` SELECT id, ts_rank(documents."searchVector", to_tsquery('english', :query)) as "searchRanking", ts_headline('english', "text", to_tsquery('english', :query), 'MaxFragments=1, MinWords=20, MaxWords=30') as "searchContext" FROM documents - WHERE "searchVector" @@ to_tsquery('english', :query) AND - "teamId" = :teamId AND - "collectionId" IN(:collectionIds) AND - "deletedAt" IS NULL AND - "publishedAt" IS NOT NULL + WHERE ${whereClause} ORDER BY "searchRanking" DESC, "updatedAt" DESC @@ -307,17 +314,34 @@ Document.searchForTeam = async ( OFFSET :offset; `; - const results = await sequelize.query(sql, { + const countSql = ` + SELECT COUNT(id) + FROM documents + WHERE ${whereClause} + `; + + const queryReplacements = { + teamId: team.id, + query: wildcardQuery, + collectionIds, + }; + + const resultsQuery = sequelize.query(selectSql, { type: sequelize.QueryTypes.SELECT, replacements: { - teamId: team.id, - query: wildcardQuery, + ...queryReplacements, limit, offset, - collectionIds, }, }); + const countQuery = sequelize.query(countSql, { + type: sequelize.QueryTypes.SELECT, + replacements: queryReplacements, + }); + + const [results, [{ count }]] = await Promise.all([resultsQuery, countQuery]); + // Final query to get associated document data const documents = await Document.findAll({ where: { @@ -330,20 +354,23 @@ Document.searchForTeam = async ( ], }); - return map(results, (result) => ({ - ranking: result.searchRanking, - context: removeMarkdown(unescape(result.searchContext), { - stripHTML: false, - }), - document: find(documents, { id: result.id }), - })); + return { + results: map(results, (result) => ({ + ranking: result.searchRanking, + context: removeMarkdown(unescape(result.searchContext), { + stripHTML: false, + }), + document: find(documents, { id: result.id }), + })), + totalCount: count, + }; }; Document.searchForUser = async ( user, query, options: SearchOptions = {} -): Promise => { +): Promise => { const limit = options.limit || 15; const offset = options.offset || 0; const wildcardQuery = `${escape(query)}:*`; @@ -360,7 +387,7 @@ Document.searchForUser = async ( // If the user has access to no collections then shortcircuit the rest of this if (!collectionIds.length) { - return []; + return { results: [], totalCount: 0 }; } let dateFilter; @@ -369,13 +396,8 @@ Document.searchForUser = async ( } // Build the SQL query to get documentIds, ranking, and search term context - const sql = ` - SELECT - id, - ts_rank(documents."searchVector", to_tsquery('english', :query)) as "searchRanking", - ts_headline('english', "text", to_tsquery('english', :query), 'MaxFragments=1, MinWords=20, MaxWords=30') as "searchContext" - FROM documents - WHERE "searchVector" @@ to_tsquery('english', :query) AND + const whereClause = ` + "searchVector" @@ to_tsquery('english', :query) AND "teamId" = :teamId AND "collectionId" IN(:collectionIds) AND ${ @@ -393,27 +415,52 @@ Document.searchForUser = async ( ? '("publishedAt" IS NOT NULL OR "createdById" = :userId)' : '"publishedAt" IS NOT NULL' } + `; + + const selectSql = ` + SELECT + id, + ts_rank(documents."searchVector", to_tsquery('english', :query)) as "searchRanking", + ts_headline('english', "text", to_tsquery('english', :query), 'MaxFragments=1, MinWords=20, MaxWords=30') as "searchContext" + FROM documents + WHERE ${whereClause} ORDER BY "searchRanking" DESC, "updatedAt" DESC LIMIT :limit OFFSET :offset; -`; + `; - const results = await sequelize.query(sql, { + const countSql = ` + SELECT COUNT(id) + FROM documents + WHERE ${whereClause} + `; + + const queryReplacements = { + teamId: user.teamId, + userId: user.id, + collaboratorIds: options.collaboratorIds, + query: wildcardQuery, + collectionIds, + dateFilter, + }; + + const resultsQuery = sequelize.query(selectSql, { type: sequelize.QueryTypes.SELECT, replacements: { - teamId: user.teamId, - userId: user.id, - collaboratorIds: options.collaboratorIds, - query: wildcardQuery, + ...queryReplacements, limit, offset, - collectionIds, - dateFilter, }, }); + const countQuery = sequelize.query(countSql, { + type: sequelize.QueryTypes.SELECT, + replacements: queryReplacements, + }); + + const [results, [{ count }]] = await Promise.all([resultsQuery, countQuery]); // Final query to get associated document data const documents = await Document.scope( { @@ -432,13 +479,16 @@ Document.searchForUser = async ( ], }); - return map(results, (result) => ({ - ranking: result.searchRanking, - context: removeMarkdown(unescape(result.searchContext), { - stripHTML: false, - }), - document: find(documents, { id: result.id }), - })); + return { + results: map(results, (result) => ({ + ranking: result.searchRanking, + context: removeMarkdown(unescape(result.searchContext), { + stripHTML: false, + }), + document: find(documents, { id: result.id }), + })), + totalCount: count, + }; }; // Hooks diff --git a/server/models/Document.test.js b/server/models/Document.test.js index ae48995e..3befac7c 100644 --- a/server/models/Document.test.js +++ b/server/models/Document.test.js @@ -201,7 +201,7 @@ describe("#searchForTeam", () => { title: "test", }); - const results = await Document.searchForTeam(team, "test"); + const { results } = await Document.searchForTeam(team, "test"); expect(results.length).toBe(1); expect(results[0].document.id).toBe(document.id); }); @@ -218,15 +218,85 @@ describe("#searchForTeam", () => { title: "test", }); - const results = await Document.searchForTeam(team, "test"); + const { results } = await Document.searchForTeam(team, "test"); expect(results.length).toBe(0); }); test("should handle no collections", async () => { const team = await buildTeam(); - const results = await Document.searchForTeam(team, "test"); + const { results } = await Document.searchForTeam(team, "test"); expect(results.length).toBe(0); }); + + test("should return the total count of search results", async () => { + const team = await buildTeam(); + const collection = await buildCollection({ teamId: team.id }); + await buildDocument({ + teamId: team.id, + collectionId: collection.id, + title: "test number 1", + }); + await buildDocument({ + teamId: team.id, + collectionId: collection.id, + title: "test number 2", + }); + + const { totalCount } = await Document.searchForTeam(team, "test"); + expect(totalCount).toBe("2"); + }); +}); + +describe("#searchForUser", () => { + test("should return search results from collections", async () => { + const team = await buildTeam(); + const user = await buildUser({ teamId: team.id }); + const collection = await buildCollection({ + userId: user.id, + teamId: team.id, + }); + const document = await buildDocument({ + userId: user.id, + teamId: team.id, + collectionId: collection.id, + title: "test", + }); + + const { results } = await Document.searchForUser(user, "test"); + expect(results.length).toBe(1); + expect(results[0].document.id).toBe(document.id); + }); + + test("should handle no collections", async () => { + const team = await buildTeam(); + const user = await buildUser({ teamId: team.id }); + const { results } = await Document.searchForUser(user, "test"); + expect(results.length).toBe(0); + }); + + test("should return the total count of search results", async () => { + const team = await buildTeam(); + const user = await buildUser({ teamId: team.id }); + const collection = await buildCollection({ + userId: user.id, + teamId: team.id, + }); + await buildDocument({ + userId: user.id, + teamId: team.id, + collectionId: collection.id, + title: "test number 1", + }); + await buildDocument({ + userId: user.id, + teamId: team.id, + collectionId: collection.id, + title: "test number 2", + }); + + const { totalCount } = await Document.searchForUser(user, "test"); + expect(totalCount).toBe("2"); + }); }); describe("#delete", () => { diff --git a/server/models/SearchQuery.js b/server/models/SearchQuery.js new file mode 100644 index 00000000..d6abc0ff --- /dev/null +++ b/server/models/SearchQuery.js @@ -0,0 +1,42 @@ +// @flow +import { DataTypes, sequelize } from "../sequelize"; + +const SearchQuery = sequelize.define( + "search_queries", + { + id: { + type: DataTypes.UUID, + defaultValue: DataTypes.UUIDV4, + primaryKey: true, + }, + source: { + type: DataTypes.ENUM("slack", "app"), + allowNull: false, + }, + query: { + type: DataTypes.STRING, + allowNull: false, + }, + results: { + type: DataTypes.NUMBER, + allowNull: false, + }, + }, + { + timestamps: true, + updatedAt: false, + } +); + +SearchQuery.associate = (models) => { + SearchQuery.belongsTo(models.User, { + as: "user", + foreignKey: "userId", + }); + SearchQuery.belongsTo(models.Team, { + as: "team", + foreignKey: "teamId", + }); +}; + +export default SearchQuery; diff --git a/server/models/index.js b/server/models/index.js index 16e8fe85..c5f2da6a 100644 --- a/server/models/index.js +++ b/server/models/index.js @@ -14,6 +14,7 @@ import Integration from "./Integration"; import Notification from "./Notification"; import NotificationSetting from "./NotificationSetting"; import Revision from "./Revision"; +import SearchQuery from "./SearchQuery"; import Share from "./Share"; import Star from "./Star"; import Team from "./Team"; @@ -36,6 +37,7 @@ const models = { Notification, NotificationSetting, Revision, + SearchQuery, Share, Star, Team, @@ -66,6 +68,7 @@ export { Notification, NotificationSetting, Revision, + SearchQuery, Share, Star, Team,