move property name conversion for writability check

This way, the conversion occurs where the name is determined and representation logic does not mix with business logic.

app/contracts/base_contract.rb

@@ -139,12 +139,7 @@ class BaseContract < Disposable::Twin
   end
 
   def writable?(attribute)
-    property_name = ::API::Utilities::PropertyNameConverter.to_ar_name(
-      attribute,
-      context: model,
-      collapse_cf_name: false
-    )
-    writable_attributes.include?(property_name)
+    writable_attributes.include?(attribute.to_s)
   end
 
   def valid?(*_args)

lib/api/decorators/schema_representer.rb

@@ -230,7 +230,13 @@ module API
         def default_writable_property(property)
           -> do
             if represented.respond_to?(:writable?)
-              represented.writable?(property)
+              property_name = ::API::Utilities::PropertyNameConverter.to_ar_name(
+                property,
+                context: represented.model,
+                collapse_cf_name: false
+              )
+
+              represented.writable?(property_name)
             else
               false
             end

lib/api/v3/work_packages/schema/base_work_package_schema.rb

@@ -52,19 +52,13 @@ module API
           end
 
           def writable?(property)
-            property = property.to_sym
+            property = property.to_s
 
             # Special case for milestones + date property
-            property = :start_date if property == :date && milestone?
+            property = 'start_date' if property == 'date' && milestone?
 
             @writable_attributes ||= contract.writable_attributes
-
-            property_name = ::API::Utilities::PropertyNameConverter.to_ar_name(
-              property,
-              context: work_package,
-              collapse_cf_name: false
-            )
-            @writable_attributes.include?(property_name)
+            @writable_attributes.include?(property)
           end
 
           def milestone?
@@ -75,6 +69,11 @@ module API
             work_package.readonly_status?
           end
 
+          # Alias method will not work since work_package is only defined in subclasses.
+          def model
+            work_package
+          end
+
           private
 
           def contract

lib/api/v3/work_packages/schema/work_package_sums_schema.rb

@@ -38,6 +38,10 @@ module API
           def writable?(_property)
             false
           end
+
+          def work_package
+            nil
+          end
         end
       end
     end

modules/backlogs/spec/api/work_packages/schema/specific_work_package_schema_spec.rb

@@ -74,7 +74,7 @@ describe ::API::V3::WorkPackages::Schema::SpecificWorkPackageSchema do
     login_as(current_user)
   end
 
-  describe '#remaining_time_writable?' do
+  describe '#writable? for remaining_hours' do
     subject { described_class.new(work_package:) }
 
     context 'work_package is a leaf' do
@@ -83,7 +83,7 @@ describe ::API::V3::WorkPackages::Schema::SpecificWorkPackageSchema do
       end
 
       it 'is writable' do
-        expect(subject.writable?(:remaining_time)).to be(true)
+        expect(subject.writable?(:remaining_hours)).to be(true)
       end
     end
 
@@ -93,7 +93,7 @@ describe ::API::V3::WorkPackages::Schema::SpecificWorkPackageSchema do
       end
 
       it 'is not writable' do
-        expect(subject.writable?(:remaining_time)).to be(false)
+        expect(subject.writable?(:remaining_hours)).to be(false)
       end
     end
   end

modules/backlogs/spec/api/work_packages/work_package_schema_representer_spec.rb

@@ -127,7 +127,7 @@ describe ::API::V3::WorkPackages::Schema::WorkPackageSchemaRepresenter do
     context 'remainingTime not writable' do
       before do
         allow(schema).to receive(:writable?).and_call_original
-        allow(schema).to receive(:writable?).with(:remaining_time).and_return(false)
+        allow(schema).to receive(:writable?).with('remaining_hours').and_return(false)
       end
 
       it_behaves_like 'has basic schema properties' do

modules/costs/spec/lib/api/v3/time_entries/schemas/time_entry_schema_representer_spec.rb

@@ -40,14 +40,16 @@ describe ::API::V3::TimeEntries::Schemas::TimeEntrySchemaRepresenter do
   let(:user) { build_stubbed(:user) }
   let(:assigned_project) { nil }
   let(:activity) { build_stubbed(:time_entry_activity) }
+  let(:time_entry) { build_stubbed(:time_entry) }
   let(:writable_attributes) { %w(spent_on hours project work_package activity comment user) }
 
   let(:contract) do
-    contract = double('contract',
-                      new_record?: new_record,
-                      id: new_record ? nil : 5,
-                      project: assigned_project,
-                      project_id: project.id)
+    contract = instance_double(new_record ? TimeEntries::CreateContract : TimeEntries::UpdateContract,
+                               new_record?: new_record,
+                               id: new_record ? nil : 5,
+                               project: assigned_project,
+                               project_id: project.id,
+                               model: time_entry)
 
     allow(contract)
       .to receive(:writable?) do |attribute|
@@ -268,10 +270,10 @@ describe ::API::V3::TimeEntries::Schemas::TimeEntrySchemaRepresenter do
       end
     end
 
-    context 'custom value' do
+    context 'for a custom value' do
       let(:custom_field) { build_stubbed(:text_time_entry_custom_field) }
       let(:path) { "customField#{custom_field.id}" }
-      let(:writable_attributes) { ["customField#{custom_field.id}"] }
+      let(:writable_attributes) { ["custom_field_#{custom_field.id}"] }
 
       before do
         allow(contract)

spec/lib/api/v3/memberships/schemas/membership_schema_representer_spec.rb

@@ -40,35 +40,26 @@ describe ::API::V3::Memberships::Schemas::MembershipSchemaRepresenter do
   let(:principal) { build_stubbed(:group) }
   let(:assigned_project) { nil }
   let(:assigned_principal) { nil }
-  let(:allowed_roles) do
-    if new_record
-      [build_stubbed(:role),
-       build_stubbed(:role)]
-    end
-  end
+  let(:member) { build_stubbed(:member) }
 
   let(:contract) do
-    contract = double('contract',
-                      new_record?: new_record,
-                      project: assigned_project,
-                      principal: assigned_principal)
+    contract = instance_double(new_record ? Members::CreateContract : Members::UpdateContract,
+                               model: member,
+                               new_record?: new_record,
+                               project: assigned_project,
+                               principal: assigned_principal)
 
     allow(contract)
       .to receive(:writable?) do |attribute|
       writable = %w(roles)
 
       if new_record
-        writable = writable.concat(%w(project principal))
+        writable.concat(%w(project principal))
       end
 
       writable.include?(attribute.to_s)
     end
 
-    allow(contract)
-      .to receive(:assignable_values)
-      .with(:roles, current_user)
-      .and_return(allowed_roles)
-
     contract
   end
   let(:representer) do

spec/lib/api/v3/utilities/custom_field_injector_spec.rb

@@ -52,9 +52,11 @@ describe ::API::V3::Utilities::CustomFieldInjector do
     let(:base_class) { Class.new(::API::Decorators::SchemaRepresenter) }
     let(:modified_class) { described_class.create_schema_representer([custom_field], base_class) }
     let(:schema_writable) { true }
+    let(:model) { build_stubbed(:work_package) }
     let(:schema) do
       double('WorkPackageSchema',
              project_id: 42,
+             model:,
              defines_assignable_values?: true,
              available_custom_fields: [custom_field],
              writable?: schema_writable)

spec/lib/api/v3/versions/schemas/version_schema_representer_spec.rb

@@ -41,10 +41,12 @@ describe ::API::V3::Versions::Schemas::VersionSchemaRepresenter do
   let(:custom_field) do
     build_stubbed(:int_version_custom_field)
   end
+  let(:version) { build_stubbed(:version) }
 
   let(:contract) do
-    contract = double('contract',
-                      new_record?: new_record)
+    contract = instance_double(new_record ? Versions::CreateContract : Versions::UpdateContract,
+                               new_record?: new_record,
+                               model: version)
 
     allow(contract)
       .to receive(:writable?) do |attribute|

spec/lib/api/v3/work_packages/schema/specific_work_package_schema_spec.rb

@@ -85,8 +85,8 @@ describe ::API::V3::WorkPackages::Schema::SpecificWorkPackageSchema do
         .and_return(true)
 
       expect(subject).to be_readonly
-      expect(subject.writable?(:status)).to be_truthy
-      expect(subject.writable?(:subject)).to be_falsey
+      expect(subject).to be_writable('status')
+      expect(subject).not_to be_writable('subject')
 
       allow(work_package)
         .to receive(:readonly_status?)
@@ -95,8 +95,8 @@ describe ::API::V3::WorkPackages::Schema::SpecificWorkPackageSchema do
       # As the writability is memoized we need to have a new schema
       new_schema = described_class.new(work_package:)
       expect(new_schema).not_to be_readonly
-      expect(new_schema.writable?(:status)).to be_truthy
-      expect(new_schema.writable?(:subject)).to be_truthy
+      expect(new_schema).to be_writable('status')
+      expect(new_schema).to be_writable('subject')
     end
   end
 
@@ -176,46 +176,46 @@ describe ::API::V3::WorkPackages::Schema::SpecificWorkPackageSchema do
     context 'percentage done' do
       it 'is not writable when inferred by status' do
         allow(Setting).to receive(:work_package_done_ratio).and_return('status')
-        expect(subject.writable?(:percentage_done)).to be false
+        expect(subject.writable?('done_ratio')).to be false
       end
 
       it 'is not writable when disabled' do
         allow(Setting).to receive(:work_package_done_ratio).and_return('disabled')
-        expect(subject.writable?(:percentage_done)).to be false
+        expect(subject.writable?('done_ratio')).to be false
       end
 
       it 'is not writable when the work package is a parent' do
         allow(work_package).to receive(:leaf?).and_return(false)
-        expect(subject.writable?(:percentage_done)).to be false
+        expect(subject.writable?('done_ratio')).to be false
       end
 
       it 'is writable when the work package is a leaf' do
         allow(work_package).to receive(:leaf?).and_return(true)
-        expect(subject.writable?(:percentage_done)).to be true
+        expect(subject.writable?('done_ratio')).to be true
       end
     end
 
     context 'estimated time' do
       it 'is writable when the work package is a parent' do
         allow(work_package).to receive(:leaf?).and_return(false)
-        expect(subject.writable?(:estimated_time)).to be true
+        expect(subject.writable?('estimated_hours')).to be true
       end
 
       it 'is writable when the work package is a leaf' do
         allow(work_package).to receive(:leaf?).and_return(true)
-        expect(subject.writable?(:estimated_time)).to be true
+        expect(subject.writable?('estimated_hours')).to be true
       end
     end
 
     context 'derived estimated time' do
       it 'is not writable when the work package is a parent' do
         allow(work_package).to receive(:leaf?).and_return(false)
-        expect(subject.writable?(:derived_estimated_time)).to be false
+        expect(subject.writable?('derived_estimated_time')).to be false
       end
 
       it 'is not writable when the work package is a leaf' do
         allow(work_package).to receive(:leaf?).and_return(true)
-        expect(subject.writable?(:derived_estimated_time)).to be false
+        expect(subject.writable?('derived_estimated_time')).to be false
       end
     end
 
@@ -229,7 +229,7 @@ describe ::API::V3::WorkPackages::Schema::SpecificWorkPackageSchema do
 
         context 'scheduled automatically' do
           it 'is not writable' do
-            expect(subject.writable?(:start_date)).to be false
+            expect(subject.writable?('start_date')).to be false
           end
         end
 
@@ -239,7 +239,7 @@ describe ::API::V3::WorkPackages::Schema::SpecificWorkPackageSchema do
           end
 
           it 'is writable' do
-            expect(subject.writable?(:start_date)).to be true
+            expect(subject.writable?('start_date')).to be true
           end
         end
       end
@@ -247,7 +247,7 @@ describe ::API::V3::WorkPackages::Schema::SpecificWorkPackageSchema do
       context 'work package is a leaf' do
         it 'is writable' do
           allow(work_package).to receive(:leaf?).and_return(true)
-          expect(subject.writable?(:start_date)).to be true
+          expect(subject.writable?('start_date')).to be true
         end
       end
     end
@@ -262,7 +262,7 @@ describe ::API::V3::WorkPackages::Schema::SpecificWorkPackageSchema do
 
         context 'scheduled automatically' do
           it 'is not writable' do
-            expect(subject.writable?(:due_date)).to be false
+            expect(subject.writable?('due_date')).to be false
           end
         end
 
@@ -272,7 +272,7 @@ describe ::API::V3::WorkPackages::Schema::SpecificWorkPackageSchema do
           end
 
           it 'is writable' do
-            expect(subject.writable?(:due_date)).to be true
+            expect(subject.writable?('due_date')).to be true
           end
         end
       end
@@ -280,7 +280,7 @@ describe ::API::V3::WorkPackages::Schema::SpecificWorkPackageSchema do
       context 'work package is a leaf' do
         it 'is writable' do
           allow(work_package).to receive(:leaf?).and_return(true)
-          expect(subject.writable?(:due_date)).to be true
+          expect(subject.writable?('due_date')).to be true
         end
       end
     end
@@ -294,24 +294,24 @@ describe ::API::V3::WorkPackages::Schema::SpecificWorkPackageSchema do
 
       it 'is not writable when the work package is a parent' do
         allow(work_package).to receive(:leaf?).and_return(false)
-        expect(subject.writable?(:date)).to be false
+        expect(subject.writable?('date')).to be false
       end
 
       it 'is writable when the work package is a leaf' do
         allow(work_package).to receive(:leaf?).and_return(true)
-        expect(subject.writable?(:date)).to be true
+        expect(subject.writable?('date')).to be true
       end
     end
 
     context 'priority' do
       it 'is writable when the work package is a parent' do
         allow(work_package).to receive(:leaf?).and_return(false)
-        expect(subject.writable?(:priority)).to be true
+        expect(subject.writable?('priority')).to be true
       end
 
       it 'is writable when the work package is a leaf' do
         allow(work_package).to receive(:leaf?).and_return(true)
-        expect(subject.writable?(:priority)).to be true
+        expect(subject.writable?('priority')).to be true
       end
     end
   end

spec/lib/api/v3/work_packages/schema/typed_work_package_schema_spec.rb

@@ -64,11 +64,11 @@ describe ::API::V3::WorkPackages::Schema::TypedWorkPackageSchema do
 
   describe '#writable?' do
     it 'percentage done is writable' do
-      expect(subject.writable?(:percentage_done)).to be true
+      expect(subject.writable?(:done_ratio)).to be true
     end
 
     it 'estimated time is writable' do
-      expect(subject.writable?(:estimated_time)).to be true
+      expect(subject.writable?(:estimated_hours)).to be true
     end
 
     it 'start date is writable' do

spec/lib/api/v3/work_packages/schema/work_package_schema_representer_spec.rb

@@ -334,7 +334,7 @@ describe ::API::V3::WorkPackages::Schema::WorkPackageSchemaRepresenter do
       before do
         allow(schema)
           .to receive(:writable?)
-                .with(:ignore_non_working_days)
+                .with('ignore_non_working_days')
                 .and_return writable
       end
 
@@ -367,7 +367,7 @@ describe ::API::V3::WorkPackages::Schema::WorkPackageSchemaRepresenter do
       before do
         allow(schema)
           .to receive(:writable?)
-          .with(:date)
+          .with('date')
           .and_return true
 
         allow(schema)
@@ -387,7 +387,7 @@ describe ::API::V3::WorkPackages::Schema::WorkPackageSchemaRepresenter do
         before do
           allow(schema)
             .to receive(:writable?)
-            .with(:date)
+            .with('date')
             .and_return false
         end
 
@@ -417,7 +417,7 @@ describe ::API::V3::WorkPackages::Schema::WorkPackageSchemaRepresenter do
       before do
         allow(schema)
           .to receive(:writable?)
-          .with(:start_date)
+          .with('start_date')
           .and_return true
 
         allow(schema)
@@ -437,7 +437,7 @@ describe ::API::V3::WorkPackages::Schema::WorkPackageSchemaRepresenter do
         before do
           allow(schema)
             .to receive(:writable?)
-            .with(:start_date)
+            .with('start_date')
             .and_return false
         end
 
@@ -467,7 +467,7 @@ describe ::API::V3::WorkPackages::Schema::WorkPackageSchemaRepresenter do
       before do
         allow(schema)
           .to receive(:writable?)
-          .with(:due_date)
+          .with('due_date')
           .and_return true
 
         allow(schema)
@@ -485,7 +485,7 @@ describe ::API::V3::WorkPackages::Schema::WorkPackageSchemaRepresenter do
 
       context 'when not writable' do
         before do
-          allow(schema).to receive(:writable?).with(:due_date).and_return false
+          allow(schema).to receive(:writable?).with('due_date').and_return false
         end
 
         it_behaves_like 'has basic schema properties' do
@@ -566,7 +566,7 @@ describe ::API::V3::WorkPackages::Schema::WorkPackageSchemaRepresenter do
       before do
         allow(schema)
           .to receive(:writable?)
-          .with(:estimated_time)
+          .with('estimated_hours')
           .and_return true
       end
 
@@ -582,7 +582,7 @@ describe ::API::V3::WorkPackages::Schema::WorkPackageSchemaRepresenter do
         before do
           allow(schema)
             .to receive(:writable?)
-            .with(:estimated_time)
+            .with('estimated_hours')
             .and_return false
         end
 
@@ -640,7 +640,7 @@ describe ::API::V3::WorkPackages::Schema::WorkPackageSchemaRepresenter do
 
     describe 'percentageDone' do
       before do
-        allow(schema).to receive(:writable?).with(:percentage_done).and_return true
+        allow(schema).to receive(:writable?).with('done_ratio').and_return true
       end
 
       it_behaves_like 'has basic schema properties' do
@@ -653,7 +653,7 @@ describe ::API::V3::WorkPackages::Schema::WorkPackageSchemaRepresenter do
 
       context 'when not writable' do
         before do
-          allow(schema).to receive(:writable?).with(:percentage_done).and_return false
+          allow(schema).to receive(:writable?).with('done_ratio').and_return false
         end
 
         it_behaves_like 'has basic schema properties' do
@@ -917,7 +917,7 @@ describe ::API::V3::WorkPackages::Schema::WorkPackageSchemaRepresenter do
 
     describe 'priorities' do
       before do
-        allow(schema).to receive(:writable?).with(:priority).and_return true
+        allow(schema).to receive(:writable?).with('priority').and_return true
       end
 
       it_behaves_like 'has basic schema properties' do
@@ -938,7 +938,7 @@ describe ::API::V3::WorkPackages::Schema::WorkPackageSchemaRepresenter do
 
       context 'when not writable' do
         before do
-          allow(schema).to receive(:writable?).with(:priority).and_return false
+          allow(schema).to receive(:writable?).with('priority').and_return false
         end
 
         it_behaves_like 'has basic schema properties' do