Skip to content
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/controllers/notifications/settings_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,6 @@ def set_settings
end

def settings_params
params.expect(user_settings: :bundle_email_frequency)
params.expect(user_settings: [ :bundle_email_frequency, :push_notification_level ])
end
end
8 changes: 8 additions & 0 deletions app/helpers/notifications_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,14 @@ def bundle_email_frequency_options_for(settings)
], settings.bundle_email_frequency)
end

def push_notification_level_options_for(settings)
options_for_select([
[ "All activity", "all_activity" ],
[ "Comments and @mentions", "comments_and_mentions" ],
[ "Only @mentions", "only_mentions" ]
], settings.push_notification_level)
end

private
def event_notification_action(event)
if event.action.card_published? && event.eventable.assigned_to?(event.creator)
Expand Down
1 change: 1 addition & 0 deletions app/models/notification/pushable.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ def push_later

def push
return unless pushable?
return unless user.settings.push_notification_enabled_for?(self)

self.class.push_targets.each { |target| push_to(target) }
end
Expand Down
14 changes: 14 additions & 0 deletions app/models/user/settings.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ class User::Settings < ApplicationRecord

enum :bundle_email_frequency, %i[ never every_few_hours daily weekly ],
default: :every_few_hours, prefix: :bundle_email
enum :push_notification_level, %i[ all_activity comments_and_mentions only_mentions ],
default: :all_activity, prefix: :push_notifications

after_update :review_pending_bundles, if: :saved_change_to_bundle_email_frequency?

Expand All @@ -24,6 +26,14 @@ def bundling_emails?
!bundle_email_never? && !user.system? && user.active? && user.verified?
end

def push_notification_enabled_for?(notification)
return true if push_notifications_all_activity?
return true if notification.source_type == "Mention"
return false if push_notifications_only_mentions?

comment_notification?(notification)
end

def timezone
if timezone_name.present?
ActiveSupport::TimeZone[timezone_name] || default_timezone
Expand Down Expand Up @@ -54,4 +64,8 @@ def flush_pending_bundles
def default_timezone
ActiveSupport::TimeZone["UTC"]
end

def comment_notification?(notification)
notification.source_type == "Event" && notification.source.action.comment_created?
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment_notification? relies on notification.source_type == "Event" and then calls notification.source.action. There’s an existing race-condition note in Notifier about source_type/source_id mismatches; if a mismatch ever occurs, notification.source may not be an Event and this will raise at runtime. Prefer guarding on the actual loaded type (e.g., notification.source.is_a?(Event)) before calling action, or otherwise ensure source responds to action in this path.

Suggested change
notification.source_type == "Event" && notification.source.action.comment_created?
source = notification.source
return false unless source.is_a?(Event)
action = source.action
return false unless action.respond_to?(:comment_created?)
action.comment_created?

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rosa is this a valid concern?

end
end
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<section class="notifications__status settings__section panel fill-shade center hide-on-native" data-controller="notifications" data-notifications-subscriptions-url-value="<%= user_push_subscriptions_path(Current.user) %>" data-notifications-enabled-class="notifications--on">
<heading>
<h2 class="txt-medium">
Push notifications are
Browser notifications are
<span class="notifications__on-message">ON</span>
<span class="notifications__off-message">OFF</span>
</h2>
Expand Down
14 changes: 14 additions & 0 deletions app/views/notifications/settings/_push_notification_level.html.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
<section class="settings__section">
<heading>
<h2 class="divider">Push Notifications</h2>
<div>Notifications are sent to your registered devices.</div>
</heading>

<%= form_with model: settings, url: notifications_settings_path,
method: :patch, local: true, data: { controller: "form" } do |form| %>
<div class="margin-block-end-half"><strong><%= form.label :push_notification_level, "Send push notifications for..." %></strong></div>
<%= form.select :push_notification_level, push_notification_level_options_for(settings), {}, class: "input input--select txt-align-center", data: { action: "change->form#submit" } %>
<% end %>

<%= render "notifications/settings/native_devices" if Fizzy.saas? %>
</section>
4 changes: 2 additions & 2 deletions app/views/notifications/settings/show.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@
</div>

<div class="settings__panel panel shadow">
<%= render "notifications/settings/push_notifications" %>
<%= render "notifications/settings/native_devices" if Fizzy.saas? %>
<%= render "notifications/settings/push_notification_level", settings: @settings %>
<%= render "notifications/settings/browser_push_notifications" %>
<%= render "notifications/settings/email", settings: @settings %>
</div>
</section>
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class AddPushNotificationLevelToUserSettings < ActiveRecord::Migration[8.2]
def change
add_column :user_settings, :push_notification_level, :integer, default: 0, null: false
end
end
3 changes: 2 additions & 1 deletion db/schema.rb

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion db/schema_sqlite.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
#
# It's strongly recommended that you check this file into your version control system.

ActiveRecord::Schema[8.2].define(version: 2026_02_18_120000) do
ActiveRecord::Schema[8.2].define(version: 2026_03_03_100000) do
create_table "accesses", id: :uuid, force: :cascade do |t|
t.datetime "accessed_at"
t.uuid "account_id", null: false
Expand Down Expand Up @@ -539,6 +539,7 @@
t.uuid "account_id", null: false
t.integer "bundle_email_frequency", default: 0, null: false
t.datetime "created_at", null: false
t.integer "push_notification_level", default: 0, null: false
t.string "timezone_name", limit: 255
t.datetime "updated_at", null: false
t.uuid "user_id", null: false
Expand Down
9 changes: 9 additions & 0 deletions test/controllers/notifications/settings_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,4 +21,13 @@ class Notifications::SettingsControllerTest < ActionDispatch::IntegrationTest
assert_redirected_to notifications_settings_path
assert_equal "Settings updated", flash[:notice]
end

test "update push notification level" do
assert_changes -> { @user.reload.settings.push_notification_level }, from: "all_activity", to: "only_mentions" do
put notifications_settings_path, params: { user_settings: { push_notification_level: "only_mentions" } }
end

assert_redirected_to notifications_settings_path
assert_equal "Settings updated", flash[:notice]
end
end
30 changes: 30 additions & 0 deletions test/models/notification/pushable_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -94,4 +94,34 @@ class Notification::PushableTest < ActiveSupport::TestCase
ensure
Notification.push_targets = original_targets
end

test "push skips targets when push notification level excludes notification type" do
notification = notifications(:layout_commented_kevin)
notification.user.settings.update!(push_notification_level: :only_mentions)

target_class = mock("push_target_class")
target_class.expects(:process).never

original_targets = Notification.push_targets
Notification.push_targets = [ target_class ]

notification.push
ensure
Notification.push_targets = original_targets
end
Comment on lines +99 to +111
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this test, original_targets is assigned after calls that can raise (e.g., update!). If an exception happens before original_targets is set, the ensure block will raise NameError when trying to restore it, obscuring the real failure. Assign original_targets at the top of the test (or initialize it to Notification.push_targets) before any potentially-raising code.

Copilot uses AI. Check for mistakes.

test "push processes targets when push notification level allows notification type" do
notification = notifications(:layout_commented_kevin)
notification.user.settings.update!(push_notification_level: :comments_and_mentions)

target_class = mock("push_target_class")
target_class.expects(:process).with(notification)

original_targets = Notification.push_targets
Notification.push_targets = [ target_class ]

notification.push
ensure
Notification.push_targets = original_targets
end
Comment on lines +114 to +126
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same cleanup issue as the previous test: original_targets is set late, so the ensure block can raise NameError if an earlier update! (or other line) fails. Capture original_targets before any potentially-raising operations so the ensure cleanup is reliable.

Copilot uses AI. Check for mistakes.
end
24 changes: 24 additions & 0 deletions test/models/user/settings_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -54,4 +54,28 @@ class User::SettingsTest < ActiveSupport::TestCase
@user.update_column(:verified_at, nil)
assert_not @user.settings.bundling_emails?, "Unverified users should not receive bundled emails"
end

test "push_notification_enabled_for? with only_mentions" do
@settings.update!(push_notification_level: :only_mentions)

assert @settings.push_notification_enabled_for?(notifications(:logo_mentioned_david))
assert_not @settings.push_notification_enabled_for?(notifications(:logo_assignment_kevin))
assert_not @settings.push_notification_enabled_for?(notifications(:layout_commented_kevin))
end

test "push_notification_enabled_for? with comments_and_mentions" do
@settings.update!(push_notification_level: :comments_and_mentions)

assert @settings.push_notification_enabled_for?(notifications(:logo_mentioned_david))
assert @settings.push_notification_enabled_for?(notifications(:layout_commented_kevin))
assert_not @settings.push_notification_enabled_for?(notifications(:logo_assignment_kevin))
end

test "push_notification_enabled_for? with all_activity" do
@settings.update!(push_notification_level: :all_activity)

assert @settings.push_notification_enabled_for?(notifications(:logo_mentioned_david))
assert @settings.push_notification_enabled_for?(notifications(:layout_commented_kevin))
assert @settings.push_notification_enabled_for?(notifications(:logo_assignment_kevin))
end
end
Loading