[openstreetmap/openstreetmap-website] Notifications badge (PR #7316)
Andy Allan
notifications at github.com
Wed Sep 9 15:53:00 UTC 2026
@gravitystorm requested changes on this pull request.
OK, plenty of inline commentary here.
I'm also not entirely convinced that we should be deleting the notifications. I think it might be better to keep the currently proposed interface, but to have this as "mark as read" instead.
We already have messages, and the badge shows which ones are not read, and you don't need to delete messages to make this number change. We could highlight the unread notifications with the same background colour as unread messages, and potentially even have individual "mark read" buttons on the notification index page too.
I'm not proposing at this stage any way to actually "read" a notification, in the way that message#show marks the message as read automatically. I think a mechanism to manage the read status from the notification index page is enough for now.
This way, the two systems behave similar to each other - a green badge for unread, you don't need to delete things to get the number to change, etc.
> @@ -0,0 +1,13 @@
+# frozen_string_literal: true
+
+module HeaderHelper
+ def notifications_count(which = :all)
+ which = [:messages, :notifications] if which == :all
+ which = Array.wrap(which)
+
+ total = 0
+ total += current_user.new_messages.size if which.include?(:messages)
+ total += current_user.notifications.size if which.include?(:notifications)
This is counting all notifications, whereas the notifications page is filtered by `.where(:type => LISTABLE_NOTIFICATIONS)`. So there will be a number of notifications shown in this badge which are not shown (and not removable) on the page.
This isn't picked up by the tests, since the test creates two `:changeset_comment_notification` and one `:message` - it doesn't create the corresponding notification triggered in the messages_controller#create
> @@ -11,5 +15,14 @@
<% if @notifications.items.empty? %>
<p><%= t(".no_notifications") %></p>
<% else %>
- <%= render "page", :notifications => @notifications, :params => @params %>
+ <%= form_with :url => notifications_path, :method => :delete do |form| %>
+ <div class="row">
+ <div class="col-1 text-center">
+ <%= check_box_tag "select_page", :class => "align-bottom" %>
+ </div>
+ <div class="col-11"><%= form.submit t(".delete_selected"), :class => "submit-read" %></div>
- `submit-read` is an unused class
- this button isn't styled using any bootstrap `btn` classes
> @@ -11,5 +15,14 @@
<% if @notifications.items.empty? %>
<p><%= t(".no_notifications") %></p>
<% else %>
- <%= render "page", :notifications => @notifications, :params => @params %>
+ <%= form_with :url => notifications_path, :method => :delete do |form| %>
+ <div class="row">
+ <div class="col-1 text-center">
+ <%= check_box_tag "select_page", :class => "align-bottom" %>
This check_box_tag isn't picking up any `form-check-input` bootstrap class
> @@ -11,5 +15,14 @@
<% if @notifications.items.empty? %>
<p><%= t(".no_notifications") %></p>
<% else %>
- <%= render "page", :notifications => @notifications, :params => @params %>
+ <%= form_with :url => notifications_path, :method => :delete do |form| %>
+ <div class="row">
+ <div class="col-1 text-center">
I'd like to see screenshots of how this looks on narrow screens
> @@ -0,0 +1,33 @@
+$(function () {
+ const selectPageCheckbox = $("#select_page");
+ const individualCheckboxes = $(".notification-mark-for-deletion");
+
+ individualCheckboxes.on("click", function () {
maybe "change" instead of "click" but I'm not a JS expert
>
def index
notifications = current_user.notifications.where(:type => LISTABLE_NOTIFICATIONS)
@notifications = get_page_items(notifications)
@params = params.permit
end
+
+ def destroy
+ ids_to_delete =
+ params
+ .expect(:notifications => {})
+ .to_unsafe_h
+ .select { |_k, v| v == "delete" }
+ .keys
+ .map { |id| Integer(id) }
+
+ current_user.notifications.where(:id => ids_to_delete).delete_all
Is `delete_all` the correct thing here, or would `destroy_all` be better? I believe there are [counter_cache declarations](https://github.com/excid3/noticed/blob/aca286e10dd5b3e4910e87f89aaf6da151ab2d12/app/models/noticed/notification.rb#L7) inside the noticed gem that will be skipped with a `delete_all`
>
def index
notifications = current_user.notifications.where(:type => LISTABLE_NOTIFICATIONS)
@notifications = get_page_items(notifications)
@params = params.permit
end
+
+ def destroy
+ ids_to_delete =
+ params
+ .expect(:notifications => {})
pressing submit without having ticked anything leads to a 400 error here "param is missing or the value is empty or invalid: notifications".
* should work
* needs a test to ensure it keeps working
>
def index
notifications = current_user.notifications.where(:type => LISTABLE_NOTIFICATIONS)
@notifications = get_page_items(notifications)
@params = params.permit
end
+
+ def destroy
+ ids_to_delete =
+ params
+ .expect(:notifications => {})
+ .to_unsafe_h
+ .select { |_k, v| v == "delete" }
We could rework the form to avoid several issues here. Instead of each checkbox being a key/value pair, we could refactor to submit an array of ids. Here's a sketch:
```
<%= check_box_tag "notifications[]", notification.id, false,
:id => "notification_#{notification.id}",
:class => "form-check-input notification-mark-for-deletion" %>
```
```ruby
def destroy
ids = params.permit(:notifications => []).fetch(:notifications, [])
current_user.notifications.where(:id => ids).destroy_all
redirect_back_or_to notifications_path
end
```
This avoids problems with blank forms, hash manipulations, exceptions when casting to Integer, etc.
> @@ -357,6 +357,7 @@
get "/preferences/edit", :to => redirect(:path => "/preferences/basic"), :as => nil
resources :notifications, :only => [:index]
+ delete "/notifications" => "notifications#destroy"
I'd rather not see any more non-resourceful routes, since that's just more refactoring added to the todo list.
The challenge is deciding which noun and verb to use for what we're trying to do. I would suggest either:
* We want to _create_ a :notification_deletion , or
* We want to _destroy_ a :notification_selection
I lean towards the first:
```ruby
resources :notifications, :only => [:index]
namespace :notifications, :path => "/notifications" do
resources :deletions, :only => :create
end
```
It sort of parallels what we've done with the messages controller for bulk updating read status.
--
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/7316#pullrequestreview-5156288902
You are receiving this because you are subscribed to this thread.
Message ID: <openstreetmap/openstreetmap-website/pull/7316/review/5156288902 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20260909/1deca706/attachment-0001.htm>
More information about the rails-dev
mailing list