diff --git a/app/controllers/ajax/subscription_controller.rb b/app/controllers/ajax/subscription_controller.rb index a04520e4..fd574e78 100644 --- a/app/controllers/ajax/subscription_controller.rb +++ b/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 diff --git a/app/controllers/answer_controller.rb b/app/controllers/answer_controller.rb index d5e70881..ac52ab40 100644 --- a/app/controllers/answer_controller.rb +++ b/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 diff --git a/app/controllers/concerns/paginates_answers.rb b/app/controllers/concerns/paginates_answers.rb new file mode 100644 index 00000000..567eee25 --- /dev/null +++ b/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 diff --git a/app/controllers/question_controller.rb b/app/controllers/question_controller.rb index 9d9c2891..14dbe80e 100644 --- a/app/controllers/question_controller.rb +++ b/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 diff --git a/app/controllers/timeline_controller.rb b/app/controllers/timeline_controller.rb index 4445af6a..6b34fdb4 100644 --- a/app/controllers/timeline_controller.rb +++ b/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" } diff --git a/app/controllers/user_controller.rb b/app/controllers/user_controller.rb index 1963b496..7b9084b9 100644 --- a/app/controllers/user_controller.rb +++ b/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 diff --git a/app/models/comment.rb b/app/models/comment.rb index 3b43999e..00472037 100644 --- a/app/models/comment.rb +++ b/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 diff --git a/app/models/subscription.rb b/app/models/subscription.rb index bab3bf39..66520203 100644 --- a/app/models/subscription.rb +++ b/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 diff --git a/app/views/actions/_answer.html.haml b/app/views/actions/_answer.html.haml index de81c031..e63e452b 100644 --- a/app/views/actions/_answer.html.haml +++ b/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 diff --git a/app/views/answer/show.html.haml b/app/views/answer/show.html.haml index 5f84d47d..55f2a90b 100644 --- a/app/views/answer/show.html.haml +++ b/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 diff --git a/app/views/answerbox/_actions.html.haml b/app/views/answerbox/_actions.html.haml index 4ec1c35d..5c152abe 100644 --- a/app/views/answerbox/_actions.html.haml +++ b/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: diff --git a/app/views/application/_answerbox.html.haml b/app/views/application/_answerbox.html.haml index 36c2a90e..b84a5f17 100644 --- a/app/views/application/_answerbox.html.haml +++ b/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 diff --git a/app/views/question/show.html.haml b/app/views/question/show.html.haml index b21b70f1..e1d0b238 100644 --- a/app/views/question/show.html.haml +++ b/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 diff --git a/app/views/question/show.turbo_stream.haml b/app/views/question/show.turbo_stream.haml index 34f6be75..96facf13 100644 --- a/app/views/question/show.turbo_stream.haml +++ b/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 diff --git a/app/views/timeline/timeline.html.haml b/app/views/timeline/timeline.html.haml index 2b3089b8..4ed197a8 100644 --- a/app/views/timeline/timeline.html.haml +++ b/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 diff --git a/app/views/timeline/timeline.turbo_stream.haml b/app/views/timeline/timeline.turbo_stream.haml index 561309e6..1802f630 100644 --- a/app/views/timeline/timeline.turbo_stream.haml +++ b/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 diff --git a/app/views/user/show.html.haml b/app/views/user/show.html.haml index 98f36de8..e110ca20 100644 --- a/app/views/user/show.html.haml +++ b/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 diff --git a/app/views/user/show.turbo_stream.haml b/app/views/user/show.turbo_stream.haml index c341e74b..ae7889ad 100644 --- a/app/views/user/show.turbo_stream.haml +++ b/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 diff --git a/db/migrate/20230227174822_remove_is_active_from_subscriptions.rb b/db/migrate/20230227174822_remove_is_active_from_subscriptions.rb new file mode 100644 index 00000000..3b8cb4b5 --- /dev/null +++ b/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 diff --git a/db/schema.rb b/db/schema.rb index 3cabc240..39dc6424 100644 --- a/db/schema.rb +++ b/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 diff --git a/spec/controllers/ajax/subscription_controller_spec.rb b/spec/controllers/ajax/subscription_controller_spec.rb index 6a14aeee..23499211 100644 --- a/spec/controllers/ajax/subscription_controller_spec.rb +++ b/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" diff --git a/spec/models/subscription_spec.rb b/spec/models/subscription_spec.rb new file mode 100644 index 00000000..eac76a29 --- /dev/null +++ b/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