From 96aca6d7e6dc3c05d761098a19f96b82d2783b1c Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Mon, 9 Jul 2018 16:54:18 +0800 Subject: [PATCH] Remove legacy vote post action code. (#6009) --- app/models/post.rb | 4 +- app/models/post_action.rb | 3 - app/models/post_action_type.rb | 3 +- app/models/topic.rb | 8 +-- config/locales/client.en.yml | 9 --- config/locales/server.en.yml | 5 -- db/fixtures/000_delayed_drops.rb | 72 ++++++++++++++----- db/fixtures/003_post_action_types.rb | 7 -- .../20120928170023_add_sort_order_to_posts.rb | 6 -- ...8_drop_vote_count_from_topics_and_posts.rb | 9 +++ ...0180619092320_clean_up_vote_post_action.rb | 10 +++ lib/migration/base_dropper.rb | 4 +- lib/migration/column_dropper.rb | 10 +-- lib/migration/table_dropper.rb | 16 +++-- spec/components/guardian_spec.rb | 13 ---- .../migration/column_dropper_spec.rb | 14 +++- spec/serializers/post_serializer_spec.rb | 8 +-- 17 files changed, 114 insertions(+), 87 deletions(-) create mode 100644 db/migrate/20180619091608_drop_vote_count_from_topics_and_posts.rb create mode 100644 db/migrate/20180619092320_clean_up_vote_post_action.rb diff --git a/app/models/post.rb b/app/models/post.rb index 99cc14b7dc7..de539046ee5 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -10,6 +10,9 @@ require 'archetype' require 'digest/sha1' class Post < ActiveRecord::Base + # TODO: Remove this after 19th Dec 2018 + self.ignored_columns = %w{vote_count} + include RateLimiter::OnCreateRecord include Trashable include Searchable @@ -834,7 +837,6 @@ end # score :float # reads :integer default(0), not null # post_type :integer default(1), not null -# vote_count :integer default(0), not null # sort_order :integer # last_editor_id :integer # hidden :boolean default(FALSE), not null diff --git a/app/models/post_action.rb b/app/models/post_action.rb index 7cc74de526a..5af5e8cdf55 100644 --- a/app/models/post_action.rb +++ b/app/models/post_action.rb @@ -470,9 +470,6 @@ class PostAction < ActiveRecord::Base # We probably want to refactor this method to something cleaner. case post_action_type_key - when :vote - # Voting also changes the sort_order - Post.where(id: post_id).update_all ["vote_count = :count, sort_order = :max - :count", count: count, max: Topic.max_sort_order] when :like # 'like_score' is weighted higher for staff accounts score = PostAction.joins(:user) diff --git a/app/models/post_action_type.rb b/app/models/post_action_type.rb index 9f757aa013e..031c1e11972 100644 --- a/app/models/post_action_type.rb +++ b/app/models/post_action_type.rb @@ -70,8 +70,7 @@ class PostActionType < ActiveRecord::Base unless @types @types = Enum.new( bookmark: 1, - like: 2, - vote: 5 + like: 2 ) @types.merge!(flag_settings.flag_types) end diff --git a/app/models/topic.rb b/app/models/topic.rb index 86ce4a7a59a..f49b8ec052f 100644 --- a/app/models/topic.rb +++ b/app/models/topic.rb @@ -13,6 +13,9 @@ require_dependency 'topic_posters_summary' require_dependency 'topic_featured_users' class Topic < ActiveRecord::Base + # TODO: Remove this after 19th Dec 2018 + self.ignored_columns = %w{vote_count} + class UserExists < StandardError; end include ActionView::Helpers::SanitizeHelper include RateLimiter::OnCreateRecord @@ -44,10 +47,6 @@ class Topic < ActiveRecord::Base end end - def self.max_sort_order - @max_sort_order ||= (2**31) - 1 - end - def self.max_fancy_title_length 400 end @@ -1429,7 +1428,6 @@ end # archived :boolean default(FALSE), not null # bumped_at :datetime not null # has_summary :boolean default(FALSE), not null -# vote_count :integer default(0), not null # archetype :string default("regular"), not null # featured_user4_id :integer # notify_moderators_count :integer default(0), not null diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index c851f10b7e9..6aeedf2c4ae 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -2110,7 +2110,6 @@ en: inappropriate: "Undo flag" bookmark: "Undo bookmark" like: "Undo like" - vote: "Undo vote" people: off_topic: "flagged this as off-topic" spam: "flagged this as spam" @@ -2122,7 +2121,6 @@ en: like_capped: one: "and {{count}} other liked this" other: "and {{count}} others liked this" - vote: "voted for this" by_you: off_topic: "You flagged this as off-topic" spam: "You flagged this as spam" @@ -2131,7 +2129,6 @@ en: notify_user: "You sent a message to this user" bookmark: "You bookmarked this post" like: "You liked this" - vote: "You voted for this post" by_you_and_others: off_topic: one: "You and 1 other flagged this as off-topic" @@ -2154,9 +2151,6 @@ en: like: one: "You and 1 other liked this" other: "You and {{count}} other people liked this" - vote: - one: "You and 1 other voted for this post" - other: "You and {{count}} other people voted for this post" by_others: off_topic: one: "1 person flagged this as off-topic" @@ -2179,9 +2173,6 @@ en: like: one: "1 person liked this" other: "{{count}} people liked this" - vote: - one: "1 person voted for this post" - other: "{{count}} people voted for this post" delete: confirm: diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 2d0602e7672..96ed9082ca4 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -758,11 +758,6 @@ en: description: 'Like this post' short_description: 'Like this post' long_form: 'liked this' - vote: - title: 'Vote' - description: 'Vote for this post' - short_description: 'Vote for this post' - long_form: 'voted for this post' user_activity: no_default: diff --git a/db/fixtures/000_delayed_drops.rb b/db/fixtures/000_delayed_drops.rb index a6a5eebde36..b5d2ab446ea 100644 --- a/db/fixtures/000_delayed_drops.rb +++ b/db/fixtures/000_delayed_drops.rb @@ -54,34 +54,70 @@ Migration::ColumnDropper.drop( Migration::ColumnDropper.drop( table: 'topics', - after_migration: 'DropUnreadTrackingColumns', - columns: %w{ - inappropriate_count - bookmark_count - off_topic_count - illegal_count - notify_user_count - last_unread_at - }, - on_drop: ->() { - STDERR.puts "Removing superflous topic columns!" - } -) - -Migration::ColumnDropper.drop( - table: 'topics', - after_migration: 'RemoveAutoCloseColumnsFromTopics', + after_migration: 'DropVoteCountFromTopicsAndPosts', columns: %w{ auto_close_at auto_close_user_id auto_close_started_at auto_close_based_on_last_post auto_close_hours + inappropriate_count + bookmark_count + off_topic_count + illegal_count + notify_user_count + last_unread_at + vote_count }, on_drop: ->() { STDERR.puts "Removing superflous topic columns!" + } +) + +VIEW_NAME = "badge_posts".freeze + +def badge_posts_view_exists? + sql = <<~SQL + SELECT 1 + FROM pg_catalog.pg_views + WHERE schemaname + IN ('public') + AND viewname = '#{VIEW_NAME}'; + SQL + + DB.exec(sql) == 1 +end + +Migration::ColumnDropper.drop( + table: 'posts', + after_migration: 'DropVoteCountFromTopicsAndPosts', + columns: %w{ + vote_count }, - delay: 3600 + on_drop: ->() { + STDERR.puts "Removing superflous post columns!" + + DB.exec("DROP VIEW #{VIEW_NAME}") + raise "Failed to drop '#{VIEW_NAME}' view" if badge_posts_view_exists? + }, + after_drop: -> () { + sql = <<~SQL + CREATE VIEW #{VIEW_NAME} AS + SELECT p.* + FROM posts p + JOIN topics t ON t.id = p.topic_id + JOIN categories c ON c.id = t.category_id + WHERE c.allow_badges AND + p.deleted_at IS NULL AND + t.deleted_at IS NULL AND + NOT c.read_restricted AND + t.visible AND + p.post_type IN (1,2,3) + SQL + + DB.exec(sql) + raise "Failed to create '#{VIEW_NAME}' view" unless badge_posts_view_exists? + } ) Migration::ColumnDropper.drop( diff --git a/db/fixtures/003_post_action_types.rb b/db/fixtures/003_post_action_types.rb index a9cafe763f0..f0094764fae 100644 --- a/db/fixtures/003_post_action_types.rb +++ b/db/fixtures/003_post_action_types.rb @@ -27,13 +27,6 @@ PostActionType.seed do |s| s.position = 4 end -PostActionType.seed do |s| - s.id = PostActionType.types[:vote] - s.name_key = 'vote' - s.is_flag = false - s.position = 5 -end - PostActionType.seed do |s| s.id = PostActionType.types[:spam] s.name_key = 'spam' diff --git a/db/migrate/20120928170023_add_sort_order_to_posts.rb b/db/migrate/20120928170023_add_sort_order_to_posts.rb index 6a0bf192ad2..3965dc4a3a5 100644 --- a/db/migrate/20120928170023_add_sort_order_to_posts.rb +++ b/db/migrate/20120928170023_add_sort_order_to_posts.rb @@ -3,11 +3,5 @@ class AddSortOrderToPosts < ActiveRecord::Migration[4.2] add_column :posts, :sort_order, :integer remove_index :posts, :user_id execute "UPDATE posts AS p SET sort_order = post_number FROM forum_threads AS ft WHERE ft.id = p.forum_thread_id AND ft.archetype_id = 1" - execute "UPDATE posts AS p SET sort_order = - CASE WHEN post_number = 1 THEN 1 - ELSE 2147483647 - p.vote_count - END - FROM forum_threads AS ft - WHERE ft.id = p.forum_thread_id AND ft.archetype_id = 2" end end diff --git a/db/migrate/20180619091608_drop_vote_count_from_topics_and_posts.rb b/db/migrate/20180619091608_drop_vote_count_from_topics_and_posts.rb new file mode 100644 index 00000000000..1e061449d62 --- /dev/null +++ b/db/migrate/20180619091608_drop_vote_count_from_topics_and_posts.rb @@ -0,0 +1,9 @@ +class DropVoteCountFromTopicsAndPosts < ActiveRecord::Migration[5.2] + def up + # Delayed drop + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/db/migrate/20180619092320_clean_up_vote_post_action.rb b/db/migrate/20180619092320_clean_up_vote_post_action.rb new file mode 100644 index 00000000000..21d66722b24 --- /dev/null +++ b/db/migrate/20180619092320_clean_up_vote_post_action.rb @@ -0,0 +1,10 @@ +class CleanUpVotePostAction < ActiveRecord::Migration[5.2] + def up + execute "DELETE FROM post_actions WHERE post_action_type_id = 5" + execute "DELETE FROM post_action_types WHERE id = 5" + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/lib/migration/base_dropper.rb b/lib/migration/base_dropper.rb index d8e2f03d932..39fa11ac095 100644 --- a/lib/migration/base_dropper.rb +++ b/lib/migration/base_dropper.rb @@ -1,8 +1,9 @@ module Migration class BaseDropper - def initialize(after_migration, delay, on_drop) + def initialize(after_migration, delay, on_drop, after_drop) @after_migration = after_migration @on_drop = on_drop + @after_drop = after_drop # in production we need some extra delay to allow for slow migrations @delay = delay || (Rails.env.production? ? 3600 : 0) @@ -12,6 +13,7 @@ module Migration if droppable? @on_drop&.call execute_drop! + @after_drop&.call Discourse.reset_active_record_cache end diff --git a/lib/migration/column_dropper.rb b/lib/migration/column_dropper.rb index 5d9e508e808..10e7b37b1c4 100644 --- a/lib/migration/column_dropper.rb +++ b/lib/migration/column_dropper.rb @@ -2,11 +2,13 @@ require_dependency 'migration/base_dropper' module Migration class ColumnDropper < BaseDropper - def self.drop(table:, after_migration:, columns:, delay: nil, on_drop: nil) + def self.drop(table:, after_migration:, columns:, delay: nil, on_drop: nil, after_drop: nil) validate_table_name(table) columns.each { |column| validate_column_name(column) } - ColumnDropper.new(table, columns, after_migration, delay, on_drop).delayed_drop + ColumnDropper.new( + table, columns, after_migration, delay, on_drop, after_drop + ).delayed_drop end def self.mark_readonly(table_name, column_name) @@ -24,8 +26,8 @@ module Migration private - def initialize(table, columns, after_migration, delay, on_drop) - super(after_migration, delay, on_drop) + def initialize(table, columns, after_migration, delay, on_drop, after_drop) + super(after_migration, delay, on_drop, after_drop) @table = table @columns = columns diff --git a/lib/migration/table_dropper.rb b/lib/migration/table_dropper.rb index c95640b78dc..0e1a938983a 100644 --- a/lib/migration/table_dropper.rb +++ b/lib/migration/table_dropper.rb @@ -2,17 +2,21 @@ require_dependency 'migration/base_dropper' module Migration class Migration::TableDropper < BaseDropper - def self.delayed_drop(table_name:, after_migration:, delay: nil, on_drop: nil) + def self.delayed_drop(table_name:, after_migration:, delay: nil, on_drop: nil, after_drop: nil) validate_table_name(table_name) - TableDropper.new(table_name, nil, after_migration, delay, on_drop).delayed_drop + TableDropper.new( + table_name, nil, after_migration, delay, on_drop, after_drop + ).delayed_drop end - def self.delayed_rename(old_name:, new_name:, after_migration:, delay: nil, on_drop: nil) + def self.delayed_rename(old_name:, new_name:, after_migration:, delay: nil, on_drop: nil, after_drop: nil) validate_table_name(old_name) validate_table_name(new_name) - TableDropper.new(old_name, new_name, after_migration, delay, on_drop).delayed_drop + TableDropper.new( + old_name, new_name, after_migration, delay, on_drop, after_drop + ).delayed_drop end def self.read_only_table(table_name) @@ -29,8 +33,8 @@ module Migration private - def initialize(old_name, new_name, after_migration, delay, on_drop) - super(after_migration, delay, on_drop) + def initialize(old_name, new_name, after_migration, delay, on_drop, after_drop) + super(after_migration, delay, on_drop, after_drop) @old_name = old_name @new_name = new_name diff --git a/spec/components/guardian_spec.rb b/spec/components/guardian_spec.rb index fc7d7d09a74..9b95f66e329 100644 --- a/spec/components/guardian_spec.rb +++ b/spec/components/guardian_spec.rb @@ -407,7 +407,6 @@ describe Guardian do expect(guardian.can_see_post_actors?(topic, PostActionType.types[:bookmark])).to be_falsey expect(guardian.can_see_post_actors?(topic, PostActionType.types[:off_topic])).to be_falsey expect(guardian.can_see_post_actors?(topic, PostActionType.types[:spam])).to be_falsey - expect(guardian.can_see_post_actors?(topic, PostActionType.types[:vote])).to be_truthy expect(guardian.can_see_post_actors?(topic, PostActionType.types[:notify_user])).to be_falsey expect(Guardian.new(moderator).can_see_post_actors?(topic, PostActionType.types[:notify_user])).to be_truthy @@ -1037,18 +1036,6 @@ describe Guardian do expect(guardian.post_can_act?(post, :vote)).to be_truthy end - it "doesn't allow voting if the user has an action from voting already" do - expect(guardian.post_can_act?(post, :vote, opts: { - taken_actions: { PostActionType.types[:vote] => 1 } - })).to be_falsey - end - - it "allows voting if the user has performed a different action" do - expect(guardian.post_can_act?(post, :vote, opts: { - taken_actions: { PostActionType.types[:like] => 1 } - })).to be_truthy - end - it "isn't allowed on archived topics" do topic.archived = true expect(Guardian.new(user).post_can_act?(post, :like)).to be_falsey diff --git a/spec/components/migration/column_dropper_spec.rb b/spec/components/migration/column_dropper_spec.rb index b83aa32fc75..6dad4a4f7a6 100644 --- a/spec/components/migration/column_dropper_spec.rb +++ b/spec/components/migration/column_dropper_spec.rb @@ -40,6 +40,7 @@ RSpec.describe Migration::ColumnDropper do it "can correctly drop columns after correct delay" do dropped_proc_called = false + after_dropped_proc_called = false update_first_migration_date(2.years.ago) Migration::ColumnDropper.drop( @@ -47,35 +48,42 @@ RSpec.describe Migration::ColumnDropper do after_migration: migration_name, columns: ['junk'], delay: 20.minutes, - on_drop: ->() { dropped_proc_called = true } + on_drop: ->() { dropped_proc_called = true }, + after_drop: ->() { after_dropped_proc_called = true } ) expect(has_column?('topics', 'junk')).to eq(true) expect(dropped_proc_called).to eq(false) + expect(dropped_proc_called).to eq(false) Migration::ColumnDropper.drop( table: 'topics', after_migration: migration_name, columns: ['junk'], delay: 10.minutes, - on_drop: ->() { dropped_proc_called = true } + on_drop: ->() { dropped_proc_called = true }, + after_drop: ->() { after_dropped_proc_called = true } ) expect(has_column?('topics', 'junk')).to eq(false) expect(dropped_proc_called).to eq(true) + expect(after_dropped_proc_called).to eq(true) dropped_proc_called = false + after_dropped_proc_called = false Migration::ColumnDropper.drop( table: 'topics', after_migration: migration_name, columns: ['junk'], delay: 10.minutes, - on_drop: ->() { dropped_proc_called = true } + on_drop: ->() { dropped_proc_called = true }, + after_drop: ->() { after_dropped_proc_called = true } ) # it should call "on_drop" only when there are columns to drop expect(dropped_proc_called).to eq(false) + expect(after_dropped_proc_called).to eq(false) end it "drops the columns immediately if the first migration was less than 10 minutes ago" do diff --git a/spec/serializers/post_serializer_spec.rb b/spec/serializers/post_serializer_spec.rb index 40d2574d708..cabf0143608 100644 --- a/spec/serializers/post_serializer_spec.rb +++ b/spec/serializers/post_serializer_spec.rb @@ -29,10 +29,10 @@ describe PostSerializer do end it "displays the correct info" do - expect(visible_actions_for(actor).sort).to eq([:like, :notify_user, :spam, :vote]) - expect(visible_actions_for(post.user).sort).to eq([:like, :vote]) - expect(visible_actions_for(nil).sort).to eq([:like, :vote]) - expect(visible_actions_for(admin).sort).to eq([:like, :notify_user, :spam, :vote]) + expect(visible_actions_for(actor).sort).to eq([:like, :notify_user, :spam]) + expect(visible_actions_for(post.user).sort).to eq([:like]) + expect(visible_actions_for(nil).sort).to eq([:like]) + expect(visible_actions_for(admin).sort).to eq([:like, :notify_user, :spam]) end it "can't flag your own post to notify yourself" do