From d3cc79a5bcb4c0b41f7edfe2cea9345e8848c47a Mon Sep 17 00:00:00 2001 From: Rosa Gutierrez Date: Wed, 21 Jan 2026 13:20:14 +0100 Subject: [PATCH] Simplify device routes and use ActiveRecord validations - Use RESTful DELETE /devices/:id where :id can be token or database ID - Remove redundant unregister collection route - Remove old Users::DevicesController - Return 404 when device not found instead of silently succeeding - Return 422 for invalid platform via ActiveRecord validation - Update action_push_native to main branch (includes validate: true on enum) Co-Authored-By: Claude Opus 4.5 --- Gemfile.saas | 2 +- Gemfile.saas.lock | 11 +++--- saas/app/controllers/devices_controller.rb | 18 +++++---- .../controllers/users/devices_controller.rb | 38 ------------------- saas/lib/fizzy/saas/engine.rb | 4 +- .../controllers/devices_controller_test.rb | 34 +++++++---------- 6 files changed, 30 insertions(+), 77 deletions(-) delete mode 100644 saas/app/controllers/users/devices_controller.rb diff --git a/Gemfile.saas b/Gemfile.saas index 28b3f9b11..913dad385 100644 --- a/Gemfile.saas +++ b/Gemfile.saas @@ -12,7 +12,7 @@ gem "console1984", bc: "console1984" gem "audits1984", bc: "audits1984", branch: "flavorjones/coworker-api" # Native push notifications (iOS/Android) -gem "action_push_native", github: "rails/action_push_native", branch: "use-registered-config-path" +gem "action_push_native", github: "rails/action_push_native" # Telemetry gem "rails_structured_logging", bc: "rails-structured-logging" diff --git a/Gemfile.saas.lock b/Gemfile.saas.lock index ce398e00d..0a56a7f67 100644 --- a/Gemfile.saas.lock +++ b/Gemfile.saas.lock @@ -62,8 +62,7 @@ GIT GIT remote: https://github.com/rails/action_push_native.git - revision: d5c44514e13faf919261ca7943fc43aedd8e992e - branch: use-registered-config-path + revision: 9fb4a2bfe54270b1a3508028f00aaa586e257655 specs: action_push_native (0.3.0) activejob (>= 8.0) @@ -221,8 +220,8 @@ GEM activemodel (>= 7.0) activemodel-serializers-xml (~> 1.0) activesupport (>= 7.0) - addressable (2.8.7) - public_suffix (>= 2.0.2, < 7.0) + addressable (2.8.8) + public_suffix (>= 2.0.2, < 8.0) anyway_config (2.7.2) ruby-next-core (~> 1.0) ast (2.4.3) @@ -319,7 +318,7 @@ GEM base64 (~> 0.2) faraday (>= 1.0, < 3.a) google-logging-utils (0.2.0) - googleauth (1.16.0) + googleauth (1.16.1) faraday (>= 1.0, < 3.a) google-cloud-env (~> 2.2) google-logging-utils (~> 0.1) @@ -495,7 +494,7 @@ GEM psych (5.3.1) date stringio - public_suffix (6.0.2) + public_suffix (7.0.2) puma (7.2.0) nio4r (~> 2.0) raabro (1.4.0) diff --git a/saas/app/controllers/devices_controller.rb b/saas/app/controllers/devices_controller.rb index fd4b7ed83..081c322b8 100644 --- a/saas/app/controllers/devices_controller.rb +++ b/saas/app/controllers/devices_controller.rb @@ -1,4 +1,6 @@ class DevicesController < ApplicationController + before_action :set_device, only: :destroy + def index @devices = Current.user.devices.order(created_at: :desc) end @@ -6,21 +8,21 @@ class DevicesController < ApplicationController 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" + @device.destroy + respond_to do |format| + format.html { redirect_to devices_path, notice: "Device removed" } + format.json { head :no_content } end end private + def set_device + @device = Current.user.devices.find_by(token: params[:id]) || Current.user.devices.find(params[:id]) + end + def device_params params.require([ :token, :platform ]) params.permit(:token, :platform, :name).to_h.symbolize_keys diff --git a/saas/app/controllers/users/devices_controller.rb b/saas/app/controllers/users/devices_controller.rb deleted file mode 100644 index 935502408..000000000 --- a/saas/app/controllers/users/devices_controller.rb +++ /dev/null @@ -1,38 +0,0 @@ -class Users::DevicesController < ApplicationController - before_action :set_devices - - rescue_from ActiveRecord::NotNullViolation, ArgumentError, with: :bad_request - - def index - end - - def create - @devices.create!(device_params) - head :created - end - - def destroy - if params[:token].present? - @devices.destroy_by(token: params[:token]) - head :no_content - else - @devices.destroy_by(id: params[:id]) - redirect_to users_devices_path, notice: "Device removed" - end - end - - private - def set_devices - @devices = Current.user.devices.order(created_at: :desc) - end - - def device_params - params.permit(:token, :platform, :name).tap do |permitted| - permitted[:platform] = permitted[:platform].to_s.downcase if permitted[:platform].present? - end - end - - def bad_request - head :bad_request - end -end diff --git a/saas/lib/fizzy/saas/engine.rb b/saas/lib/fizzy/saas/engine.rb index 0c394ed3a..e2e86c033 100644 --- a/saas/lib/fizzy/saas/engine.rb +++ b/saas/lib/fizzy/saas/engine.rb @@ -44,9 +44,7 @@ module Fizzy resource :webhooks, only: :create end - resources :devices, only: [ :index, :create, :destroy ] do - delete :destroy, on: :collection, as: :unregister - end + resources :devices, only: [ :index, :create, :destroy ] end end diff --git a/saas/test/controllers/devices_controller_test.rb b/saas/test/controllers/devices_controller_test.rb index 331c2f722..0cfbbd612 100644 --- a/saas/test/controllers/devices_controller_test.rb +++ b/saas/test/controllers/devices_controller_test.rb @@ -6,8 +6,6 @@ class DevicesControllerTest < ActionDispatch::IntegrationTest sign_in_as @user end - # === Index (Web) === - test "index shows user devices" do @user.devices.create!(token: "test_token_123", platform: "apple", name: "iPhone 15 Pro") @@ -35,8 +33,6 @@ class DevicesControllerTest < ActionDispatch::IntegrationTest assert_response :redirect end - # === Create (API) === - test "creates a new device via api" do token = SecureRandom.hex(32) @@ -108,7 +104,7 @@ class DevicesControllerTest < ActionDispatch::IntegrationTest name: "Surface" }, as: :json - assert_response :bad_request + assert_response :unprocessable_entity end test "rejects missing token" do @@ -131,8 +127,6 @@ class DevicesControllerTest < ActionDispatch::IntegrationTest assert_response :redirect end - # === Destroy (Web) === - test "destroys device by id" do device = @user.devices.create!( token: "token_to_delete", @@ -148,15 +142,15 @@ class DevicesControllerTest < ActionDispatch::IntegrationTest assert_not ActionPushNative::Device.exists?(device.id) end - test "does nothing when device not found by id" do + test "returns not found when device not found by id" do assert_no_difference "ActionPushNative::Device.count" do delete device_path(id: "nonexistent") end - assert_redirected_to devices_path + assert_response :not_found end - test "cannot destroy another user's device by id" do + test "returns not found for another user's device by id" do other_user = users(:kevin) device = other_user.devices.create!( token: "other_users_token", @@ -168,7 +162,7 @@ class DevicesControllerTest < ActionDispatch::IntegrationTest delete device_path(device) end - assert_redirected_to devices_path + assert_response :not_found assert ActionPushNative::Device.exists?(device.id) end @@ -187,8 +181,6 @@ class DevicesControllerTest < ActionDispatch::IntegrationTest assert ActionPushNative::Device.exists?(device.id) end - # === Destroy by Token (API) === - test "destroys device by token" do device = @user.devices.create!( token: "token_to_unregister", @@ -197,22 +189,22 @@ class DevicesControllerTest < ActionDispatch::IntegrationTest ) assert_difference "ActionPushNative::Device.count", -1 do - delete unregister_devices_path, params: { token: "token_to_unregister" }, as: :json + delete device_path("token_to_unregister"), as: :json end assert_response :no_content assert_not ActionPushNative::Device.exists?(device.id) end - test "does nothing when device not found by token" do + test "returns not found when device not found by token" do assert_no_difference "ActionPushNative::Device.count" do - delete unregister_devices_path, params: { token: "nonexistent_token" }, as: :json + delete device_path("nonexistent_token"), as: :json end - assert_response :no_content + assert_response :not_found end - test "cannot destroy another user's device by token" do + test "returns not found for another user's device by token" do other_user = users(:kevin) device = other_user.devices.create!( token: "other_users_token", @@ -221,10 +213,10 @@ class DevicesControllerTest < ActionDispatch::IntegrationTest ) assert_no_difference "ActionPushNative::Device.count" do - delete unregister_devices_path, params: { token: "other_users_token" }, as: :json + delete device_path("other_users_token"), as: :json end - assert_response :no_content + assert_response :not_found assert ActionPushNative::Device.exists?(device.id) end @@ -237,7 +229,7 @@ class DevicesControllerTest < ActionDispatch::IntegrationTest sign_out - delete unregister_devices_path, params: { token: "my_token" }, as: :json + delete device_path("my_token"), as: :json assert_response :redirect assert ActionPushNative::Device.exists?(device.id)