-
-
Notifications
You must be signed in to change notification settings - Fork 71
Split ThreadFollower into separate models for posts and threads #1920
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from all commits
17986cd
ffd7791
8882785
9a3e5c0
97c2649
5da697b
f7c79f0
ac4f738
c82fd1f
a2af203
5b77c12
5eb10d1
59ca21c
8d26314
c0acdf0
b890d38
58848e8
c1b8251
6d8e9a2
74b2c2e
550bf1f
0304650
b030287
c19a3a7
f94f99f
393e1c1
0117d76
9c7b116
6a9472b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| class CleanUpNewThreadFollowersJob < ApplicationJob | ||
| queue_as :default | ||
|
|
||
| def perform | ||
| sql = File.read(Rails.root.join('db/scripts/posts_with_duplicate_new_thread_followers.sql')) | ||
| posts = ActiveRecord::Base.connection.execute(sql).to_a | ||
|
|
||
| posts.each do |post| | ||
| user_id, post_id = post | ||
|
|
||
| followers = NewThreadFollower.where(post_id: post_id, user_id: user_id) | ||
|
|
||
| next unless followers.many? | ||
|
|
||
| duplicate = followers.first | ||
| result = duplicate.destroy | ||
|
|
||
| unless result | ||
| puts "failed to destroy new thread follower duplicate \"#{duplicate.id}\"" | ||
| duplicate.errors.each { |e| puts e.full_message } | ||
| end | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| class NewThreadFollower < ApplicationRecord | ||
| belongs_to :user | ||
| belongs_to :post | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,22 +1,13 @@ | ||
| class ThreadFollower < ApplicationRecord | ||
| belongs_to :comment_thread, optional: true | ||
| belongs_to :post, optional: true | ||
| belongs_to :comment_thread | ||
| belongs_to :user | ||
|
|
||
| after_create :bump_thread_last_activity | ||
| before_destroy :bump_thread_last_activity | ||
|
|
||
| validate :thread_or_post | ||
|
|
||
| private | ||
|
|
||
| def bump_thread_last_activity | ||
| comment_thread&.bump_last_activity(persist_changes: true) | ||
| end | ||
|
|
||
| def thread_or_post | ||
| if comment_thread.nil? && post.nil? | ||
| errors.add(:base, 'Must refer to either a comment thread or a post.') | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| class CreateNewThreadFollowers < ActiveRecord::Migration[7.2] | ||
| def up | ||
| create_table_new_thread_followers | ||
| if column_exists?(:thread_followers, :post_id) | ||
| move_rows_with_non_nil_post_id | ||
| remove_post_id_column_from_thread_followers | ||
| end | ||
| end | ||
|
|
||
| def down | ||
| if !column_exists?(:thread_followers, :post_id) | ||
| add_post_id_column_to_thread_followers | ||
| end | ||
| move_rows_back_from_new_thread_followers | ||
| delete_table_new_thread_followers | ||
| end | ||
|
|
||
| def create_table_new_thread_followers | ||
| create_table :new_thread_followers, if_not_exists: true do |t| | ||
| t.bigint :user_id | ||
| t.bigint :post_id | ||
|
|
||
| t.timestamps | ||
| end | ||
| add_index :new_thread_followers, [:user_id, :post_id], if_not_exists: true | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would add indexes here for
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Revision after discussion in Discord: let's add an additional index for
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for explaining that the composite index works differently in MySQL. I've now added the index on |
||
| add_index :new_thread_followers, :post_id, if_not_exists: true | ||
| end | ||
|
|
||
| def move_rows_with_non_nil_post_id | ||
| NewThreadFollower.insert_all( | ||
| ThreadFollower.select(:user_id, :post_id, :created_at, :updated_at) | ||
| .where.not(post_id:nil) | ||
| .to_a | ||
| .map(&:attributes) | ||
| ) | ||
| ThreadFollower.where.not(post_id:nil).delete_all | ||
ArtOfCode- marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| end | ||
|
|
||
| def remove_post_id_column_from_thread_followers | ||
| remove_reference :thread_followers, :post, index: true, foreign_key: true, if_exists: true | ||
| end | ||
|
|
||
| def add_post_id_column_to_thread_followers | ||
| add_reference :thread_followers, :post, index: true, foreign_key: true, if_not_exists: true | ||
| end | ||
|
|
||
| def move_rows_back_from_new_thread_followers | ||
| ThreadFollower.insert_all( | ||
| NewThreadFollower.select(:user_id, :post_id, :created_at, :updated_at) | ||
| .to_a | ||
| .map(&:attributes) | ||
| ) | ||
| NewThreadFollower.delete_all | ||
ArtOfCode- marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| end | ||
|
|
||
| def delete_table_new_thread_followers | ||
| remove_index :new_thread_followers, :post_id, if_exists: true | ||
| remove_index :new_thread_followers, [:user_id, :post_id], if_exists: true | ||
| remove_reference :new_thread_followers, :user_id, foreign_key: true, if_exists: true | ||
| remove_reference :new_thread_followers, :post_id, foreign_key: true, if_exists: true | ||
| drop_table :new_thread_followers, if_exists: true | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| select | ||
| user_id, | ||
| post_id, | ||
| count(*) as count | ||
| from | ||
| new_thread_followers | ||
| group by | ||
| user_id, | ||
| post_id | ||
| having | ||
| count > 1; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| CleanUpNewThreadFollowersJob.perform_later |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| # Read about fixtures at https://api.rubyonrails.org/classes/ActiveRecord/FixtureSet.html | ||
| standard_author_question_one: | ||
| user: standard_user | ||
| post: question_one | ||
|
|
||
| merge_source_question_one: | ||
| user: merge_source | ||
| post: question_one |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-blocking: let's make the check more robust with our identity helper: