Discource-C
/
discourse
mirror of https://github.com/discourse/discourse.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

FIX: Move read state when moving posts 

* Moves / copies post timings
* Moves / copies topic users
* Fixes a small bug in the calculation of post numbers
This commit is contained in:
Gerhard Schlager 2019-09-06 20:48:57 +02:00
parent 631315624d
commit 52461abad9
4 changed files with 462 additions and 15 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

31
app/jobs/onceoff/clean_up_post_timings.rb Normal file
View File

@ -0,0 +1,31 @@
# frozen_string_literal: true
module Jobs
class CleanUpPostTimings < Jobs::Onceoff
# Remove post timings that are remnants of previous post moves
# or other shenanigans and don't reference a valid user or post anymore.
def execute_onceoff(args)
DB.exec <<~SQL
DELETE
FROM post_timings pt
WHERE NOT EXISTS(
SELECT 1
FROM posts p
WHERE p.topic_id = pt.topic_id
AND p.post_number = pt.post_number
)
SQL
DB.exec <<~SQL
DELETE
FROM post_timings pt
WHERE NOT EXISTS(
SELECT 1
FROM users u
WHERE pt.user_id = u.id
)
SQL
end
end
end

136
app/models/post_mover.rb
View File

@ -90,7 +90,10 @@ class PostMover
new_topic_title VARCHAR,
new_post_id INTEGER,
new_post_number INTEGER
)
);
CREATE INDEX moved_posts_old_post_number ON moved_posts(old_post_number);
CREATE INDEX moved_posts_old_post_id ON moved_posts(old_post_id);
SQL
end
@ -105,11 +108,8 @@ class PostMover
@move_map = {}
@reply_count = {}
posts.each_with_index do |post, offset|
unless post.is_first_post?
@move_map[post.post_number] = offset + max_post_number
else
@move_map[post.post_number] = 1
end
@move_map[post.post_number] = offset + max_post_number
if post.reply_to_post_number.present?
@reply_count[post.reply_to_post_number] = (@reply_count[post.reply_to_post_number] || 0) + 1
end
@ -131,6 +131,9 @@ class PostMover
update_reply_counts
move_first_post_replies
delete_post_replies
copy_first_post_timings
move_post_timings
copy_topic_users
end
def create_first_post(post)
@ -260,7 +263,7 @@ class PostMover
DB.exec <<~SQL
UPDATE post_replies pr
SET post_id = mp.new_post_id
FROM moved_posts mp, moved_posts mr
FROM moved_posts mp
WHERE mp.old_post_id <> mp.new_post_id AND pr.post_id = mp.old_post_id AND
EXISTS (SELECT 1 FROM moved_posts mr WHERE mr.new_post_id = pr.reply_id)
SQL
@ -275,11 +278,124 @@ class PostMover
SQL
end
def copy_first_post_timings
DB.exec <<~SQL
INSERT INTO post_timings (topic_id, user_id, post_number, msecs)
SELECT mp.new_topic_id, pt.user_id, mp.new_post_number, pt.msecs
FROM post_timings pt
JOIN moved_posts mp ON (pt.topic_id = mp.old_topic_id AND pt.post_number = mp.old_post_number)
WHERE mp.old_post_id <> mp.new_post_id
ON CONFLICT (topic_id, post_number, user_id) DO UPDATE
SET msecs = GREATEST(post_timings.msecs, excluded.msecs)
SQL
end
def move_post_timings
DB.exec <<~SQL
UPDATE post_timings pt
SET topic_id = mp.new_topic_id,
post_number = mp.new_post_number
FROM moved_posts mp
WHERE pt.topic_id = mp.old_topic_id
AND pt.post_number = mp.old_post_number
AND mp.old_post_id = mp.new_post_id
SQL
end
def copy_topic_users
params = {
old_topic_id: original_topic.id,
new_topic_id: destination_topic.id,
old_highest_post_number: destination_topic.highest_post_number,
old_highest_staff_post_number: destination_topic.highest_staff_post_number
}
DB.exec(<<~SQL, params)
INSERT INTO topic_users(user_id, topic_id, posted, last_read_post_number, highest_seen_post_number,
last_emailed_post_number, first_visited_at, last_visited_at, notification_level,
notifications_changed_at, notifications_reason_id)
SELECT tu.user_id,
:new_topic_id AS topic_id,
EXISTS(
SELECT 1
FROM posts p
WHERE p.topic_id = tu.topic_id
AND p.user_id = tu.user_id
) AS posted,
MAX(lr.new_post_number) AS last_read_post_number,
MAX(hs.new_post_number) AS highest_seen_post_number,
MAX(le.new_post_number) AS last_emailed_post_number,
GREATEST(tu.first_visited_at, t.created_at) AS first_visited_at,
GREATEST(tu.last_visited_at, t.created_at) AS last_visited_at,
tu.notification_level,
tu.notifications_changed_at,
tu.notifications_reason_id
FROM topic_users tu
JOIN topics t
ON (t.id = :new_topic_id)
LEFT OUTER JOIN moved_posts lr
ON (lr.old_topic_id = tu.topic_id AND lr.old_post_number <= tu.last_read_post_number)
LEFT OUTER JOIN moved_posts hs
ON (hs.old_topic_id = tu.topic_id AND hs.old_post_number <= tu.highest_seen_post_number)
LEFT OUTER JOIN moved_posts le
ON (le.old_topic_id = tu.topic_id AND le.old_post_number <= tu.last_emailed_post_number)
WHERE tu.topic_id = :old_topic_id
AND GREATEST(
tu.last_read_post_number,
tu.highest_seen_post_number,
tu.last_emailed_post_number
) >= (SELECT MIN(old_post_number) FROM moved_posts)
GROUP BY tu.topic_id, tu.user_id, tu.first_visited_at, tu.last_visited_at, t.created_at, tu.notification_level,
tu.notifications_changed_at,
tu.notifications_reason_id
ON CONFLICT (topic_id, user_id) DO UPDATE
SET posted = excluded.posted,
last_read_post_number = CASE
WHEN topic_users.last_read_post_number = :old_highest_staff_post_number OR (
:old_highest_post_number < :old_highest_staff_post_number
AND topic_users.last_read_post_number = :old_highest_post_number
AND NOT EXISTS(SELECT 1
FROM users u
WHERE u.id = topic_users.user_id
AND (admin OR moderator))
) THEN
GREATEST(topic_users.last_read_post_number,
excluded.last_read_post_number)
ELSE topic_users.last_read_post_number END,
highest_seen_post_number = CASE
WHEN topic_users.highest_seen_post_number = :old_highest_staff_post_number OR (
:old_highest_post_number < :old_highest_staff_post_number
AND topic_users.highest_seen_post_number = :old_highest_post_number
AND NOT EXISTS(SELECT 1
FROM users u
WHERE u.id = topic_users.user_id
AND (admin OR moderator))
) THEN
GREATEST(topic_users.highest_seen_post_number,
excluded.highest_seen_post_number)
ELSE topic_users.highest_seen_post_number END,
last_emailed_post_number = CASE
WHEN topic_users.last_emailed_post_number = :old_highest_staff_post_number OR (
:old_highest_post_number < :old_highest_staff_post_number
AND topic_users.last_emailed_post_number = :old_highest_post_number
AND NOT EXISTS(SELECT 1
FROM users u
WHERE u.id = topic_users.user_id
AND (admin OR moderator))
) THEN
GREATEST(topic_users.last_emailed_post_number,
excluded.last_emailed_post_number)
ELSE topic_users.last_emailed_post_number END,
first_visited_at = LEAST(topic_users.first_visited_at, excluded.first_visited_at),
last_visited_at = GREATEST(topic_users.last_visited_at, excluded.last_visited_at)
SQL
end
def update_statistics
destination_topic.update_statistics
original_topic.update_statistics
TopicUser.update_post_action_cache(topic_id: original_topic.id, post_action_type: :bookmark)
TopicUser.update_post_action_cache(topic_id: destination_topic.id, post_action_type: :bookmark)
TopicUser.update_post_action_cache(topic_id: original_topic.id)
TopicUser.update_post_action_cache(topic_id: destination_topic.id)
end
def update_user_actions
@ -340,7 +456,7 @@ class PostMover
attrs = {}
attrs[:last_posted_at] = post.created_at
attrs[:last_post_user_id] = post.user_id
attrs[:bumped_at] = post.created_at unless post.no_bump
attrs[:bumped_at] = Time.now
attrs[:updated_at] = Time.now
destination_topic.update_columns(attrs)
end

4
spec/fabricators/post_fabricator.rb
View File

@ -162,3 +162,7 @@ Fabricator(:post_via_email, from: :post) do
incoming_email.user = post.user
end
end
Fabricator(:whisper, from: :post) do
post_type Post.types[:whisper]
end

306
spec/models/post_mover_spec.rb
View File

@ -114,6 +114,17 @@ describe PostMover do
before do
TopicUser.update_last_read(user, topic.id, p4.post_number, p4.post_number, 0)
TopicLink.extract_from(p2)
freeze_time Time.now
end
def create_post_timing(post, user, msecs)
PostTiming.create!(
topic_id: post.topic_id,
user_id: user.id,
post_number: post.post_number,
msecs: msecs
)
end
context "post replies" do
@ -207,7 +218,7 @@ describe PostMover do
p4.reload
expect(new_topic.last_post_user_id).to eq(p4.user_id)
expect(new_topic.last_posted_at).to eq(p4.created_at)
expect(new_topic.bumped_at).to eq(p4.created_at)
expect(new_topic.bumped_at).to be_within(1.second).of(Time.now)
p2.reload
expect(p2.sort_order).to eq(1)
@ -234,7 +245,7 @@ describe PostMover do
notification_level: TopicUser.notification_levels[:watching],
notifications_reason_id: TopicUser.notification_reasons[:created_topic]
)).to eq(true)
expect(TopicUser.exists?(user_id: user, topic_id: new_topic.id)).to eq(false)
expect(TopicUser.exists?(user_id: user, topic_id: new_topic.id)).to eq(true)
end
it "moving all posts will close the topic" do
@ -334,11 +345,113 @@ describe PostMover do
expect(Notification.exists?(user_notification.id)).to eq(false)
expect(Notification.exists?(admin_notification.id)).to eq(true)
end
it "moves post timings" do
some_user = Fabricate(:user)
create_post_timing(p1, some_user, 500)
create_post_timing(p2, some_user, 1000)
create_post_timing(p3, some_user, 1500)
create_post_timing(p4, some_user, 750)
new_topic = topic.move_posts(user, [p1.id, p4.id], title: "new testing topic name")
expect(PostTiming.where(topic_id: topic.id, user_id: some_user.id).pluck(:post_number, :msecs))
.to contain_exactly([1, 500], [2, 1000], [3, 1500])
expect(PostTiming.where(topic_id: new_topic.id, user_id: some_user.id).pluck(:post_number, :msecs))
.to contain_exactly([1, 500], [2, 750])
end
context "read state and other stats per user" do
def create_topic_user(user, opts = {})
notification_level = opts.delete(:notification_level) || :regular
Fabricate(:topic_user, opts.merge(
notification_level: TopicUser.notification_levels[notification_level],
topic: topic,
user: user
))
end
fab!(:user1) { Fabricate(:user) }
fab!(:user2) { Fabricate(:user) }
fab!(:user3) { Fabricate(:user) }
fab!(:admin1) { Fabricate(:admin) }
fab!(:admin2) { Fabricate(:admin) }
it "correctly moves topic_user records" do
create_topic_user(
user1,
last_read_post_number: 4,
highest_seen_post_number: 4,
last_emailed_post_number: 3,
notification_level: :tracking
)
create_topic_user(
user3,
last_read_post_number: 1,
highest_seen_post_number: 2,
last_emailed_post_number: 4,
notification_level: :watching
)
new_topic = topic.move_posts(user, [p1.id, p2.id], title: "new testing topic name")
expect(TopicUser.where(topic_id: topic.id).count).to eq(3)
expect(TopicUser.find_by(topic: topic, user: user))
.to have_attributes(
last_read_post_number: 4,
highest_seen_post_number: 4,
last_emailed_post_number: nil,
notification_level: TopicUser.notification_levels[:tracking]
)
expect(TopicUser.find_by(topic: topic, user: user1))
.to have_attributes(
last_read_post_number: 4,
highest_seen_post_number: 4,
last_emailed_post_number: 3,
notification_level: TopicUser.notification_levels[:tracking]
)
expect(TopicUser.find_by(topic: topic, user: user3))
.to have_attributes(
last_read_post_number: 1,
highest_seen_post_number: 2,
last_emailed_post_number: 4,
notification_level: TopicUser.notification_levels[:watching]
)
expect(TopicUser.where(topic_id: new_topic.id).count).to eq(3)
expect(TopicUser.find_by(topic: new_topic, user: user))
.to have_attributes(
last_read_post_number: 1,
highest_seen_post_number: 1,
last_emailed_post_number: nil,
notification_level: TopicUser.notification_levels[:watching],
posted: true
)
expect(TopicUser.find_by(topic: new_topic, user: user1))
.to have_attributes(
last_read_post_number: 2,
highest_seen_post_number: 2,
last_emailed_post_number: 2,
notification_level: TopicUser.notification_levels[:tracking],
posted: false
)
expect(TopicUser.find_by(topic: new_topic, user: user3))
.to have_attributes(
last_read_post_number: 1,
highest_seen_post_number: 2,
last_emailed_post_number: 2,
notification_level: TopicUser.notification_levels[:watching],
posted: false
)
end
end
end
context "to an existing topic" do
fab!(:destination_topic) { Fabricate(:topic, user: another_user) }
fab!(:destination_op) { Fabricate(:post, topic: destination_topic, user: another_user) }
fab!(:destination_op) { Fabricate(:post, topic: destination_topic, user: another_user, created_at: 1.day.ago) }
it "works correctly" do
topic.expects(:add_moderator_post).once
@ -355,7 +468,7 @@ describe PostMover do
p4.reload
expect(moved_to.last_post_user_id).to eq(p4.user_id)
expect(moved_to.last_posted_at).to eq(p4.created_at)
expect(moved_to.bumped_at).to eq(p4.created_at)
expect(moved_to.bumped_at).to be_within(1.second).of(Time.now)
# Posts should be re-ordered
p2.reload
@ -440,6 +553,189 @@ describe PostMover do
expect(Notification.exists?(user_notification.id)).to eq(false)
expect(Notification.exists?(admin_notification.id)).to eq(true)
end
it "moves post timings" do
some_user = Fabricate(:user)
create_post_timing(p1, some_user, 500)
create_post_timing(p2, some_user, 1000)
create_post_timing(p3, some_user, 1500)
create_post_timing(p4, some_user, 750)
moved_to = topic.move_posts(user, [p1.id, p4.id], destination_topic_id: destination_topic.id)
expect(PostTiming.where(topic_id: topic.id, user_id: some_user.id).pluck(:post_number, :msecs))
.to contain_exactly([1, 500], [2, 1000], [3, 1500])
expect(PostTiming.where(topic_id: moved_to.id, user_id: some_user.id).pluck(:post_number, :msecs))
.to contain_exactly([2, 500], [3, 750])
end
context "read state and other stats per user" do
def create_topic_user(user, topic, opts = {})
notification_level = opts.delete(:notification_level) || :regular
Fabricate(:topic_user, opts.merge(
notification_level: TopicUser.notification_levels[notification_level],
topic: topic,
user: user
))
end
fab!(:user1) { Fabricate(:user) }
fab!(:user2) { Fabricate(:user) }
fab!(:user3) { Fabricate(:user) }
fab!(:admin1) { Fabricate(:admin) }
fab!(:admin2) { Fabricate(:admin) }
it "leaves post numbers unchanged when they were lower then the topic's highest post number" do
Fabricate(:post, topic: destination_topic)
Fabricate(:whisper, topic: destination_topic)
destination_topic.reload
expect(destination_topic.highest_post_number).to eq(2)
expect(destination_topic.highest_staff_post_number).to eq(3)
create_topic_user(
user1, topic,
last_read_post_number: 3,
highest_seen_post_number: 3,
last_emailed_post_number: 3
)
create_topic_user(
user1, destination_topic,
last_read_post_number: 1,
highest_seen_post_number: 2,
last_emailed_post_number: 1
)
create_topic_user(
user2, topic,
last_read_post_number: 3,
highest_seen_post_number: 3,
last_emailed_post_number: 3
)
create_topic_user(
user2, destination_topic,
last_read_post_number: 2,
highest_seen_post_number: 1,
last_emailed_post_number: 2
)
create_topic_user(
admin1, topic,
last_read_post_number: 3,
highest_seen_post_number: 3,
last_emailed_post_number: 3
)
create_topic_user(
admin1, destination_topic,
last_read_post_number: 2,
highest_seen_post_number: 3,
last_emailed_post_number: 1
)
create_topic_user(
admin2, topic,
last_read_post_number: 3,
highest_seen_post_number: 3,
last_emailed_post_number: 3
)
create_topic_user(
admin2, destination_topic,
last_read_post_number: 3,
highest_seen_post_number: 2,
last_emailed_post_number: 3
)
moved_to_topic = topic.move_posts(user, [p1.id, p2.id], destination_topic_id: destination_topic.id)
expect(TopicUser.find_by(topic: moved_to_topic, user: user1))
.to have_attributes(
last_read_post_number: 1,
highest_seen_post_number: 5,
last_emailed_post_number: 1
)
expect(TopicUser.find_by(topic: moved_to_topic, user: user2))
.to have_attributes(
last_read_post_number: 5,
highest_seen_post_number: 1,
last_emailed_post_number: 5
)
expect(TopicUser.find_by(topic: moved_to_topic, user: admin1))
.to have_attributes(
last_read_post_number: 2,
highest_seen_post_number: 5,
last_emailed_post_number: 1
)
expect(TopicUser.find_by(topic: moved_to_topic, user: admin2))
.to have_attributes(
last_read_post_number: 5,
highest_seen_post_number: 2,
last_emailed_post_number: 5
)
end
it "correctly updates existing topic_user records" do
destination_topic.update!(created_at: 1.day.ago)
original_topic_user1 = create_topic_user(
user1, topic,
highest_seen_post_number: 5,
first_visited_at: 5.hours.ago,
last_visited_at: 30.minutes.ago,
notification_level: :tracking
).reload
destination_topic_user1 = create_topic_user(
user1, destination_topic,
highest_seen_post_number: 5,
first_visited_at: 7.hours.ago,
last_visited_at: 2.hours.ago,
notification_level: :watching
).reload
original_topic_user2 = create_topic_user(
user2, topic,
highest_seen_post_number: 5,
first_visited_at: 3.hours.ago,
last_visited_at: 1.hour.ago,
notification_level: :watching
).reload
destination_topic_user2 = create_topic_user(
user2, destination_topic,
highest_seen_post_number: 5,
first_visited_at: 2.hours.ago,
last_visited_at: 1.hour.ago,
notification_level: :tracking
).reload
new_topic = topic.move_posts(user, [p1.id, p2.id], destination_topic_id: destination_topic.id)
expect(TopicUser.find_by(topic: new_topic, user: user))
.to have_attributes(
notification_level: TopicUser.notification_levels[:tracking],
posted: true
)
expect(TopicUser.find_by(topic: new_topic, user: user1))
.to have_attributes(
first_visited_at: destination_topic_user1.first_visited_at,
last_visited_at: original_topic_user1.last_visited_at,
notification_level: destination_topic_user1.notification_level,
posted: false
)
expect(TopicUser.find_by(topic: new_topic, user: user2))
.to have_attributes(
first_visited_at: original_topic_user2.first_visited_at,
last_visited_at: destination_topic_user2.last_visited_at,
notification_level: destination_topic_user2.notification_level,
posted: false
)
end
end
end
context "to a message" do
@ -465,7 +761,7 @@ describe PostMover do
p4.reload
expect(new_topic.last_post_user_id).to eq(p4.user_id)
expect(new_topic.last_posted_at).to eq(p4.created_at)
expect(new_topic.bumped_at).to eq(p4.created_at)
expect(new_topic.bumped_at).to be_within(1.second).of(Time.now)
p2.reload
expect(p2.sort_order).to eq(1)