[openstreetmap/openstreetmap-website] A new foundation for user notifications (PR #6837)

[Tom Hughes]

@tomhughes commented on this pull request.

Others may disagree but I think I would restructure this as just two commits .

One that adds the framework made up of the first five commits squashed together, but excluding the boilerplate version of the changeset notifier and with the migrations merged as discussed inline.

The second would then add port the changeset notifications to the new framework.

> @@ -203,3 +203,5 @@ group :development, :test do
   # See https://guides.rubyonrails.org/debugging_rails_applications.html#debugging-with-the-debug-gem
   gem "debug", :require => "debug/prelude"
 end
+
+gem "noticed", "~> 2.9"

I think I'd put this above the groups, and with a comment as with other gems.

> @@ -0,0 +1,8 @@
+# frozen_string_literal: true
+
+# This migration comes from noticed (originally 20240129184740)
+class AddNotificationsCountToNoticedEvent < ActiveRecord::Migration[8.1]
+  def change
+    add_column :noticed_events, :notifications_count, :integer

I understand why this is in a separate migration but as we're only just starting to use the gem now it seems to me that we should just include this column in the other migration and then drop this one.

-- 
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/6837#pullrequestreview-3962810572
You are receiving this because you are subscribed to this thread.

Message ID: <openstreetmap/openstreetmap-website/pull/6837/review/3962810572 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20260317/0ceda866/attachment.htm>