Merge pull request #1088 from Retrospring/precache-subscriptions

Refactor subscriptions

app/controllers/ajax/subscription_controller.rb

@@ -4,14 +4,14 @@ class Ajax::SubscriptionController < AjaxController
   def subscribe
     params.require :answer
     @response[:status] = :okay
-    state = Subscription.subscribe(current_user, Answer.find(params[:answer])).nil?
-    @response[:success] = state == false
+    result = Subscription.subscribe(current_user, Answer.find(params[:answer]))
+    @response[:success] = result.present?
   end
 
   def unsubscribe
     params.require :answer
     @response[:status] = :okay
-    state = Subscription.unsubscribe(current_user, Answer.find(params[:answer])).nil?
-    @response[:success] = state == false
+    result = Subscription.unsubscribe(current_user, Answer.find(params[:answer]))
+    @response[:success] = result&.destroyed? || false
   end
 end

app/controllers/answer_controller.rb

@@ -10,17 +10,12 @@ class AnswerController < ApplicationController
   def show
     @answer = Answer.includes(comments: %i[user smiles], question: [:user], smiles: [:user]).find(params[:id])
     @display_all = true
+    @subscribed_answer_ids = []
 
-    if user_signed_in?
-      notif = Notification.where(type: "Notification::QuestionAnswered", target_id: @answer.id, recipient_id: current_user.id, new: true).first
-      notif&.update(new: false)
-      notif = Notification.where(type: "Notification::Commented", target_id: @answer.comments.pluck(:id), recipient_id: current_user.id, new: true)
-      notif.update_all(new: false) unless notif.empty?
-      notif = Notification.where(type: "Notification::Smiled", target_id: @answer.smiles.pluck(:id), recipient_id: current_user.id, new: true)
-      notif.update_all(new: false) unless notif.empty?
-      notif = Notification.where(type: "Notification::CommentSmiled", target_id: @answer.comment_smiles.pluck(:id), recipient_id: current_user.id, new: true)
-      notif.update_all(new: false) unless notif.empty?
-    end
+    return unless user_signed_in?
+
+    @subscribed_answer_ids = Subscription.where(user: current_user, answer: @answer).pluck(:answer_id)
+    mark_notifications_as_read
   end
 
   def pin
@@ -52,4 +47,15 @@ class AnswerController < ApplicationController
       end
     end
   end
+
+  private
+
+  def mark_notifications_as_read
+    Notification.where(recipient_id: current_user.id, new: true)
+                .and(Notification.where(type: "Notification::QuestionAnswered", target_id: @answer.id)
+                .or(Notification.where(type: "Notification::Commented", target_id: @answer.comments.pluck(:id)))
+                .or(Notification.where(type: "Notification::Smiled", target_id: @answer.smiles.pluck(:id)))
+                .or(Notification.where(type: "Notification::CommentSmiled", target_id: @answer.comment_smiles.pluck(:id))))
+                .update_all(new: false) # rubocop:disable Rails/SkipsModelValidations
+  end
 end

app/controllers/concerns/paginates_answers.rb

@@ -0,0 +1,12 @@
+# frozen_string_literal: true
+
+module PaginatesAnswers
+  def paginate_answers
+    @answers = yield(last_id: params[:last_id])
+    answer_ids = @answers.map(&:id)
+    answer_ids += @pinned_answers.pluck(:id) if @pinned_answers.present?
+    @answers_last_id = answer_ids.min
+    @more_data_available = !yield(last_id: @answers_last_id, size: 1).count.zero?
+    @subscribed_answer_ids = Subscription.where(user: current_user, answer_id: answer_ids).pluck(:answer_id) if user_signed_in?
+  end
+end

app/controllers/question_controller.rb

@@ -1,11 +1,15 @@
 # frozen_string_literal: true
 
 class QuestionController < ApplicationController
+  include PaginatesAnswers
+
   def show
     @question = Question.find(params[:id])
     @answers = @question.cursored_answers(last_id: params[:last_id], current_user:)
-    @answers_last_id = @answers.map(&:id).min
+    answer_ids = @answers.map(&:id)
+    @answers_last_id = answer_ids.min
     @more_data_available = !@question.cursored_answers(last_id: @answers_last_id, size: 1, current_user:).count.zero?
+    @subscribed = Subscription.where(user: current_user, answer_id: answer_ids).pluck(:answer_id) if user_signed_in?
 
     respond_to do |format|
       format.html

app/controllers/timeline_controller.rb

@@ -32,8 +32,10 @@ class TimelineController < ApplicationController
 
   def paginate_timeline
     @timeline = yield(last_id: params[:last_id])
-    @timeline_last_id = @timeline.map(&:id).min
+    timeline_ids = @timeline.map(&:id)
+    @timeline_last_id = timeline_ids.min
     @more_data_available = !yield(last_id: @timeline_last_id, size: 1).count.zero?
+    @subscribed_answer_ids = Subscription.where(user: current_user, answer_id: timeline_ids).pluck(:answer_id)
 
     respond_to do |format|
       format.html { render "timeline/timeline" }

app/controllers/user_controller.rb

@@ -1,19 +1,19 @@
 # frozen_string_literal: true
 
 class UserController < ApplicationController
+  include PaginatesAnswers
+
   before_action :set_user
   before_action :hidden_social_graph_redirect, only: %i[followers followings]
   after_action :mark_notification_as_read, only: %i[show]
 
   def show
-    @answers = @user.cursored_answers(last_id: params[:last_id])
     @pinned_answers = @user.answers.pinned.order(pinned_at: :desc).limit(10)
-    @answers_last_id = @answers.map(&:id).min
-    @more_data_available = !@user.cursored_answers(last_id: @answers_last_id, size: 1).count.zero?
+    paginate_answers { |args| @user.cursored_answers(**args) }
 
     respond_to do |format|
       format.html
-      format.turbo_stream
+      format.turbo_stream { render layout: false }
     end
   end
 

app/models/comment.rb

@@ -8,7 +8,7 @@ class Comment < ApplicationRecord
   validates :content, length: { maximum: 512 }
 
   after_create do
-    Subscription.subscribe self.user, answer, false
+    Subscription.subscribe user, answer
     Subscription.notify self, answer
   end
 

app/models/subscription.rb

@@ -1,73 +1,52 @@
+# frozen_string_literal: true
+
 class Subscription < ApplicationRecord
   belongs_to :user
   belongs_to :answer
 
   class << self
-    def for(target)
-      Subscription.where(answer: target)
-    end
-
-    def is_subscribed(recipient, target)
+    def subscribe(recipient, target)
       existing = Subscription.find_by(user: recipient, answer: target)
-      if existing.nil?
-        false
-      else
-        existing.is_active
-      end
-    end
+      return true if existing.present?
 
-    def subscribe(recipient, target, force = true)
-      existing = Subscription.find_by(user: recipient, answer: target)
-      if existing.nil?
-        Subscription.new(user: recipient, answer: target).save!
-      elsif force
-        existing.update(is_active: true)
-      end
+      Subscription.create!(user: recipient, answer: target)
     end
 
     def unsubscribe(recipient, target)
-      if recipient.nil? or target.nil?
-        return nil
-      end
+      return nil if recipient.nil? || target.nil?
 
       subs = Subscription.find_by(user: recipient, answer: target)
-      subs.update(is_active: false) unless subs.nil?
+      subs&.destroy
     end
 
     def destruct(target)
-      if target.nil?
-        return nil
-      end
+      return nil if target.nil?
+
       Subscription.where(answer: target).destroy_all
     end
 
-    def destruct_by(recipient, target)
-      if recipient.nil? or target.nil?
-        return nil
-      end
-
-      subs = Subscription.find_by(user: recipient, answer: target)
-      subs.destroy unless subs.nil?
-    end
-
     def notify(source, target)
-      if source.nil? or target.nil?
-        return nil
+      return nil if source.nil? || target.nil?
+
+      muted_by = Relationships::Mute.where(target: source.user).pluck(&:source_id)
+
+      # As we will need to notify for each person subscribed,
+      # it's much faster to bulk insert than to use +Notification.notify+
+      notifications = Subscription.where(answer: target)
+                                  .where.not(user: source.user)
+                                  .where.not(user_id: muted_by)
+                                  .map do |s|
+        { target_id: source.id, target_type: Comment, recipient_id: s.user_id, new: true, type: Notification::Commented }
       end
 
-      Subscription.where(answer: target, is_active: true).each do |subs|
-        next unless not subs.user == source.user
-        Notification.notify subs.user, source
-      end
+      Notification.insert_all!(notifications) unless notifications.empty? # rubocop:disable Rails/SkipsModelValidations
     end
 
     def denotify(source, target)
-      if source.nil? or target.nil?
-        return nil
-      end
-      Subscription.where(answer: target).each do |subs|
-        Notification.denotify subs.user, source
-      end
+      return nil if source.nil? || target.nil?
+
+      subs = Subscription.where(answer: target)
+      Notification.where(target:, recipient: subs.map(&:user)).delete_all
     end
   end
 end

app/views/actions/_answer.html.haml

@@ -1,5 +1,5 @@
 .dropdown-menu.dropdown-menu-end{ role: :menu }
-  - if Subscription.is_subscribed(current_user, answer)
+  - if subscribed_answer_ids&.include?(answer.id)
     -#                                            fun joke    should subscribe?
     %a.dropdown-item{ href: "#", data: { a_id: answer.id, action: "ab-submarine", torpedo: "no" } }
       %i.fa.fa-fw.fa-anchor

app/views/answer/show.html.haml

@@ -1,4 +1,4 @@
 - provide(:title, answer_title(@answer))
 - provide(:og, answer_opengraph(@answer))
 .container-lg.container--main
-  = render 'answerbox', a: @answer, display_all: @display_all
+  = render "answerbox", a: @answer, display_all: @display_all, subscribed_answer_ids: @subscribed_answer_ids

app/views/answerbox/_actions.html.haml

@@ -13,4 +13,4 @@
   .btn-group
     %button.btn.btn-default.btn-sm.dropdown-toggle{ data: { bs_toggle: :dropdown }, aria: { expanded: false } }
       %span.caret
-    = render "actions/answer", answer: a
+    = render "actions/answer", answer: a, subscribed_answer_ids:

app/views/application/_answerbox.html.haml

@@ -21,7 +21,7 @@
               .answerbox__answer-date
                 = link_to(raw(t("time.distance_ago", time: time_tooltip(a))), answer_path(a.user.screen_name, a.id), data: { selection_hotkey: "l" })
         .col-md-6.d-flex.d-md-block.answerbox__actions
-          = render "answerbox/actions", a: a, display_all: display_all
+          = render "answerbox/actions", a:, display_all:, subscribed_answer_ids:
     - else
       .row
         .col-md-6.text-start.text-muted
@@ -33,7 +33,7 @@
               %i.fa.fa-thumbtack
               = t(".pinned")
         .col-md-6.d-md-flex.answerbox__actions
-          = render "answerbox/actions", a: a, display_all: display_all
+          = render "answerbox/actions", a:, display_all:, subscribed_answer_ids:
   .card-footer{ id: "ab-comments-section-#{a.id}", class: display_all.nil? ? "d-none" : nil }
     %div{ id: "ab-smiles-#{a.id}" }= render "answerbox/smiles", a: a
     %div{ id: "ab-comments-#{a.id}" }= render "answerbox/comments", a: a

app/views/question/show.html.haml

@@ -6,7 +6,7 @@
     %button.d-none{ data: { hotkey: "j", action: "navigation#down" } }
     %button.d-none{ data: { hotkey: "k", action: "navigation#up" } }
     - @answers.each do |a|
-      = render "answerbox", a: a, show_question: false
+      = render "answerbox", a:, show_question: false, subscribed_answer_ids: @subscribed_answer_ids
 
   - if @more_data_available
     .d-flex.justify-content-center.justify-content-sm-start#paginator

app/views/question/show.turbo_stream.haml

@@ -1,6 +1,6 @@
 = turbo_stream.append "answers" do
   - @answers.each do |a|
-    = render "answerbox", a: a, show_question: false
+    = render "answerbox", a:, show_question: false, subscribed_answer_ids: @subscribed_answer_ids
 
 = turbo_stream.update "paginator" do
   - if @more_data_available

app/views/timeline/timeline.html.haml

@@ -2,7 +2,7 @@
   %button.d-none{ data: { hotkey: "j", action: "navigation#down" } }
   %button.d-none{ data: { hotkey: "k", action: "navigation#up" } }
   - @timeline.each do |answer|
-    = render "answerbox", a: answer
+    = render "answerbox", a: answer, subscribed_answer_ids: @subscribed_answer_ids
 
 - if @more_data_available
   .d-flex.justify-content-center#paginator

app/views/timeline/timeline.turbo_stream.haml

@@ -1,6 +1,6 @@
 = turbo_stream.append "timeline" do
   - @timeline.each do |answer|
-    = render "answerbox", a: answer
+    = render "answerbox", a: answer, subscribed_answer_ids: @subscribed_answer_ids
 
 = turbo_stream.update "paginator" do
   - if @more_data_available

app/views/user/show.html.haml

@@ -4,11 +4,11 @@
     %button.d-none{ data: { hotkey: "k", action: "navigation#up" } }
     #pinned-answers
       - @pinned_answers.each do |a|
-        = render "answerbox", a:
+        = render "answerbox", a:, subscribed_answer_ids: @subscribed_answer_ids
 
     #answers
       - @answers.each do |a|
-        = render "answerbox", a:
+        = render "answerbox", a:, subscribed_answer_ids: @subscribed_answer_ids
 
 - if @more_data_available
   .d-flex.justify-content-center.justify-content-sm-start#paginator

app/views/user/show.turbo_stream.haml

@@ -1,6 +1,6 @@
 = turbo_stream.append "answers" do
   - @answers.each do |a|
-    = render 'answerbox', a: a
+    = render "answerbox", a:
 
 = turbo_stream.update "paginator" do
   - if @more_data_available

db/migrate/20230227174822_remove_is_active_from_subscriptions.rb

@@ -0,0 +1,8 @@
+# frozen_string_literal: true
+
+class RemoveIsActiveFromSubscriptions < ActiveRecord::Migration[6.1]
+  def up
+    execute "DELETE FROM subscriptions WHERE is_active = FALSE"
+    remove_column :subscriptions, :is_active
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2023_02_25_143633) do
+ActiveRecord::Schema.define(version: 2023_02_27_174822) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -261,7 +261,6 @@ ActiveRecord::Schema.define(version: 2023_02_25_143633) do
     t.bigint "answer_id", null: false
     t.datetime "created_at", null: false
     t.datetime "updated_at", null: false
-    t.boolean "is_active", default: true
     t.index ["user_id", "answer_id"], name: "index_subscriptions_on_user_id_and_answer_id"
   end
 

spec/controllers/ajax/subscription_controller_spec.rb

@@ -33,7 +33,7 @@ describe Ajax::SubscriptionController, :ajax_controller, type: :controller do
         context "when subscription does not exist" do
           it "creates a subscription on the answer" do
             expect { subject }.to(change { answer.subscriptions.count }.by(1))
-            expect(answer.subscriptions.where(is_active: true).map { |s| s.user.id }.sort).to eq([answer_user.id, user.id].sort)
+            expect(answer.subscriptions.map { |s| s.user.id }.sort).to eq([answer_user.id, user.id].sort)
           end
 
           include_examples "returns the expected response"
@@ -44,7 +44,7 @@ describe Ajax::SubscriptionController, :ajax_controller, type: :controller do
 
           it "does not modify the answer's subscriptions" do
             expect { subject }.to(change { answer.subscriptions.count }.by(0))
-            expect(answer.subscriptions.where(is_active: true).map { |s| s.user.id }.sort).to eq([answer_user.id, user.id].sort)
+            expect(answer.subscriptions.map { |s| s.user.id }.sort).to eq([answer_user.id, user.id].sort)
           end
 
           include_examples "returns the expected response"
@@ -105,8 +105,8 @@ describe Ajax::SubscriptionController, :ajax_controller, type: :controller do
           before(:each) { Subscription.subscribe(user, answer) }
 
           it "removes an active subscription from the answer" do
-            expect { subject }.to(change { answer.subscriptions.where(is_active: true).count }.by(-1))
-            expect(answer.subscriptions.where(is_active: true).map { |s| s.user.id }.sort).to eq([answer_user.id].sort)
+            expect { subject }.to(change { answer.subscriptions.count }.by(-1))
+            expect(answer.subscriptions.map { |s| s.user.id }.sort).to eq([answer_user.id].sort)
           end
 
           include_examples "returns the expected response"
@@ -123,7 +123,7 @@ describe Ajax::SubscriptionController, :ajax_controller, type: :controller do
 
           it "does not modify the answer's subscriptions" do
             expect { subject }.to(change { answer.subscriptions.count }.by(0))
-            expect(answer.subscriptions.where(is_active: true).map { |s| s.user.id }.sort).to eq([answer_user.id].sort)
+            expect(answer.subscriptions.map { |s| s.user.id }.sort).to eq([answer_user.id].sort)
           end
 
           include_examples "returns the expected response"

spec/models/subscription_spec.rb

@@ -0,0 +1,28 @@
+# frozen_string_literal: true
+
+require "rails_helper"
+
+describe Subscription do
+  describe "singleton object" do
+    describe "#notify" do
+      subject { Subscription.notify(source, target) }
+
+      context "answer with one comment" do
+        let(:answer_author) { FactoryBot.create(:user) }
+        let(:answer) { FactoryBot.create(:answer, user: answer_author) }
+        let(:commenter) { FactoryBot.create(:user) }
+        let!(:comment) { FactoryBot.create(:comment, answer:, user: commenter) }
+        let(:source) { comment }
+        let(:target) { answer }
+
+        it "notifies the target about source" do
+          # The method we're testing here is already called the +after_create+ of +Comment+ so there already is a notification
+          expect { subject }.to change { Notification.count }.from(1).to(2)
+          created = Notification.order(:created_at).first!
+          expect(created.target).to eq(comment)
+          expect(created.recipient).to eq(answer_author)
+        end
+      end
+    end
+  end
+end