Fix #3965: Extraneous API attributes.

Remove the updater_id/updater_ip_addr virtual attributes from
pools/notes. Juss pass them in as params to create_version instead.

app/models/note.rb

@@ -1,14 +1,11 @@
 class Note < ApplicationRecord
   class RevertError < Exception ; end
 
-  attribute :updater_id, :integer
-  attribute :updater_ip_addr, :inet
   attr_accessor :html_id
   belongs_to :post
   belongs_to_creator
-  belongs_to_updater
   has_many :versions, -> {order("note_versions.id ASC")}, :class_name => "NoteVersion", :dependent => :destroy
-  validates_presence_of :post_id, :creator_id, :updater_id, :x, :y, :width, :height, :body
+  validates_presence_of :post_id, :creator_id, :x, :y, :width, :height, :body
   validate :post_must_exist
   validate :note_within_image
   after_save :update_post
@@ -115,15 +112,15 @@ class Note < ApplicationRecord
     end
   end
 
-  def create_version
+  def create_version(updater: CurrentUser.user, updater_ip_addr: CurrentUser.ip_addr)
     return unless saved_change_to_versioned_attributes?
 
-    if merge_version?
+    if merge_version?(updater.id)
       merge_version
     else
       Note.where(:id => id).update_all("version = coalesce(version, 0) + 1")
       reload
-      create_new_version
+      create_new_version(updater.id, updater_ip_addr)
     end
   end
 
@@ -131,7 +128,7 @@ class Note < ApplicationRecord
     new_record? || saved_change_to_x? || saved_change_to_y? || saved_change_to_width? || saved_change_to_height? || saved_change_to_is_active? || saved_change_to_body?
   end
 
-  def create_new_version
+  def create_new_version(updater_id, updater_ip_addr)
     versions.create(
       :updater_id => updater_id,
       :updater_ip_addr => updater_ip_addr,
@@ -158,9 +155,9 @@ class Note < ApplicationRecord
     )
   end
 
-  def merge_version?
+  def merge_version?(updater_id)
     prev = versions.last
-    prev && prev.updater_id == CurrentUser.user.id && prev.updated_at > 1.hour.ago && !saved_change_to_is_active?
+    prev && prev.updater_id == updater_id && prev.updated_at > 1.hour.ago && !saved_change_to_is_active?
   end
 
   def revert_to(version)
@@ -175,8 +172,6 @@ class Note < ApplicationRecord
     self.width = version.width
     self.height = version.height
     self.is_active = version.is_active
-    self.updater_id = CurrentUser.id
-    self.updater_ip_addr = CurrentUser.ip_addr
   end
 
   def revert_to!(version)

app/models/pool.rb

@@ -3,7 +3,6 @@ require 'ostruct'
 class Pool < ApplicationRecord
   class RevertError < Exception ; end
 
-  attribute :updater_id, :integer
   validates_uniqueness_of :name, :case_sensitive => false, :if => :saved_change_to_name?
   validate :validate_name, :if => :saved_change_to_name?
   validates_inclusion_of :category, :in => %w(series collection)
@@ -11,7 +10,6 @@ class Pool < ApplicationRecord
   validate :updater_can_remove_posts
   validate :updater_can_edit_deleted
   belongs_to_creator
-  belongs_to_updater
   before_validation :normalize_post_ids
   before_validation :normalize_name
   before_validation :initialize_is_active, :on => :create
@@ -323,9 +321,9 @@ class Pool < ApplicationRecord
     post_ids[/^(\d+)/, 1]
   end
 
-  def create_version(force = false)
+  def create_version(updater: CurrentUser.user, updater_ip_addr: CurrentUser.ip_addr)
     if PoolArchive.enabled?
-      PoolArchive.queue(self)
+      PoolArchive.queue(self, updater, updater_ip_addr)
     else
       Rails.logger.warn("Archive service is not configured. Pool versions will not be saved.")
     end

app/models/pool_archive.rb

@@ -43,7 +43,7 @@ class PoolArchive < ApplicationRecord
     SqsService.new(Danbooru.config.aws_sqs_archives_url)
   end
 
-  def self.queue(pool)
+  def self.queue(pool, updater, updater_ip_addr)
     # queue updates to sqs so that if archives goes down for whatever reason it won't
     # block pool updates
     raise NotImplementedError.new("Archive service is not configured.") if !enabled?
@@ -51,8 +51,8 @@ class PoolArchive < ApplicationRecord
     json = {
       pool_id: pool.id,
       post_ids: pool.post_ids.scan(/\d+/).map(&:to_i),
-      updater_id: CurrentUser.id,
-      updater_ip_addr: CurrentUser.ip_addr.to_s,
+      updater_id: updater.id,
+      updater_ip_addr: updater_ip_addr.to_s,
       created_at: pool.created_at.try(:iso8601),
       updated_at: pool.updated_at.try(:iso8601),
       description: pool.description,

app/models/user_feedback.rb

@@ -2,12 +2,12 @@ class UserFeedback < ApplicationRecord
   self.table_name = "user_feedback"
   belongs_to :user
   belongs_to_creator
-  attribute :disable_dmail_notification, :boolean
+  attr_accessor :disable_dmail_notification
   validates_presence_of :user, :creator, :body, :category
   validates_inclusion_of :category, :in => %w(positive negative neutral)
   validate :creator_is_gold
   validate :user_is_not_creator
-  after_create :create_dmail
+  after_create :create_dmail, unless: :disable_dmail_notification
   after_update(:if => ->(rec) { CurrentUser.id != rec.creator_id}) do |rec|
     ModAction.log(%{#{CurrentUser.name} updated user feedback for "#{rec.user_name}":/users/#{rec.user_id}},:user_feedback_update)
   end
@@ -93,10 +93,8 @@ class UserFeedback < ApplicationRecord
   end
 
   def create_dmail
-    unless disable_dmail_notification
-      body = %{#{disclaimer}@#{creator_name} created a "#{category} record":/user_feedbacks?search[user_id]=#{user_id} for your account:\n\n#{self.body}}
-      Dmail.create_automated(:to_id => user_id, :title => "Your user record has been updated", :body => body)
-    end
+    body = %{#{disclaimer}@#{creator_name} created a "#{category} record":/user_feedbacks?search[user_id]=#{user_id} for your account:\n\n#{self.body}}
+    Dmail.create_automated(:to_id => user_id, :title => "Your user record has been updated", :body => body)
   end
 
   def creator_is_gold

test/factories/note.rb

@@ -7,6 +7,5 @@ FactoryBot.define do
     height 1
     is_active true
     body {FFaker::Lorem.sentences.join(" ")}
-    updater_ip_addr "127.0.0.1"
   end
 end

test/unit/note_test.rb

@@ -85,6 +85,8 @@ class NoteTest < ActiveSupport::TestCase
         assert_equal(@note.body, @note.versions.first.body)
         assert_equal(1, @note.version)
         assert_equal(1, @note.versions.first.version)
+        assert_equal(@user.id, @note.versions.first.updater_id)
+        assert_equal(CurrentUser.ip_addr, @note.versions.first.updater_ip_addr.to_s)
       end
 
       should "update the post's last_noted_at field" do
@@ -141,6 +143,8 @@ class NoteTest < ActiveSupport::TestCase
         assert_equal(2, @note.versions.last.version)
         assert_equal("fafafa", @note.versions.last.body)
         assert_equal(2, @note.version)
+        assert_equal(@user.id, @note.versions.last.updater_id)
+        assert_equal(CurrentUser.ip_addr, @note.versions.last.updater_ip_addr.to_s)
       end
 
       context "for a note-locked post" do

test/unit/pool_test.rb

@@ -252,6 +252,8 @@ class PoolTest < ActiveSupport::TestCase
 
       @pool.reload
       assert_equal(2, @pool.versions.size)
+      assert_equal(user2.id, @pool.versions.last.updater_id)
+      assert_equal("127.0.0.2", @pool.versions.last.updater_ip_addr.to_s)
 
       CurrentUser.scoped(user2, "127.0.0.3") do
         @pool.post_ids = "#{@p1.id} #{@p2.id}"
@@ -260,6 +262,8 @@ class PoolTest < ActiveSupport::TestCase
 
       @pool.reload
       assert_equal(3, @pool.versions.size)
+      assert_equal(user2.id, @pool.versions.last.updater_id)
+      assert_equal("127.0.0.3", @pool.versions.last.updater_ip_addr.to_s)
     end
 
     should "should create a version if the name changes" do

test/unit/user_test.rb

@@ -35,6 +35,7 @@ class UserTest < ActiveSupport::TestCase
         end
 
         assert(@user.dmails.exists?(from: bot, to: @user, title: "You have been promoted"))
+        refute(@user.dmails.exists?(from: bot, to: @user, title: "Your user record has been updated"))
       end
     end