From b159b40488b21c6ce014b27ad79e1efc01e593d5 Mon Sep 17 00:00:00 2001 From: ishikawa999 <14245262+ishikawa999@users.noreply.github.com> Date: Mon, 24 Aug 2026 09:28:30 +0900 Subject: [PATCH] Add REST API support for create on custom fields The type parameter is also accepted nested under custom_field, which is the only place an XML request body can carry it. The HTML form keeps passing it as a top-level parameter, so that path is unchanged. HTML requests to the show action now return 406 instead of 404, following MembersController#show which is API only as well. The destroy action now treats a destroy that returns false as a failure, not only one that raises. Previously that case redirected without any message on the HTML side. --- app/controllers/custom_fields_controller.rb | 45 ++- .../custom_fields_controller_test.rb | 32 ++- .../api_test/custom_fields_test.rb | 260 +++++++++++++++++- .../integration/routing/custom_fields_test.rb | 1 + 4 files changed, 312 insertions(+), 26 deletions(-) diff --git a/app/controllers/custom_fields_controller.rb b/app/controllers/custom_fields_controller.rb index 476ff13d8..863a72988 100644 --- a/app/controllers/custom_fields_controller.rb +++ b/app/controllers/custom_fields_controller.rb @@ -24,7 +24,7 @@ class CustomFieldsController < ApplicationController before_action :require_admin before_action :build_new_custom_field, :only => [:new, :create] before_action :find_custom_field, :only => [:show, :edit, :update, :destroy] - accept_api_auth :index, :show, :update, :destroy + accept_api_auth :index, :show, :create, :update, :destroy def index respond_to do |format| @@ -46,7 +46,7 @@ class CustomFieldsController < ApplicationController def show respond_to do |format| - format.html {render_404} + format.html {head :not_acceptable} format.api end end @@ -58,15 +58,25 @@ class CustomFieldsController < ApplicationController def create if @custom_field.save - flash[:notice] = l(:notice_successful_create) call_hook(:controller_custom_fields_new_after_save, :params => params, :custom_field => @custom_field) - if params[:continue] - redirect_to new_custom_field_path({:type => @custom_field.type}) - else - redirect_to custom_fields_path({:tab => @custom_field.type}) + respond_to do |format| + format.html do + flash[:notice] = l(:notice_successful_create) + if params[:continue] + redirect_to new_custom_field_path({:type => @custom_field.type}) + else + redirect_to custom_fields_path({:tab => @custom_field.type}) + end + end + format.api do + render :action => 'show', :status => :created, :location => custom_field_url(@custom_field) + end end else - render :action => 'new' + respond_to do |format| + format.html {render :action => 'new'} + format.api {render_validation_errors(@custom_field)} + end end end @@ -124,9 +134,13 @@ class CustomFieldsController < ApplicationController private def build_new_custom_field - @custom_field = CustomField.new_subclass_instance(params[:type]) + @custom_field = CustomField.new_subclass_instance(custom_field_type_from_params) if @custom_field.nil? - render :action => 'select_type' + if api_request? + render_api_errors("#{l(:field_type)} #{l('activerecord.errors.messages.invalid')}") + else + render :action => 'select_type' + end else if params[:copy].present? && (@copy_from = CustomField.find_by(id: params[:copy])) @custom_field.copy_from(@copy_from) @@ -135,6 +149,17 @@ class CustomFieldsController < ApplicationController end end + # The HTML form and the query string pass type as a top-level parameter, + # while an XML body has to nest it under custom_field, so API requests also + # look there. + def custom_field_type_from_params + return params[:type] if params[:type].present? + return unless api_request? + + custom_field_params = params[:custom_field] + custom_field_params[:type] if custom_field_params.is_a?(ActionController::Parameters) + end + def find_custom_field @custom_field = CustomField.find(params[:id]) rescue ActiveRecord::RecordNotFound diff --git a/test/functional/custom_fields_controller_test.rb b/test/functional/custom_fields_controller_test.rb index a11bc1f41..7b33c892a 100644 --- a/test/functional/custom_fields_controller_test.rb +++ b/test/functional/custom_fields_controller_test.rb @@ -31,9 +31,9 @@ class CustomFieldsControllerTest < Redmine::ControllerTest assert_select 'table.custom_fields' end - def test_show_html_format_should_respond_with_404 + def test_show_html_format_should_respond_with_406 get :show, :params => {:id => 1} - assert_response :not_found + assert_response :not_acceptable end def test_new_without_type_should_render_select_type @@ -602,6 +602,34 @@ class CustomFieldsControllerTest < Redmine::ControllerTest assert_nil CustomValue.find_by_custom_field_id(1) end + def test_create_should_ignore_the_type_nested_in_custom_field_params + assert_no_difference 'CustomField.count' do + post( + :create, + :params => { + :custom_field => { + :type => 'IssueCustomField', + :name => 'New field', + :field_format => 'string' + } + } + ) + end + assert_response :success + assert_select 'input[name=type]', CustomFieldsHelper::CUSTOM_FIELDS_TABS.size + end + + def test_destroy_failure_should_display_an_error + CustomField.any_instance.stubs(:destroy).returns(false) + + assert_no_difference 'CustomField.count' do + delete(:destroy, :params => {:id => 1}) + end + + assert_redirected_to '/custom_fields?tab=IssueCustomField' + assert_equal 'Unable to delete custom field', flash[:error] + end + def custom_field_classes classes = Dir.glob(Rails.root.join('app/models/*_custom_field.rb')).map do |f| diff --git a/test/integration/api_test/custom_fields_test.rb b/test/integration/api_test/custom_fields_test.rb index b9178eb5e..a4cbb0be8 100644 --- a/test/integration/api_test/custom_fields_test.rb +++ b/test/integration/api_test/custom_fields_test.rb @@ -172,11 +172,239 @@ class Redmine::ApiTest::CustomFieldsTest < Redmine::ApiTest::Base assert_response :not_found end - test "GET /custom_fields/:id.xml should respond with 403 for a non administrator" do - get '/custom_fields/1.xml', :headers => credentials('jsmith') + test "GET /custom_fields/:id should respond with 406 for the HTML format" do + get '/custom_fields/1', :headers => credentials('admin') + assert_response :not_acceptable + end + + test "GET /custom_fields/:id.xml should respond with 401 without credentials" do + get '/custom_fields/1.xml' + assert_response :unauthorized + end + + test "POST /custom_fields.xml should create the custom field" do + assert_difference 'IssueCustomField.count' do + post( + '/custom_fields.xml', + :params => { + :type => 'IssueCustomField', + :custom_field => { + :name => 'New field', + :field_format => 'string', + :is_required => '1', + :tracker_ids => ['1', '2'] + } + }, + :headers => credentials('admin') + ) + end + assert_response :created + assert_equal 'application/xml', response.media_type + + field = IssueCustomField.order(:id => :desc).first + assert_equal 'New field', field.name + assert_equal 'string', field.field_format + assert field.is_required? + assert_equal [1, 2], field.tracker_ids.sort + assert_match %r{/custom_fields/#{field.id}\z}, response.headers['Location'] + + assert_select 'custom_field' do + assert_select 'id', :text => field.id.to_s + assert_select 'name', :text => 'New field' + assert_select 'customized_type', :text => 'issue' + end + end + + test "POST /custom_fields.xml should create a project custom field" do + assert_difference 'ProjectCustomField.count' do + post( + '/custom_fields.xml', + :params => { + :type => 'ProjectCustomField', + :custom_field => {:name => 'New field', :field_format => 'string'} + }, + :headers => credentials('admin') + ) + end + assert_response :created + assert_equal 'New field', ProjectCustomField.order(:id => :desc).first.name + end + + test "POST /custom_fields.xml should create a list custom field with possible values" do + assert_difference 'IssueCustomField.count' do + post( + '/custom_fields.xml', + :params => { + :type => 'IssueCustomField', + :custom_field => { + :name => 'New list field', + :field_format => 'list', + :possible_values => ['Foo', 'Bar'] + } + }, + :headers => credentials('admin') + ) + end + assert_response :created + assert_equal ['Foo', 'Bar'], IssueCustomField.order(:id => :desc).first.possible_values + end + + test "POST /custom_fields.xml without type should respond with errors" do + assert_no_difference 'CustomField.count' do + post( + '/custom_fields.xml', + :params => {:custom_field => {:name => 'New field', :field_format => 'string'}}, + :headers => credentials('admin') + ) + end + assert_response :unprocessable_content + assert_select 'errors error', :text => 'Type is invalid' + end + + test "POST /custom_fields.json with type in custom field parameters should create the custom field" do + assert_difference 'IssueCustomField.count' do + post( + '/custom_fields.json', + :params => { + :custom_field => { + :type => 'IssueCustomField', + :name => 'New field', + :field_format => 'string' + } + }, + :headers => credentials('admin'), + :as => :json + ) + end + assert_response :created + + field = IssueCustomField.order(:id => :desc).first + assert_equal 'New field', field.name + assert_equal 'string', field.field_format + end + + test "POST /custom_fields.xml with type in custom field parameters should create the custom field" do + payload = <<~XML + + + IssueCustomField + New XML field + string + + XML + + assert_difference 'IssueCustomField.count' do + post( + '/custom_fields.xml', + :params => payload, + :headers => {"CONTENT_TYPE" => 'application/xml'}.merge(credentials('admin')) + ) + end + assert_response :created + + field = IssueCustomField.order(:id => :desc).first + assert_equal 'New XML field', field.name + assert_equal 'string', field.field_format + end + + test "POST /custom_fields.xml with an invalid type should respond with errors" do + assert_no_difference 'CustomField.count' do + post( + '/custom_fields.xml', + :params => {:type => 'UnknownCustomField', :custom_field => {:name => 'New field'}}, + :headers => credentials('admin') + ) + end + assert_response :unprocessable_content + assert_select 'errors error', :text => 'Type is invalid' + end + + test "POST /custom_fields.xml with a scalar custom field parameter should respond with errors" do + assert_no_difference 'CustomField.count' do + post( + '/custom_fields.xml?custom_field=foo', + :headers => credentials('admin') + ) + end + assert_response :unprocessable_content + assert_select 'errors error', :text => 'Type is invalid' + end + + test "POST /custom_fields.xml with invalid parameters should respond with errors" do + assert_no_difference 'CustomField.count' do + post( + '/custom_fields.xml', + :params => { + :type => 'IssueCustomField', + :custom_field => {:name => '', :field_format => 'string'} + }, + :headers => credentials('admin') + ) + end + assert_response :unprocessable_content + assert_select 'errors error', :text => "Name cannot be blank" + end + + test "POST /custom_fields.xml should respond with 403 for a non administrator" do + assert_no_difference 'CustomField.count' do + post( + '/custom_fields.xml', + :params => { + :type => 'IssueCustomField', + :custom_field => {:name => 'New field', :field_format => 'string'} + }, + :headers => credentials('jsmith') + ) + end assert_response :forbidden end + test "POST /custom_fields.xml should copy a custom field" do + source = IssueCustomField.find(1) + + assert_difference 'IssueCustomField.count' do + post( + "/custom_fields.xml?copy=#{source.id}", + :params => { + :type => 'IssueCustomField', + :custom_field => {:name => 'Copied field'} + }, + :headers => credentials('admin') + ) + end + assert_response :created + + field = IssueCustomField.order(:id => :desc).first + assert_equal 'Copied field', field.name + assert_equal source.field_format, field.field_format + assert_equal source.possible_values, field.possible_values + end + + test "POST /custom_fields.xml should save roles and projects when visibility is restricted" do + assert_difference 'IssueCustomField.count' do + post( + '/custom_fields.xml', + :params => { + :type => 'IssueCustomField', + :custom_field => { + :name => 'Restricted field', + :field_format => 'string', + :visible => '0', + :role_ids => ['1', '2'], + :is_for_all => '0', + :project_ids => ['1', '2'] + } + }, + :headers => credentials('admin') + ) + end + assert_response :created + + field = IssueCustomField.order(:id => :desc).first + assert_equal [1, 2], field.role_ids.sort + assert_equal [1, 2], field.project_ids.sort + end + test "PUT /custom_fields/:id.xml should update the custom field" do put( '/custom_fields/1.xml', @@ -197,15 +425,6 @@ class Redmine::ApiTest::CustomFieldsTest < Redmine::ApiTest::Base assert_select 'errors error', :text => "Name cannot be blank" end - test "PUT /custom_fields/:id.xml should respond with 403 for a non administrator" do - put( - '/custom_fields/1.xml', - :params => {:custom_field => {:name => 'Renamed'}}, - :headers => credentials('jsmith') - ) - assert_response :forbidden - end - test "DELETE /custom_fields/:id.xml should destroy the custom field" do assert_difference 'CustomField.count', -1 do delete '/custom_fields/1.xml', :headers => credentials('admin') @@ -214,10 +433,23 @@ class Redmine::ApiTest::CustomFieldsTest < Redmine::ApiTest::Base assert_nil CustomField.find_by_id(1) end - test "DELETE /custom_fields/:id.xml should respond with 403 for a non administrator" do + test "DELETE /custom_fields/:id.xml with failure should return errors" do + CustomField.any_instance.stubs(:destroy).returns(false) + assert_no_difference 'CustomField.count' do - delete '/custom_fields/1.xml', :headers => credentials('jsmith') + delete '/custom_fields/1.xml', :headers => credentials('admin') end - assert_response :forbidden + assert_response :unprocessable_content + assert_select 'errors error', :text => 'Unable to delete custom field' + end + + test "DELETE /custom_fields/:id.xml raising an exception should return errors" do + CustomField.any_instance.stubs(:destroy).raises(RuntimeError) + + assert_no_difference 'CustomField.count' do + delete '/custom_fields/1.xml', :headers => credentials('admin') + end + assert_response :unprocessable_content + assert_select 'errors error', :text => 'Unable to delete custom field' end end diff --git a/test/integration/routing/custom_fields_test.rb b/test/integration/routing/custom_fields_test.rb index 639d292ca..3705fd481 100644 --- a/test/integration/routing/custom_fields_test.rb +++ b/test/integration/routing/custom_fields_test.rb @@ -25,6 +25,7 @@ class RoutingCustomFieldsTest < Redmine::RoutingTest should_route 'GET /custom_fields/new' => 'custom_fields#new' should_route 'POST /custom_fields' => 'custom_fields#create' + should_route 'GET /custom_fields/2' => 'custom_fields#show', :id => '2' should_route 'GET /custom_fields/2/edit' => 'custom_fields#edit', :id => '2' should_route 'PUT /custom_fields/2' => 'custom_fields#update', :id => '2' should_route 'DELETE /custom_fields/2' => 'custom_fields#destroy', :id => '2' -- 2.55.0