[Blips] Add Updater & Tests/Move modactions to callbacks (#540)

* [Blips] Add `updater_id` & update notice

* [Blips] Move modaction generation logic into rails callbacks

* [Tests] Add blip tests

* Move update notice to right place

* Assert amount of difference

* Rebase oopsie

---------

Co-authored-by: Earlopain <14981592+Earlopain@users.noreply.github.com>

app/controllers/blips_controller.rb

@@ -29,10 +29,7 @@ class BlipsController < ApplicationController
   def update
     @blip = Blip.find(params[:id])
     check_edit_privilege(@blip)
-    Blip.transaction do
-      @blip.update(blip_params(:update))
-      ModAction.log(:blip_update, { blip_id: @blip.id, user_id: @blip.creator_id }) if CurrentUser.user != @blip.creator
-    end
+    @blip.update(blip_params(:update))
     flash[:notice] = 'Blip updated'
     respond_with(@blip)
   end
@@ -40,27 +37,18 @@ class BlipsController < ApplicationController
   def hide
     @blip = Blip.find(params[:id])
     check_hide_privilege(@blip)
-
-    Blip.transaction do
-      @blip.update(is_hidden: true)
-      ModAction.log(:blip_hide, { blip_id: @blip.id, user_id: @blip.creator_id }) if CurrentUser.user != @blip.creator
-    end
+    @blip.hide!
     respond_with(@blip)
   end
 
   def unhide
     @blip = Blip.find(params[:id])
-    Blip.transaction do
-      @blip.update(is_hidden: false)
-      ModAction.log(:blip_unhide, {blip_id: @blip.id, user_id: @blip.creator_id})
-    end
+    @blip.unhide!
     respond_with(@blip)
   end
 
   def destroy
     @blip = Blip.find(params[:id])
-
-    ModAction.log(:blip_delete, {blip_id: @blip.id, user_id: @blip.creator_id})
     @blip.destroy
     flash[:notice] = 'Blip deleted'
     respond_with(@blip) do |format|

app/models/blip.rb

@@ -2,11 +2,23 @@ class Blip < ApplicationRecord
   include UserWarnable
   simple_versioning
   belongs_to_creator
+  belongs_to_updater optional: true
   validates :body, presence: true
   validates :body, length: { minimum: 5, maximum: Danbooru.config.blip_max_size }
   validate :validate_parent_exists, on: :create
   validate :validate_creator_is_not_limited, on: :create
 
+  after_update(if: ->(rec) { !rec.saved_change_to_is_hidden? && CurrentUser.id != rec.creator_id }) do |rec|
+    ModAction.log(:blip_update, { blip_id: rec.id, user_id: rec.creator_id })
+  end
+  after_destroy do |rec|
+    ModAction.log(:blip_delete, { blip_id: rec.id, user_id: rec.creator_id })
+  end
+  after_save(if: ->(rec) { rec.saved_change_to_is_hidden? && CurrentUser.id != rec.creator_id }) do |rec|
+    action = rec.is_hidden? ? :blip_hide : :blip_unhide
+    ModAction.log(action, { blip_id: rec.id, user_id: rec.creator_id })
+  end
+
   user_status_counter :blip_count
   belongs_to :parent, class_name: "Blip", foreign_key: "response_to", optional: true
   belongs_to :warning_user, class_name: "User", optional: true
@@ -104,4 +116,12 @@ class Blip < ApplicationRecord
   include PermissionsMethods
   extend SearchMethods
   include ApiMethods
+
+  def hide!
+    update(is_hidden: true)
+  end
+
+  def unhide!
+    update(is_hidden: false)
+  end
 end

app/views/blips/partials/show/_blip.html.erb

@@ -23,6 +23,7 @@
       <div class="body dtext-container">
         <%= format_text(blip.body, allow_color: blip.creator.is_privileged?) %>
       </div>
+      <%= render "application/update_notice", record: blip %>
       <%= render "application/warned_notice", record: blip %>
       <div class="content-menu">
         <menu>

db/migrate/20230531080817_add_blips_updater_id.rb

@@ -0,0 +1,6 @@
+class AddBlipsUpdaterId < ActiveRecord::Migration[7.0]
+  def change
+    add_column :blips, :updater_id, :integer
+    add_foreign_key :blips, :users, column: :updater_id
+  end
+end

db/structure.sql

@@ -251,7 +251,8 @@ CREATE TABLE public.blips (
     created_at timestamp without time zone NOT NULL,
     updated_at timestamp without time zone NOT NULL,
     warning_type integer,
-    warning_user_id integer
+    warning_user_id integer,
+    updater_id integer
 );
 
 
@@ -4420,6 +4421,14 @@ ALTER TABLE ONLY public.staff_audit_logs
     ADD CONSTRAINT fk_rails_02329e5ef9 FOREIGN KEY (user_id) REFERENCES public.users(id);
 
 
+--
+-- Name: blips fk_rails_23e7479aac; Type: FK CONSTRAINT; Schema: public; Owner: -
+--
+
+ALTER TABLE ONLY public.blips
+    ADD CONSTRAINT fk_rails_23e7479aac FOREIGN KEY (updater_id) REFERENCES public.users(id);
+
+
 --
 -- Name: tickets fk_rails_45cd696dba; Type: FK CONSTRAINT; Schema: public; Owner: -
 --
@@ -4733,6 +4742,7 @@ INSERT INTO "schema_migrations" (version) VALUES
 ('20230506161827'),
 ('20230513074838'),
 ('20230517155547'),
-('20230518182034');
+('20230518182034'),
+('20230531080817');
 
 

test/factories/blip.rb

@@ -1,6 +1,5 @@
 FactoryBot.define do
   factory(:blip) do
-    creator
     creator_ip_addr { "127.0.0.1" }
     sequence(:body) { |n| "blip_body_#{n}" }
   end

test/unit/blip_test.rb

@@ -0,0 +1,106 @@
+require "test_helper"
+
+class BlipTest < ActiveSupport::TestCase
+  context "A blip" do
+    setup do
+      @user = create(:user)
+      CurrentUser.user = @user
+    end
+
+    context "created by a limited user" do
+      setup do
+        Danbooru.config.stubs(:disable_throttles?).returns(false)
+      end
+
+      should "fail creation" do
+        blip = build(:blip)
+        blip.save
+        assert_equal(["Creator can not yet perform this action. Account is too new"], blip.errors.full_messages)
+      end
+    end
+
+    context "created by an unlimited user" do
+      setup do
+        Danbooru.config.stubs(:blip_limit).returns(100)
+      end
+
+      should "be created" do
+        blip = build(:blip)
+        blip.save
+        assert(blip.errors.empty?, blip.errors.full_messages.join(", "))
+      end
+
+      should "be searchable" do
+        b1 = create(:blip, body: "aaa bbb ccc")
+        b2 = create(:blip, body: "aaa ddd")
+
+        matches = Blip.search(body_matches: "aaa")
+        assert_equal(2, matches.count)
+        assert_equal(b2.id, matches.all[0].id)
+        assert_equal(b1.id, matches.all[1].id)
+      end
+
+      should "default to id_desc order when searched with no options specified" do
+        blips = create_list(:blip, 3)
+        matches = Blip.search({})
+
+        assert_equal([blips[2].id, blips[1].id, blips[0].id], matches.map(&:id))
+      end
+
+      context "that is edited by a moderator" do
+        setup do
+          @blip = create(:blip)
+          @mod = create(:moderator_user)
+          CurrentUser.user = @mod
+        end
+
+        should "create a mod action" do
+          assert_difference(-> { ModAction.count }, 1) do
+            @blip.update(body: "nopearino")
+          end
+        end
+
+        should "credit the moderator as the updater" do
+          @blip.update(body: "testing")
+          assert_equal(@mod.id, @blip.updater_id)
+        end
+      end
+
+      context "that is hidden by a moderator" do
+        setup do
+          @blip = create(:blip)
+          @mod = create(:moderator_user)
+          CurrentUser.user = @mod
+        end
+
+        should "create a mod action" do
+          assert_difference(-> { ModAction.count }, 1) do
+            @blip.update(is_hidden: true)
+          end
+        end
+
+        should "credit the moderator as the updater" do
+          @blip.update(is_hidden: true)
+          assert_equal(@mod.id, @blip.updater_id)
+        end
+      end
+
+      context "that is deleted" do
+        setup do
+          @blip = create(:blip)
+        end
+
+        should "create a mod action" do
+          assert_difference(-> { ModAction.count }, 1) do
+            @blip.destroy
+          end
+        end
+      end
+    end
+
+    context "during validation" do
+      subject { build(:blip) }
+      should_not allow_value(" ").for(:body)
+    end
+  end
+end

test/unit/paginator_test.rb

@@ -10,6 +10,11 @@ class PaginatorTest < ActiveSupport::TestCase
 
   { active_record: Blip, opensearch: Post }.each do |name, model| # rubocop:disable Metrics/BlockLength
     context name do
+      setup do
+        @user = create(:user)
+        CurrentUser.user = @user
+      end
+
       context "sequential pagination (before)" do
         should "return the correct set of records" do
           @records = create_list(model.name.underscore, 4)