From 13528ac2d3a96b38a9b9f9c9b446327ec56721b7 Mon Sep 17 00:00:00 2001 From: evazion Date: Sun, 19 Jan 2020 13:44:09 -0600 Subject: [PATCH] Drop forum subscriptions. Few people used forum subscriptions (only around 100), and even fewer people were subscribed to active threads. Most subscriptions were for old threads that will never be bumped again. The implementation also had a few problems: * Unsubscribe links in emails didn't work (they unset the user's receive_email_notifications flag, but forum subscriptions didn't respect this flag). * Some users had invalid email addresses, which caused notifications to bounce. There was no mechanism for preventing bounces. * The implementation wasn't scalable. It involved a daily linear scan over _all_ forum subscriptions looking for any topics that had been updated. --- app/controllers/forum_topics_controller.rb | 18 +----- app/logical/danbooru_maintenance.rb | 1 - app/mailers/user_mailer.rb | 7 -- app/models/forum_subscription.rb | 24 ------- app/models/forum_topic.rb | 8 --- .../forum_topics/_secondary_links.html.erb | 5 -- app/views/user_mailer/forum_notice.html.erb | 13 ---- config/routes.rb | 2 - ...20200119193110_drop_forum_subscriptions.rb | 7 ++ db/structure.sql | 64 +------------------ test/factories/forum_subscription.rb | 3 - test/mailers/previews/user_mailer_preview.rb | 8 --- 12 files changed, 11 insertions(+), 149 deletions(-) delete mode 100644 app/models/forum_subscription.rb delete mode 100644 app/views/user_mailer/forum_notice.html.erb create mode 100644 db/migrate/20200119193110_drop_forum_subscriptions.rb delete mode 100644 test/factories/forum_subscription.rb diff --git a/app/controllers/forum_topics_controller.rb b/app/controllers/forum_topics_controller.rb index ce434e9fe..b91ac50fe 100644 --- a/app/controllers/forum_topics_controller.rb +++ b/app/controllers/forum_topics_controller.rb @@ -4,8 +4,8 @@ class ForumTopicsController < ApplicationController before_action :member_only, :except => [:index, :show] before_action :moderator_only, :only => [:new_merge, :create_merge] before_action :normalize_search, :only => :index - before_action :load_topic, :only => [:edit, :show, :update, :destroy, :undelete, :new_merge, :create_merge, :subscribe, :unsubscribe] - before_action :check_min_level, :only => [:show, :edit, :update, :new_merge, :create_merge, :destroy, :undelete, :subscribe, :unsubscribe] + before_action :load_topic, :only => [:edit, :show, :update, :destroy, :undelete, :new_merge, :create_merge] + before_action :check_min_level, :only => [:show, :edit, :update, :new_merge, :create_merge, :destroy, :undelete] skip_before_action :api_check def new @@ -88,20 +88,6 @@ class ForumTopicsController < ApplicationController redirect_to forum_topic_path(@merged_topic) end - def subscribe - subscription = ForumSubscription.where(:forum_topic_id => @forum_topic.id, :user_id => CurrentUser.user.id).first - unless subscription - ForumSubscription.create(:forum_topic_id => @forum_topic.id, :user_id => CurrentUser.user.id, :last_read_at => @forum_topic.updated_at) - end - respond_with(@forum_topic) - end - - def unsubscribe - subscription = ForumSubscription.where(:forum_topic_id => @forum_topic.id, :user_id => CurrentUser.user.id).first - subscription&.destroy - respond_with(@forum_topic) - end - private def normalize_search diff --git a/app/logical/danbooru_maintenance.rb b/app/logical/danbooru_maintenance.rb index c0e349a1b..2fbe595b5 100644 --- a/app/logical/danbooru_maintenance.rb +++ b/app/logical/danbooru_maintenance.rb @@ -9,7 +9,6 @@ module DanbooruMaintenance safely { Upload.prune! } safely { Delayed::Job.where('created_at < ?', 45.days.ago).delete_all } safely { PostDisapproval.prune! } - safely { ForumSubscription.process_all! } safely { PostDisapproval.dmail_messages! } safely { regenerate_post_counts! } safely { SuperVoter.init! } diff --git a/app/mailers/user_mailer.rb b/app/mailers/user_mailer.rb index 00bdab9dc..ddbe65632 100644 --- a/app/mailers/user_mailer.rb +++ b/app/mailers/user_mailer.rb @@ -7,11 +7,4 @@ class UserMailer < ActionMailer::Base @dmail = dmail mail(:to => "#{dmail.to.name} <#{dmail.to.email}>", :subject => "#{Danbooru.config.app_name} - Message received from #{dmail.from.name}") end - - def forum_notice(user, forum_topic, forum_posts) - @user = user - @forum_topic = forum_topic - @forum_posts = forum_posts - mail(:to => "#{user.name} <#{user.email}>", :subject => "#{Danbooru.config.app_name} forum topic #{forum_topic.title} updated") - end end diff --git a/app/models/forum_subscription.rb b/app/models/forum_subscription.rb deleted file mode 100644 index 07fd1d17f..000000000 --- a/app/models/forum_subscription.rb +++ /dev/null @@ -1,24 +0,0 @@ -class ForumSubscription < ApplicationRecord - belongs_to :user - belongs_to :forum_topic - - def self.prune! - where("last_read_at < ?", 3.months.ago).delete_all - end - - def self.process_all! - ForumSubscription.find_each do |subscription| - forum_topic = subscription.forum_topic - if forum_topic.updated_at > subscription.last_read_at - CurrentUser.scoped(subscription.user, "127.0.0.1") do - forum_posts = forum_topic.posts.where("created_at > ?", subscription.last_read_at).order("id desc") - begin - UserMailer.forum_notice(subscription.user, forum_topic, forum_posts).deliver_now - rescue Net::SMTPSyntaxError - end - subscription.update_attribute(:last_read_at, forum_topic.updated_at) - end - end - end - end -end diff --git a/app/models/forum_topic.rb b/app/models/forum_topic.rb index 680001305..c2666fe7e 100644 --- a/app/models/forum_topic.rb +++ b/app/models/forum_topic.rb @@ -16,7 +16,6 @@ class ForumTopic < ApplicationRecord has_many :posts, -> {order("forum_posts.id asc")}, :class_name => "ForumPost", :foreign_key => "topic_id", :dependent => :destroy has_many :moderation_reports, through: :posts has_one :original_post, -> {order("forum_posts.id asc")}, class_name: "ForumPost", foreign_key: "topic_id", inverse_of: :topic - has_many :subscriptions, :class_name => "ForumSubscription" before_validation :initialize_is_deleted, :on => :create validates_presence_of :title validates_associated :original_post @@ -126,16 +125,9 @@ class ForumTopic < ApplicationRecord end end - module SubscriptionMethods - def user_subscription(user) - subscriptions.where(:user_id => user.id).first - end - end - extend SearchMethods include CategoryMethods include VisitMethods - include SubscriptionMethods def editable_by?(user) (creator_id == user.id || user.is_moderator?) && visible?(user) diff --git a/app/views/forum_topics/_secondary_links.html.erb b/app/views/forum_topics/_secondary_links.html.erb index d2aacbd89..715f0b694 100644 --- a/app/views/forum_topics/_secondary_links.html.erb +++ b/app/views/forum_topics/_secondary_links.html.erb @@ -18,11 +18,6 @@ <% if CurrentUser.is_member? && @forum_topic && !@forum_topic.new_record? %>
  • |
    • <%= subnav_link_to "Reply", new_forum_post_path(:topic_id => @forum_topic.id) %> - <% if @forum_topic.user_subscription(CurrentUser.user) %> - <%= subnav_link_to "Unsubscribe", unsubscribe_forum_topic_path(@forum_topic), :method => :post %> - <% else %> - <%= subnav_link_to "Subscribe", subscribe_forum_topic_path(@forum_topic), :method => :post, :data => {:confirm => "Are you sure you want to receive email notifications for this forum topic?"} %> - <% end %> <% if !@forum_topic.new_record? && @forum_topic.editable_by?(CurrentUser.user) %> <%= subnav_link_to "Edit", edit_forum_topic_path(@forum_topic), "data-shortcut": "e" %> <% if CurrentUser.is_moderator? %> diff --git a/app/views/user_mailer/forum_notice.html.erb b/app/views/user_mailer/forum_notice.html.erb deleted file mode 100644 index 3a0243444..000000000 --- a/app/views/user_mailer/forum_notice.html.erb +++ /dev/null @@ -1,13 +0,0 @@ -

    "<%= @forum_topic.title %>" was updated:

    - -<% @forum_posts.each do |forum_post| %> -

    - <%= forum_post.creator.name %> said: -

    - <%= format_text(forum_post.body, base_url: root_url) %> -
    -
    -
    -<% end %> - -

    <%= link_to "View topic", forum_topic_url(@forum_topic, :page => @forum_topic.last_page, :host => Danbooru.config.hostname, :only_path => false) %> | <%= link_to "Unsubscribe", maintenance_user_email_notification_url(:user_id => @user.id, :sig => email_sig(@user), :host => Danbooru.config.hostname, :only_path => false) %>

    diff --git a/config/routes.rb b/config/routes.rb index 7bb4ae82b..84b1af4dd 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -142,8 +142,6 @@ Rails.application.routes.draw do post :undelete get :new_merge post :create_merge - post :subscribe - post :unsubscribe end collection do post :mark_all_as_read diff --git a/db/migrate/20200119193110_drop_forum_subscriptions.rb b/db/migrate/20200119193110_drop_forum_subscriptions.rb new file mode 100644 index 000000000..961929525 --- /dev/null +++ b/db/migrate/20200119193110_drop_forum_subscriptions.rb @@ -0,0 +1,7 @@ +require_relative "20140725003232_create_forum_subscriptions" + +class DropForumSubscriptions < ActiveRecord::Migration[6.0] + def change + revert CreateForumSubscriptions + end +end diff --git a/db/structure.sql b/db/structure.sql index 3f524b6e9..31bec31a9 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -2014,38 +2014,6 @@ CREATE SEQUENCE public.forum_posts_id_seq ALTER SEQUENCE public.forum_posts_id_seq OWNED BY public.forum_posts.id; --- --- Name: forum_subscriptions; Type: TABLE; Schema: public; Owner: - --- - -CREATE TABLE public.forum_subscriptions ( - id integer NOT NULL, - user_id integer, - forum_topic_id integer, - last_read_at timestamp without time zone, - delete_key character varying -); - - --- --- Name: forum_subscriptions_id_seq; Type: SEQUENCE; Schema: public; Owner: - --- - -CREATE SEQUENCE public.forum_subscriptions_id_seq - START WITH 1 - INCREMENT BY 1 - NO MINVALUE - NO MAXVALUE - CACHE 1; - - --- --- Name: forum_subscriptions_id_seq; Type: SEQUENCE OWNED BY; Schema: public; Owner: - --- - -ALTER SEQUENCE public.forum_subscriptions_id_seq OWNED BY public.forum_subscriptions.id; - - -- -- Name: forum_topic_visits; Type: TABLE; Schema: public; Owner: - -- @@ -4040,13 +4008,6 @@ ALTER TABLE ONLY public.forum_post_votes ALTER COLUMN id SET DEFAULT nextval('pu ALTER TABLE ONLY public.forum_posts ALTER COLUMN id SET DEFAULT nextval('public.forum_posts_id_seq'::regclass); --- --- Name: forum_subscriptions id; Type: DEFAULT; Schema: public; Owner: - --- - -ALTER TABLE ONLY public.forum_subscriptions ALTER COLUMN id SET DEFAULT nextval('public.forum_subscriptions_id_seq'::regclass); - - -- -- Name: forum_topic_visits id; Type: DEFAULT; Schema: public; Owner: - -- @@ -4394,14 +4355,6 @@ ALTER TABLE ONLY public.forum_posts ADD CONSTRAINT forum_posts_pkey PRIMARY KEY (id); --- --- Name: forum_subscriptions forum_subscriptions_pkey; Type: CONSTRAINT; Schema: public; Owner: - --- - -ALTER TABLE ONLY public.forum_subscriptions - ADD CONSTRAINT forum_subscriptions_pkey PRIMARY KEY (id); - - -- -- Name: forum_topic_visits forum_topic_visits_pkey; Type: CONSTRAINT; Schema: public; Owner: - -- @@ -6425,20 +6378,6 @@ CREATE INDEX index_forum_posts_on_topic_id ON public.forum_posts USING btree (to CREATE INDEX index_forum_posts_on_updated_at ON public.forum_posts USING btree (updated_at); --- --- Name: index_forum_subscriptions_on_forum_topic_id; Type: INDEX; Schema: public; Owner: - --- - -CREATE INDEX index_forum_subscriptions_on_forum_topic_id ON public.forum_subscriptions USING btree (forum_topic_id); - - --- --- Name: index_forum_subscriptions_on_user_id; Type: INDEX; Schema: public; Owner: - --- - -CREATE INDEX index_forum_subscriptions_on_user_id ON public.forum_subscriptions USING btree (user_id); - - -- -- Name: index_forum_topic_visits_on_forum_topic_id; Type: INDEX; Schema: public; Owner: - -- @@ -7426,6 +7365,7 @@ INSERT INTO "schema_migrations" (version) VALUES ('20200115010442'), ('20200117220602'), ('20200118015014'), -('20200119184442'); +('20200119184442'), +('20200119193110'); diff --git a/test/factories/forum_subscription.rb b/test/factories/forum_subscription.rb deleted file mode 100644 index 62fa7203b..000000000 --- a/test/factories/forum_subscription.rb +++ /dev/null @@ -1,3 +0,0 @@ -FactoryBot.define do - factory(:forum_subscription) -end diff --git a/test/mailers/previews/user_mailer_preview.rb b/test/mailers/previews/user_mailer_preview.rb index 326b665a5..ec87e1e02 100644 --- a/test/mailers/previews/user_mailer_preview.rb +++ b/test/mailers/previews/user_mailer_preview.rb @@ -3,12 +3,4 @@ class UserMailerPreview < ActionMailer::Preview dmail = User.admins.first.dmails.first UserMailer.dmail_notice(dmail) end - - def forum_notice - topic = ForumTopic.first - posts = topic.posts - user = topic.creator - - UserMailer.forum_notice(user, topic, posts) - end end