DEV: Rewrite post-migration in pure SQL

app/models/assignment.rb

@@ -56,13 +56,13 @@ class Assignment < ActiveRecord::Base
     target
   end
 
-  def create_missing_notifications!(mark_as_read: false)
+  def create_missing_notifications!
     assigned_users.each do |user|
       next if user.notifications.for_assignment(self).exists?
       DiscourseAssign::CreateNotification.call(
         assignment: self,
         user: user,
-        mark_as_read: mark_as_read || assigned_by_user == user,
+        mark_as_read: assigned_by_user == user,
       )
     end
   end

db/post_migrate/20231011152903_ensure_notifications_consistency.rb

@@ -2,23 +2,78 @@
 
 class EnsureNotificationsConsistency < ActiveRecord::Migration[7.0]
   def up
-    Notification
+    DB.exec(<<~SQL)
-      .assigned
+      DELETE FROM notifications
-      .joins(
+      WHERE id IN (
-        "LEFT OUTER JOIN assignments ON assignments.id = ((notifications.data::jsonb)->'assignment_id')::int",
+        SELECT notifications.id FROM notifications
+        LEFT OUTER JOIN assignments ON assignments.id = ((notifications.data::jsonb)->'assignment_id')::int
+        WHERE (notification_type = 34 AND assignments.id IS NULL OR assignments.active = FALSE)
       )
-      .where(assignments: { id: nil })
+    SQL
-      .or(Assignment.inactive)
-      .destroy_all
 
-    Assignment
+    DB.exec(<<~SQL)
-      .active
+      WITH tmp AS (
-      .left_joins(:topic)
+        SELECT
-      .where.not(topics: { id: nil })
+          assignments.assigned_to_id AS user_id,
-      .find_each do |assignment|
+          assignments.created_at,
-        next if !assignment.target || !assignment.assigned_to
+          assignments.updated_at,
-        assignment.create_missing_notifications!(mark_as_read: true)
+          assignments.topic_id,
-      end
+          (
+            CASE WHEN assignments.target_type = 'Topic' THEN 1
+                 ELSE (SELECT posts.post_number FROM posts WHERE posts.id = assignments.target_id)
+            END
+          ) AS post_number,
+          json_strip_nulls(json_build_object(
+            'message', 'discourse_assign.assign_notification',
+            'display_username', (SELECT users.username FROM users WHERE users.id = assignments.assigned_by_user_id),
+            'topic_title', topics.title,
+            'assignment_id', assignments.id
+          ))::text AS data
+        FROM assignments
+        LEFT OUTER JOIN topics ON topics.deleted_at IS NULL AND topics.id = assignments.topic_id
+        LEFT OUTER JOIN users ON users.id = assignments.assigned_to_id AND assignments.assigned_to_type = 'User'
+        LEFT OUTER JOIN notifications ON ((data::jsonb)->'assignment_id')::int = assignments.id
+        WHERE assignments.active = TRUE
+          AND NOT (topics.id IS NULL OR users.id IS NULL)
+          AND assignments.assigned_to_type = 'User'
+          AND notifications.id IS NULL
+      )
+      INSERT INTO notifications (notification_type, high_priority, read, user_id, created_at, updated_at, topic_id, post_number, data)
+        SELECT 34, TRUE, TRUE, tmp.* FROM tmp
+    SQL
+
+    DB.exec(<<~SQL)
+      WITH tmp AS (
+        SELECT
+          users.id AS user_id,
+          assignments.created_at,
+          assignments.updated_at,
+          assignments.topic_id,
+          (
+            CASE WHEN assignments.target_type = 'Topic' THEN 1
+                 ELSE (SELECT posts.post_number FROM posts WHERE posts.id = assignments.target_id)
+            END
+          ) AS post_number,
+          json_strip_nulls(json_build_object(
+            'message', 'discourse_assign.assign_group_notification',
+            'display_username', (SELECT groups.name FROM groups WHERE groups.id = assignments.assigned_to_id),
+            'topic_title', topics.title,
+            'assignment_id', assignments.id
+          ))::text AS data
+        FROM assignments
+        LEFT OUTER JOIN topics ON topics.deleted_at IS NULL AND topics.id = assignments.topic_id
+        LEFT OUTER JOIN groups ON groups.id = assignments.assigned_to_id AND assignments.assigned_to_type = 'Group'
+        LEFT OUTER JOIN group_users ON groups.id = group_users.group_id
+        LEFT OUTER JOIN users ON users.id = group_users.user_id
+        LEFT OUTER JOIN notifications ON ((data::jsonb)->'assignment_id')::int = assignments.id AND notifications.user_id = users.id
+        WHERE assignments.active = TRUE
+          AND NOT (topics.id IS NULL OR groups.id IS NULL)
+          AND assignments.assigned_to_type = 'Group'
+          AND notifications.id IS NULL
+      )
+      INSERT INTO notifications (notification_type, high_priority, read, user_id, created_at, updated_at, topic_id, post_number, data)
+        SELECT 34, TRUE, TRUE, tmp.* FROM tmp
+    SQL
   end
 
   def down

spec/models/assignment_spec.rb

@@ -83,14 +83,11 @@ RSpec.describe Assignment do
   end
 
   describe "#create_missing_notifications!" do
-    subject(:create_missing_notifications) do
+    subject(:create_missing_notifications) { assignment.create_missing_notifications! }
-      assignment.create_missing_notifications!(mark_as_read: mark_as_read)
-    end
 
     let(:assignment) do
       Fabricate(:topic_assignment, assigned_to: assigned_to, assigned_by_user: assigned_by_user)
     end
-    let(:mark_as_read) { false }
     let(:assigned_by_user) { Fabricate(:user) }
 
     context "when assigned to a user" do
@@ -113,8 +110,8 @@ RSpec.describe Assignment do
       end
 
       context "when notification does not exist yet" do
-        context "when `mark_as_read` is true" do
+        context "when user is the one that assigned" do
-          let(:mark_as_read) { true }
+          let(:assigned_by_user) { assigned_to }
 
           it "creates the missing notification" do
             DiscourseAssign::CreateNotification.expects(:call).with(
@@ -126,29 +123,14 @@ RSpec.describe Assignment do
           end
         end
 
-        context "when `mark_as_read` is false" do
+        context "when user is not the one that assigned" do
-          context "when user is the one that assigned" do
+          it "creates the missing notification" do
-            let(:assigned_by_user) { assigned_to }
+            DiscourseAssign::CreateNotification.expects(:call).with(
-
+              assignment: assignment,
-            it "creates the missing notification" do
+              user: assigned_to,
-              DiscourseAssign::CreateNotification.expects(:call).with(
+              mark_as_read: false,
-                assignment: assignment,
+            )
-                user: assigned_to,
+            create_missing_notifications
-                mark_as_read: true,
-              )
-              create_missing_notifications
-            end
-          end
-
-          context "when user is not the one that assigned" do
-            it "creates the missing notification" do
-              DiscourseAssign::CreateNotification.expects(:call).with(
-                assignment: assignment,
-                user: assigned_to,
-                mark_as_read: false,
-              )
-              create_missing_notifications
-            end
           end
         end
       end

spec/post_migrations/ensure_notifications_consistency_spec.rb

@@ -1,66 +0,0 @@
-# frozen_string_literal: true
-
-require "rails_helper"
-require Rails.root.join(
-          "plugins/discourse-assign/db/post_migrate/20231011152903_ensure_notifications_consistency",
-        )
-
-# As this post migration is calling app code, we want to ensure its behavior
-# won’t change over time.
-RSpec.describe EnsureNotificationsConsistency do
-  describe "#up" do
-    subject(:migrate) { described_class.new.up }
-
-    context "when notification targeting a non-existing assignment exists" do
-      let(:post) { Fabricate(:post) }
-      let!(:notifications) do
-        Fabricate(
-          :notification,
-          notification_type: Notification.types[:assigned],
-          post: post,
-          data: { assignment_id: 1 }.to_json,
-        )
-      end
-
-      it "deletes it" do
-        expect { migrate }.to change { Notification.count }.by(-1)
-      end
-    end
-
-    context "when notification targeting an inactive assignment exists" do
-      let(:post) { Fabricate(:post) }
-      let(:assignment) { Fabricate(:topic_assignment, topic: post.topic, active: false) }
-      let!(:notifications) do
-        Fabricate(
-          :notification,
-          notification_type: Notification.types[:assigned],
-          post: post,
-          data: { assignment_id: assignment.id }.to_json,
-        )
-      end
-
-      it "deletes it" do
-        expect { migrate }.to change { Notification.count }.by(-1)
-      end
-    end
-
-    context "when some active assignments exist" do
-      let(:post) { Fabricate(:post) }
-      let(:group) { Fabricate(:group) }
-      let!(:assignment) { Fabricate(:topic_assignment, topic: post.topic, assigned_to: group) }
-      let!(:inactive_assignment) { Fabricate(:post_assignment, post: post, active: false) }
-      let!(:assignment_with_deleted_topic) { Fabricate(:topic_assignment) }
-
-      before do
-        group.users << Fabricate(:user)
-        assignment_with_deleted_topic.topic.trash!
-      end
-
-      context "when notifications are missing" do
-        it "creates them" do
-          expect { migrate }.to change { Notification.assigned.count }.by(1)
-        end
-      end
-    end
-  end
-end