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

[Tom Hughes]

@tomhughes commented on this pull request.



> +    # Example of notification settings in action
+    # config.if = -> { recipient.receives_notifications?(:for => :changeset_comment, :via => email) }
+  end

Are we going to include this boilerplate (which I assume comes from a generator) in each notifier? Personally I'd suggest getting rid of it.

> +  def primary_and_foreign_key_types
+    config = Rails.configuration.generators
+    setting = config.options[config.orm][:primary_key_type]
+    primary_key_type = setting || :primary_key
+    foreign_key_type = setting || :bigint
+    [primary_key_type, foreign_key_type]
+  end

This is boilerplate that came from the generator I assume? But we know what our types are so we don't need a lot of complicated logic to figure them out...

> @@ -0,0 +1,18 @@
+# frozen_string_literal: true
+
+class ChangesetCommentNotifier < ApplicationNotifier
+  recipients lambda {

I think `lambda` is a synonym for `->` here? If I'm right then as far as I can see we've always used `->` before but I don't know if it really matters. I'm slightly surprised rubocop isn't trying to enforce a consistent style...

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

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