Merge pull request #2893 from evazion/fix-automated-dmails

Send automated dmails from DanbooruBot

app/controllers/dmails_controller.rb

@@ -9,7 +9,7 @@ class DmailsController < ApplicationController
       check_privilege(parent)
       @dmail = parent.build_response(:forward => params[:forward])
     else
-      @dmail = Dmail.new(params[:dmail])
+      @dmail = Dmail.new(create_params)
     end
 
     respond_with(@dmail)
@@ -39,7 +39,7 @@ class DmailsController < ApplicationController
   end
 
   def create
-    @dmail = Dmail.create_split(params[:dmail].merge(:creator_ip_addr => request.remote_ip))
+    @dmail = Dmail.create_split(create_params)
     respond_with(@dmail)
   end
 
@@ -66,4 +66,8 @@ private
       raise User::PrivilegeError
     end
   end
+
+  def create_params
+    params.fetch(:dmail, {}).permit(:title, :body, :to_name, :to_id)
+  end
 end

app/helpers/dmails_helper.rb

@@ -2,11 +2,23 @@ module DmailsHelper
   def dmails_current_folder_path
     case cookies[:dmail_folder]
     when "sent"
-      dmails_path(:search => {:owner_id => CurrentUser.id, :from_id => CurrentUser.id}, :folder => "sent")
-    when "all"
-      dmails_path(:search => {:owner_id => CurrentUser.id}, :folder => "all")
+      sent_dmails_path
+    when "received"
+      received_dmails_path
     else
-      dmails_path(:search => {:owner_id => CurrentUser.id, :to_id => CurrentUser.id}, :folder => "received")
+      all_dmails_path
     end
   end
+
+  def all_dmails_path(params = {})
+    dmails_path(folder: "all", **params)
+  end
+
+  def sent_dmails_path(params = {})
+    dmails_path(search: {from_id: CurrentUser.id}, folder: "sent", **params)
+  end
+
+  def received_dmails_path(params = {})
+    dmails_path(search: {to_id: CurrentUser.id}, folder: "received", **params)
+  end
 end

app/logical/approver_pruner.rb

@@ -22,7 +22,7 @@ class ApproverPruner
           user.save
         end
  
-        Dmail.create_split(
+        Dmail.create_automated(
           :to_id => user.id,
           :title => "Approver inactivity",
           :body => "You haven't approved a post in the past three months. In order to make sure the list of active approvers is up-to-date, you have lost your approver privileges. Please reply to this message if you want to be reinstated."

app/logical/user_promotion.rb

@@ -84,7 +84,7 @@ private
   end
 
   def create_dmail
-    Dmail.create_split(
+    Dmail.create_automated(
       :to_id => user.id,
       :title => "You have been promoted",
       :body => build_messages

app/models/dmail.rb

@@ -1,19 +1,22 @@
 require 'digest/sha1'
 
 class Dmail < ActiveRecord::Base
-  validates_presence_of :to_id
-  validates_presence_of :from_id
-  validates_format_of :title, :with => /\S/
-  validates_format_of :body, :with => /\S/
-  validate :validate_sender_is_not_banned
-  before_validation :initialize_from_id, :on => :create
+  with_options on: :create do
+    validates_presence_of :to_id
+    validates_presence_of :from_id
+    validates_format_of :title, :with => /\S/
+    validates_format_of :body, :with => /\S/
+    validate :validate_sender_is_not_banned
+  end
+
   belongs_to :owner, :class_name => "User"
   belongs_to :to, :class_name => "User"
   belongs_to :from, :class_name => "User"
+
+  after_initialize :initialize_attributes, if: :new_record?
   before_create :auto_read_if_filtered
   after_create :update_recipient
   after_create :send_dmail
-  attr_accessible :title, :body, :is_deleted, :to_id, :to, :to_name, :creator_ip_addr
 
   module AddressMethods
     def to_name
@@ -25,13 +28,12 @@ class Dmail < ActiveRecord::Base
     end
 
     def to_name=(name)
-      user = User.find_by_name(name)
-      return if user.nil?
-      self.to_id = user.id
+      self.to_id = User.name_to_id(name)
     end
 
-    def initialize_from_id
-      self.from_id = CurrentUser.id
+    def initialize_attributes
+      self.from_id ||= CurrentUser.id
+      self.creator_ip_addr ||= CurrentUser.ip_addr
     end
   end
 
@@ -43,14 +45,14 @@ class Dmail < ActiveRecord::Base
         copy = nil
 
         Dmail.transaction do
+          # recipient's copy
           copy = Dmail.new(params)
           copy.owner_id = copy.to_id
-          unless copy.to_id == CurrentUser.id
-            copy.save
-          end
+          copy.save unless copy.to_id == copy.from_id
 
+          # sender's copy
           copy = Dmail.new(params)
-          copy.owner_id = CurrentUser.id
+          copy.owner_id = copy.from_id
           copy.is_read = true
           copy.save
         end
@@ -58,10 +60,8 @@ class Dmail < ActiveRecord::Base
         copy
       end
 
-      def new_blank
-        Dmail.new do |dmail|
-          dmail.from_id = CurrentUser.id
-        end
+      def create_automated(params)
+        create_split(from: Danbooru.config.system_user, **params)
       end
     end
 
@@ -91,18 +91,6 @@ class Dmail < ActiveRecord::Base
   end
   
   module SearchMethods
-    def for(user)
-      where("owner_id = ?", user)
-    end
-
-    def inbox
-      where("to_id = owner_id")
-    end
-
-    def sent
-      where("from_id = owner_id")
-    end
-
     def active
       where("is_deleted = ?", false)
     end
@@ -144,10 +132,6 @@ class Dmail < ActiveRecord::Base
         q = q.search_message(params[:message_matches])
       end
 
-      if params[:owner_id].present?
-        q = q.for(params[:owner_id].to_i)
-      end
-
       if params[:to_name].present?
         q = q.to_name_matches(params[:to_name])
       end
@@ -206,6 +190,10 @@ class Dmail < ActiveRecord::Base
     end
   end
 
+  def is_automated?
+    from == Danbooru.config.system_user
+  end
+
   def filtered?
     CurrentUser.dmail_filter.try(:filtered?, self)
   end

app/models/janitor_trial.rb

@@ -73,7 +73,7 @@ class JanitorTrial < ActiveRecord::Base
   def send_dmail
     body = "You have been selected as a test janitor. You can now approve pending posts and have access to the moderation interface. You should reacquaint yourself with the [[howto:upload]] guide to make sure you understand the site rules.\n\nOver the next several weeks your approvals will be monitored. If the majority of them are not quality uploads you will fail the trial period and lose your approval privileges. You will also receive a negative user record indicating you previously attempted and failed a test janitor trial.\n\nThere is a minimum quota of 1 approval a month to indicate that you are being active. Remember, the goal isn't to approve as much as possible. It's to filter out borderline-quality art.\n\nIf you have any questions please respond to this message."
 
-    Dmail.create_split(:title => "Test Janitor Trial Period", :body => body, :to_id => user_id)
+    Dmail.create_automated(:title => "Test Janitor Trial Period", :body => body, :to_id => user_id)
   end
 
   def promote_user

app/models/post_disapproval.rb

@@ -17,27 +17,20 @@ class PostDisapproval < ActiveRecord::Base
   end
 
   def self.dmail_messages!
-    admin = User.admins.first
-    disapprovals = {}
-
-    PostDisapproval.with_message.where("created_at >= ?", 1.day.ago).find_each do |disapproval|
-      disapprovals[disapproval.post.uploader_id] ||= []
-      disapprovals[disapproval.post.uploader_id] << disapproval
+    disapprovals = PostDisapproval.with_message.where("created_at >= ?", 1.day.ago).group_by do |pd|
+      pd.post.uploader
     end
 
-    disapprovals.each do |user_id, list|
-      user = User.find(user_id)
-      CurrentUser.scoped(admin, "127.0.0.1") do
-        message = list.map do |x|
-          "* post ##{x.post_id}: #{x.message}"
-        end.join("\n")
+    disapprovals.each do |uploader, list|
+      message = list.map do |x|
+        "* post ##{x.post_id}: #{x.message}"
+      end.join("\n")
 
-        Dmail.create_split(
-          :to_id => user.id,
-          :title => "Some of your uploads have been critiqued by the moderators",
-          :body => message
-        )
-      end
+      Dmail.create_automated(
+        :to_id => uploader.id,
+        :title => "Some of your uploads have been critiqued by the moderators",
+        :body => message
+      )
     end
   end
 

app/models/user_feedback.rb

@@ -92,7 +92,7 @@ class UserFeedback < ActiveRecord::Base
   def create_dmail
     unless disable_dmail_notification
       body = %{#{creator_name} created a "#{category} record":/user_feedbacks?search[user_id]=#{user_id} for your account. #{body}}
-      Dmail.create_split(:to_id => user_id, :title => "Your user record has been updated", :body => body)
+      Dmail.create_automated(:to_id => user_id, :title => "Your user record has been updated", :body => body)
     end
   end
 

app/models/user_name_change_request.rb

@@ -8,7 +8,6 @@ class UserNameChangeRequest < ActiveRecord::Base
   validates_length_of :desired_name, :within => 2..100, :on => :create
   validates_format_of :desired_name, :with => /\A[^\s:]+\Z/, :on => :create, :message => "cannot have whitespace or colons"
   before_validation :normalize_name
-  # after_create :notify_admins
   attr_accessible :status, :user_id, :original_name, :desired_name, :change_reason, :rejection_reason, :approver_id
   
   def self.pending
@@ -49,19 +48,11 @@ class UserNameChangeRequest < ActiveRecord::Base
     UserFeedback.for_user(user_id).order("id desc")
   end
   
-  def notify_admins
-    title = "#{original_name} is requesting a name change to #{desired_name}"
-    body = title + "\n\n\"See request\":/user_name_change_requests/#{id}"
-    User.admins.find_each do |user|
-      Dmail.create_split(:title => title, :body => body, :to_id => user.id)
-    end
-  end
-  
   def approve!
     update_attributes(:status => "approved", :approver_id => CurrentUser.user.id)
     user.update_attribute(:name, desired_name)
     body = "Your name change request has been approved. Be sure to log in with your new user name."
-    Dmail.create_split(:title => "Name change request approved", :body => body, :to_id => user_id)
+    Dmail.create_automated(:title => "Name change request approved", :body => body, :to_id => user_id)
     UserFeedback.create(:user_id => user_id, :category => "neutral", :body => "Name changed from #{original_name} to #{desired_name}")
     ModAction.log("Name changed from #{original_name} to #{desired_name}")
   end
@@ -69,7 +60,7 @@ class UserNameChangeRequest < ActiveRecord::Base
   def reject!(reason)
     update_attributes(:status => "rejected", :rejection_reason => reason)
     body = "Your name change request has been rejected for the following reason: #{rejection_reason}"
-    Dmail.create_split(:title => "Name change request rejected", :body => body, :to_id => user_id)
+    Dmail.create_automated(:title => "Name change request rejected", :body => body, :to_id => user_id)
   end
   
   def not_limited

app/views/dmails/_secondary_links.html.erb

@@ -1,11 +1,14 @@
 <% content_for(:secondary_links) do %>
   <menu>
     <li><%= render "quick_search" %></li>
-    <li><%= link_to "All", dmails_path(:search => {:owner_id => CurrentUser.id}, :folder => "all", :set_default_folder => true) %></li>
-    <li><%= link_to "Received", dmails_path(:search => {:owner_id => CurrentUser.id, :to_id => CurrentUser.id}, :folder => "received", :set_default_folder => true) %></li>
-    <li><%= link_to "Sent", dmails_path(:search => {:owner_id => CurrentUser.id, :from_id => CurrentUser.id}, :folder => "sent", :set_default_folder => true) %></li>
+    <li><%= link_to "All", all_dmails_path(set_default_folder: true) %></li>
+    <li><%= link_to "Received", received_dmails_path(set_default_folder: true) %></li>
+    <li><%= link_to "Sent", sent_dmails_path(set_default_folder: true) %></li>
+    <li>|</li>
     <li><%= link_to "New", new_dmail_path %></li>
     <li><%= link_to "Search", search_dmails_path %></li>
     <li><%= link_to "Mark all as read", {:controller => "dmails", :action => "mark_all_as_read"}, :method => :post, :remote => true %></li>
+    <li>|</li>
+    <li><%= link_to "Help", wiki_pages_path(title: "help:dmail") %></li>
   </menu>
 <% end %>

app/views/dmails/edit.html.erb

@@ -1,12 +0,0 @@
-<div id="c-dmails">
-  <div id="a-edit">
-    <h1>Edit Message</h1>
-    <%= render "form", :dmail => @dmail %>
-  </div>
-</div>
-
-<%= render "secondary_links" %>
-
-<% content_for(:page_title) do %>
-  Edit Message - <%= Danbooru.config.app_name %>
-<% end %>

app/views/dmails/show.html.erb

@@ -16,6 +16,12 @@
       <h3>Body</h3>
       <div class="prose">
         <%= format_text(@dmail.body, :ragel => true) %>
+
+        <% if @dmail.is_automated? %>
+          <p class="tn">
+            This is an automated message. Post in the forums if you have any questions.
+          </p>
+        <% end %>
       </div>
 
       <p>

config/danbooru_default_config.rb

@@ -30,6 +30,11 @@ module Danbooru
       "webmaster@#{server_host}"
     end
 
+    # System actions, such as sending automated dmails, will be performed with this account.
+    def system_user
+      User.find_by_name("DanbooruBot") || User.admins.first
+    end
+
     def upgrade_account_email
       contact_email
     end

config/routes.rb

@@ -110,7 +110,7 @@ Rails.application.routes.draw do
     end
   end
   resources :delayed_jobs, :only => [:index]
-  resources :dmails do
+  resources :dmails, :only => [:new, :create, :index, :show, :destroy] do
     collection do
       get :search
       post :mark_all_as_read

test/factories/post_disapproval.rb

@@ -0,0 +1,6 @@
+FactoryGirl.define do
+  factory(:post_disapproval) do
+    reason { %w(breaks_rules poor_quality disinterest).sample }
+    message { FFaker::Lorem.sentence }
+  end
+end

test/functional/dmails_controller_test.rb

@@ -48,17 +48,17 @@ class DmailsControllerTest < ActionController::TestCase
 
     context "index action" do
       should "show dmails owned by the current user" do
-        get :index, {:owner_id_equals => @dmail.owner_id, :folder => "sent"}, {:user_id => @dmail.owner_id}
+        get :index, {:search => {:owner_id => @dmail.owner_id, :folder => "sent"}}, {:user_id => @dmail.owner_id}
         assert_response :success
         assert_equal(1, assigns[:dmails].size)
 
-        get :index, {:owner_id_equals => @dmail.owner_id, :folder => "received"}, {:user_id => @dmail.owner_id}
+        get :index, {:search => {:owner_id => @dmail.owner_id, :folder => "received"}}, {:user_id => @dmail.owner_id}
         assert_response :success
         assert_equal(1, assigns[:dmails].size)
       end
 
       should "not show dmails not owned by the current user" do
-        get :index, {:owner_id_equals => @dmail.owner_id}, {:user_id => @unrelated_user.id}
+        get :index, {:search => {:owner_id => @dmail.owner_id}}, {:user_id => @unrelated_user.id}
         assert_response :success
         assert_equal(0, assigns[:dmails].size)
       end

test/unit/post_disapproval_test.rb

@@ -64,6 +64,33 @@ class PostDisapprovalTest < ActiveSupport::TestCase
           end
         end
       end
+
+      context "when sending dmails" do
+        setup do
+          @uploaders = FactoryGirl.create_list(:user, 2)
+          @disapprovers = FactoryGirl.create_list(:mod_user, 2)
+
+          # 2 uploaders, with 2 uploads each, and 2 disapprovals on each upload.
+          @uploaders.each do |uploader|
+            FactoryGirl.create_list(:post, 2, uploader: uploader).each do |post|
+              FactoryGirl.create(:post_disapproval, post: post, user: @disapprovers[0])
+              FactoryGirl.create(:post_disapproval, post: post, user: @disapprovers[1])
+            end
+          end
+        end
+
+        should "dmail the uploaders" do
+          bot = FactoryGirl.create(:user)
+          Danbooru.config.stubs(:system_user).returns(bot)
+
+          assert_difference(["@uploaders[0].dmails.count", "@uploaders[1].dmails.count"], 1) do
+            PostDisapproval.dmail_messages!
+          end
+
+          assert(@uploaders[0].dmails.exists?(from: bot, to: @uploaders[0]))
+          assert(@uploaders[1].dmails.exists?(from: bot, to: @uploaders[1]))
+        end
+      end
     end
   end
 end

test/unit/user_test.rb

@@ -13,7 +13,7 @@ class UserTest < ActiveSupport::TestCase
       CurrentUser.user = nil
       CurrentUser.ip_addr = nil
     end
-    
+
     context "promoting a user" do
       setup do
         CurrentUser.user = FactoryGirl.create(:moderator_user)
@@ -27,10 +27,16 @@ class UserTest < ActiveSupport::TestCase
         assert_equal("You have been promoted to a Gold level account from Member.", @user.feedback.last.body)
       end
 
-      should "create a dmail" do
+      should "send an automated dmail to the user" do
+        bot = FactoryGirl.create(:user)
+        Danbooru.config.stubs(:system_user).returns(bot)
+
         assert_difference("Dmail.count", 2) do
           @user.promote_to!(User::Levels::GOLD)
         end
+
+        assert(@user.dmails.exists?(from: bot, to: @user, title: "You have been promoted"))
+        assert(bot.dmails.exists?(from: bot, to: @user, title: "You have been promoted"))
       end
     end