From 0a0a85ee70a77f95250a6de92585ed8f69ee1298 Mon Sep 17 00:00:00 2001 From: evazion Date: Mon, 3 Aug 2020 14:40:04 -0500 Subject: [PATCH] Fix #4568: Send appealed posts back to the mod queue * Include appealed posts in the modqueue. * Add `status` field to appeals. Appeals start out as `pending`, then become `rejected` if the post isn't approved within three days. If the post is approved, the appeal's status becomes `succeeded`. * Add `status` field to flags. Flags start out as `pending` then become `rejected` if the post is approved within three days. If the post isn't approved, the flag's status becomes `succeeded`. * Leave behind a "Unapproved in three days" dummy flag when an appeal goes unapproved, just like when a pending post is unapproved. * Only allow deleted posts to be appealed. Don't allow flagged posts to be appealed. * Add `status:appealed` metatag. `status:appealed` is separate from `status:pending`. * Include appealed posts in `status:modqueue`. Search `status:modqueue order:modqueue` to view the modqueue as a normal search. * Retroactively set old flags and appeals as succeeded or rejected. This may not be correct for posts that were appealed or flagged multiple times. This is difficult to set correctly because we don't have approval records for old posts, so we can't tell the actual outcome of old flags and appeals. * Deprecate the `is_resolved` field on post flags. A resolved flag is a flag that isn't pending. * Known bug: appealed posts have a black border instead of a blue border. Checking whether a post has been appealed would require either an extra query on the posts/index page, or an is_appealed flag on posts, neither of which are very desirable. * Known bug: you can't use `status:appealed` in blacklists, for the same reason as above. --- app/controllers/modqueue_controller.rb | 12 +- app/logical/post_pruner.rb | 17 ++- app/logical/post_query_builder.rb | 14 ++- app/logical/post_sets/post.rb | 2 +- app/models/application_record.rb | 4 + app/models/post.rb | 36 +++++- app/models/post_appeal.rb | 23 ++-- app/models/post_approval.rb | 6 +- app/models/post_disapproval.rb | 2 +- app/models/post_flag.rb | 16 +-- app/presenters/post_presenter.rb | 2 +- app/views/modqueue/_post.html.erb | 2 +- .../posts/partials/show/_information.html.erb | 18 ++- .../posts/partials/show/_notices.html.erb | 14 ++- .../posts/partials/show/_options.html.erb | 23 ++-- ...59_add_status_to_post_appeals_and_flags.rb | 9 ++ db/structure.sql | 23 +++- ...066_initialize_appeal_and_flag_statuses.rb | 26 ++++ test/factories/post_flag.rb | 2 +- test/functional/modqueue_controller_test.rb | 8 ++ .../post_appeals_controller_test.rb | 52 +++++++- .../post_approvals_controller_test.rb | 23 ++++ .../functional/post_events_controller_test.rb | 1 + test/functional/posts_controller_test.rb | 3 +- test/unit/post_appeal_test.rb | 2 +- test/unit/post_event_test.rb | 1 + test/unit/post_flag_test.rb | 2 +- test/unit/post_pruner_test.rb | 118 +++++++++++++++--- test/unit/post_query_builder_test.rb | 18 ++- test/unit/post_test.rb | 9 +- test/unit/upload_limit_test.rb | 2 +- 31 files changed, 372 insertions(+), 118 deletions(-) create mode 100644 db/migrate/20200803022359_add_status_to_post_appeals_and_flags.rb create mode 100755 script/fixes/066_initialize_appeal_and_flag_statuses.rb diff --git a/app/controllers/modqueue_controller.rb b/app/controllers/modqueue_controller.rb index 773b2cae7..44b63fb73 100644 --- a/app/controllers/modqueue_controller.rb +++ b/app/controllers/modqueue_controller.rb @@ -4,14 +4,14 @@ class ModqueueController < ApplicationController def index authorize :modqueue - @posts = Post.includes(:appeals, :disapprovals, :uploader, flags: [:creator]).pending_or_flagged.available_for_moderation(CurrentUser.user, hidden: search_params[:hidden]) + @posts = Post.includes(:appeals, :disapprovals, :uploader, flags: [:creator]).in_modqueue.available_for_moderation(CurrentUser.user, hidden: search_params[:hidden]) @posts = @posts.paginated_search(params, order: "modqueue", count_pages: true) - @modqueue_posts = @posts.except(:offset, :limit, :order) - @pending_post_count = @modqueue_posts.pending.count - @flagged_post_count = @modqueue_posts.flagged.count - @disapproval_reasons = PostDisapproval.where(post: @modqueue_posts).where.not(reason: "disinterest").group(:reason).order(count: :desc).distinct.count(:post_id) - @uploaders = @modqueue_posts.group(:uploader).order(count: :desc).limit(20).count + @modqueue_posts = @posts.reselect(nil).reorder(nil).offset(nil).limit(nil) + @pending_post_count = @modqueue_posts.select(&:is_pending?).count + @flagged_post_count = @modqueue_posts.select(&:is_flagged?).count + @disapproval_reasons = PostDisapproval.where(post: @modqueue_posts.reselect(:id)).where.not(reason: "disinterest").group(:reason).order(count: :desc).distinct.count(:post_id) + @uploaders = @modqueue_posts.map(&:uploader).tally.sort_by(&:last).reverse.take(20).to_h @tags = RelatedTagCalculator.frequent_tags_for_post_relation(@modqueue_posts) @artist_tags = @tags.select(&:artist?).sort_by(&:overlap_count).reverse.take(10) diff --git a/app/logical/post_pruner.rb b/app/logical/post_pruner.rb index 02eef81de..924fc7152 100644 --- a/app/logical/post_pruner.rb +++ b/app/logical/post_pruner.rb @@ -1,7 +1,10 @@ -class PostPruner +module PostPruner + module_function + def prune! prune_pending! prune_flagged! + prune_appealed! end def prune_pending! @@ -11,10 +14,14 @@ class PostPruner end def prune_flagged! - Post.flagged.each do |post| - if post.flags.unresolved.old.any? - post.delete!("Unapproved in three days after returning to moderation queue", user: User.system) - end + PostFlag.expired.each do |flag| + flag.post.delete!("Unapproved in three days after returning to moderation queue", user: User.system) + end + end + + def prune_appealed! + PostAppeal.expired.each do |appeal| + appeal.post.delete!("Unapproved in three days after returning to moderation queue", user: User.system) end end end diff --git a/app/logical/post_query_builder.rb b/app/logical/post_query_builder.rb index 0829ce3ba..9c6a03ba6 100644 --- a/app/logical/post_query_builder.rb +++ b/app/logical/post_query_builder.rb @@ -274,8 +274,10 @@ class PostQueryBuilder Post.pending when "flagged" Post.flagged + when "appealed" + Post.appealed when "modqueue" - Post.pending_or_flagged + Post.in_modqueue when "deleted" Post.deleted when "banned" @@ -283,7 +285,7 @@ class PostQueryBuilder when "active" Post.active when "unmoderated" - Post.pending_or_flagged.available_for_moderation(current_user, hidden: false) + Post.in_modqueue.available_for_moderation(current_user, hidden: false) when "all", "any" Post.all else @@ -307,7 +309,7 @@ class PostQueryBuilder Post.where(parent: nil) when "any" Post.where.not(parent: nil) - when /pending|flagged|modqueue|deleted|banned|active|unmoderated/ + when "pending", "flagged", "appealed", "modqueue", "deleted", "banned", "active", "unmoderated" Post.where.not(parent: nil).where(parent: status_matches(parent)) when /\A\d+\z/ Post.where(id: parent).or(Post.where(parent: parent)) @@ -322,7 +324,7 @@ class PostQueryBuilder Post.where(has_children: false) when "any" Post.where(has_children: true) - when /pending|flagged|modqueue|deleted|banned|active|unmoderated/ + when "pending", "flagged", "appealed", "modqueue", "deleted", "banned", "active", "unmoderated" Post.where(has_children: true).where(children: status_matches(child)) else Post.none @@ -606,10 +608,10 @@ class PostQueryBuilder .order("contributor_fav_count DESC, posts.fav_count DESC, posts.id DESC") when "modqueue", "modqueue_desc" - relation = relation.left_outer_joins(:flags).order(Arel.sql("GREATEST(posts.created_at, post_flags.created_at) DESC, posts.id DESC")) + relation = relation.with_queued_at.order("queued_at DESC, posts.id DESC") when "modqueue_asc" - relation = relation.left_outer_joins(:flags).order(Arel.sql("GREATEST(posts.created_at, post_flags.created_at) ASC, posts.id ASC")) + relation = relation.with_queued_at.order("queued_at ASC, posts.id ASC") when "none" relation = relation.reorder(nil) diff --git a/app/logical/post_sets/post.rb b/app/logical/post_sets/post.rb index 80052c527..7d5d2a47f 100644 --- a/app/logical/post_sets/post.rb +++ b/app/logical/post_sets/post.rb @@ -149,7 +149,7 @@ module PostSets end def not_shown(post) - post.is_deleted? && tag_string !~ /status:(?:all|any|deleted|banned)/ + post.is_deleted? && tag_string !~ /status:(?:all|any|deleted|banned|modqueue)/ end def none_shown diff --git a/app/models/application_record.rb b/app/models/application_record.rb index ccd96df91..bd366d214 100644 --- a/app/models/application_record.rb +++ b/app/models/application_record.rb @@ -111,6 +111,10 @@ class ApplicationRecord < ActiveRecord::Base ensure connection.execute("SET STATEMENT_TIMEOUT = #{CurrentUser.user.try(:statement_timeout) || 3_000}") unless Rails.env == "test" end + + def update!(*args) + all.each { |record| record.update!(*args) } + end end end diff --git a/app/models/post.rb b/app/models/post.rb index 5e307b5ee..a826a71ff 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -61,8 +61,9 @@ class Post < ApplicationRecord scope :pending, -> { where(is_pending: true) } scope :flagged, -> { where(is_flagged: true) } scope :banned, -> { where(is_banned: true) } - scope :active, -> { where(is_pending: false, is_deleted: false, is_flagged: false) } - scope :pending_or_flagged, -> { pending.or(flagged) } + scope :active, -> { where(is_pending: false, is_deleted: false, is_flagged: false).where.not(id: PostAppeal.pending) } + scope :appealed, -> { where(id: PostAppeal.pending.select(:post_id)) } + scope :in_modqueue, -> { pending.or(flagged).or(appealed) } scope :expired, -> { where("posts.created_at < ?", 3.days.ago) } scope :unflagged, -> { where(is_flagged: false) } @@ -281,8 +282,24 @@ class Post < ApplicationRecord end module ApprovalMethods + def in_modqueue? + is_pending? || is_flagged? || is_appealed? + end + + def is_active? + !is_deleted? && !in_modqueue? + end + + def is_appealed? + is_deleted? && appeals.any?(&:pending?) + end + + def is_appealable? + is_deleted? && !is_appealed? + end + def is_approvable?(user = CurrentUser.user) - !is_status_locked? && (is_pending? || is_flagged? || is_deleted?) && uploader != user + !is_status_locked? && !is_active? && uploader != user end def flag!(reason, is_deletion: false) @@ -991,7 +1008,10 @@ class Post < ApplicationRecord transaction do automated = (user == User.system) - flags.create!(reason: reason, is_deletion: true, creator: user) + flags.pending.update!(status: :succeeded) + appeals.pending.update!(status: :rejected) + + flags.create!(reason: reason, is_deletion: true, creator: user, status: :succeeded) update!(is_deleted: true, is_pending: false, is_flagged: false) # XXX This must happen *after* the `is_deleted` flag is set to true (issue #3419). @@ -1224,6 +1244,14 @@ class Post < ApplicationRecord relation end + def with_queued_at + relation = group(:id) + relation = relation.left_outer_joins(:flags, :appeals) + relation = relation.select("posts.*") + relation = relation.select(Arel.sql("MAX(GREATEST(posts.created_at, post_flags.created_at, post_appeals.created_at)) AS queued_at")) + relation + end + def with_stats(tables) return all if tables.empty? diff --git a/app/models/post_appeal.rb b/app/models/post_appeal.rb index d758f945f..925fca5ae 100644 --- a/app/models/post_appeal.rb +++ b/app/models/post_appeal.rb @@ -5,20 +5,27 @@ class PostAppeal < ApplicationRecord belongs_to :creator, :class_name => "User" belongs_to :post - validates_presence_of :reason + validates :reason, presence: true, length: { in: 1..140 } - validate :validate_post_is_inactive - validate :validate_creator_is_not_limited - validates_uniqueness_of :creator_id, :scope => :post_id, :message => "have already appealed this post" + validate :validate_post_is_appealable, on: :create + validate :validate_creator_is_not_limited, on: :create + validates :creator, uniqueness: { scope: :post, message: "have already appealed this post" }, on: :create + + enum status: { + pending: 0, + succeeded: 1, + rejected: 2 + } scope :resolved, -> { where(post: Post.undeleted.unflagged) } scope :unresolved, -> { where(post: Post.deleted.or(Post.flagged)) } scope :recent, -> { where("post_appeals.created_at >= ?", 1.day.ago) } + scope :expired, -> { pending.where("post_appeals.created_at <= ?", 3.days.ago) } module SearchMethods def search(params) q = super - q = q.search_attributes(params, :creator, :post, :reason) + q = q.search_attributes(params, :creator, :post, :reason, :status) q = q.text_attribute_matches(:reason, params[:reason_matches]) q = q.resolved if params[:is_resolved].to_s.truthy? @@ -44,10 +51,8 @@ class PostAppeal < ApplicationRecord end end - def validate_post_is_inactive - if resolved? - errors[:post] << "is active" - end + def validate_post_is_appealable + errors[:post] << "cannot be appealed" if post.is_status_locked? || !post.is_appealable? end def appeal_count_for_creator diff --git a/app/models/post_approval.rb b/app/models/post_approval.rb index 2981143d2..b2efa6437 100644 --- a/app/models/post_approval.rb +++ b/app/models/post_approval.rb @@ -12,7 +12,7 @@ class PostApproval < ApplicationRecord errors.add(:post, "is locked and cannot be approved") end - if post.status == "active" + if post.is_active? errors.add(:post, "is already active and cannot be approved") end @@ -28,7 +28,9 @@ class PostApproval < ApplicationRecord def approve_post is_undeletion = post.is_deleted - post.flags.each(&:resolve!) + post.flags.pending.update!(status: :rejected) + post.appeals.pending.update!(status: :succeeded) + post.update(approver: user, is_flagged: false, is_pending: false, is_deleted: false) ModAction.log("undeleted post ##{post_id}", :post_undelete) if is_undeletion diff --git a/app/models/post_disapproval.rb b/app/models/post_disapproval.rb index bf00344fe..fc1076d6b 100644 --- a/app/models/post_disapproval.rb +++ b/app/models/post_disapproval.rb @@ -46,7 +46,7 @@ class PostDisapproval < ApplicationRecord end def validate_disapproval - if post.status == "active" + if post.is_active? errors[:post] << "is already active and cannot be disapproved" end end diff --git a/app/models/post_flag.rb b/app/models/post_flag.rb index 1a2febf0b..4f8542723 100644 --- a/app/models/post_flag.rb +++ b/app/models/post_flag.rb @@ -17,13 +17,19 @@ class PostFlag < ApplicationRecord before_save :update_post attr_accessor :is_deletion + enum status: { + pending: 0, + succeeded: 1, + rejected: 2 + } + scope :by_users, -> { where.not(creator: User.system) } scope :by_system, -> { where(creator: User.system) } scope :in_cooldown, -> { by_users.where("created_at >= ?", COOLDOWN_PERIOD.ago) } scope :resolved, -> { where(is_resolved: true) } scope :unresolved, -> { where(is_resolved: false) } scope :recent, -> { where("post_flags.created_at >= ?", 1.day.ago) } - scope :old, -> { where("post_flags.created_at <= ?", 3.days.ago) } + scope :expired, -> { pending.where("post_flags.created_at <= ?", 3.days.ago) } module SearchMethods def creator_matches(creator, searcher) @@ -56,7 +62,7 @@ class PostFlag < ApplicationRecord def search(params) q = super - q = q.search_attributes(params, :post, :is_resolved, :reason) + q = q.search_attributes(params, :post, :is_resolved, :reason, :status) q = q.text_attribute_matches(:reason, params[:reason_matches]) if params[:creator_id].present? @@ -113,12 +119,8 @@ class PostFlag < ApplicationRecord def validate_post errors[:post] << "is pending and cannot be flagged" if post.is_pending? && !is_deletion + errors[:post] << "is deleted and cannot be flagged" if post.is_deleted? && !is_deletion errors[:post] << "is locked and cannot be flagged" if post.is_status_locked? - errors[:post] << "is deleted" if post.is_deleted? - end - - def resolve! - update_column(:is_resolved, true) end def flag_count_for_creator diff --git a/app/presenters/post_presenter.rb b/app/presenters/post_presenter.rb index aed0843b5..2f956e469 100644 --- a/app/presenters/post_presenter.rb +++ b/app/presenters/post_presenter.rb @@ -7,7 +7,7 @@ class PostPresenter return "none".html_safe end - if !options[:show_deleted] && post.is_deleted? && options[:tags] !~ /status:(?:all|any|deleted|banned)/ + if !options[:show_deleted] && post.is_deleted? && options[:tags] !~ /status:(?:all|any|deleted|banned|modqueue)/ return "" end diff --git a/app/views/modqueue/_post.html.erb b/app/views/modqueue/_post.html.erb index dfa6d9865..ce523b029 100644 --- a/app/views/modqueue/_post.html.erb +++ b/app/views/modqueue/_post.html.erb @@ -1,6 +1,6 @@ <%= content_tag(:div, { id: "post-#{post.id}", class: ["post", "mod-queue-preview", "column-container", *PostPresenter.preview_class(post)].join(" ") }.merge(PostPresenter.data_attributes(post))) do %>
diff --git a/app/views/posts/partials/show/_information.html.erb b/app/views/posts/partials/show/_information.html.erb index 4e6dae192..dcfc2b11c 100644 --- a/app/views/posts/partials/show/_information.html.erb +++ b/app/views/posts/partials/show/_information.html.erb @@ -43,22 +43,18 @@ Status: <% if post.is_pending? %> Pending - <% end %> - - <% if post.is_deleted? %> - Deleted - <% end %> - - <% if post.is_flagged? %> + <% elsif post.is_flagged? %> Flagged + <% elsif post.is_appealed? %> + Appealed + <% elsif post.is_deleted? %> + Deleted + <% else %> + Active <% end %> <% if post.is_banned? %> Banned <% end %> - - <% if !post.is_pending? && !post.is_deleted? && !post.is_banned? %> - Active - <% end %> diff --git a/app/views/posts/partials/show/_notices.html.erb b/app/views/posts/partials/show/_notices.html.erb index d3119ea8f..256d33109 100644 --- a/app/views/posts/partials/show/_notices.html.erb +++ b/app/views/posts/partials/show/_notices.html.erb @@ -1,4 +1,4 @@ -<% if post.is_flagged? && !post.is_deleted? && post.flags.any? %> +<% if post.is_flagged? %>

This post was flagged for review (<%= link_to_wiki "learn more", "howto:flag" %>):

@@ -30,13 +30,14 @@
<% end %> -<% if post.is_pending? || post.is_flagged? %> +<% if post.in_modqueue? %>
<% if post.is_pending? %> - This post is pending approval. - (<%= link_to_wiki "learn more", "about:mod_queue" %>) - <% else %> + This post is pending approval. (<%= link_to_wiki "learn more", "about:mod_queue" %>) + <% elsif post.is_flagged? %> This post was flagged and is pending approval (<%= link_to_wiki "learn more", "about:mod_queue" %>) + <% elsif post.is_appealed? %> + This post was appealed and is pending approval (<%= link_to_wiki "learn more", "about:mod_queue" %>) <% end %> <%= render "post_disapprovals/counts", :disapprovals => post.disapprovals, :post => post %> @@ -48,7 +49,8 @@
<% end %> -<% if (post.is_flagged? || post.is_deleted?) && post.appeals.any? %> +<% #XXX %> +<% if post.is_deleted? && post.appeals.any? %>

This post was appealed:

<%= render "post_appeals/reasons", appeals: post.appeals %> diff --git a/app/views/posts/partials/show/_options.html.erb b/app/views/posts/partials/show/_options.html.erb index e55116e2f..d071f7bb6 100644 --- a/app/views/posts/partials/show/_options.html.erb +++ b/app/views/posts/partials/show/_options.html.erb @@ -51,25 +51,24 @@ <% if post.is_status_locked? %>
  • Status locked
    • <% else %> - <% if (!post.is_deleted? && !post.is_pending? && !post.is_flagged?) && policy(PostFlag).create? %> + <% if post.is_active? && policy(PostFlag).create? %>
  • <%= link_to "Flag", new_post_flag_path(post_flag: { post_id: post.id }), remote: true %>
    • - <% elsif (post.is_flagged? || post.is_deleted?) && policy(PostAppeal).create? %> + <% elsif post.is_appealable? && policy(PostAppeal).create? %>
  • <%= link_to "Appeal", new_post_appeal_path(post_appeal: { post_id: post.id }), remote: true %>
    • <% end %> <% if policy(PostApproval).create? %> - <% if post.is_deleted? %> -
  • <%= link_to "Undelete", post_approvals_path(post_id: post.id), remote: true, method: :post, "data-confirm": "Are you sure you want to undelete this post?" %>
    • - <% if policy(post).move_favorites? %> -
  • <%= link_to "Move favorites", confirm_move_favorites_moderator_post_post_path(post_id: post.id) %>
    • - <% end %> - <% elsif policy(post).delete? %> -
  • <%= link_to "Delete", post, method: :delete, remote: true %>
    • + <% if post.is_approvable? %> +
  • <%= link_to (post.is_deleted? ? "Undelete" : "Approve"), post_approvals_path(post_id: post.id), remote: true, method: :post, "data-shortcut": "shift+o", "data-confirm": "Are you sure you want to approve this post?" %>
    • +
  • <%= link_to "Hide from queue", post_disapprovals_path(post_disapproval: { post_id: post.id, reason: "disinterest" }), remote: true, method: :post %>
    • <% end %> - <% if post.is_approvable? && !post.is_deleted? %> -
  • <%= link_to "Approve", post_approvals_path(post_id: post.id), remote: true, method: :post, id: "approve", "data-shortcut": "shift+o", "data-confirm": "Are you sure you want to approve this post?" %>
    • -
  • <%= link_to "Hide from queue", post_disapprovals_path(post_disapproval: { post_id: post.id, reason: "disinterest" }), remote: true, method: :post, id: "disapprove" %>
    • + <% if post.is_deleted? && policy(post).move_favorites? %> +
  • <%= link_to "Move favorites", confirm_move_favorites_moderator_post_post_path(post_id: post.id) %>
    • + <% end %> + + <% if !post.is_deleted? && policy(post).delete? %> +
  • <%= link_to "Delete", post, method: :delete, remote: true %>
    • <% end %> <% if policy(post).unban? %> diff --git a/db/migrate/20200803022359_add_status_to_post_appeals_and_flags.rb b/db/migrate/20200803022359_add_status_to_post_appeals_and_flags.rb new file mode 100644 index 000000000..fd2420b67 --- /dev/null +++ b/db/migrate/20200803022359_add_status_to_post_appeals_and_flags.rb @@ -0,0 +1,9 @@ +class AddStatusToPostAppealsAndFlags < ActiveRecord::Migration[6.0] + def change + add_column :post_appeals, :status, :integer, default: 0, null: false + add_index :post_appeals, :status + + add_column :post_flags, :status, :integer, default: 0, null: false + add_index :post_flags, :status + end +end diff --git a/db/structure.sql b/db/structure.sql index c8e7ff934..e600fba4c 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -2584,7 +2584,8 @@ CREATE TABLE public.post_appeals ( creator_id integer NOT NULL, reason text, created_at timestamp without time zone NOT NULL, - updated_at timestamp without time zone NOT NULL + updated_at timestamp without time zone NOT NULL, + status integer DEFAULT 0 NOT NULL ); @@ -2684,7 +2685,8 @@ CREATE TABLE public.post_flags ( reason text, is_resolved boolean DEFAULT false NOT NULL, created_at timestamp without time zone NOT NULL, - updated_at timestamp without time zone NOT NULL + updated_at timestamp without time zone NOT NULL, + status integer DEFAULT 0 NOT NULL ); @@ -6622,6 +6624,13 @@ CREATE INDEX index_post_appeals_on_post_id ON public.post_appeals USING btree (p CREATE INDEX index_post_appeals_on_reason_tsvector ON public.post_appeals USING gin (to_tsvector('english'::regconfig, reason)); +-- +-- Name: index_post_appeals_on_status; Type: INDEX; Schema: public; Owner: - +-- + +CREATE INDEX index_post_appeals_on_status ON public.post_appeals USING btree (status); + + -- -- Name: index_post_approvals_on_post_id; Type: INDEX; Schema: public; Owner: - -- @@ -6671,6 +6680,13 @@ CREATE INDEX index_post_flags_on_post_id ON public.post_flags USING btree (post_ CREATE INDEX index_post_flags_on_reason_tsvector ON public.post_flags USING gin (to_tsvector('english'::regconfig, reason)); +-- +-- Name: index_post_flags_on_status; Type: INDEX; Schema: public; Owner: - +-- + +CREATE INDEX index_post_flags_on_status ON public.post_flags USING btree (status); + + -- -- Name: index_post_replacements_on_creator_id; Type: INDEX; Schema: public; Owner: - -- @@ -7393,6 +7409,7 @@ INSERT INTO "schema_migrations" (version) VALUES ('20200325074859'), ('20200403210353'), ('20200406054838'), -('20200427190519'); +('20200427190519'), +('20200803022359'); diff --git a/script/fixes/066_initialize_appeal_and_flag_statuses.rb b/script/fixes/066_initialize_appeal_and_flag_statuses.rb new file mode 100755 index 000000000..c48e7079a --- /dev/null +++ b/script/fixes/066_initialize_appeal_and_flag_statuses.rb @@ -0,0 +1,26 @@ +#!/usr/bin/env ruby + +require_relative "../../config/environment" + +PostFlag.transaction do + # Mark all old flags and appeals as succeeded or rejected. Recent flags and + # appeals are left as pending. This is not strictly correct for posts that + # may have been flagged or appealed multiple times. + PostAppeal.expired.where(post: Post.undeleted).update_all(status: "succeeded") + PostAppeal.expired.where(post: Post.deleted).update_all(status: "rejected") + PostFlag.where(post: Post.undeleted).update_all(status: "rejected") + PostFlag.where(post: Post.deleted).update_all(status: "succeeded") + + # Mark all unapproved in three days flags as successful. + PostFlag.category_matches("deleted").update_all(status: "succeeded") + + # Mark all currently flagged posts as pending. + PostFlag.where(post: Post.flagged).update_all(status: "pending") + + puts "Appeals pending: #{PostAppeal.pending.count}" + puts "Appeals succeeded: #{PostAppeal.succeeded.count}" + puts "Appeals rejected: #{PostAppeal.rejected.count}" + puts "Flags pending: #{PostFlag.pending.count}" + puts "Flags succeeded: #{PostFlag.succeeded.count}" + puts "Flags rejected: #{PostFlag.rejected.count}" +end diff --git a/test/factories/post_flag.rb b/test/factories/post_flag.rb index ed7c3d942..baf373052 100644 --- a/test/factories/post_flag.rb +++ b/test/factories/post_flag.rb @@ -1,7 +1,7 @@ FactoryBot.define do factory(:post_flag) do creator - post + post { build(:post, is_flagged: true) } reason {"xxx"} is_resolved {false} end diff --git a/test/functional/modqueue_controller_test.rb b/test/functional/modqueue_controller_test.rb index 9ab23dc88..8312ea051 100644 --- a/test/functional/modqueue_controller_test.rb +++ b/test/functional/modqueue_controller_test.rb @@ -13,6 +13,14 @@ class ModqueueControllerTest < ActionDispatch::IntegrationTest get_auth modqueue_index_path, @admin assert_response :success end + + should "include appealed posts in the modqueue" do + @appeal = create(:post_appeal) + get_auth modqueue_index_path, @admin + + assert_response :success + assert_select "#post-#{@appeal.post_id}" + end end end end diff --git a/test/functional/post_appeals_controller_test.rb b/test/functional/post_appeals_controller_test.rb index faa5982de..a8dcc1607 100644 --- a/test/functional/post_appeals_controller_test.rb +++ b/test/functional/post_appeals_controller_test.rb @@ -48,14 +48,54 @@ class PostAppealsControllerTest < ActionDispatch::IntegrationTest end context "create action" do - setup do - @post = as(@user) { create(:post, is_deleted: true) } + context "appealing a deleted post" do + should "create a new appeal" do + @post = create(:post, is_deleted: true) + + assert_difference("PostAppeal.count", 1) do + post_auth post_appeals_path, @user, params: { post_appeal: { post_id: @post.id, reason: "xxx" }}, as: :json + end + + assert_response :success + end end - should "create a new appeal" do - assert_difference("PostAppeal.count", 1) do - post_auth post_appeals_path, @user, params: {:format => "js", :post_appeal => {:post_id => @post.id, :reason => "xxx"}} - assert_response :success + context "appealing a flagged post" do + should "fail" do + @flag = create(:post_flag) + + assert_no_difference("PostAppeal.count") do + post_auth post_appeals_path, @user, params: { post_appeal: { post_id: @flag.post.id, reason: "xxx" }}, as: :json + end + + assert_response 422 + assert_equal(["cannot be appealed"], response.parsed_body.dig("errors", "post")) + end + end + + context "appealing a pending post" do + should "fail" do + @post = create(:post, is_pending: true) + + assert_no_difference("PostAppeal.count") do + post_auth post_appeals_path, @user, params: { post_appeal: { post_id: @post.id, reason: "xxx" }}, as: :json + end + + assert_response 422 + assert_equal(["cannot be appealed"], response.parsed_body.dig("errors", "post")) + end + end + + context "appealing an already appealed post" do + should "fail" do + @appeal = create(:post_appeal) + + assert_no_difference("PostAppeal.count") do + post_auth post_appeals_path, @user, params: { post_appeal: { post_id: @appeal.post.id, reason: "xxx" }}, as: :json + end + + assert_response 422 + assert_equal(["cannot be appealed"], response.parsed_body.dig("errors", "post")) end end end diff --git a/test/functional/post_approvals_controller_test.rb b/test/functional/post_approvals_controller_test.rb index e4b93c65f..813cf62ef 100644 --- a/test/functional/post_approvals_controller_test.rb +++ b/test/functional/post_approvals_controller_test.rb @@ -27,6 +27,29 @@ class PostApprovalsControllerTest < ActionDispatch::IntegrationTest end end + context "for an appealed post" do + should "undelete the post and mark the appeal as successful" do + @appeal = create(:post_appeal) + post_auth post_approvals_path(post_id: @appeal.post_id, format: :js), @approver + + assert_response :success + assert_equal(false, @appeal.reload.post.is_deleted?) + assert_equal(true, @appeal.succeeded?) + end + end + + context "for a flagged post" do + should "approve the post and mark the flag as rejected" do + @flag = create(:post_flag) + post_auth post_approvals_path(post_id: @flag.post_id, format: :js), @approver + + assert_response :success + assert_equal(false, @flag.reload.post.is_deleted?) + assert_equal(false, @flag.post.is_flagged?) + assert_equal(true, @flag.rejected?) + end + end + should "not allow non-approvers to approve posts" do @post = create(:post, is_pending: true) post_auth post_approvals_path(post_id: @post.id, format: :js), create(:user) diff --git a/test/functional/post_events_controller_test.rb b/test/functional/post_events_controller_test.rb index af3f42b9b..623b9d2c1 100644 --- a/test/functional/post_events_controller_test.rb +++ b/test/functional/post_events_controller_test.rb @@ -10,6 +10,7 @@ class PostEventsControllerTest < ActionDispatch::IntegrationTest as(@user) do @post = create(:post) @post.flag!("aaa") + @post.update(is_deleted: true) create(:post_appeal, post: @post) @post.approve!(@mod) end diff --git a/test/functional/posts_controller_test.rb b/test/functional/posts_controller_test.rb index f3aeb2826..e70c2d358 100644 --- a/test/functional/posts_controller_test.rb +++ b/test/functional/posts_controller_test.rb @@ -483,7 +483,7 @@ class PostsControllerTest < ActionDispatch::IntegrationTest create(:note, post: @post) create(:artist_commentary, post: @post) create(:post_flag, post: @post, creator: @user) - create(:post_appeal, post: @post, creator: @user) + #create(:post_appeal, post: @post, creator: @user) create(:post_vote, post: @post, user: @user) create(:favorite, post: @post, user: @user) create(:moderation_report, model: @comment, creator: @builder) @@ -667,7 +667,6 @@ class PostsControllerTest < ActionDispatch::IntegrationTest assert_response :success assert_equal(false, @post.is_deleted?) end - end context "revert action" do diff --git a/test/unit/post_appeal_test.rb b/test/unit/post_appeal_test.rb index ef0b9c27f..a37906ce2 100644 --- a/test/unit/post_appeal_test.rb +++ b/test/unit/post_appeal_test.rb @@ -32,7 +32,7 @@ class PostAppealTest < ActiveSupport::TestCase @post_appeal = build(:post_appeal, post: @post, creator: @alice) assert_equal(false, @post_appeal.valid?) - assert_equal(["Post is active"], @post_appeal.errors.full_messages) + assert_equal(["Post cannot be appealed"], @post_appeal.errors.full_messages) end end end diff --git a/test/unit/post_event_test.rb b/test/unit/post_event_test.rb index 460414504..46072a443 100644 --- a/test/unit/post_event_test.rb +++ b/test/unit/post_event_test.rb @@ -5,6 +5,7 @@ class PostEventTest < ActiveSupport::TestCase @user = create(:user, created_at: 2.weeks.ago) @post = create(:post) @post_flag = create(:post_flag, creator: @user, post: @post) + @post.update(is_deleted: true) @post_appeal = create(:post_appeal, creator: @user, post: @post) end diff --git a/test/unit/post_flag_test.rb b/test/unit/post_flag_test.rb index a59a0eb49..ced0cbc36 100644 --- a/test/unit/post_flag_test.rb +++ b/test/unit/post_flag_test.rb @@ -53,7 +53,7 @@ class PostFlagTest < ActiveSupport::TestCase @post_flag = build(:post_flag, post: @post, creator: @bob) @post_flag.save - assert_equal(["Post is deleted"], @post_flag.errors.full_messages) + assert_equal(["Post is deleted and cannot be flagged"], @post_flag.errors.full_messages) end should "not be able to flag a pending post" do diff --git a/test/unit/post_pruner_test.rb b/test/unit/post_pruner_test.rb index 09e491fa7..dd288eca0 100644 --- a/test/unit/post_pruner_test.rb +++ b/test/unit/post_pruner_test.rb @@ -1,30 +1,108 @@ require 'test_helper' class PostPrunerTest < ActiveSupport::TestCase - def setup - @old_post = FactoryBot.create(:post, :created_at => 5.days.ago, :is_pending => true) - @unresolved_flagged_post = FactoryBot.create(:post, :is_flagged => true) - @resolved_flagged_post = FactoryBot.create(:post, :is_flagged => true) + context "PostPruner" do + context "for a pending post" do + should "prune expired posts" do + @post = create(:post, created_at: 5.days.ago, is_pending: true) + PostPruner.prune! - @flagger = create(:gold_user, created_at: 2.weeks.ago) - @unresolved_post_flag = create(:post_flag, creator: @flagger, created_at: 5.days.ago, is_resolved: false, post: @unresolved_flagged_post) - @resolved_post_flag = create(:post_flag, creator: @flagger, created_at: 5.days.ago, is_resolved: true, post: @resolved_flagged_post) + assert_equal(true, @post.reload.is_deleted?) + assert_equal(false, @post.is_pending?) - PostPruner.new.prune! - end + assert_equal(1, @post.flags.size) + assert_equal("Unapproved in three days", @post.flags.last.reason) + end + end - should "prune old pending posts" do - @old_post.reload - assert(@old_post.is_deleted?) - end + context "for a flagged post" do + should "prune expired flags" do + @post = create(:post, created_at: 4.weeks.ago, is_flagged: true) + @flag = create(:post_flag, post: @post, created_at: 5.days.ago) + PostPruner.prune! - should "prune old flagged posts that are still unresolved" do - @unresolved_flagged_post.reload - assert(@unresolved_flagged_post.is_deleted?) - end + assert_equal(true, @post.reload.is_deleted?) + assert_equal(false, @post.is_pending?) + assert_equal(false, @post.is_flagged?) + assert_equal(true, @flag.reload.succeeded?) - should "not prune old flagged posts that are resolved" do - @resolved_flagged_post.reload - assert(!@resolved_flagged_post.is_deleted?) + assert_equal(2, @post.flags.size) + assert_equal("Unapproved in three days after returning to moderation queue", @post.flags.last.reason) + end + + should "not prune unexpired flags" do + @post = create(:post, created_at: 4.weeks.ago, is_flagged: true) + @flag = create(:post_flag, post: @post, created_at: 1.day.ago) + PostPruner.prune! + + assert_equal(false, @post.reload.is_deleted?) + assert_equal(false, @post.is_pending?) + assert_equal(true, @post.is_flagged?) + assert_equal(true, @flag.reload.pending?) + + assert_equal(1, @post.flags.size) + end + + should "leave the status of old flags unchanged" do + @post = create(:post, created_at: 4.weeks.ago, is_flagged: true) + @flag1 = create(:post_flag, post: @post, created_at: 3.weeks.ago, status: :succeeded) + @flag2 = create(:post_flag, post: @post, created_at: 2.weeks.ago, status: :rejected) + @flag3 = create(:post_flag, post: @post, created_at: 1.weeks.ago, status: :pending) + PostPruner.prune! + + assert_equal(true, @post.reload.is_deleted?) + assert_equal(false, @post.is_pending?) + assert_equal(false, @post.is_flagged?) + + assert_equal(true, @flag1.reload.succeeded?) + assert_equal(true, @flag2.reload.rejected?) + assert_equal(true, @flag3.reload.succeeded?) + end + end + + context "for an appealed post" do + should "prune expired appeals" do + @post = create(:post, created_at: 4.weeks.ago, is_deleted: true) + @appeal = create(:post_appeal, post: @post, created_at: 5.days.ago) + PostPruner.prune! + + assert_equal(false, @post.reload.is_pending?) + assert_equal(false, @post.is_flagged?) + assert_equal(true, @post.is_deleted?) + assert_equal(true, @appeal.reload.rejected?) + + assert_equal(1, @post.flags.size) + assert_equal("Unapproved in three days after returning to moderation queue", @post.flags.last.reason) + end + + should "not prune unexpired appeals" do + @post = create(:post, created_at: 4.weeks.ago, is_deleted: true) + @appeal = create(:post_appeal, post: @post, created_at: 1.day.ago) + PostPruner.prune! + + assert_equal(false, @post.reload.is_pending?) + assert_equal(false, @post.is_flagged?) + assert_equal(true, @post.is_deleted?) + assert_equal(true, @appeal.reload.pending?) + + assert_equal(0, @post.flags.size) + end + + should "leave the status of old appeals unchanged" do + @post = create(:post, created_at: 4.weeks.ago, is_deleted: true) + @appeal1 = create(:post_appeal, post: @post, created_at: 3.weeks.ago, status: :succeeded) + @appeal2 = create(:post_appeal, post: @post, created_at: 2.weeks.ago, status: :rejected) + @appeal3 = create(:post_appeal, post: @post, created_at: 1.weeks.ago, status: :pending) + PostPruner.prune! + + assert_equal(true, @post.reload.is_deleted?) + assert_equal(false, @post.is_pending?) + assert_equal(false, @post.is_flagged?) + + assert_equal(true, @appeal1.reload.succeeded?) + assert_equal(true, @appeal2.reload.rejected?) + assert_equal(false, @appeal3.reload.pending?) + end + end end end diff --git a/test/unit/post_query_builder_test.rb b/test/unit/post_query_builder_test.rb index 54b8ca27f..5810a9b92 100644 --- a/test/unit/post_query_builder_test.rb +++ b/test/unit/post_query_builder_test.rb @@ -584,22 +584,26 @@ class PostQueryBuilderTest < ActiveSupport::TestCase flagged = create(:post, is_flagged: true) deleted = create(:post, is_deleted: true) banned = create(:post, is_banned: true) - all = [banned, deleted, flagged, pending] + appealed = create(:post, is_deleted: true) + appeal = create(:post_appeal, post: appealed) + all = [appealed, banned, deleted, flagged, pending] - assert_tag_match([flagged, pending], "status:modqueue") + assert_tag_match([appealed, flagged, pending], "status:modqueue") assert_tag_match([pending], "status:pending") assert_tag_match([flagged], "status:flagged") - assert_tag_match([deleted], "status:deleted") + assert_tag_match([appealed], "status:appealed") + assert_tag_match([appealed, deleted], "status:deleted") assert_tag_match([banned], "status:banned") assert_tag_match([banned], "status:active") assert_tag_match([banned], "status:active status:banned") assert_tag_match(all, "status:any") assert_tag_match(all, "status:all") - assert_tag_match(all - [flagged, pending], "-status:modqueue") + assert_tag_match(all - [flagged, pending, appealed], "-status:modqueue") assert_tag_match(all - [pending], "-status:pending") assert_tag_match(all - [flagged], "-status:flagged") - assert_tag_match(all - [deleted], "-status:deleted") + assert_tag_match(all - [appealed], "-status:appealed") + assert_tag_match(all - [deleted, appealed], "-status:deleted") assert_tag_match(all - [banned], "-status:banned") assert_tag_match(all - [banned], "-status:active") @@ -611,11 +615,13 @@ class PostQueryBuilderTest < ActiveSupport::TestCase flagged = create(:post, is_flagged: true) pending = create(:post, is_pending: true) disapproved = create(:post, is_pending: true) + appealed = create(:post, is_deleted: true) create(:post_flag, post: flagged, creator: create(:user, created_at: 2.weeks.ago)) + create(:post_appeal, post: appealed) create(:post_disapproval, user: CurrentUser.user, post: disapproved, reason: "disinterest") - assert_tag_match([pending, flagged], "status:unmoderated") + assert_tag_match([appealed, pending, flagged], "status:unmoderated") assert_tag_match([disapproved], "-status:unmoderated") end diff --git a/test/unit/post_test.rb b/test/unit/post_test.rb index c65956a32..c45d83bc3 100644 --- a/test/unit/post_test.rb +++ b/test/unit/post_test.rb @@ -513,24 +513,23 @@ class PostTest < ActiveSupport::TestCase end context "A status locked post" do - setup do - @post = FactoryBot.create(:post, is_status_locked: true) - end - should "not allow new flags" do assert_raises(PostFlag::Error) do + @post = create(:post, is_status_locked: true) @post.flag!("wrong") end end should "not allow new appeals" do + @post = create(:post, is_status_locked: true, is_deleted: true) @appeal = build(:post_appeal, post: @post) assert_equal(false, @appeal.valid?) - assert_equal(["Post is active"], @appeal.errors.full_messages) + assert_equal(["Post cannot be appealed"], @appeal.errors.full_messages) end should "not allow approval" do + @post = create(:post, is_status_locked: true, is_pending: true) approval = @post.approve! assert_includes(approval.errors.full_messages, "Post is locked and cannot be approved") end diff --git a/test/unit/upload_limit_test.rb b/test/unit/upload_limit_test.rb index f58c8c95a..06127b4fb 100644 --- a/test/unit/upload_limit_test.rb +++ b/test/unit/upload_limit_test.rb @@ -12,7 +12,7 @@ class UploadLimitTest < ActiveSupport::TestCase @post = create(:post, uploader: @user, is_pending: true, created_at: 7.days.ago) assert_equal(1000, @user.reload.upload_points) - PostPruner.new.prune! + PostPruner.prune! assert_equal(967, @user.reload.upload_points) end end