@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/[email protected]>_______________________________________________
rails-dev mailing list
[email protected]
https://lists.openstreetmap.org/listinfo/rails-dev
