This is a proposal for a new way to handle event notifications. There are no changes to functionality. I have marked it as "draft" as there are some decisions pending.
This proposal uses the gem [noticed](https://github.com/excid3/noticed) as foundation. This introduces a relatively light layer that allows us to declare notifications and ways to deliver them. I chose this gem after a bit of research, where [I concluded that](https://github.com/openstreetmap/openstreetmap-website/issues/1387#issuecomment-3382190265): - it's an active project, with a responsive maintainer, - it's widely used (I asked around and it seems to be the preferred option), - the design uses the right abstraction, separating the events from the multiple notifications they may generate (in line with @gravitystorm's thoughts at https://github.com/openstreetmap/openstreetmap-website/issues/908#issuecomment-508106079). Even if in the end we decide to go in another direction, I believe that this structure would be common to any solution we end up adopting. ### Noticed, the gem This gem is quite simple. These are the main concepts: - **Events** model _what_ happens in the system. Eg: a comment was left on a changeset. They are stored in `notified_events` - **Notifications** model _who_ is notified of events. Eg: which individual users are made aware of the event. They are stored in `notified_notifications`. - **Notifiers** are where we put code. They handle events and decide who receives notifications and by which means (eg: email, Matrix, push, etc). They can also run validation and other hooks. Also a couple of oddities: - By default it stores the number of notifications sent for each event. Eg: "5 users were notified when this changeset comment was created". I'm not sure this is very useful, but it catches the eye as it comes with its own migration, due to it having been added later in the history of the gem. I don't think it's a big deal, but worth raising. - Notifications include `seen_at` and `read_at` fields. Their exact intent is not apparent, either in documentation or code. In [a discussion on this topic](https://github.com/excid3/noticed/discussions/497) it is suggested that apps (that's us) mark notifications as "seen" when a user has seen a notification, and "read" when the contents have actually been fully read. Other interpretations could be used, I guess. In any case, this will not always be useful, so we don't necessarily have to use these fields. ### Rails's two styles of mailer methods In this PR I have ported two existing notifications to use Noticed. This is to illustrate what it would be like to port all current emails, which could or could not be done as part of this PR. There's one particular detail to consider: the style of mailer methods. Rails has two ways of calling a mailer to deliver a message: - The "classic" style, which uses positional arguments. Eg: `UserMailer.changeset_comment_notification(comment, user).deliver_later`. - The "modern" style, which uses keyword arguments. Eg: `UserMailer.with(comment:, user:).note_comment_notification.deliver_later`. There's a lot of nuance here. I believe both styles can be mixed (I haven't tried). The "modern" style has been the one in the Rails guides since v5.2. I understand that the "classic" one is not actually deprecated, and there are no plans for that. Both are used in current codebases. This is a lot of intro to say: by default, Noticed expects the "modern" style. It can use the "classic" style, but it requires an additional configuration line for each notifier. The examples I have implemented here show this distinction: - `ChangesetCommentNotifier` expects the "modern" style. This requires adapting `UserMailer#changeset_comment_notification` to receive arguments via `params`, plus method calls like those in `UserMailerTest`. - `NoteCommentNotifier` expects the "classic" style. It requires adding a configuration line `config.args = -> { [record, recipient] }` to the notifier so that it knows how to position the arguments. My soft preference would be for moving all our mailers to use the modern style in a separate PR. Rationale: - For the benefit of contributors new to the framework, as this is the style demonstrated on the Rails guides. - Each individual notifier would need need less code (one fewer line). ### Going forward Once this is in place, we can implement new functionality around notifications. For example: - A list of notifications that users can see in their dashboards (or wherever, just an example). - Preferences on whether to receive emails for each notification type. - Additional means of delivery. Whatever we decide to implement, we'll also have to consider adding an API for other clients. We'll also need to be careful that we don't enable abusers in some way. That however has no bearing on this current PR. ### Proof of concept As part of my research, I have gone ahead and implemented a proof of concept of how we could implement a list of notifications: <img width="975" height="321" alt="The user dashboard, with a new &quot;Notifications&quot; section showing a bullet list with two notifications: one about a comment on a note, and another about a comment on a changeset. The username dropdown on the top right also shows a number 2, suggesting that there are two notifications to see." src="https://github.com/user-attachments/assets/bdb10e2f-4922-4ed6-b361-0dab89c31c3d" /> Again, that's purely a proof of concept; don't get too hung up on the UI. The only intention is to demonstrate what it would be to work with this architecture. The code for this proof of concept is at https://github.com/pablobm/openstreetmap-website/compare/noticed...pablobm:openstreetmap-website:notifications. This is also deployed at https://pablobm.apis.dev.openstreetmap.org, where you can test yourself. Two users are already created: `mapper` and `anothermapper`, both with password `password.` There's a changeset and a note by Trafalgar Square in London. You can view, comment on, or merge this pull request online at: https://github.com/openstreetmap/openstreetmap-website/pull/6837 -- Commit Summary -- * $ bundle add noticed * $ bin/rails noticed:install:migrations # Plus linting * $ bin/rails db:migrate * $ bin/rails generate noticed:notifier ChangesetComment * First use of gem `noticed` to deliver notifications * Example with a mailer that uses classic, positional arguments * Example of how to disable emails while still being notified -- File Changes -- M Gemfile (2) M Gemfile.lock (3) M app/controllers/api/changeset_comments_controller.rb (4) M app/controllers/api/notes_controller.rb (4) M app/mailers/user_mailer.rb (3) A app/notifiers/application_notifier.rb (4) A app/notifiers/changeset_comment_notifier.rb (18) A app/notifiers/note_comment_notifier.rb (19) A db/migrate/20260223105922_create_noticed_tables.noticed.rb (39) A db/migrate/20260223105923_add_notifications_count_to_noticed_event.noticed.rb (8) M db/structure.sql (124) M test/mailers/user_mailer_test.rb (2) A test/notifiers/note_comment_notifier_test.rb (32) -- Patch Links -- https://github.com/openstreetmap/openstreetmap-website/pull/6837.patch https://github.com/openstreetmap/openstreetmap-website/pull/6837.diff -- Reply to this email directly or view it on GitHub: https://github.com/openstreetmap/openstreetmap-website/pull/6837 You are receiving this because you are subscribed to this thread. Message ID: <openstreetmap/openstreetmap-website/pull/[email protected]>
_______________________________________________ rails-dev mailing list [email protected] https://lists.openstreetmap.org/listinfo/rails-dev
