Judge a variant switch in a contract, not a form object. FND-109

Every other Primer form in the codebase binds an ActiveRecord record or no model at all, so the form object was ours alone to maintain. The dialog now submits straight to the service like the add dialog beside it, and the contract is the only place a switch is judged. A refusal reads as a flash rather than a message under the select.

Author: dfriquet

app/components/projects/settings/work_packages/types/switch_dialog_component.html.erb

@@ -36,5 +36,7 @@ See COPYRIGHT and LICENSE files for more details.
     ) do |dialog| %>
   <% dialog.with_header(variant: :large) %>
 
-  <%= render(Projects::Settings::WorkPackages::Types::SwitchFormComponent.new(switch: @switch)) %>
+  <%= render(
+        Projects::Settings::WorkPackages::Types::SwitchFormComponent.new(project:, source:)
+      ) %>
 <% end %>

app/components/projects/settings/work_packages/types/switch_dialog_component.rb

@@ -39,13 +39,16 @@ class SwitchDialogComponent < ApplicationComponent
 
           DIALOG_ID = "project-types-switch-dialog"
 
-          def initialize(switch:)
+          def initialize(project:, source:)
             super()
 
-            @switch = switch
+            @project = project
+            @source = source
           end
 
-          delegate :source, to: :@switch, private: true
+          private
+
+          attr_reader :project, :source
         end
       end
     end

app/components/projects/settings/work_packages/types/switch_form_component.html.erb

@@ -28,7 +28,7 @@ See COPYRIGHT and LICENSE files for more details.
 ++#%>
 
 <%= component_wrapper do %>
-  <%= primer_form_with(model: @switch, scope: :switch, url: submit_path) do |form| %>
+  <%= primer_form_with(url: switch_path, method: :post) do |form| %>
     <%= render(Primer::Alpha::Dialog::Body.new) do %>
       <%= flex_layout(direction: :column) do |body| %>
         <% body.with_row(mb: 3) do %>
@@ -42,7 +42,11 @@ See COPYRIGHT and LICENSE files for more details.
           <% end %>
         <% end %>
         <% body.with_row do %>
-          <%= render(Projects::Settings::WorkPackages::Types::SwitchForm.new(form, switch: @switch)) %>
+          <%= render(
+                Projects::Settings::WorkPackages::Types::SwitchForm.new(
+                  form, targets: available_targets, selected:, validation_message:
+                )
+              ) %>
         <% end %>
       <% end %>
     <% end %>

app/components/projects/settings/work_packages/types/switch_form_component.rb

@@ -32,27 +32,34 @@ module Projects
   module Settings
     module WorkPackages
       module Types
-        # The switch dialog's contents. Separate from the dialog so a validation
-        # failure can replace it: replacing the dialog component would swap out
-        # the <dialog> element and close it.
+        # The switch dialog's form. Separate from the dialog so a refused switch can replace
+        # it: replacing the dialog component would swap out the <dialog> element and close it.
         class SwitchFormComponent < ApplicationComponent
           include OpPrimer::ComponentHelpers
           include OpTurbo::Streamable
 
-          def initialize(switch:)
+          def initialize(project:, source:, selected: source, validation_message: nil)
             super()
 
-            @switch = switch
+            @project = project
+            @source = source
+            @selected = selected
+            @validation_message = validation_message
           end
 
           private
 
-          delegate :project, :source, to: :@switch
+          attr_reader :project, :source, :selected, :validation_message
 
-          def submit_path
+          def switch_path
             project_settings_work_packages_type_switch_path(project, source)
           end
 
+          def available_targets
+            source.family
+          end
+
+          # Constant lookup in a compiled template does not walk the enclosing modules.
           def dialog_id
             SwitchDialogComponent::DIALOG_ID
           end

app/models/projects/types/switch.rb …rojects/types/switch_variant_contract.rbapp/models/projects/types/switch.rb renamed to app/contracts/projects/types/switch_variant_contract.rb app/models/projects/types/switch.rb renamed to app/contracts/projects/types/switch_variant_contract.rb

@@ -30,48 +30,31 @@
 
 module Projects
   module Types
-    # Which member of a type family a project should use. Backs the switch
-    # dialog: the form binds to it, and it owns the preview of the switch it
-    # describes.
-    class Switch
-      include ActiveModel::Model
-
-      attr_accessor :project, :source
-      attr_writer :target_id
-
-      validate :target_selectable
-
-      def target_id
-        @target_id.presence&.to_i
-      end
-
-      def target
-        return @target if defined?(@target)
-
-        @target = target_id && ::Type.find_by(id: target_id)
-      end
-
-      def available_targets
-        source.family
-      end
-
-      # The dialog opens on the member the project uses now, so applying without
-      # choosing anything is a visible no-op rather than an empty field.
-      def selected_target
-        target || source
+    # Whether a project may move from one member of a type family to another.
+    #
+    # The pair being switched arrives through the contract options rather than off the model:
+    # a switch changes which member the project resolves to, which is a row in project_types
+    # rather than an attribute of the project the contract validates.
+    class SwitchVariantContract < ManageTypesContract
+      validate :validate_target_selectable
+
+      protected
+
+      def validate_target_selectable
+        if target.nil?
+          errors.add(:types, :switch_target_blank)
+        elsif target == source
+          errors.add(:types, :switch_target_identical)
+        elsif source.root_id != target.root_id
+          errors.add(:types, :switch_target_not_in_family)
+        end
       end
 
       private
 
-      def target_selectable
-        if target_id.blank?
-          errors.add(:target_id, :blank)
-        elsif available_targets.exclude?(target)
-          errors.add(:target_id, :not_in_family)
-        elsif target == source
-          errors.add(:target_id, :unchanged)
-        end
-      end
+      def source = options[:source]
+
+      def target = options[:target]
     end
   end
 end

app/controllers/projects/settings/work_packages/types/switches_controller.rb

@@ -31,30 +31,26 @@
 class Projects::Settings::WorkPackages::Types::SwitchesController < Projects::SettingsController
   include WorkPackageTypes::TypeVariantsFeature
   include OpTurbo::ComponentStream
-  include FlashMessagesOutputSafetyHelper
 
   menu_item :settings_work_packages
 
   before_action :require_type_variants_feature
   before_action :load_source
 
   def new
-    respond_with_dialog Projects::Settings::WorkPackages::Types::SwitchDialogComponent.new(switch: build_switch)
+    respond_with_dialog Projects::Settings::WorkPackages::Types::SwitchDialogComponent
+                          .new(project: @project, source: @source)
   end
 
   def create
-    switch = build_switch
-
-    return render_invalid(switch) unless switch.valid?
+    target = ::Type.find_by(id: params[:target_id])
 
     result = ::Projects::Types::SwitchVariantService
                .new(user: current_user, model: @project)
-               .call(source: switch.source, target: switch.target)
+               .call(source: @source, target:)
 
-    result.on_success { on_switched(switch) }
-    result.on_failure do
-      render_error_flash_message_via_turbo_stream(message: join_flash_messages(result.errors.full_messages))
-    end
+    result.on_success { on_switched(target) }
+    result.on_failure { on_refused(target, result) }
 
     respond_to_with_turbo_streams(status: result)
   end
@@ -75,26 +71,28 @@ def load_source
     respond_to_with_turbo_streams(status: :unprocessable_entity)
   end
 
-  def build_switch
-    ::Projects::Types::Switch.new(project: @project, source: @source, target_id: params.dig(:switch, :target_id))
-  end
+  # Repainted with the refusal under the select, so the choice can be corrected where it was
+  # made. A refusal that belongs to no field is the contract turning away a user the permission
+  # map already turned away, and has nowhere to show.
+  def on_refused(target, result)
+    message = result.errors.messages_for(:types).first
+    return if message.blank?
 
-  def render_invalid(switch)
     update_via_turbo_stream(
-      component: Projects::Settings::WorkPackages::Types::SwitchFormComponent.new(switch:)
+      component: Projects::Settings::WorkPackages::Types::SwitchFormComponent.new(
+        project: @project, source: @source, selected: target || @source, validation_message: message
+      )
     )
-
-    respond_to_with_turbo_streams(status: :unprocessable_entity)
   end
 
   # Reload so the repainted list no longer sees the association's cached types.
-  def on_switched(switch)
+  def on_switched(target)
     close_dialog_via_turbo_stream("##{Projects::Settings::WorkPackages::Types::SwitchDialogComponent::DIALOG_ID}")
     replace_via_turbo_stream(
       component: Projects::Settings::WorkPackages::Types::ListComponent.new(project: @project.reload)
     )
     render_success_flash_message_via_turbo_stream(
-      message: t("projects.settings.types.switch_dialog.success", type: switch.target.composite_name)
+      message: t("projects.settings.types.switch_dialog.success", type: target.composite_name)
     )
   end
 end

app/forms/projects/settings/work_packages/types/switch_form.rb

@@ -33,10 +33,12 @@ module Settings
     module WorkPackages
       module Types
         class SwitchForm < ApplicationForm
-          def initialize(switch:)
+          def initialize(targets:, selected:, validation_message: nil)
             super()
 
-            @switch = switch
+            @targets = targets
+            @selected = selected
+            @validation_message = validation_message
           end
 
           form do |switch_form|
@@ -45,12 +47,13 @@ def initialize(switch:)
               label: I18n.t("projects.settings.types.switch_dialog.target_label"),
               include_blank: false,
               input_width: :medium,
+              validation_message: @validation_message,
               data: { test_selector: "project-types-switch-select" }
             ) do |list|
               # Composite rather than own names: repeating the family on every
               # option is what makes it evident that nothing outside it is on offer.
-              @switch.available_targets.each do |target|
-                list.option(value: target.id, label: target.composite_name, selected: target == @switch.selected_target)
+              @targets.each do |target|
+                list.option(value: target.id, label: target.composite_name, selected: target == @selected)
               end
             end
           end

app/services/projects/types/switch_variant_service.rb

@@ -35,20 +35,24 @@ module Types
     # project's work packages are untouched: they store the root either way, so the switch is
     # a change of which configuration the project resolves to, not a retype.
     class SwitchVariantService < BaseService
+      def initialize(user:, model:, contract_class: SwitchVariantContract)
+        super
+      end
+
       private
 
+      # The pair is what the contract judges, and it only arrives with the call, so the
+      # options cannot be handed over at construction time like a contract class can.
+      def before_perform(service_call)
+        self.contract_options = params.slice(:source, :target)
+
+        service_call
+      end
+
       def persist(service_call)
-        source = params[:source]
-        target = params[:target]
+        switch(params[:target])
 
-        if source == target
-          failure(:switch_target_identical)
-        elsif source.root_id != target.root_id
-          failure(:switch_target_not_in_family)
-        else
-          switch(target)
-          service_call
-        end
+        service_call
       end
 
       def switch(target)

config/locales/en.yml

@@ -84,15 +84,6 @@ en:
     attributes:
       projects/copy_options:
         dependencies: "Dependencies"
-      projects/types/switch:
-        target_id: "Variant"
-    errors:
-      models:
-        projects/types/switch:
-          attributes:
-            target_id:
-              not_in_family: "must belong to the same type family"
-              unchanged: "must be different from the one the project uses now"
 
   activerecord:
     attributes:
@@ -718,6 +709,7 @@ en:
               cannot_assign_variant_and_parent: "Cannot assign a variant and its parent "
               cannot_assign_variants_yet: "Variants cannot be enabled on a project yet. Enable the feature flag to try out variants"
               in_use_by_work_packages: "still in use by work packages: %{types}"
+              switch_target_blank: "Select the variant to switch to"
               switch_target_identical: "The target type must be different from the type the project uses now"
               switch_target_not_in_family: "Can only switch to another type of the same family"
           cannot_be_assigned_to_artifact_work_package: "The chosen user is not allowed to be assigned to work packages."

spec/features/projects/settings/work_package_types_spec.rb

@@ -130,12 +130,15 @@
     end
   end
 
+  # Reported under the select rather than as a flash, so the choice can be corrected where it
+  # was made.
   it "refuses to apply the variant the project already uses" do
     settings_page.open_switch_dialog(design)
     settings_page.apply_switch
 
     within(settings_page.switch_dialog) do
-      expect(page).to have_text("must be different from the one the project uses now")
+      expect(page).to have_text("The target type must be different from the type the project uses now")
+      expect(page).to have_select("Variant", selected: "Epic: Design")
     end
     expect(project.reload.project_types.find_by(type: epic).variant).to eq(design)
   end