diff --git a/backend/app/controllers/api/v1/notifications_controller.rb b/backend/app/controllers/api/v1/notifications_controller.rb index 0e4e6d6e..6fb17303 100644 --- a/backend/app/controllers/api/v1/notifications_controller.rb +++ b/backend/app/controllers/api/v1/notifications_controller.rb @@ -2,7 +2,11 @@ module Api module V1 class NotificationsController < ApplicationController def index - notifications = Notification.where(encrypted_notify_user_id: current_user.encrypted_id) + # The aggregation reaches through `notificateable` to title each group, so without + # eager loading this is one query per notification. + notifications = Notification + .where(encrypted_notify_user_id: current_user.encrypted_id) + .includes(:notificateable) authorize_collection :index, notifications @@ -41,7 +45,9 @@ def notification_params parameters[:notificateable_type] = parameters[:notificateable_type].titleize parameters[:encrypted_notify_user_id] = current_user.encrypted_id - parameters + # Mongoid 9 requires a query expression to be a Hash and raises InvalidQuery on + # an ActionController::Parameters, which Mongoid 8 accepted. + parameters.to_h end def authorize_collection(name, collection) diff --git a/backend/app/models/notification.rb b/backend/app/models/notification.rb index d837ece8..10e96c2c 100644 --- a/backend/app/models/notification.rb +++ b/backend/app/models/notification.rb @@ -3,7 +3,11 @@ class Notification include Mongoid::Timestamps include Usernameable - after_initialize :set_defaults + # Only derive post_id when it is not already stored. This runs on every instantiation, + # including records loaded from the database, and it reaches through `notificateable`, + # so without the guard every Notification loaded costs an extra query to recompute a + # value it was already holding. + after_initialize :set_defaults, if: -> { post_id.blank? } field :kind, type: String field :encrypted_user_id, type: String, encrypted: {type: :integer} diff --git a/backend/spec/controllers/api/v1/notifications_controller_spec.rb b/backend/spec/controllers/api/v1/notifications_controller_spec.rb new file mode 100644 index 00000000..31cb8ebc --- /dev/null +++ b/backend/spec/controllers/api/v1/notifications_controller_spec.rb @@ -0,0 +1,95 @@ +require "rails_helper" + +RSpec.describe Api::V1::NotificationsController do + let(:user) { create(:user) } + let(:notifier) { create(:user) } + let(:notified_post) { create(:post, encrypted_user_id: user.encrypted_id) } + + let!(:notification) do + create(:notification, + kind: "comment", + notificateable: notified_post, + encrypted_user_id: notifier.encrypted_id, + encrypted_notify_user_id: user.encrypted_id) + end + + before { sign_in user } + + describe "index" do + it "returns the signed-in user's notifications grouped by kind and subject" do + get :index + + expect(response).to have_http_status :ok + expect(response_body[:notifications].size).to eq 1 + expect(response_body[:notifications].first[:kind]).to eq "comment" + expect(response_body[:notifications].first[:count]).to eq 1 + end + + it "collapses repeated notifications on the same subject into a count" do + create(:notification, + kind: "comment", + notificateable: notified_post, + encrypted_user_id: create(:user).encrypted_id, + encrypted_notify_user_id: user.encrypted_id) + + get :index + + expect(response_body[:notifications].size).to eq 1 + expect(response_body[:notifications].first[:count]).to eq 2 + end + + it "does not return notifications addressed to somebody else" do + create(:notification, + kind: "reaction", + notificateable: create(:post), + encrypted_notify_user_id: create(:user).encrypted_id) + + get :index + + expect(response_body[:notifications].map { |n| n[:kind] }).to eq ["comment"] + end + end + + describe "update" do + let(:subject_params) do + {notificateable_id: notified_post.id.to_s, notificateable_type: "post"} + end + + it "marks the notifications for that subject as read" do + put :update, params: subject_params + + expect(response).to have_http_status :ok + expect(notification.reload.unread).to be false + end + + it "returns the regrouped notifications" do + put :update, params: subject_params + + expect(response_body[:notifications].first[:unread]).to be false + end + + it "leaves another user's notifications on the same subject alone" do + theirs = create(:notification, + kind: "comment", + notificateable: notified_post, + encrypted_notify_user_id: create(:user).encrypted_id) + + put :update, params: subject_params + + expect(theirs.reload.unread).to be true + end + end + + describe "destroy" do + it "removes the notifications for that subject" do + expect { + delete :destroy, params: { + notificateable_id: notified_post.id.to_s, + notificateable_type: "post" + } + }.to change { Notification.count }.by(-1) + + expect(response).to have_http_status :no_content + end + end +end