From 890489c0d23bb392cda0696a7bb3cad1591a268d Mon Sep 17 00:00:00 2001 From: Robin van Boven <497556+Beanow@users.noreply.github.com> Date: Mon, 2 Dec 2019 20:29:18 +0100 Subject: [PATCH] Discourse: make MockFetcher API similar to real forum (#1473) This extends the MockFetcher in the tests to provide new semantics update mode 2 relies on. They're based on the below changes to the Fetcher: - add categoryId and bumpedMs to Topic data #1454 - make topicWithPosts fetch all posts #1455 - add categoryDefinitionTopicIds to fetcher #1456 - implement topicsBumpedSince in fetcher #1457 Particularly because the addition of two new concepts (categories and category definition topics), the API of the MockFetcher got rather convoluted. This refactor makes it behave a lot more like you'd be familiar with within Discourse. Such as, creating a topic creates it's opening post as a side effect. Instead of a post with an unknown topic ID creating a topic as a side effect. And creating a category creates it's category definition topic as a side effect. Also, we're being a lot more explicit, using objects instead of positional arguments. --- src/plugins/discourse/mirror.test.js | 685 +++++++++++++++++++-------- 1 file changed, 498 insertions(+), 187 deletions(-) diff --git a/src/plugins/discourse/mirror.test.js b/src/plugins/discourse/mirror.test.js index 778dd24..b3f86e8 100644 --- a/src/plugins/discourse/mirror.test.js +++ b/src/plugins/discourse/mirror.test.js @@ -1,11 +1,13 @@ // @flow +import {max} from "d3-array"; import sortBy from "lodash.sortby"; import Database from "better-sqlite3"; import {Mirror, type MirrorOptions} from "./mirror"; import {SqliteMirrorRepository} from "./mirrorRepository"; import { type Discourse, + type CategoryId, type TopicId, type PostId, type Topic, @@ -19,47 +21,102 @@ import * as MapUtil from "../../util/map"; import * as NullUtil from "../../util/null"; import {TestTaskReporter} from "../../util/taskReporter"; -type PostInfo = {| - +indexWithinTopic: number, - +replyToPostIndex: number | null, - +topicId: number, - +authorUsername: string, - +cooked: string, +type TopicOptions = {| + // Defaults to: 1 (synonymous to "uncategorized") + categoryId?: CategoryId, + // Defaults to: "credbot" + authorUsername?: string, + // Defaults to: "
Here's some edited content.
"}); + await mirror.update(reporter); + expect(repo.posts()).toEqual([ + fetcher._post(t1.postId), + fetcher._post(t2.postId), + fetcher._post(p3), + ]); + expect(repo.posts()[2].cooked).toEqual( + "Here's some edited content.
" + ); + }); + + it("mirrors remainder when a post is deleted from the fetcher", async () => { + const {mirror, fetcher, reporter, repo} = example(); + const t1 = fetcher.addTopic(); + const t2 = fetcher.addTopic(); + const p3 = fetcher.addPost({topicId: t2.topicId}); + fetcher.deletePost(p3); + await mirror.update(reporter); + expect(repo.posts()).toEqual([ + fetcher._post(t1.postId), + fetcher._post(t2.postId), + ]); + }); + it("provides usernames for all active users", async () => { const {mirror, fetcher, reporter, repo} = example(); - fetcher.addPost(2, null, "alpha"); - fetcher.addPost(3, null, "beta"); - fetcher.addPost(3, 1, "alpha"); + const [_, t2] = [ + fetcher.addTopic({authorUsername: "alpha"}), + fetcher.addTopic({authorUsername: "beta"}), + ]; + fetcher.addPost({authorUsername: "delta", topicId: t2.topicId}); await mirror.update(reporter); - // credbot appears because it is the nominal author of all topics - expect( - repo - .users() - .slice() - .sort() - ).toEqual(["alpha", "beta", "credbot"]); + expect([...repo.users()].sort()).toEqual(["alpha", "beta", "delta"]); }); function expectLikesSorted(as, bs) { @@ -257,37 +501,49 @@ describe("plugins/discourse/mirror", () => { it("provides all the likes by users that have posted", async () => { const {mirror, fetcher, reporter, repo} = example(); - fetcher.addPost(1, null, "alpha"); - fetcher.addPost(2, null, "alpha"); - fetcher.addPost(3, null, "beta"); - const l1 = fetcher.addLike("beta", 1, 5); - const l2 = fetcher.addLike("beta", 2, 6); - const l3 = fetcher.addLike("beta", 3, 7); - const l4 = fetcher.addLike("alpha", 1, 8); + const [t1, t2, t3] = [ + fetcher.addTopic({authorUsername: "alpha"}), + fetcher.addTopic({authorUsername: "alpha"}), + fetcher.addTopic({authorUsername: "beta"}), + ]; + const [l1, l2, l3, l4] = [ + fetcher.addLike({username: "beta", postId: t1.postId, timestampMs: 5}), + fetcher.addLike({username: "beta", postId: t2.postId, timestampMs: 6}), + fetcher.addLike({username: "beta", postId: t3.postId, timestampMs: 7}), + fetcher.addLike({username: "alpha", postId: t1.postId, timestampMs: 8}), + ]; await mirror.update(reporter); expectLikesSorted(repo.likes(), [l1, l2, l3, l4]); - const l5 = fetcher.addLike("alpha", 2, 9); - fetcher.addPost(4, null, "credbot"); - const l6 = fetcher.addLike("credbot", 2, 10); - const l7 = fetcher.addLike("beta", 4, 11); + + const t4 = fetcher.addTopic({authorUsername: "credbot"}); + const [l5, l6, l7] = [ + fetcher.addLike({username: "alpha", postId: t2.postId, timestampMs: 9}), + fetcher.addLike({ + username: "credbot", + postId: t2.postId, + timestampMs: 10, + }), + fetcher.addLike({username: "beta", postId: t4.postId, timestampMs: 11}), + ]; + await mirror.update(reporter); expectLikesSorted(repo.likes(), [l1, l2, l3, l4, l5, l6, l7]); }); it("doesn't find likes of users that never posted", async () => { const {mirror, fetcher, reporter, repo} = example(); - fetcher.addPost(1, null); - fetcher.addLike("nope", 1, 1); + const t1 = fetcher.addTopic(); + fetcher.addLike({username: "nope", postId: t1.postId, timestampMs: 1}); await mirror.update(reporter); expect(repo.likes()).toEqual([]); }); - it("only fetches new topics on `update`", async () => { + it("should not fetch existing topics when adding a new one on second `update`", async () => { const {mirror, fetcher, reporter, repo} = example(); - fetcher.addPost(1, null); - fetcher.addPost(2, null); + fetcher.addTopic(); + fetcher.addTopic(); await mirror.update(reporter); - fetcher.addPost(3, null); + fetcher.addTopic(); const fetchTopicWithPosts = jest.spyOn(fetcher, "topicWithPosts"); await mirror.update(reporter); expect(fetchTopicWithPosts).toHaveBeenCalledTimes(1); @@ -297,62 +553,70 @@ describe("plugins/discourse/mirror", () => { it("gets new posts on old topics on update", async () => { const {mirror, fetcher, reporter, repo} = example(); - fetcher.addPost(1, null); - fetcher.addPost(2, null); + const t1 = fetcher.addTopic(); + fetcher.addTopic(); await mirror.update(reporter); - const id = fetcher.addPost(1, 1); - fetcher.addPost(3, null); + const p3 = fetcher.addPost({topicId: t1.topicId, replyToPostIndex: 1}); + fetcher.addTopic(); await mirror.update(reporter); const latestPosts = await fetcher.latestPosts(); // The post added to the old topic wasn't retrieved by latest post - expect(latestPosts.map((x) => x.id)).not.toContain(id); + expect(latestPosts.map((x) => x.id)).not.toContain(p3); const allPostIds = repo.posts().map((x) => x.id); // The post was still included, meaning the mirror scanned for new posts by id - expect(allPostIds).toContain(id); + expect(allPostIds).toContain(p3); }); it("skips null/missing topics", async () => { const {mirror, fetcher, reporter, repo} = example(); - fetcher.addPost(1, null); - fetcher.addPost(3, null); + fetcher.addTopic(); + // Force the internal counter to skip an ID. + fetcher._nextTopicId++; + fetcher.addTopic(); await mirror.update(reporter); expect(repo.topics().map((x) => x.id)).toEqual([1, 3]); }); it("skips null/missing posts", async () => { const {mirror, fetcher, reporter, repo} = example(); - const p1 = fetcher.addPost(1, null); - fetcher._latestPostId += 2; - const p2 = fetcher.addPost(3, null); + const t1 = fetcher.addTopic(); + // Force the internal counter to skip an ID. + fetcher._nextPostId++; + const t2 = fetcher.addTopic(); await mirror.update(reporter); - expect(repo.posts().map((x) => x.id)).toEqual([p1, p2]); + expect(repo.posts().map((x) => x.id)).toEqual([t1.postId, t2.postId]); }); it("queries explicitly for posts that are not present in topicWithPosts.posts", async () => { const {mirror, fetcher, reporter, repo} = example(); - const p1 = fetcher.addPost(1, null); - const p2 = fetcher.addPost(1, 1); - const p3 = fetcher.addPost(1, 1); + // As topicWithPosts.posts now returns all posts in the topic (#1455), + // test this by skipping an ID and spy to see the ID in between is fetched. + const t1 = fetcher.addTopic(); + const p2 = fetcher.addPost({topicId: t1.topicId}); + // Force the internal counter to skip an ID. + fetcher._nextPostId++; + const p3 = fetcher.addPost({topicId: t1.topicId}); const fetchPost = jest.spyOn(fetcher, "post"); await mirror.update(reporter); const getId = (x) => x.id; - const postsFromTopic = NullUtil.get(await fetcher.topicWithPosts(1)) - .posts; - expect(postsFromTopic.map(getId)).toEqual([p1]); + const {posts: postsFromTopic} = NullUtil.get( + await fetcher.topicWithPosts(t1.topicId) + ); + expect(postsFromTopic.map(getId)).toEqual([t1.postId, p2, p3]); const postsFromLatest = await fetcher.latestPosts(); expect(postsFromLatest.map(getId)).toEqual([p3]); expect(fetchPost).toHaveBeenCalledTimes(1); - expect(fetchPost).toHaveBeenCalledWith(p2); + expect(fetchPost).toHaveBeenCalledWith(p3 - 1); - expect(repo.posts().map(getId)).toEqual([p1, p2, p3]); + expect(repo.posts().map(getId)).toEqual([t1.postId, p2, p3]); }); it("does not explicitly query for posts that were in topicWithPosts.posts", async () => { const {mirror, fetcher, reporter} = example(); - fetcher.addPost(1, null); + fetcher.addTopic(); const fetchPost = jest.spyOn(fetcher, "post"); await mirror.update(reporter); expect(fetchPost).not.toHaveBeenCalled(); @@ -360,18 +624,18 @@ describe("plugins/discourse/mirror", () => { it("does not explicitly query for posts that were provided in latest posts", async () => { const {mirror, fetcher, reporter, repo} = example(); - fetcher.addPost(1, null); + const t1 = fetcher.addTopic(); await mirror.update(reporter); - const id = fetcher.addPost(1, 1); + const p2 = fetcher.addPost({topicId: t1.topicId, replyToPostIndex: 1}); const fetchPost = jest.spyOn(fetcher, "post"); await mirror.update(reporter); expect(fetchPost).not.toHaveBeenCalled(); - expect(repo.posts().map((x) => x.id)).toContain(id); + expect(repo.posts().map((x) => x.id)).toContain(p2); }); it("does not query for topics at all if there were no new topics", async () => { const {mirror, fetcher, reporter} = example(); - fetcher.addPost(1, null); + fetcher.addTopic(); await mirror.update(reporter); const fetchTopic = jest.spyOn(fetcher, "topicWithPosts"); await mirror.update(reporter); @@ -380,22 +644,23 @@ describe("plugins/discourse/mirror", () => { it("queries for likes for every user", async () => { const {mirror, fetcher, reporter} = example(); - fetcher.addPost(1, null, "alpha"); + fetcher.addTopic({authorUsername: "alpha"}); const fetchLikes = jest.spyOn(fetcher, "likesByUser"); await mirror.update(reporter); - expect(fetchLikes).toHaveBeenCalledTimes(2); - expect(fetchLikes).toHaveBeenCalledWith("credbot", 0); + expect(fetchLikes).toHaveBeenCalledTimes(1); expect(fetchLikes).toHaveBeenCalledWith("alpha", 0); }); it("queries with offset, as needed", async () => { const {mirror, fetcher, reporter} = example(); - fetcher.addPost(1, null, "credbot"); - fetcher.addPost(2, null, "credbot"); - fetcher.addPost(3, null, "credbot"); - fetcher.addLike("credbot", 1, 1); - fetcher.addLike("credbot", 2, 2); - fetcher.addLike("credbot", 3, 3); + const [t1, t2, t3] = [ + fetcher.addTopic({authorUsername: "credbot"}), + fetcher.addTopic({authorUsername: "credbot"}), + fetcher.addTopic({authorUsername: "credbot"}), + ]; + fetcher.addLike({username: "credbot", postId: t1.postId, timestampMs: 1}); + fetcher.addLike({username: "credbot", postId: t2.postId, timestampMs: 2}); + fetcher.addLike({username: "credbot", postId: t3.postId, timestampMs: 3}); const fetchLikes = jest.spyOn(fetcher, "likesByUser"); await mirror.update(reporter); expect(fetchLikes).toHaveBeenCalledTimes(2); @@ -405,13 +670,15 @@ describe("plugins/discourse/mirror", () => { it("ceases querying once it has found all the new likes", async () => { const {mirror, fetcher, reporter} = example(); - fetcher.addPost(1, null, "credbot"); - fetcher.addPost(2, null, "credbot"); - fetcher.addPost(3, null, "credbot"); - fetcher.addLike("credbot", 1, 1); - fetcher.addLike("credbot", 2, 2); + const [t1, t2, t3] = [ + fetcher.addTopic({authorUsername: "credbot"}), + fetcher.addTopic({authorUsername: "credbot"}), + fetcher.addTopic({authorUsername: "credbot"}), + ]; + fetcher.addLike({username: "credbot", postId: t1.postId, timestampMs: 1}); + fetcher.addLike({username: "credbot", postId: t2.postId, timestampMs: 2}); await mirror.update(reporter); - fetcher.addLike("credbot", 3, 3); + fetcher.addLike({username: "credbot", postId: t3.postId, timestampMs: 3}); const fetchLikes = jest.spyOn(fetcher, "likesByUser"); await mirror.update(reporter); expect(fetchLikes).toHaveBeenCalledTimes(1); @@ -419,18 +686,33 @@ describe("plugins/discourse/mirror", () => { }); it("warns if one of the latest posts has no topic", async () => { + /* + What this test does, is to create topic 1 and 2. + The order of posts will then be: + - T1's opening post + - T2's opening post + + Next by lowering the _nextTopicId counter we fool the + mock fetcher into returning T1 as the latest topic ID. + Meaning T2 won't be found. + + However, because T2's opening post is the most recent + post, it should be returned by .latestPosts(). + */ const {mirror, fetcher, reporter, repo} = example(); - const pid1 = fetcher.addPost(1, null, "credbot"); - const pid2 = fetcher.addPost(2, null, "credbot"); + const [t1, t2] = [ + fetcher.addTopic({authorUsername: "credbot"}), + fetcher.addTopic({authorUsername: "credbot"}), + ]; // Verify that the problem post is one of the latest posts const latestPostIds = (await fetcher.latestPosts()).map((x) => x.id); - expect(latestPostIds).toContain(pid2); - // Force the fetcher not to return topic 2 - fetcher._latestTopicId--; + expect(latestPostIds).toContain(t2.postId); + // Force the fetcher not to return topic 2 as the latest. + fetcher._nextTopicId--; await mirror.update(reporter); const topics = normalizeMode1Topics([fetcher._topic(1)]); expect(repo.topics()).toEqual(topics); - const posts = [fetcher._post(pid1)]; + const posts = [fetcher._post(t1.postId)]; expect(repo.posts()).toEqual(posts); expect(console.warn).toHaveBeenCalledWith( "Warning: Encountered error 'FOREIGN KEY constraint failed' " + @@ -441,19 +723,36 @@ describe("plugins/discourse/mirror", () => { }); it("warns if it finds a (non-latest) post with no topic", async () => { + /* + What this test does, is to create topic 1 and 2. + Next it adds a reply to topic 1. + The order of posts will then be: + - T1's opening post + - T2's opening post + - Reply to T1 + + Next by lowering the _nextTopicId counter we fool the + mock fetcher into returning T1 as the latest topic ID. + Meaning T2 won't be found. + + Additionally the opening post for T2 won't be the latest + post, as the reply to T1 came after that. + */ const {mirror, fetcher, reporter, repo} = example(); - const pid1 = fetcher.addPost(1, null, "credbot"); - const pid2 = fetcher.addPost(2, null, "credbot"); - const pid3 = fetcher.addPost(1, null, "credbot"); + const [t1, t2] = [ + fetcher.addTopic({authorUsername: "credbot"}), + fetcher.addTopic({authorUsername: "credbot"}), + ]; + const p3 = fetcher.addPost({topicId: t1.topicId, replyToPostIndex: 1}); // Verify that the problem post is not one of the latest posts const latestPostIds = (await fetcher.latestPosts()).map((x) => x.id); - expect(latestPostIds).not.toContain(pid2); - // Force the fetcher not to return topic 2 - fetcher._latestTopicId--; + expect(latestPostIds).not.toContain(t2.postId); + // Force the fetcher not to return topic 2 as the latest. + fetcher._nextTopicId--; await mirror.update(reporter); const topics = normalizeMode1Topics([fetcher._topic(1)]); expect(repo.topics()).toEqual(topics); - const posts = [pid1, pid3].map((x) => fetcher._post(x)); + const posts = [t1.postId, p3].map((x) => fetcher._post(x)); expect(repo.posts()).toEqual(posts); expect(console.warn).toHaveBeenCalledWith( "Warning: Encountered error 'FOREIGN KEY constraint failed' " + @@ -465,13 +764,13 @@ describe("plugins/discourse/mirror", () => { it("warns if it gets a like that doesn't correspond to any post", async () => { const {mirror, fetcher, reporter, repo} = example(); - const pid = fetcher.addPost(1, null, "credbot"); + const t1 = fetcher.addTopic({authorUsername: "credbot"}); const badLike = {username: "credbot", postId: 37, timestampMs: 0}; fetcher._likes.push(badLike); await mirror.update(reporter); - const topics = normalizeMode1Topics([fetcher._topic(1)]); + const topics = normalizeMode1Topics([fetcher._topic(t1.topicId)]); expect(repo.topics()).toEqual(topics); - expect(repo.posts()).toEqual([fetcher._post(pid)]); + expect(repo.posts()).toEqual([fetcher._post(t1.postId)]); expect(repo.likes()).toEqual([]); expect(console.warn).toHaveBeenCalledWith( "Warning: Encountered error 'FOREIGN KEY constraint failed' " + @@ -481,21 +780,30 @@ describe("plugins/discourse/mirror", () => { spyWarn().mockReset(); }); - it("warns if a user's likes are missing", async () => { + it("ignores if a user's likes are missing", async () => { const {mirror, fetcher, reporter, repo} = example(); - const pid = fetcher.addPost(1, null, "credbot"); + const t1 = fetcher.addTopic({authorUsername: "credbot"}); (fetcher: any).likesByUser = async () => null; await mirror.update(reporter); - expect(repo.topics()).toEqual(normalizeMode1Topics([fetcher._topic(1)])); - expect(repo.posts()).toEqual([fetcher._post(pid)]); + expect(repo.topics()).toEqual( + normalizeMode1Topics([fetcher._topic(t1.topicId)]) + ); + expect(repo.posts()).toEqual([fetcher._post(t1.postId)]); expect(repo.likes()).toEqual([]); }); it("inserts other likes if one user's likes are missing", async () => { const {mirror, fetcher, reporter, repo} = example(); - const p1 = fetcher.addPost(1, null, "credbot"); - const p2 = fetcher.addPost(1, 1, "otheruser"); - const l1 = fetcher.addLike("otheruser", 1, 123); + const t1 = fetcher.addTopic({authorUsername: "credbot"}); + const p2 = fetcher.addPost({ + topicId: t1.topicId, + authorUsername: "otheruser", + }); + const l1 = fetcher.addLike({ + username: "otheruser", + postId: t1.postId, + timestampMs: 123, + }); const _likesByUser = fetcher.likesByUser.bind(fetcher); (fetcher: any).likesByUser = async ( targetUsername: string, @@ -506,13 +814,16 @@ describe("plugins/discourse/mirror", () => { }; await mirror.update(reporter); expect(repo.topics()).toEqual(normalizeMode1Topics([fetcher._topic(1)])); - expect(repo.posts()).toEqual([fetcher._post(p1), fetcher._post(p2)]); + expect(repo.posts()).toEqual([ + fetcher._post(t1.postId), + fetcher._post(p2), + ]); expect(repo.likes()).toEqual([l1]); }); it("sends the right tasks to the TaskReporter", async () => { const {mirror, fetcher, reporter} = example(); - fetcher.addPost(1, null, "credbot"); + fetcher.addTopic({authorUsername: "credbot"}); await mirror.update(reporter); expect(reporter.activeTasks()).toEqual([]); expect(reporter.entries()).toEqual([ @@ -530,36 +841,36 @@ describe("plugins/discourse/mirror", () => { describe("findPostInTopic", () => { it("works for the first post in a topic", async () => { const {mirror, fetcher, reporter, repo} = example(); - const id = fetcher.addPost(5, null); - const post = NullUtil.get(fetcher._post(id)); - expect(post.topicId).toEqual(5); + const t1 = fetcher.addTopic(); + const post = NullUtil.get(fetcher._post(t1.postId)); + expect(post.topicId).toEqual(t1.topicId); expect(post.indexWithinTopic).toEqual(1); await mirror.update(reporter); - expect(repo.findPostInTopic(5, 1)).toEqual(id); + expect(repo.findPostInTopic(t1.topicId, 1)).toEqual(t1.postId); }); it("works for the second post in a topic", async () => { const {mirror, fetcher, reporter, repo} = example(); - fetcher.addPost(1, null); - const id = fetcher.addPost(1, 1); - const post = NullUtil.get(fetcher._post(id)); + const t1 = fetcher.addTopic(); + const p2 = fetcher.addPost({topicId: t1.topicId}); + const post = NullUtil.get(fetcher._post(p2)); expect(post.indexWithinTopic).toEqual(2); await mirror.update(reporter); - expect(repo.findPostInTopic(1, 2)).toEqual(id); + expect(repo.findPostInTopic(t1.topicId, 2)).toEqual(p2); }); it("returns undefined for a post with too high an index", async () => { const {mirror, fetcher, reporter, repo} = example(); - fetcher.addPost(1, null); + const t1 = fetcher.addTopic(); await mirror.update(reporter); - expect(repo.findPostInTopic(1, 2)).toBe(undefined); + expect(repo.findPostInTopic(t1.topicId, 2)).toBe(undefined); }); it("returns undefined for topic that doesnt exist", async () => { const {mirror, fetcher, reporter, repo} = example(); - fetcher.addPost(1, null); + const t1 = fetcher.addTopic(); await mirror.update(reporter); - expect(repo.findPostInTopic(2, 1)).toBe(undefined); + expect(repo.findPostInTopic(t1.topicId + 1, 1)).toBe(undefined); }); it("returns undefined for a mirror that never updated", async () => {