FIX: correct link in the notification about moved post (#11399)

Notification is created by a job. If the job is evaluated before changes are committed to a database, a notification will have an incorrect URL.

Therefore, the job should be lodged in enqueue_jobs method which is triggered after the transaction:

```ruby
Topic.transaction do
  move_posts_to topic
end
add_allowed_users(participants) if participants.present? && @move_to_pm
enqueue_jobs(topic)
```

I improved a little bit specs to ensure that the destination topic_id is set. However, that tests are passing even without code improvements. I couldn't find an easy way to "delay" database transaction.

Meta: https://meta.discourse.org/t/bug-with-notifications-for-moved-posts/168937
This commit is contained in:
Krzysztof Kotlarek 2020-12-04 08:43:42 +11:00 committed by GitHub
parent bcbe2de646
commit e4d51e5b0a
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 11 additions and 15 deletions

View File

@ -71,7 +71,7 @@ class PostMover
create_temp_table
delete_invalid_post_timings
move_each_post
notify_users_that_posts_have_moved
create_moderator_post_in_original_topic
update_statistics
update_user_actions
update_last_post_stats
@ -438,19 +438,6 @@ class PostMover
UserAction.synchronize_target_topic_ids(posts.map(&:id))
end
def notify_users_that_posts_have_moved
enqueue_notification_job
create_moderator_post_in_original_topic
end
def enqueue_notification_job
Jobs.enqueue(
:notify_moved_posts,
post_ids: post_ids,
moved_by_id: user.id
)
end
def create_moderator_post_in_original_topic
move_type_str = PostMover.move_types[@move_type].to_s
move_type_str.sub!("topic", "message") if @move_to_pm
@ -538,6 +525,12 @@ class PostMover
def enqueue_jobs(topic)
@post_creator.enqueue_jobs if @post_creator
Jobs.enqueue(
:notify_moved_posts,
post_ids: post_ids,
moved_by_id: user.id
)
Jobs.enqueue(
:delete_inaccessible_notifications,
topic_id: topic.id

View File

@ -75,6 +75,7 @@ describe PostMover do
notification = p2.user.notifications.where(notification_type: Notification.types[:moved_post]).first
expect(notification.topic_id).to eq(p2.topic_id)
expect(notification.topic_id).not_to eq(old_topic_id)
expect(notification.post_number).to eq(1)
# no message for person who made the move
@ -84,6 +85,7 @@ describe PostMover do
notification = p6.user.notifications.where(notification_type: Notification.types[:moved_post]).first
expect(notification.topic_id).to eq(p2.topic_id)
expect(notification.topic_id).not_to eq(old_topic_id)
# this is the 3rd post we moved
expect(notification.post_number).to eq(3)
@ -541,7 +543,7 @@ describe PostMover do
# Should notify correctly
notification = p2.user.notifications.where(notification_type: Notification.types[:moved_post]).first
expect(notification.topic_id).to eq(p2.topic_id)
expect(notification.topic_id).to eq(destination_topic.id)
expect(notification.post_number).to eq(p2.post_number)
# Should update last reads
@ -1145,6 +1147,7 @@ describe PostMover do
notification = p2.user.notifications.where(notification_type: Notification.types[:moved_post]).first
expect(notification.topic_id).to eq(p2.topic_id)
expect(notification.topic_id).not_to eq(old_message_id)
expect(notification.post_number).to eq(1)
# no message for person who made the move