From 20b94bc71432b1987bfd3e55d08c62e66967133a Mon Sep 17 00:00:00 2001 From: Gerhard Schlager Date: Fri, 25 May 2018 11:31:45 +0200 Subject: [PATCH] FIX: Extraction of quoted posts failed in some cases * It stored only oneboxed "quotes" when [quote] and links to topics or posts were mixed. * Revising a post didn't add or remove records from the quoted_posts table. --- app/models/quoted_post.rb | 31 +++++++++-------------- lib/post_revisor.rb | 2 +- spec/models/quoted_post_spec.rb | 34 ++++++++++++++++++++------ spec/services/username_changer_spec.rb | 13 ---------- 4 files changed, 40 insertions(+), 40 deletions(-) diff --git a/app/models/quoted_post.rb b/app/models/quoted_post.rb index ecfe19e73d3..c6d24675f5b 100644 --- a/app/models/quoted_post.rb +++ b/app/models/quoted_post.rb @@ -10,7 +10,8 @@ class QuotedPost < ActiveRecord::Base doc = Nokogiri::HTML.fragment(post.cooked) uniq = {} - ids = [] + + exec_sql("DELETE FROM quoted_posts WHERE post_id = :post_id", post_id: post.id) doc.css("aside.quote[data-topic]").each do |a| topic_id = a['data-topic'].to_i @@ -22,34 +23,26 @@ class QuotedPost < ActiveRecord::Base begin # It would be so much nicer if we used post_id in quotes - results = exec_sql "INSERT INTO quoted_posts(post_id, quoted_post_id, created_at, updated_at) - SELECT :post_id, p.id, current_timestamp, current_timestamp - FROM posts p - LEFT JOIN quoted_posts q on q.post_id = :post_id AND q.quoted_post_id = p.id - WHERE post_number = :post_number AND - topic_id = :topic_id AND - q.id IS NULL - RETURNING quoted_post_id - ", post_id: post.id, post_number: post_number, topic_id: topic_id - - results = results.to_a - ids << results[0]["quoted_post_id"].to_i if results.length > 0 + exec_sql(<<~SQL, post_id: post.id, post_number: post_number, topic_id: topic_id) + INSERT INTO quoted_posts (post_id, quoted_post_id, created_at, updated_at) + SELECT :post_id, p.id, current_timestamp, current_timestamp + FROM posts p + LEFT JOIN quoted_posts q on q.post_id = :post_id AND q.quoted_post_id = p.id + WHERE post_number = :post_number AND + topic_id = :topic_id AND + q.id IS NULL + SQL rescue ActiveRecord::RecordNotUnique, PG::UniqueViolation # it's fine end end - if ids.length > 0 - exec_sql "DELETE FROM quoted_posts WHERE post_id = :post_id AND quoted_post_id NOT IN (:ids)", - post_id: post.id, ids: ids - end - # simplest place to add this code reply_quoted = false if post.reply_to_post_number reply_post_id = Post.where(topic_id: post.topic_id, post_number: post.reply_to_post_number).pluck(:id).first - reply_quoted = !!(reply_post_id && ids.include?(reply_post_id)) + reply_quoted = reply_post_id.present? && QuotedPost.where(post_id: post.id, quoted_post_id: reply_post_id).count > 0 end if reply_quoted != post.reply_quoted diff --git a/lib/post_revisor.rb b/lib/post_revisor.rb index 81ab76494a0..d74e50dd95b 100644 --- a/lib/post_revisor.rb +++ b/lib/post_revisor.rb @@ -190,6 +190,7 @@ class PostRevisor # WARNING: do not pull this into the transaction # it can fire events in sidekiq before the post is done saving # leading to corrupt state + QuotedPost.extract_from(@post) post_process_post update_topic_word_counts @@ -198,7 +199,6 @@ class PostRevisor grant_badge TopicLink.extract_from(@post) - QuotedPost.extract_from(@post) successfully_saved_post_and_topic end diff --git a/spec/models/quoted_post_spec.rb b/spec/models/quoted_post_spec.rb index 4c61952c693..0e7c6b0ef61 100644 --- a/spec/models/quoted_post_spec.rb +++ b/spec/models/quoted_post_spec.rb @@ -1,14 +1,34 @@ require 'rails_helper' describe QuotedPost do - it 'correctly extracts quotes in integration test' do - post1 = create_post - post2 = create_post(topic_id: post1.topic_id, - raw: "[quote=\"#{post1.user.username}, post: 1, topic:#{post1.topic_id}\"]\ntest\n[/quote]\nthis is a test post", - reply_to_post_number: 1) + it 'correctly extracts quotes' do + topic = Fabricate(:topic) + post1 = create_post(topic: topic, post_number: 1, raw: "foo bar") + post2 = create_post(topic: topic, post_number: 2, raw: "lorem ipsum") + post3 = create_post(topic: topic, post_number: 3, raw: "test post") - expect(QuotedPost.find_by(post_id: post2.id, quoted_post_id: post1.id)).not_to eq(nil) - expect(post2.reply_quoted).to eq(true) + raw = <<~RAW + #{post1.full_url} + + [quote="#{post2.user.username}, post:#{post2.post_number}, topic:#{post2.topic.id}"] + lorem + [/quote] + + this is a test post + + #{post3.full_url} + RAW + + post4 = create_post(topic: topic, raw: raw, post_number: 4, reply_to_post_number: post3.post_number) + + expect(QuotedPost.where(post_id: post4.id).pluck(:quoted_post_id)).to contain_exactly(post1.id, post2.id, post3.id) + expect(post4.reload.reply_quoted).to eq(true) + + SiteSetting.editing_grace_period = 1.minute.to_i + post5 = create_post(topic: topic, post_number: 5, raw: "post 5") + raw.sub!(post3.full_url, post5.full_url) + post4.revise(post4.user, { raw: raw }, revised_at: post4.updated_at + 2.minutes) + expect(QuotedPost.where(post_id: post4.id).pluck(:quoted_post_id)).to contain_exactly(post1.id, post2.id, post5.id) end it 'correctly handles deltas' do diff --git a/spec/services/username_changer_spec.rb b/spec/services/username_changer_spec.rb index 92bdb27a574..2ceeb66fda9 100644 --- a/spec/services/username_changer_spec.rb +++ b/spec/services/username_changer_spec.rb @@ -387,19 +387,6 @@ describe UsernameChanger do expect(post.raw).to eq(expected_raw) expect(post.cooked).to match_html(expected_cooked) end - - it 'replaces the username in quote tags when the post is marked as deleted' do - post = create_post_and_change_username(raw: raw) do |p| - PostDestroyer.new(p.user, p).destroy - end - - expect(post.raw).to_not include("foo") - expect(post.cooked).to_not include("foo") - expect(post.revisions.count).to eq(1) - - expect(post.revisions[0].modifications["raw"][0]).to eq(expected_raw) - expect(post.revisions[0].modifications["cooked"][0]).to match_html(expected_cooked) - end end end