`ActiveAdmin.register WarpEngine::Pipeline` declared no `permit_params`, so every edit handed unpermitted attributes to the model and Rails raised ForbiddenAttributesError. That is not new: the form has been unable to save for as long as it has existed. My flash message on `update` sat at the top of the traceback and made it look like the cause, which it was not — and it is gone anyway, because overriding an ActiveAdmin action to say something is a poor trade for what it can break. The move is written to the log instead. Adding a spec that would have caught it, in the host app, because that is where the ActiveAdmin instance lives: it signs in, PUTs the form, and checks both that the record saves and that the software link moves off the pipeline that had it. Driven the same way by hand against the development database first — 302, the link moved, the previous holder left without one. The engine's other admin resources were checked for the same omission: downloads and releases are read-only and the file manager posts to its own routes, so pipelines was the only one affected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -0,0 +1,56 @@
|
||||
require "rails_helper"
|
||||
require "warden/test/helpers"
|
||||
|
||||
# The pipeline admin form could not save at all: the resource never declared
|
||||
# `permit_params`, so ActiveAdmin handed unpermitted attributes to the model and Rails
|
||||
# raised ForbiddenAttributesError on every edit. No model spec could have caught that —
|
||||
# the fault was one layer up — so the check belongs here, where the host's ActiveAdmin
|
||||
# instance actually runs.
|
||||
RSpec.describe "Admin pipelines", type: :request do
|
||||
include Warden::Test::Helpers
|
||||
|
||||
let(:admin) { AdminUser.create!(email: "pipelines-spec@example.org", password: "password123") }
|
||||
let(:software) { create(:software) }
|
||||
let!(:holder) do
|
||||
WarpEngine::Pipeline.create!(woodpecker_repo_id: 990_001, repo_owner: "spec", repo_name: "holder",
|
||||
platform: "tic80", software: software)
|
||||
end
|
||||
let!(:taker) do
|
||||
WarpEngine::Pipeline.create!(woodpecker_repo_id: 990_002, repo_owner: "spec", repo_name: "taker",
|
||||
platform: "tic80")
|
||||
end
|
||||
|
||||
before do
|
||||
Warden.test_mode!
|
||||
login_as(admin, scope: :admin_user)
|
||||
end
|
||||
|
||||
after { Warden.test_reset! }
|
||||
|
||||
# This app keeps forgery protection on in the test environment, and a request spec has
|
||||
# no rendered form to take a token from. The token is not what is under test here, so it
|
||||
# is switched off for the duration and put back afterwards.
|
||||
around do |example|
|
||||
protection = ActionController::Base.allow_forgery_protection
|
||||
ActionController::Base.allow_forgery_protection = false
|
||||
example.run
|
||||
ActionController::Base.allow_forgery_protection = protection
|
||||
end
|
||||
|
||||
it "saves the form" do
|
||||
put "/admin/pipelines/#{taker.id}", params: { pipeline: { platform: "godot" } }
|
||||
|
||||
expect(response).to have_http_status(:found)
|
||||
expect(taker.reload.platform).to eq("godot")
|
||||
end
|
||||
|
||||
it "moves the software off the pipeline that had it" do
|
||||
put "/admin/pipelines/#{taker.id}",
|
||||
params: { pipeline: { platform: "tic80", software_id: software.id } }
|
||||
|
||||
expect(response).to have_http_status(:found)
|
||||
expect(taker.reload.software).to eq(software)
|
||||
expect(holder.reload.software).to be_nil
|
||||
expect(software.reload.pipeline).to eq(taker)
|
||||
end
|
||||
end
|
||||
@@ -1,6 +1,11 @@
|
||||
ActiveAdmin.register WarpEngine::Pipeline, as: "Pipeline" do
|
||||
actions :index, :show, :edit, :update
|
||||
|
||||
# Without this the edit form cannot save at all: ActiveAdmin hands unpermitted params to
|
||||
# the model and Rails raises ForbiddenAttributesError. The two fields here are the two
|
||||
# the form offers; everything else about a pipeline comes from the Woodpecker sync.
|
||||
permit_params :platform, :software_id
|
||||
|
||||
menu parent: "🌀 WarpEngine", priority: 10, label: "🚀 Pipelines"
|
||||
|
||||
config.sort_order = "repo_name_asc"
|
||||
@@ -48,24 +53,12 @@ ActiveAdmin.register WarpEngine::Pipeline, as: "Pipeline" do
|
||||
collection: WarpEngine::Software.order(:title).map { |s| [ s.title, s.id ] },
|
||||
include_blank: "- none -",
|
||||
hint: "One pipeline per software. Picking one that another pipeline already " \
|
||||
"has moves the link here rather than refusing it."
|
||||
"has moves the link here — that pipeline is left without a software, " \
|
||||
"and the move is written to the log."
|
||||
end
|
||||
f.actions
|
||||
end
|
||||
|
||||
controller do
|
||||
# The reassignment itself is the model's job; this only makes it visible. Without a
|
||||
# word about it, the other pipeline loses its software with nothing on screen to say
|
||||
# that it happened.
|
||||
def update
|
||||
super
|
||||
taken = resource.software_taken_from
|
||||
return if taken.blank?
|
||||
|
||||
flash[:notice] = [ flash[:notice], "Software taken from #{taken.join(', ')}." ].compact.join(" ")
|
||||
end
|
||||
end
|
||||
|
||||
sidebar "Details", only: :show do
|
||||
attributes_table_for resource do
|
||||
row :id
|
||||
|
||||
@@ -55,6 +55,16 @@ module WarpEngine
|
||||
|
||||
others = Pipeline.where(software_id: software_id).where.not(id: id)
|
||||
@software_taken_from = others.map(&:full_name)
|
||||
return if @software_taken_from.empty?
|
||||
|
||||
# Logged rather than flashed. The first attempt at this put a message on screen by
|
||||
# overriding the admin's `update` action, which bypassed the permitted-params path
|
||||
# and made every pipeline edit fail with ForbiddenAttributesError. A silent
|
||||
# reassignment is a small problem; an admin page that cannot save is a large one.
|
||||
Rails.logger.info(
|
||||
"[WarpEngine::Pipeline] #{full_name} took software #{software_id} from " \
|
||||
"#{@software_taken_from.join(', ')}"
|
||||
)
|
||||
others.update_all(software_id: nil, updated_at: Time.current)
|
||||
end
|
||||
|
||||
|
||||
Reference in New Issue
Block a user