[UserFeedbacks] Add ability to soft delete feedbacks (#670)

app/controllers/user_feedbacks_controller.rb

@@ -1,7 +1,7 @@
 # frozen_string_literal: true
 
 class UserFeedbacksController < ApplicationController
-  before_action :moderator_only, :only => [:new, :edit, :create, :update, :destroy]
+  before_action :moderator_only, except: %i[index show]
   respond_to :html, :json
 
   def new
@@ -11,17 +11,18 @@ class UserFeedbacksController < ApplicationController
 
   def edit
     @user_feedback = UserFeedback.find(params[:id])
-    check_privilege(@user_feedback)
+    check_edit_privilege(@user_feedback)
     respond_with(@user_feedback)
   end
 
   def show
     @user_feedback = UserFeedback.find(params[:id])
+    raise(User::PrivilegeError) if !CurrentUser.user.is_moderator? && @user_feedback.is_deleted?
     respond_with(@user_feedback)
   end
 
   def index
-    @user_feedbacks = UserFeedback.search(search_params).paginate(params[:page], limit: params[:limit])
+    @user_feedbacks = UserFeedback.visible(CurrentUser.user).search(search_params).paginate(params[:page], limit: params[:limit])
     respond_with(@user_feedbacks)
   end
 
@@ -32,7 +33,7 @@ class UserFeedbacksController < ApplicationController
 
   def update
     @user_feedback = UserFeedback.find(params[:id])
-    check_privilege(@user_feedback)
+    check_edit_privilege(@user_feedback)
     params_update = user_feedback_params(:update)
 
     @user_feedback.update(params_update)
@@ -41,17 +42,47 @@ class UserFeedbacksController < ApplicationController
     respond_with(@user_feedback)
   end
 
+  def delete
+    @user_feedback = UserFeedback.find(params[:id])
+    check_delete_privilege(@user_feedback)
+    @user_feedback.update(is_deleted: true)
+    flash[:notice] = @user_feedback.errors.any? ? @user_feedback.errors.full_messages.join("; ") : "Feedback deleted"
+    respond_with(@user_feedback) do |format|
+      format.html { redirect_back(fallback_location: user_feedbacks_path(search: { user_id: @user_feedback.user_id })) }
+    end
+  end
+
+  def undelete
+    @user_feedback = UserFeedback.find(params[:id])
+    check_delete_privilege(@user_feedback)
+    @user_feedback.update(is_deleted: false)
+    flash[:notice] = @user_feedback.errors.any? ? @user_feedback.errors.full_messages.join("; ") : "Feedback undeleted"
+    respond_with(@user_feedback) do |format|
+      format.html { redirect_back(fallback_location: user_feedbacks_path(search: { user_id: @user_feedback.user_id })) }
+    end
+  end
+
   def destroy
     @user_feedback = UserFeedback.find(params[:id])
-    check_privilege(@user_feedback)
+    check_destroy_privilege(@user_feedback)
     @user_feedback.destroy
-    redirect_back fallback_location: user_feedbacks_path
+    respond_with(@user_feedback) do |format|
+      format.html { redirect_back(fallback_location: user_feedbacks_path(search: { user_id: @user_feedback.user_id })) }
+    end
   end
 
   private
 
-  def check_privilege(user_feedback)
+  def check_edit_privilege(user_feedback)
-    raise User::PrivilegeError unless user_feedback.editable_by?(CurrentUser.user)
+    raise(User::PrivilegeError) unless user_feedback.editable_by?(CurrentUser.user)
+  end
+
+  def check_delete_privilege(user_feedback)
+    raise(User::PrivilegeError) unless user_feedback.deletable_by?(CurrentUser.user)
+  end
+
+  def check_destroy_privilege(user_feedback)
+    raise(User::PrivilegeError) unless user_feedback.destroyable_by?(CurrentUser.user)
   end
 
   def user_feedback_params(context)
@@ -61,4 +92,8 @@ class UserFeedbacksController < ApplicationController
 
     params.fetch(:user_feedback, {}).permit(permitted_params)
   end
+
+  def search_params
+    permit_search_params(%i[deleted body_matches user_id user_name creator_id creator_name category])
+  end
 end

app/decorators/mod_action_decorator.rb

@@ -130,6 +130,10 @@ class ModActionDecorator < ApplicationDecorator
       end
     when "user_feedback_delete"
       "Deleted #{vals['type']} record ##{vals['record_id']} for #{user} with reason: #{vals['reason']}"
+    when "user_feedback_undelete"
+      "Undeleted #{vals['type']} record ##{vals['record_id']} for #{user} with reason: #{vals['reason']}"
+    when "user_feedback_destroy"
+      "Destroyed #{vals['type']} record ##{vals['record_id']} for #{user} with reason: #{vals['reason']}"
       ### Legacy User Record ###
     when "created_positive_record"
       "Created positive record ##{vals['record_id']} for #{user} with reason: #{vals['reason']}"

app/javascript/src/styles/specific/user_feedback.scss

@@ -1,5 +1,3 @@
-
-
 .user-feedback-positive {
   color: $positive-record-color;
 }
@@ -25,7 +23,18 @@ div#c-user-feedbacks, div#c-moderator-dashboards .activity-container {
     background: $neutral-record-background;
   }
 
+  tr.user-feedback[data-is-deleted="true"] {
+    background: #827428;
+  }
+
   blockquote {
     padding: 0.5em;
   }
+
+  .show-all-user-feedbacks-link {
+    font-size: 1.5em;
+    width: 100%;
+    text-align: center;
+    padding: 0.5em 0;
+  }
 }

app/models/mod_action.rb

@@ -72,6 +72,8 @@ class ModAction < ApplicationRecord
       :user_feedback_create,
       :user_feedback_update,
       :user_feedback_delete,
+      :user_feedback_undelete,
+      :user_feedback_destroy,
       :wiki_page_rename,
       :wiki_page_delete,
       :wiki_page_lock,

app/models/user.rb

@@ -96,7 +96,7 @@ class User < ApplicationRecord
   has_many :bans, -> { order("bans.id desc") }
   has_many :dmails, -> { order("dmails.id desc") }, foreign_key: "owner_id"
   has_many :favorites, -> { order(id: :desc) }
-  has_many :feedback, class_name: "UserFeedback", dependent: :destroy
+  has_many :feedback, -> { active }, class_name: "UserFeedback", dependent: :destroy
   has_many :forum_posts, -> { order("forum_posts.created_at, forum_posts.id") }, foreign_key: "creator_id"
   has_many :forum_topic_visits
   has_many :note_versions, foreign_key: "updater_id"

app/models/user_feedback.rb

@@ -10,22 +10,39 @@ class UserFeedback < ApplicationRecord
   validates :body, length: { minimum: 1, maximum: Danbooru.config.user_feedback_max_size }
   validate :creator_is_moderator, on: :create
   validate :user_is_not_creator
+  after_create :log_create
+  after_update :log_update
+  after_destroy :log_destroy
   after_save :create_dmail
-  after_create do |rec|
-    ModAction.log(:user_feedback_create, { user_id: rec.user_id, reason: rec.body, type: rec.category, record_id: rec.id })
-  end
-  after_update do |rec|
-    ModAction.log(:user_feedback_update, { user_id: rec.user_id, reason: rec.body, reason_was: rec.body_before_last_save, type: rec.category, type_was: rec.category_before_last_save, record_id: rec.id })
-  end
-  after_destroy do |rec|
-    ModAction.log(:user_feedback_delete, { user_id: rec.user_id, reason: rec.body, type: rec.category, record_id: rec.id })
-    deletion_user = "\"#{CurrentUser.name}\":/users/#{CurrentUser.id}"
-    creator_user = "\"#{creator.name}\":/users/#{creator.id}"
-    StaffNote.create(body: "#{deletion_user} deleted #{rec.category} feedback, created #{created_at.to_date} by #{creator_user}: #{rec.body}", user_id: rec.user_id, creator: User.system)
-  end
 
   attr_accessor :send_update_dmail
 
+  scope :active, -> { where(is_deleted: false) }
+  scope :deleted, -> { where(is_deleted: true) }
+
+  module LogMethods
+    def log_create
+      ModAction.log(:user_feedback_create, { user_id: user_id, reason: body, type: category, record_id: id })
+    end
+
+    def log_update
+      details = { user_id: user_id, reason: body, reason_was: body_before_last_save, type: category, type_was: category_before_last_save, record_id: id }
+      if saved_change_to_is_deleted?
+        action = is_deleted? ? :user_feedback_delete : :user_feedback_undelete
+        ModAction.log(action, details)
+        return unless saved_change_to_category? || saved_change_to_body?
+      end
+      ModAction.log(:user_feedback_update, details)
+    end
+
+    def log_destroy
+      ModAction.log(:user_feedback_de, { user_id: user_id, reason: body, type: category, record_id: id })
+      deletion_user = "\"#{CurrentUser.name}\":/users/#{CurrentUser.id}"
+      creator_user = "\"#{creator.name}\":/users/#{creator.id}"
+      StaffNote.create(body: "#{deletion_user} deleted #{category} feedback, created #{created_at.to_date} by #{creator_user}: #{body}", user_id: user_id, creator: User.system)
+    end
+  end
+
   module SearchMethods
     def positive
       where("category = ?", "positive")
@@ -47,11 +64,22 @@ class UserFeedback < ApplicationRecord
       order(created_at: :desc)
     end
 
+    def visible(user)
+      if user.is_moderator?
+        all
+      else
+        active
+      end
+    end
+
     def search(params)
       q = super
 
-      q = q.attribute_matches(:body, params[:body_matches])
+      deleted = (params[:deleted].presence || "excluded").downcase
+      q = q.active if deleted == "excluded"
+      q = q.deleted if deleted == "only"
 
+      q = q.attribute_matches(:body, params[:body_matches])
       q = q.where_user(:user_id, :user, params)
       q = q.where_user(:creator_id, :creator, params)
 
@@ -63,6 +91,7 @@ class UserFeedback < ApplicationRecord
     end
   end
 
+  include LogMethods
   extend SearchMethods
 
   def user_name
@@ -93,4 +122,12 @@ class UserFeedback < ApplicationRecord
   def editable_by?(editor)
     editor.is_moderator? && editor != user
   end
+
+  def deletable_by?(deleter)
+    editable_by?(deleter)
+  end
+
+  def destroyable_by?(destroyer)
+    deletable_by?(destroyer) && (destroyer.is_admin? || destroyer == creator)
+  end
 end

app/views/user_feedbacks/_search.html.erb

@@ -2,5 +2,8 @@
   <%= f.user :user %>
   <%= f.user :creator %>
   <%= f.input :body_matches, label: "Message" %>
+  <% if CurrentUser.is_moderator? %>
+    <%= f.input :deleted, label: "Deleted?", collection: [%w[Excluded excluded], %w[Included included], %w[Only only]], include_blank: true %>
+  <% end %>
   <%= f.input :category, collection: %w[positive negative neutral], include_blank: true %>
 <% end %>

app/views/user_feedbacks/index.html.erb

@@ -6,19 +6,24 @@
     <table class="striped">
       <thead>
         <tr>
-          <th width="15%">User</th>
+          <th width="12%">User</th>
-          <th width="15%">Creator</th>
+          <th width="12%">Creator</th>
-          <th width="15%">When</th>
+          <th width="12%">When</th>
           <th width="45%">Message</th>
-          <th></th>
+          <th width="14%"></th>
         </tr>
       </thead>
       <tbody>
         <% @user_feedbacks.each do |feedback| %>
-          <tr class="feedback-category-<%= feedback.category %>">
+          <tr class="user-feedback feedback-category-<%= feedback.category %>" data-is-deleted="<%= feedback.is_deleted? %>">
             <td><%= link_to_user feedback.user %></td>
             <td><%= link_to_user feedback.creator %></td>
-            <td><%= compact_time(feedback.created_at) %></td>
+            <td>
+              <%= compact_time(feedback.created_at) %>
+              <% if feedback.is_deleted? %>
+                (deleted)
+              <% end %>
+            </td>
             <td>
               <div class="dtext-container">
                 <%= format_text(feedback.body) %>
@@ -28,7 +33,16 @@
             <td>
               <% if feedback.editable_by?(CurrentUser.user) %>
                 <%= link_to "edit", edit_user_feedback_path(feedback) %>
-                | <%= link_to "delete", user_feedback_path(feedback), :method => :delete, :data => {:confirm => "Are you sure you want to delete this user feedback?"} %>
+              <% end %>
+               <% if feedback.deletable_by?(CurrentUser.user) %>
+                <% if feedback.is_deleted? %>
+                  | <%= link_to "undelete", undelete_user_feedback_path(feedback), method: :put, data: { confirm: "Are you sure you want to undelete this user feedback?" } %>
+                <% else %>
+                  | <%= link_to "delete", delete_user_feedback_path(feedback), method: :put, data: { confirm: "Are you sure you want to delete this user feedback?" } %>
+                <% end %>
+              <% end %>
+              <% if feedback.destroyable_by?(CurrentUser.user) %>
+                | <%= link_to "destroy", user_feedback_path(feedback), method: :delete, data: { confirm: "Are you sure you want to destroy this user feedback? This cannot be undone." } %>
               <% end %>
             </td>
           </tr>
@@ -36,6 +50,13 @@
       </tbody>
     </table>
 
+    <% if CurrentUser.is_moderator? && params.dig(:search, :user_id).present? && params.dig(:search, :deleted).blank? %>
+      <% count = UserFeedback.for_user(params.dig(:search, :user_id)).deleted.count %>
+      <% if count > 0 %>
+        <%= link_to("Show All (#{count})", { search: params[:search].permit!.merge(deleted: "included") }, class: "show-all-user-feedbacks-link button btn-neutral") %>
+      <% end %>
+    <% end %>
+
     <%= numbered_paginator(@user_feedbacks) %>
   </div>
 </div>

app/views/user_feedbacks/show.html.erb

@@ -5,6 +5,7 @@
     <ul>
       <li><strong>Creator</strong> <%= link_to_user @user_feedback.creator %></li>
       <li><strong>Date</strong> <%= @user_feedback.created_at %></li>
+      <li><strong>Status</strong> <%= @user_feedback.is_deleted? ? "Deleted" : "Active" %></li>
       <li><strong>Category</strong> <%= @user_feedback.category %></li>
       <li><strong>Message</strong>
         <div class="dtext-container"><%= format_text @user_feedback.body %></div></li>

config/routes.rb

@@ -288,6 +288,10 @@ Rails.application.routes.draw do
     collection do
       get :search
     end
+    member do
+      put :delete
+      put :undelete
+    end
   end
   resources :user_name_change_requests
   resource :user_revert, :only => [:new, :create]

db/fixes/115_convert_user_feedback_delete_modactions.rb

@@ -0,0 +1,6 @@
+#!/usr/bin/env ruby
+# frozen_string_literal: true
+
+require File.expand_path(File.join(File.dirname(__FILE__), "..", "..", "config", "environment"))
+
+ModAction.where(action: "user_feedback_delete").update_all("action = 'user_feedback_destroy'")

db/migrate/20240706061122_add_is_deleted_to_user_feedback.rb

@@ -0,0 +1,7 @@
+# frozen_string_literal: true
+
+class AddIsDeletedToUserFeedback < ActiveRecord::Migration[7.1]
+  def change
+    add_column(:user_feedback, :is_deleted, :boolean, null: false, default: false)
+  end
+end

db/structure.sql

@@ -2099,7 +2099,8 @@ CREATE TABLE public.user_feedback (
     created_at timestamp without time zone NOT NULL,
     updated_at timestamp without time zone NOT NULL,
     creator_ip_addr inet,
-    updater_id integer
+    updater_id integer,
+    is_deleted boolean DEFAULT false NOT NULL
 );
 
 
@@ -4496,6 +4497,7 @@ SET search_path TO "$user", public;
 
 INSERT INTO "schema_migrations" (version) VALUES
 ('20240709134926'),
+('20240706061122'),
 ('20240101042716'),
 ('20230531080817'),
 ('20230518182034'),

test/functional/user_feedbacks_controller_test.rb

@@ -8,6 +8,7 @@ class UserFeedbacksControllerTest < ActionDispatch::IntegrationTest
       @user = create(:user)
       @critic = create(:moderator_user)
       @mod = create(:moderator_user)
+      @admin = create(:admin_user)
     end
 
     context "new action" do
@@ -52,7 +53,7 @@ class UserFeedbacksControllerTest < ActionDispatch::IntegrationTest
 
     context "create action" do
       should "create a new feedback" do
-        assert_difference([-> { UserFeedback.count }, -> { Dmail.count }], 1) do
+        assert_difference(%w[UserFeedback.count Dmail.count ModAction.count], 1) do
           post_auth user_feedbacks_path, @critic, params: { user_feedback: { category: "positive", user_name: @user.name, body: "xxx" } }
         end
       end
@@ -64,7 +65,7 @@ class UserFeedbacksControllerTest < ActionDispatch::IntegrationTest
           @feedback = create(:user_feedback, user: @user, category: "negative")
         end
 
-        assert_no_difference(-> { Dmail.count }) do
+        assert_difference({ "Dmail.count" => 0, "ModAction.count" => 1 }) do
           put_auth user_feedback_path(@feedback), @critic, params: { id: @feedback.id, user_feedback: { category: "positive" } }
         end
 
@@ -77,7 +78,7 @@ class UserFeedbacksControllerTest < ActionDispatch::IntegrationTest
         as(@critic) do
           @feedback = create(:user_feedback, user: @user, category: "negative")
         end
-        assert_difference(-> { Dmail.count }, 1) do
+        assert_difference(%w[Dmail.count ModAction.count], 1) do
           put_auth user_feedback_path(@feedback), @critic, params: { id: @feedback.id, user_feedback: { body: "changed", send_update_dmail: true } }
         end
         assert_match(/updated a/, Dmail.last.body)
@@ -92,25 +93,121 @@ class UserFeedbacksControllerTest < ActionDispatch::IntegrationTest
       end
 
       should "delete a feedback" do
-        assert_difference(-> { UserFeedback.count }, -1) do
+        assert_difference({ "UserFeedback.count" => -1, "ModAction.count" => 1 }) do
           delete_auth user_feedback_path(@user_feedback), @critic
         end
       end
 
       context "by a moderator" do
-        should "allow deleting feedbacks given to other users" do
+        should "allow destroying feedbacks they created" do
-          assert_difference(-> { UserFeedback.count }, -1) do
+          as(@mod) { @user_feedback = create(:user_feedback, user: @user) }
+          assert_difference({ "UserFeedback.count" => -1, "ModAction.count" => 1 }) do
             delete_auth user_feedback_path(@user_feedback), @mod
           end
         end
 
+        should "now allow destroying feedbacks they did not create" do
+          assert_difference(%w[UserFeedback.count ModAction.count], 0) do
+            delete_auth user_feedback_path(@user_feedback), @mod
+          end
+        end
+
+        should "not allow destroying feedbacks given to themselves" do
+          as(@critic) do
+            @user_feedback = create(:user_feedback, user: @mod)
+          end
+
+          assert_no_difference("UserFeedback.count") do
+            delete_auth user_feedback_path(@user_feedback), @mod
+          end
+        end
+      end
+
+      context "by an admin" do
+        should "allow destroying feedbacks they created" do
+          as(@admin) { @user_feedback = create(:user_feedback, user: @user) }
+          assert_difference({ "UserFeedback.count" => -1, "ModAction.count" => 1 }) do
+            delete_auth user_feedback_path(@user_feedback), @admin
+          end
+        end
+
+        should "allow destroying feedbacks they did not create" do
+          assert_difference({ "UserFeedback.count" => -1, "ModAction.count" => 1 }) do
+            delete_auth user_feedback_path(@user_feedback, format: :json), @admin
+          end
+        end
+
+        should "not allow destroying feedbacks given to themselves" do
+          as(@critic) do
+            @user_feedback = create(:user_feedback, user: @admin)
+          end
+
+          assert_no_difference("UserFeedback.count") do
+            delete_auth user_feedback_path(@user_feedback), @admin
+          end
+        end
+      end
+    end
+
+    context "delete action" do
+      setup do
+        as(@critic) do
+          @user_feedback = create(:user_feedback, user: @user)
+        end
+      end
+
+      should "delete a feedback" do
+        assert_difference("ModAction.count", 1) do
+          put_auth delete_user_feedback_path(@user_feedback), @critic
+        end
+      end
+
+      context "by a moderator" do
+        should "allow deleting feedbacks given to other users" do
+          assert_difference({ "UserFeedback.count" => 0, "ModAction.count" => 1, "@user.feedback.count" => -1 }) do
+            put_auth delete_user_feedback_path(@user_feedback), @mod
+          end
+        end
+
         should "not allow deleting feedbacks given to themselves" do
           as(@critic) do
             @user_feedback = create(:user_feedback, user: @mod)
           end
 
-          assert_no_difference(-> { UserFeedback.count }) do
+          assert_no_difference(%w[UserFeedback.count ModAction.count @mod.feedback.count]) do
-            delete_auth user_feedback_path(@user_feedback), @mod
+            put_auth delete_user_feedback_path(@user_feedback), @mod
+          end
+        end
+      end
+    end
+
+    context "undelete action" do
+      setup do
+        as(@critic) do
+          @user_feedback = create(:user_feedback, user: @user, is_deleted: true)
+        end
+      end
+
+      should "delete a feedback" do
+        assert_difference("ModAction.count", 1) do
+          put_auth undelete_user_feedback_path(@user_feedback), @critic
+        end
+      end
+
+      context "by a moderator" do
+        should "allow deleting feedbacks given to other users" do
+          assert_difference({ "UserFeedback.count" => 0, "ModAction.count" => 1, "@user.feedback.count" => 1 }) do
+            put_auth undelete_user_feedback_path(@user_feedback), @mod
+          end
+        end
+
+        should "not allow deleting feedbacks given to themselves" do
+          as(@critic) do
+            @user_feedback = create(:user_feedback, user: @mod)
+          end
+
+          assert_no_difference(%w[UserFeedback.count ModAction.count @mod.feedback.count]) do
+            put_auth undelete_user_feedback_path(@user_feedback), @mod
           end
         end
       end