Fix notification read and dismiss under Mongoid 9

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)

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}

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