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

[Andy Allan]

@gravitystorm approved this pull request.

This gets general approval from me. Obviously this will need to be revised after changing the whole email-named-parameters thing, as previously discussed.

I've put two super-nitpicky comments inline, mostly just to prove I've read through the PR 😆  Feel free to ignore either.

This will need followup work to ensure the notification tables don't grow unbounded. We might want to e.g. only store the events and notification records for e.g. 6 months or 1 year or something.

Other followups would include:
* changing all other notifications (e.g. UserMessage notifications) to use the notifier system
* allowing users to configure notification preferences
* showing all notifications in the UI (as demoed in the description)

I'm happy for each of these to be considered as separate PRs.

> @@ -44,9 +44,7 @@ def create
                                           :author => current_user)
 
       # Notify current subscribers of the new comment
-      changeset.subscribers.visible.each do |user|
-        UserMailer.changeset_comment_notification(comment, user).deliver_later if current_user != user
-      end
+      ChangesetCommentNotifier.with(:record => comment).deliver

[`Noticed::Event` has a `deliver_later` alias defined](https://github.com/excid3/noticed/blob/ddab2d1485342a181b4f3708ffa70ed1481a2380/app/models/concerns/noticed/deliverable.rb#L78) - perhaps it's clearer to use this in the code? It's just an alias so it's a purely code taste thing but it might help future readers understand that these notifications are queued asynchronously.

> @@ -0,0 +1,39 @@
+# frozen_string_literal: true
+
+# This migration comes from noticed (originally 20231215190233)
+class CreateNoticedTables < ActiveRecord::Migration[6.1]

Is it worth changing these files to use rails 8 migrations? The precise definition of t.timestamps has changed in rails since 6.1, for example.

> +    read_at timestamp without time zone,
+    seen_at timestamp without time zone,
+    created_at timestamp(6) without time zone NOT NULL,
+    updated_at timestamp(6) without time zone NOT NULL

(inconsistency in timestamp vs timestamp(6)

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

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