From 6468df6d44feff10bcb07593d26a57d5da7592b0 Mon Sep 17 00:00:00 2001 From: evazion Date: Fri, 31 Jan 2020 15:47:58 -0600 Subject: [PATCH] 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 | 8 ++- app/helpers/users_helper.rb | 12 +++++ app/models/dmail.rb | 22 ++++---- app/models/user.rb | 9 +--- app/views/dmails/index.html.erb | 7 +++ app/views/layouts/_main_links.html.erb | 2 +- app/views/layouts/default.html.erb | 2 +- app/views/static/site_map.html.erb | 2 +- app/views/users/_dmail_notice.html.erb | 4 +- app/views/users/_secondary_links.html.erb | 2 +- test/functional/dmails_controller_test.rb | 63 ++++++++++++++++++++++- test/unit/dmail_test.rb | 24 ++------- 12 files changed, 107 insertions(+), 50 deletions(-) diff --git a/app/controllers/dmails_controller.rb b/app/controllers/dmails_controller.rb index 87421bd02..32f347476 100644 --- a/app/controllers/dmails_controller.rb +++ b/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| - x.update_column(:is_read, true) - end - CurrentUser.user.update(has_mail: false, unread_dmail_count: 0) + @dmails = CurrentUser.user.dmails.mark_all_as_read + respond_with(@dmails) end private diff --git a/app/helpers/users_helper.rb b/app/helpers/users_helper.rb index 5ba80cbf3..8b17b7efc 100644 --- a/app/helpers/users_helper.rb +++ b/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) diff --git a/app/models/dmail.rb b/app/models/dmail.rb index 8c19b8fa4..956fc022e 100644 --- a/app/models/dmail.rb +++ b/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 - if is_recipient? && !is_deleted? && !is_read? - to.update(has_mail: true, unread_dmail_count: to.dmails.unread.count) + def update_unread_dmail_count + return unless saved_change_to_id? || saved_change_to_is_read? || saved_change_to_is_deleted? + + owner.with_lock do + unread_count = owner.dmails.active.unread.count + owner.update!(unread_dmail_count: unread_count) end end diff --git a/app/models/user.rb b/app/models/user.rb index 4e1c96c40..aa179ff28 100644 --- a/app/models/user.rb +++ b/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 diff --git a/app/views/dmails/index.html.erb b/app/views/dmails/index.html.erb index 8f8e14a7b..02c582b2f 100644 --- a/app/views/dmails/index.html.erb +++ b/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 %> diff --git a/app/views/layouts/_main_links.html.erb b/app/views/layouts/_main_links.html.erb index 42f8a6492..32ebfd521 100644 --- a/app/views/layouts/_main_links.html.erb +++ b/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")) %> diff --git a/app/views/layouts/default.html.erb b/app/views/layouts/default.html.erb index 13de838a3..f0abf5cee 100644 --- a/app/views/layouts/default.html.erb +++ b/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 %> diff --git a/app/views/static/site_map.html.erb b/app/views/static/site_map.html.erb index 8927ab60b..6cd8af1fc 100644 --- a/app/views/static/site_map.html.erb +++ b/app/views/static/site_map.html.erb @@ -127,7 +127,7 @@
  • <%= link_to "Change name", new_user_name_change_request_path %>
    • <% end %>
  • <%= link_to "Delete account", maintenance_user_deletion_path %>
    • -
  • <%= link_to "Dmails", dmails_path %>
    • +
  • <%= link_to "Dmails", dmails_path(search: { folder: "received" }) %>
  • <%= link_to "Favorites", favorites_path %>
  • <%= link_to "Favorite groups", favorite_groups_path %>
  • <%= link_to "Saved searches", saved_searches_path %>
    • diff --git a/app/views/users/_dmail_notice.html.erb b/app/views/users/_dmail_notice.html.erb index 343431123..2c73500ab 100644 --- a/app/views/users/_dmail_notice.html.erb +++ b/app/views/users/_dmail_notice.html.erb @@ -1,4 +1,4 @@ -
    -

    You have <%= link_to "unread mail", dmails_path(search: {owner_id: CurrentUser.id, to_id: CurrentUser.id}, folder: "received") %>.

    +
    +

    You have <%= link_to "unread mail", dmails_path(search: { folder: "received" }) %>.

    <%= link_to "Close this", "#", id: "hide-dmail-notice" %>
    diff --git a/app/views/users/_secondary_links.html.erb b/app/views/users/_secondary_links.html.erb index e8141ae8d..468266d5d 100644 --- a/app/views/users/_secondary_links.html.erb +++ b/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 %> diff --git a/test/functional/dmails_controller_test.rb b/test/functional/dmails_controller_test.rb index 8c382c25d..eb2942003 100644 --- a/test/functional/dmails_controller_test.rb +++ b/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 diff --git a/test/unit/dmail_test.rb b/test/unit/dmail_test.rb index ab7fccc19..afbf9d1b9 100644 --- a/test/unit/dmail_test.rb +++ b/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 - recipient.reload - assert(recipient.has_mail?) - assert_equal(1, recipient.unread_dmail_count) + assert_equal(1, recipient.reload.unread_dmail_count) - CurrentUser.scoped(recipient) do - dmail.mark_as_read! - end - - recipient.reload - refute(recipient.has_mail?) - assert_equal(0, recipient.unread_dmail_count) + recipient.dmails.unread.last.update!(is_read: true) + assert_equal(0, recipient.reload.unread_dmail_count) end context "that is automated" do