From 9d32f3e8457809d5750c2949edf6a83413f8b9d5 Mon Sep 17 00:00:00 2001 From: Rosa Gutierrez Date: Tue, 20 Jan 2026 21:40:25 +0100 Subject: [PATCH] Refactor devices controller and extract registration to model - Remove Users namespace from DevicesController (now just DevicesController) - Create ApplicationPushDevice model extending ActionPushNative::Device - Move device registration logic (find_or_initialize + update) to model - Update User::Devices concern to use ApplicationPushDevice - Fix push notification tests (endpoint validation, job count expectations) - Update push_config_test to use ActionPushNative.config Co-Authored-By: Claude Opus 4.5 --- saas/app/controllers/devices_controller.rb | 28 +++++++++++++ saas/app/models/application_push_device.rb | 7 ++++ saas/app/models/user/devices.rb | 2 +- .../views/{users => }/devices/index.html.erb | 2 +- .../settings/_native_devices.html.erb | 2 +- saas/lib/fizzy/saas/engine.rb | 6 +-- .../{users => }/devices_controller_test.rb | 42 +++++++++---------- saas/test/models/notification_pusher_test.rb | 33 ++++++++------- saas/test/models/push_config_test.rb | 8 ++-- 9 files changed, 82 insertions(+), 48 deletions(-) create mode 100644 saas/app/controllers/devices_controller.rb create mode 100644 saas/app/models/application_push_device.rb rename saas/app/views/{users => }/devices/index.html.erb (78%) rename saas/test/controllers/{users => }/devices_controller_test.rb (82%) diff --git a/saas/app/controllers/devices_controller.rb b/saas/app/controllers/devices_controller.rb new file mode 100644 index 000000000..fd4b7ed83 --- /dev/null +++ b/saas/app/controllers/devices_controller.rb @@ -0,0 +1,28 @@ +class DevicesController < ApplicationController + def index + @devices = Current.user.devices.order(created_at: :desc) + end + + def create + ApplicationPushDevice.register(owner: Current.user, **device_params) + head :created + rescue ArgumentError + head :bad_request + end + + def destroy + if params[:token].present? + Current.user.devices.destroy_by(token: params[:token]) + head :no_content + else + Current.user.devices.destroy_by(id: params[:id]) + redirect_to devices_path, notice: "Device removed" + end + end + + private + def device_params + params.require([ :token, :platform ]) + params.permit(:token, :platform, :name).to_h.symbolize_keys + end +end diff --git a/saas/app/models/application_push_device.rb b/saas/app/models/application_push_device.rb new file mode 100644 index 000000000..6547ec206 --- /dev/null +++ b/saas/app/models/application_push_device.rb @@ -0,0 +1,7 @@ +class ApplicationPushDevice < ActionPushNative::Device + def self.register(owner:, token:, platform:, name: nil) + owner.devices.find_or_initialize_by(token: token).tap do |device| + device.update!(platform: platform.downcase, name: name) + end + end +end diff --git a/saas/app/models/user/devices.rb b/saas/app/models/user/devices.rb index 25bd6e4b2..df198df16 100644 --- a/saas/app/models/user/devices.rb +++ b/saas/app/models/user/devices.rb @@ -2,6 +2,6 @@ module User::Devices extend ActiveSupport::Concern included do - has_many :devices, class_name: "ActionPushNative::Device", as: :owner, dependent: :destroy + has_many :devices, class_name: "ApplicationPushDevice", as: :owner, dependent: :destroy end end diff --git a/saas/app/views/users/devices/index.html.erb b/saas/app/views/devices/index.html.erb similarity index 78% rename from saas/app/views/users/devices/index.html.erb rename to saas/app/views/devices/index.html.erb index 4a9a02486..bb7d74871 100644 --- a/saas/app/views/users/devices/index.html.erb +++ b/saas/app/views/devices/index.html.erb @@ -7,7 +7,7 @@ <%= device.name || "Unnamed device" %> (<%= device.platform == "apple" ? "iOS" : "Android" %>) Added <%= time_ago_in_words(device.created_at) %> ago - <%= button_to "Remove", users_device_path(device), method: :delete, data: { confirm: "Remove this device?" } %> + <%= button_to "Remove", device_path(device), method: :delete, data: { confirm: "Remove this device?" } %> <% end %> diff --git a/saas/app/views/notifications/settings/_native_devices.html.erb b/saas/app/views/notifications/settings/_native_devices.html.erb index a3e95b392..9931f822f 100644 --- a/saas/app/views/notifications/settings/_native_devices.html.erb +++ b/saas/app/views/notifications/settings/_native_devices.html.erb @@ -5,7 +5,7 @@

You have <%= pluralize(Current.user.devices.count, "mobile device") %> registered for push notifications.

- <%= link_to "Manage devices", users_devices_path, class: "btn txt-small" %> + <%= link_to "Manage devices", devices_path, class: "btn txt-small" %> <% else %>

No mobile devices registered. Install the iOS or Android app to receive push notifications on your phone. diff --git a/saas/lib/fizzy/saas/engine.rb b/saas/lib/fizzy/saas/engine.rb index 3dc1902eb..0c394ed3a 100644 --- a/saas/lib/fizzy/saas/engine.rb +++ b/saas/lib/fizzy/saas/engine.rb @@ -44,10 +44,8 @@ module Fizzy resource :webhooks, only: :create end - namespace :users do - resources :devices, only: [ :index, :create, :destroy ] do - delete :destroy, on: :collection, as: :unregister - end + resources :devices, only: [ :index, :create, :destroy ] do + delete :destroy, on: :collection, as: :unregister end end end diff --git a/saas/test/controllers/users/devices_controller_test.rb b/saas/test/controllers/devices_controller_test.rb similarity index 82% rename from saas/test/controllers/users/devices_controller_test.rb rename to saas/test/controllers/devices_controller_test.rb index 08383c3fe..331c2f722 100644 --- a/saas/test/controllers/users/devices_controller_test.rb +++ b/saas/test/controllers/devices_controller_test.rb @@ -1,6 +1,6 @@ require "test_helper" -class Users::DevicesControllerTest < ActionDispatch::IntegrationTest +class DevicesControllerTest < ActionDispatch::IntegrationTest setup do @user = users(:david) sign_in_as @user @@ -11,7 +11,7 @@ class Users::DevicesControllerTest < ActionDispatch::IntegrationTest test "index shows user devices" do @user.devices.create!(token: "test_token_123", platform: "apple", name: "iPhone 15 Pro") - get users_devices_path + get devices_path assert_response :success assert_select "strong", "iPhone 15 Pro" @@ -21,7 +21,7 @@ class Users::DevicesControllerTest < ActionDispatch::IntegrationTest test "index shows empty state when no devices" do @user.devices.delete_all - get users_devices_path + get devices_path assert_response :success assert_select "p", /No devices registered/ @@ -30,7 +30,7 @@ class Users::DevicesControllerTest < ActionDispatch::IntegrationTest test "index requires authentication" do sign_out - get users_devices_path + get devices_path assert_response :redirect end @@ -41,7 +41,7 @@ class Users::DevicesControllerTest < ActionDispatch::IntegrationTest token = SecureRandom.hex(32) assert_difference "ActionPushNative::Device.count", 1 do - post users_devices_path, params: { + post devices_path, params: { token: token, platform: "apple", name: "iPhone 15 Pro" @@ -58,7 +58,7 @@ class Users::DevicesControllerTest < ActionDispatch::IntegrationTest end test "creates android device" do - post users_devices_path, params: { + post devices_path, params: { token: SecureRandom.hex(32), platform: "google", name: "Pixel 8" @@ -83,7 +83,7 @@ class Users::DevicesControllerTest < ActionDispatch::IntegrationTest # Current user registers the same token with their own device assert_difference "ActionPushNative::Device.count", 1 do - post users_devices_path, params: { + post devices_path, params: { token: shared_token, platform: "apple", name: "David's iPhone" @@ -102,7 +102,7 @@ class Users::DevicesControllerTest < ActionDispatch::IntegrationTest end test "rejects invalid platform" do - post users_devices_path, params: { + post devices_path, params: { token: SecureRandom.hex(32), platform: "windows", name: "Surface" @@ -112,7 +112,7 @@ class Users::DevicesControllerTest < ActionDispatch::IntegrationTest end test "rejects missing token" do - post users_devices_path, params: { + post devices_path, params: { platform: "apple", name: "iPhone" }, as: :json @@ -123,7 +123,7 @@ class Users::DevicesControllerTest < ActionDispatch::IntegrationTest test "create requires authentication" do sign_out - post users_devices_path, params: { + post devices_path, params: { token: SecureRandom.hex(32), platform: "apple" }, as: :json @@ -141,19 +141,19 @@ class Users::DevicesControllerTest < ActionDispatch::IntegrationTest ) assert_difference "ActionPushNative::Device.count", -1 do - delete users_device_path(device) + delete device_path(device) end - assert_redirected_to users_devices_path + assert_redirected_to devices_path assert_not ActionPushNative::Device.exists?(device.id) end test "does nothing when device not found by id" do assert_no_difference "ActionPushNative::Device.count" do - delete users_device_path(id: "nonexistent") + delete device_path(id: "nonexistent") end - assert_redirected_to users_devices_path + assert_redirected_to devices_path end test "cannot destroy another user's device by id" do @@ -165,10 +165,10 @@ class Users::DevicesControllerTest < ActionDispatch::IntegrationTest ) assert_no_difference "ActionPushNative::Device.count" do - delete users_device_path(device) + delete device_path(device) end - assert_redirected_to users_devices_path + assert_redirected_to devices_path assert ActionPushNative::Device.exists?(device.id) end @@ -181,7 +181,7 @@ class Users::DevicesControllerTest < ActionDispatch::IntegrationTest sign_out - delete users_device_path(device) + delete device_path(device) assert_response :redirect assert ActionPushNative::Device.exists?(device.id) @@ -197,7 +197,7 @@ class Users::DevicesControllerTest < ActionDispatch::IntegrationTest ) assert_difference "ActionPushNative::Device.count", -1 do - delete unregister_users_devices_path, params: { token: "token_to_unregister" }, as: :json + delete unregister_devices_path, params: { token: "token_to_unregister" }, as: :json end assert_response :no_content @@ -206,7 +206,7 @@ class Users::DevicesControllerTest < ActionDispatch::IntegrationTest test "does nothing when device not found by token" do assert_no_difference "ActionPushNative::Device.count" do - delete unregister_users_devices_path, params: { token: "nonexistent_token" }, as: :json + delete unregister_devices_path, params: { token: "nonexistent_token" }, as: :json end assert_response :no_content @@ -221,7 +221,7 @@ class Users::DevicesControllerTest < ActionDispatch::IntegrationTest ) assert_no_difference "ActionPushNative::Device.count" do - delete unregister_users_devices_path, params: { token: "other_users_token" }, as: :json + delete unregister_devices_path, params: { token: "other_users_token" }, as: :json end assert_response :no_content @@ -237,7 +237,7 @@ class Users::DevicesControllerTest < ActionDispatch::IntegrationTest sign_out - delete unregister_users_devices_path, params: { token: "my_token" }, as: :json + delete unregister_devices_path, params: { token: "my_token" }, as: :json assert_response :redirect assert ActionPushNative::Device.exists?(device.id) diff --git a/saas/test/models/notification_pusher_test.rb b/saas/test/models/notification_pusher_test.rb index 2c628b416..70a9f90ef 100644 --- a/saas/test/models/notification_pusher_test.rb +++ b/saas/test/models/notification_pusher_test.rb @@ -59,14 +59,14 @@ class NotificationPusherNativeTest < ActiveSupport::TestCase # === Has Any Push Destination === test "push_destination returns true when user has native devices" do - @user.devices.create!(uuid: SecureRandom.uuid, token: "test123", platform: "apple", name: "Test iPhone") + @user.devices.create!(token: "test123", platform: "apple", name: "Test iPhone") assert @pusher.send(:push_destination?) end test "push_destination returns true when user has web subscriptions" do @user.push_subscriptions.create!( - endpoint: "https://example.com/push", + endpoint: "https://fcm.googleapis.com/fcm/send/test", p256dh_key: "test_p256dh", auth_key: "test_auth" ) @@ -85,7 +85,7 @@ class NotificationPusherNativeTest < ActiveSupport::TestCase test "push delivers to native devices when user has devices" do stub_push_services - @user.devices.create!(uuid: SecureRandom.uuid, token: "test123", platform: "apple", name: "Test iPhone") + @user.devices.create!(token: "test123", platform: "apple", name: "Test iPhone") assert_native_push_delivery(count: 1) do @pusher.push @@ -102,7 +102,7 @@ class NotificationPusherNativeTest < ActiveSupport::TestCase test "push does not deliver when creator is system user" do stub_push_services - @user.devices.create!(uuid: SecureRandom.uuid, token: "test123", platform: "apple", name: "Test iPhone") + @user.devices.create!(token: "test123", platform: "apple", name: "Test iPhone") @notification.update!(creator: users(:system)) result = @pusher.push @@ -112,10 +112,11 @@ class NotificationPusherNativeTest < ActiveSupport::TestCase test "push delivers to multiple devices" do stub_push_services - @user.devices.create!(uuid: SecureRandom.uuid, token: "token1", platform: "apple", name: "iPhone") - @user.devices.create!(uuid: SecureRandom.uuid, token: "token2", platform: "google", name: "Pixel") + @user.devices.delete_all + @user.devices.create!(token: "token1", platform: "apple", name: "iPhone") + @user.devices.create!(token: "token2", platform: "google", name: "Pixel") - assert_native_push_delivery(count: 1) do + assert_native_push_delivery(count: 2) do @pusher.push end end @@ -131,7 +132,7 @@ class NotificationPusherNativeTest < ActiveSupport::TestCase ) # Set up native device - @user.devices.create!(uuid: SecureRandom.uuid, token: "native_token", platform: "apple", name: "iPhone") + @user.devices.create!(token: "native_token", platform: "apple", name: "iPhone") # Mock web push pool to verify it receives the payload web_push_pool = mock("web_push_pool") @@ -149,7 +150,7 @@ class NotificationPusherNativeTest < ActiveSupport::TestCase # === Native Notification Building === test "native notification includes required fields" do - @user.devices.create!(uuid: SecureRandom.uuid, token: "test123", platform: "apple", name: "Test iPhone") + @user.devices.create!(token: "test123", platform: "apple", name: "Test iPhone") payload = @pusher.send(:build_payload) native = @pusher.send(:native_notification, payload) @@ -159,7 +160,7 @@ class NotificationPusherNativeTest < ActiveSupport::TestCase end test "native notification sets thread_id from card" do - @user.devices.create!(uuid: SecureRandom.uuid, token: "test123", platform: "apple", name: "Test iPhone") + @user.devices.create!(token: "test123", platform: "apple", name: "Test iPhone") payload = @pusher.send(:build_payload) native = @pusher.send(:native_notification, payload) @@ -168,7 +169,7 @@ class NotificationPusherNativeTest < ActiveSupport::TestCase test "native notification sets high_priority for assignments" do notification = notifications(:logo_assignment_kevin) - notification.user.devices.create!(uuid: SecureRandom.uuid, token: "test123", platform: "apple", name: "Test iPhone") + notification.user.devices.create!(token: "test123", platform: "apple", name: "Test iPhone") pusher = NotificationPusher.new(notification) payload = pusher.send(:build_payload) @@ -178,7 +179,7 @@ class NotificationPusherNativeTest < ActiveSupport::TestCase end test "native notification sets normal priority for non-assignments" do - @user.devices.create!(uuid: SecureRandom.uuid, token: "test123", platform: "apple", name: "Test iPhone") + @user.devices.create!(token: "test123", platform: "apple", name: "Test iPhone") payload = @pusher.send(:build_payload) native = @pusher.send(:native_notification, payload) @@ -188,7 +189,7 @@ class NotificationPusherNativeTest < ActiveSupport::TestCase # === Apple-specific Payload === test "native notification includes apple-specific fields" do - @user.devices.create!(uuid: SecureRandom.uuid, token: "test123", platform: "apple", name: "Test iPhone") + @user.devices.create!(token: "test123", platform: "apple", name: "Test iPhone") payload = @pusher.send(:build_payload) native = @pusher.send(:native_notification, payload) @@ -200,7 +201,7 @@ class NotificationPusherNativeTest < ActiveSupport::TestCase # === Google-specific Payload === test "native notification sets android notification to nil for data-only" do - @user.devices.create!(uuid: SecureRandom.uuid, token: "test123", platform: "apple", name: "Test iPhone") + @user.devices.create!(token: "test123", platform: "apple", name: "Test iPhone") payload = @pusher.send(:build_payload) native = @pusher.send(:native_notification, payload) @@ -210,11 +211,11 @@ class NotificationPusherNativeTest < ActiveSupport::TestCase # === Data Payload === test "native notification includes data payload" do - @user.devices.create!(uuid: SecureRandom.uuid, token: "test123", platform: "apple", name: "Test iPhone") + @user.devices.create!(token: "test123", platform: "apple", name: "Test iPhone") payload = @pusher.send(:build_payload) native = @pusher.send(:native_notification, payload) - assert_not_nil native.data[:path] + assert_not_nil native.data[:url] assert_equal @notification.account.external_account_id, native.data[:account_id] assert_equal @notification.creator.name, native.data[:creator_name] end diff --git a/saas/test/models/push_config_test.rb b/saas/test/models/push_config_test.rb index 979194b3f..554315818 100644 --- a/saas/test/models/push_config_test.rb +++ b/saas/test/models/push_config_test.rb @@ -4,11 +4,11 @@ class PushConfigTest < ActiveSupport::TestCase test "loads push config from the saas engine" do skip unless Fizzy.saas? - config = Rails.application.config_for(:push) + config = ActionPushNative.config - apple_team_id = config.dig("apple", "team_id") - apple_topic = config.dig("apple", "topic") - google_project_id = config.dig("google", "project_id") + apple_team_id = config.dig(:apple, :team_id) + apple_topic = config.dig(:apple, :topic) + google_project_id = config.dig(:google, :project_id) skip "Update test once APNS team_id is configured" if apple_team_id == "YOUR_TEAM_ID" skip "Update test once APNS topic is configured" if apple_topic == "com.yourcompany.fizzy"