From f5012464abceedacf8a363dea3b133457efbfa8b Mon Sep 17 00:00:00 2001 From: evazion Date: Tue, 30 Oct 2018 14:32:55 -0500 Subject: [PATCH] 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 | 19 +++++++------------ app/models/pool.rb | 6 ++---- app/models/pool_archive.rb | 6 +++--- app/models/user_feedback.rb | 10 ++++------ test/factories/note.rb | 1 - test/unit/note_test.rb | 4 ++++ test/unit/pool_test.rb | 4 ++++ test/unit/user_test.rb | 1 + 8 files changed, 25 insertions(+), 26 deletions(-) diff --git a/app/models/note.rb b/app/models/note.rb index 106795e0d..1c8fdcf81 100644 --- a/app/models/note.rb +++ b/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) diff --git a/app/models/pool.rb b/app/models/pool.rb index 3d516a55c..19b64248b 100644 --- a/app/models/pool.rb +++ b/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 diff --git a/app/models/pool_archive.rb b/app/models/pool_archive.rb index 841bbadd6..463e932a7 100644 --- a/app/models/pool_archive.rb +++ b/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, diff --git a/app/models/user_feedback.rb b/app/models/user_feedback.rb index cf64ad38f..83e7bd755 100644 --- a/app/models/user_feedback.rb +++ b/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 diff --git a/test/factories/note.rb b/test/factories/note.rb index f5c450df0..33a8b533e 100644 --- a/test/factories/note.rb +++ b/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 diff --git a/test/unit/note_test.rb b/test/unit/note_test.rb index 8b9e57331..c5fc05e9b 100644 --- a/test/unit/note_test.rb +++ b/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 diff --git a/test/unit/pool_test.rb b/test/unit/pool_test.rb index 5dd24cb06..5297dbb6f 100644 --- a/test/unit/pool_test.rb +++ b/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 diff --git a/test/unit/user_test.rb b/test/unit/user_test.rb index 77583f622..2b3f23ea5 100644 --- a/test/unit/user_test.rb +++ b/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