dmails: set sender name and ip address explicitly.

Set the sender name and IP addresses explicitly in the controller rather
than implicitly in the model.

Fixes cases where automated dmails from DanbooruBot had their IP
addresses set to the person who triggered the dmail, even though they
didn't actually send the dmail themselves.

app/controllers/dmails_controller.rb

@@ -33,7 +33,7 @@ class DmailsController < ApplicationController
   end
 
   def create
-    @dmail = Dmail.create_split(dmail_params(:create))
+    @dmail = Dmail.create_split(from: CurrentUser.user, creator_ip_addr: CurrentUser.ip_addr, **dmail_params(:create))
     respond_with(@dmail)
   end
 

app/models/dmail.rb

@@ -10,7 +10,6 @@ class Dmail < ApplicationRecord
   belongs_to :from, :class_name => "User"
   has_many :moderation_reports, as: :model
 
-  after_initialize :initialize_attributes, if: :new_record?
   before_create :autoreport_spam
   after_save :update_unread_dmail_count
   after_commit :send_email, on: :create
@@ -28,11 +27,6 @@ class Dmail < ApplicationRecord
     def to_name=(name)
       self.to = User.find_by_name(name)
     end
-
-    def initialize_attributes
-      self.from_id ||= CurrentUser.id
-      self.creator_ip_addr ||= CurrentUser.ip_addr
-    end
   end
 
   module FactoryMethods
@@ -59,12 +53,10 @@ class Dmail < ApplicationRecord
       end
 
       def create_automated(params)
-        CurrentUser.as_system do
+        dmail = Dmail.new(from: User.system, creator_ip_addr: "127.0.0.1", **params)
-          dmail = Dmail.new(from: User.system, **params)
+        dmail.owner = dmail.to
-          dmail.owner = dmail.to
+        dmail.save
-          dmail.save
+        dmail
-          dmail
-        end
       end
     end
 

test/functional/dmails_controller_test.rb

@@ -166,12 +166,10 @@ class DmailsControllerTest < ActionDispatch::IntegrationTest
         @sender = create(:user)
         @recipient = create(:user)
 
-        as(@sender) do
+        @dmail1 = create(:dmail, from: @sender, owner: @recipient, to: @recipient)
-          @dmail1 = Dmail.create_split(title: "test1", body: "test", to: @recipient)
+        @dmail2 = create(:dmail, from: @sender, owner: @recipient, to: @recipient)
-          @dmail2 = Dmail.create_split(title: "test2", body: "test", to: @recipient)
+        @dmail3 = create(:dmail, from: @sender, owner: @recipient, to: @recipient, is_read: true)
-          @dmail3 = Dmail.create_split(title: "test3", body: "test", to: @recipient, is_read: true)
+        @dmail4 = create(:dmail, from: @sender, owner: @recipient, to: @recipient, is_deleted: true)
-          @dmail4 = Dmail.create_split(title: "test4", body: "test", to: @recipient, is_deleted: true)
-        end
       end
 
       should "mark all unread, undeleted dmails as read" do
@@ -182,7 +180,7 @@ class DmailsControllerTest < ActionDispatch::IntegrationTest
 
         assert_response :success
         assert_equal(0, @recipient.reload.unread_dmail_count)
-        assert_equal(true, [@dmail1, @dmail2, @dmail3, @dmail4].all?(&:is_read))
+        assert_equal(true, [@dmail1, @dmail2, @dmail3, @dmail4].map(&:reload).all?(&:is_read))
       end
     end
 

test/unit/dmail_test.rb

@@ -29,14 +29,11 @@ class DmailTest < ActiveSupport::TestCase
     end
 
     context "from a banned user" do
-      setup do
-        @user.update_attribute(:is_banned, true)
-      end
-
       should "not validate" do
-        dmail = FactoryBot.build(:dmail, :title => "xxx", :owner => @user)
+        user = create(:banned_user)
-        dmail.save
+        dmail = build(:dmail, owner: user, from: user)
-        assert_equal(1, dmail.errors.size)
+
+        assert_equal(false, dmail.valid?)
         assert_equal(["Sender is banned and cannot send messages"], dmail.errors.full_messages)
       end
     end
@@ -85,15 +82,10 @@ class DmailTest < ActiveSupport::TestCase
     should "create a copy for each user" do
       @new_user = FactoryBot.create(:user)
       assert_difference("Dmail.count", 2) do
-        Dmail.create_split(:to_id => @new_user.id, :title => "foo", :body => "foo")
+        Dmail.create_split(from: CurrentUser.user, creator_ip_addr: "127.0.0.1", to_id: @new_user.id, title: "foo", body: "foo")
       end
     end
 
-    should "record the creator's ip addr" do
-      dmail = FactoryBot.create(:dmail, owner: @user)
-      assert_equal(CurrentUser.ip_addr, dmail.creator_ip_addr.to_s)
-    end
-
     should "send an email if the user wants it" do
       user = create(:user, receive_email_notifications: true)
       assert_difference("ActionMailer::Base.deliveries.size", 1) do
@@ -104,14 +96,14 @@ class DmailTest < ActiveSupport::TestCase
     should "create only one message for a split response" do
       user = FactoryBot.create(:user, :receive_email_notifications => true)
       assert_difference("ActionMailer::Base.deliveries.size", 1) do
-        Dmail.create_split(:to_id => user.id, :title => "foo", :body => "foo")
+        Dmail.create_split(from: CurrentUser.user, creator_ip_addr: "127.0.0.1", to_id: user.id, title: "foo", body: "foo")
       end
     end
 
     should "notify the recipient he has mail" do
       recipient = create(:user)
 
-      Dmail.create_split(title: "hello", body: "hello", to_id: recipient.id)
+      create(:dmail, owner: recipient, to: recipient)
       assert_equal(1, recipient.reload.unread_dmail_count)
 
       recipient.dmails.unread.last.update!(is_read: true)