dmails: allow marking dmails as unread.

* Add ability to mark dmails as unread.
* Fix users.unread_dmail_count to not count deleted dmails.
* Fix show action so that API calls don't mark dmails as read.
* Don't show the unread dmail notice on the /dmails page itself.
* Stop using users.has_mail flag.

app/controllers/dmails_controller.rb

@@ -22,7 +22,7 @@ class DmailsController < ApplicationController
   def show
     @dmail = Dmail.find(params[:id])
     check_privilege(@dmail)
-    @dmail.mark_as_read!
+    @dmail.update!(is_read: true) if request.format.html?
     respond_with(@dmail)
   end
 
@@ -41,10 +41,8 @@ class DmailsController < ApplicationController
   end
 
   def mark_all_as_read
-    Dmail.visible.unread.each do |x|
+    @dmails = CurrentUser.user.dmails.mark_all_as_read
-      x.update_column(:is_read, true)
+    respond_with(@dmails)
-    end
-    CurrentUser.user.update(has_mail: false, unread_dmail_count: 0)
   end
 
   private

app/helpers/users_helper.rb

@@ -1,4 +1,16 @@
 module UsersHelper
+  def unread_dmail_indicator(user)
+    "(#{user.unread_dmail_count})" if user.unread_dmail_count > 0
+  end
+
+  def has_unread_dmails?(user)
+    user.unread_dmail_count > 0 && (cookies[:hide_dmail_notice].to_i < latest_unread_dmail(user).id)
+  end
+
+  def latest_unread_dmail(user)
+    user.dmails.active.unread.first
+  end
+
   def email_sig(user)
     verifier = ActiveSupport::MessageVerifier.new(Danbooru.config.email_key, serializer: JSON, digest: "SHA256")
     verifier.generate(user.id.to_s)

app/models/dmail.rb

@@ -14,7 +14,7 @@ class Dmail < ApplicationRecord
   belongs_to :from, :class_name => "User"
 
   after_initialize :initialize_attributes, if: :new_record?
-  after_create :update_recipient
+  after_save :update_unread_dmail_count
   after_commit :send_email, on: :create
 
   api_attributes including: [:key]
@@ -152,6 +152,10 @@ class Dmail < ApplicationRecord
   include FactoryMethods
   extend SearchMethods
 
+  def self.mark_all_as_read
+    unread.update(is_read: true)
+  end
+
   def validate_sender_is_not_banned
     if from.try(:is_banned?)
       errors[:base] << "Sender is banned and cannot send messages"
@@ -168,13 +172,6 @@ class Dmail < ApplicationRecord
     end
   end
 
-  def mark_as_read!
-    update_column(:is_read, true)
-    owner.dmails.unread.count.tap do |unread_count|
-      owner.update(has_mail: (unread_count > 0), unread_dmail_count: unread_count)
-    end
-  end
-
   def is_automated?
     from == User.system
   end
@@ -187,9 +184,12 @@ class Dmail < ApplicationRecord
     owner == to
   end
 
-  def update_recipient
+  def update_unread_dmail_count
-    if is_recipient? && !is_deleted? && !is_read?
+    return unless saved_change_to_id? || saved_change_to_is_read? || saved_change_to_is_deleted?
-      to.update(has_mail: true, unread_dmail_count: to.dmails.unread.count)
+
+    owner.with_lock do
+      unread_count = owner.dmails.active.unread.count
+      owner.update!(unread_dmail_count: unread_count)
     end
   end
 

app/models/user.rb

@@ -35,6 +35,7 @@ class User < ApplicationRecord
   # - has_saved_searches
   # - opt_out_tracking
   # - enable_recommended_posts
+  # - has_mail
   BOOLEAN_ATTRIBUTES = %w(
     is_banned
     has_mail
@@ -798,14 +799,6 @@ class User < ApplicationRecord
     CurrentUser.as(self, &block)
   end
 
-  def dmail_count
-    if has_mail?
-      "(#{unread_dmail_count})"
-    else
-      ""
-    end
-  end
-
   def reportable_by?(user)
     ModerationReport.enabled? && user.is_builder? && id != user.id && !is_moderator?
   end

app/views/dmails/index.html.erb

@@ -25,11 +25,18 @@
         <%= link_to dmail.title, dmail_path(dmail) %>
       <% end %>
       <% t.column do |dmail| %>
+        <% if dmail.is_read? %>
+          <%= link_to "Unread", dmail_path(dmail, format: :js), remote: true, method: :put, "data-params": "dmail[is_read]=false" %>
+        <% else %>
+          <%= link_to "Read", dmail_path(dmail, format: :js), remote: true, method: :put, "data-params": "dmail[is_read]=true" %>
+        <% end %>
+        |
         <% if dmail.is_deleted? %>
           <%= link_to "Undelete", dmail_path(dmail, format: :js), remote: true, method: :put, "data-params": "dmail[is_deleted]=false" %>
         <% else %>
           <%= link_to "Delete", dmail_path(dmail, format: :js), remote: true, method: :put, "data-params": "dmail[is_deleted]=true", "data-confirm": "Are you sure you want to delete this dmail?" %>
         <% end %>
+
         <% if dmail.reportable_by?(CurrentUser.user) %>
           | <%= link_to "Report", new_moderation_report_path(moderation_report: { model_type: "Dmail", model_id: dmail.id }), remote: true, title: "Report this dmail to the moderators" %>
         <% end %>

app/views/layouts/_main_links.html.erb

@@ -2,7 +2,7 @@
   <% if CurrentUser.is_anonymous? %>
     <%= nav_link_to("Login", login_path) %>
   <% else %>
-    <%= nav_link_to("My Account #{CurrentUser.dmail_count}", profile_path) %>
+    <%= nav_link_to("My Account #{unread_dmail_indicator(CurrentUser.user)}", profile_path) %>
   <% end %>
   <%= nav_link_to("Posts", posts_path) %>
   <%= nav_link_to("Comments", comments_path(:group_by => "post")) %>

app/views/layouts/default.html.erb

@@ -118,7 +118,7 @@
       <%= render "users/ban_notice" %>
     <% end %>
 
-    <% if CurrentUser.has_mail? && CurrentUser.dmails.unread.first.present? && (cookies[:hide_dmail_notice].blank? || cookies[:hide_dmail_notice].to_i < CurrentUser.dmails.unread.first.id) %>
+    <% if params[:controller] != "dmails" && has_unread_dmails?(CurrentUser.user) %>
       <%= render "users/dmail_notice" %>
     <% end %>
 

app/views/static/site_map.html.erb

@@ -127,7 +127,7 @@
             <li><%= link_to "Change name", new_user_name_change_request_path %></li>
           <% end %>
           <li><%= link_to "Delete account", maintenance_user_deletion_path %></li>
-          <li><%= link_to "Dmails", dmails_path %></li>
+          <li><%= link_to "Dmails", dmails_path(search: { folder: "received" }) %></li>
           <li><%= link_to "Favorites", favorites_path %></li>
           <li><%= link_to "Favorite groups", favorite_groups_path %></li>
           <li><%= link_to "Saved searches", saved_searches_path %></li>

app/views/users/_dmail_notice.html.erb

@@ -1,4 +1,4 @@
-<div class="notice notice-info notice-large" id="dmail-notice" data-id="<%= CurrentUser.dmails.unread.first.id %>">
+<div class="notice notice-info notice-large" id="dmail-notice" data-id="<%= latest_unread_dmail(CurrentUser.user) %>">
-  <h2>You have <%= link_to "unread mail", dmails_path(search: {owner_id: CurrentUser.id, to_id: CurrentUser.id}, folder: "received") %>.</h2>
+  <h2>You have <%= link_to "unread mail", dmails_path(search: { folder: "received" }) %>.</h2>
   <div><%= link_to "Close this", "#", id: "hide-dmail-notice" %></div>
 </div>

app/views/users/_secondary_links.html.erb

@@ -12,7 +12,7 @@
     <% if @user.id == CurrentUser.user.id %>
       <%= subnav_link_to "Profile", profile_path %>
       <%= subnav_link_to "Settings", settings_path %>
-      <%= subnav_link_to "Messages #{CurrentUser.user.dmail_count}", dmails_path %>
+      <%= subnav_link_to "Messages #{unread_dmail_indicator(CurrentUser.user)}", dmails_path(search: { folder: "received" }) %>
 
       <% if !@user.is_platinum? %>
         <%= subnav_link_to "Upgrade", new_user_upgrade_path %>

test/functional/dmails_controller_test.rb

@@ -3,7 +3,7 @@ require 'test_helper'
 class DmailsControllerTest < ActionDispatch::IntegrationTest
   context "The dmails controller" do
     setup do
-      @user = create(:user)
+      @user = create(:user, unread_dmail_count: 1)
       @unrelated_user = create(:user)
       as_user do
         @dmail = create(:dmail, :owner => @user)
@@ -78,6 +78,22 @@ class DmailsControllerTest < ActionDispatch::IntegrationTest
         get_auth dmail_path(@dmail), @unrelated_user
         assert_response(403)
       end
+
+      should "mark dmails as read" do
+        assert_equal(false, @dmail.is_read)
+        get_auth dmail_path(@dmail), @dmail.owner
+
+        assert_response :success
+        assert_equal(true, @dmail.reload.is_read)
+      end
+
+      should "not mark dmails as read in the api" do
+        assert_equal(false, @dmail.is_read)
+        get_auth dmail_path(@dmail, format: :json), @dmail.owner
+
+        assert_response :success
+        assert_equal(false, @dmail.reload.is_read)
+      end
     end
 
     context "create action" do
@@ -108,6 +124,51 @@ class DmailsControllerTest < ActionDispatch::IntegrationTest
         assert_response 403
         assert_equal(false, @dmail.reload.is_deleted)
       end
+
+      should "update user's unread_dmail_count when marking dmails as read or unread" do
+        put_auth dmail_path(@dmail), @user, params: { dmail: { is_read: true } }
+        assert_equal(true, @dmail.reload.is_read)
+        assert_equal(0, @user.reload.unread_dmail_count)
+
+        put_auth dmail_path(@dmail), @user, params: { dmail: { is_read: false } }
+        assert_equal(false, @dmail.reload.is_read)
+        assert_equal(1, @user.reload.unread_dmail_count)
+      end
+    end
+
+    context "mark all as read action" do
+      setup do
+        @sender = create(:user)
+        @recipient = create(:user)
+
+        as(@sender) do
+          @dmail1 = Dmail.create_split(title: "test1", body: "test", to: @recipient)
+          @dmail2 = Dmail.create_split(title: "test2", body: "test", to: @recipient)
+          @dmail3 = Dmail.create_split(title: "test3", body: "test", to: @recipient, is_read: 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
+        assert_equal(4, @recipient.dmails.count)
+        assert_equal(2, @recipient.dmails.active.unread.count)
+        assert_equal(2, @recipient.reload.unread_dmail_count)
+        post_auth mark_all_as_read_dmails_path(format: :js), @recipient
+
+        assert_response :success
+        assert_equal(0, @recipient.reload.unread_dmail_count)
+        assert_equal(true, [@dmail1, @dmail2, @dmail3, @dmail4].all?(&:is_read))
+      end
+    end
+
+    context "when a user has unread dmails" do
+      should "show the unread dmail notice" do
+        get_auth posts_path, @user
+
+        assert_response :success
+        assert_select "#dmail-notice", 1
+        assert_select "#nav-my-account-link", text: "My Account (1)"
+      end
     end
   end
 end

test/unit/dmail_test.rb

@@ -123,28 +123,14 @@ class DmailTest < ActiveSupport::TestCase
       end
     end
 
-    should "be marked as read after the user reads it" do
-      dmail = FactoryBot.create(:dmail, :owner => @user)
-      assert(!dmail.is_read?)
-      dmail.mark_as_read!
-      assert(dmail.is_read?)
-    end
-
     should "notify the recipient he has mail" do
-      recipient = FactoryBot.create(:user)
+      recipient = create(:user)
+
       Dmail.create_split(title: "hello", body: "hello", to_id: recipient.id)
-      dmail = Dmail.where(owner_id: recipient.id).last
+      assert_equal(1, recipient.reload.unread_dmail_count)
-      recipient.reload
-      assert(recipient.has_mail?)
-      assert_equal(1, recipient.unread_dmail_count)
 
-      CurrentUser.scoped(recipient) do
+      recipient.dmails.unread.last.update!(is_read: true)
-        dmail.mark_as_read!
+      assert_equal(0, recipient.reload.unread_dmail_count)
-      end
-
-      recipient.reload
-      refute(recipient.has_mail?)
-      assert_equal(0, recipient.unread_dmail_count)
     end
 
     context "that is automated" do