Suggestion to map fields for composed interactions

[antulik]

this relates to

When a composed interaction fails, its errors are merged onto the caller. This generally produces good error messages, but there are a few cases to look out for.

it's a big assumption and restriction to keep the field names consistent between interaction. My suggestion is to provide options to compose which will map nested errors into new fields on the parent. In other words it will remap error fields.

For example

class A < ActiveInteraction::Base
  string :phone

  def execute
    compose(B, different_field: phone) do
      { different_field: :phone }
    end
  end
end

class B < ActiveInteraction::Base
  string :different_field
  validates :different_field, numericality: true

  def execute
  end
end

outcome = A.run(phone: 'asd')

As a result outcome.errors will have error for phone field.

Another idea for the syntax

   def execute
      map_errors(different_field: :phone)
        .compose(B, different_field: phone)
  end

Why?

• It gives more control to the developer when is needed.
• Current automatic method is error-prone with complex workaround

[tfausak]

This sounds a lot like #245. I agree that something like this would be conceptually nice. So far, it hasn't been worth the effort.

[antulik]

Done proof of concept

class RemapOptions
  def initialize interaction, options
    @interaction = interaction
    @options = options
  end

  def compose(*args)
    @interaction.send :compose, *args

  rescue ActiveInteraction.const_get(:Interrupt) => interrupt
    remaped_errors = ActiveInteraction::Errors.new(interrupt.errors.instance_variable_get("@base"))

    interrupt.errors.each do |attr, error|
      to = @options[attr] || attr

      remaped_errors.add to, error
    end

    raise ActiveInteraction.const_get(:Interrupt), remaped_errors
  end
end

class A < ActiveInteraction::Base
  string :phone

  def execute
    map_errors({different_field: :phone}).
      compose(B, different_field: phone)
  end

  def map_errors opts
    RemapOptions.new(self, opts)
  end
end

class B < ActiveInteraction::Base
  string :different_field
  validates :different_field, length: { minimum: 10, maximum: 15 }

  def execute
  end
end

outcome = A.run(phone: 'asd')
puts outcome.errors.full_messages.join(' and ')

outcome

Phone is too short (minimum is 10 characters)

[tfausak]

Cool! I should have clarified, though. We haven't held off on doing this because we don't know how. Rather, we don't think it's worth the additional complexity, both in terms of ActiveInteraction's interface and implementation.

[AaronLasseigne]

I think this is a problem worth solving. It doesn't come up a ton but when it does it's rather annoying. My problem with solutions like

compose(B, different_field: phone) do
  { different_field: :phone }
end

is that they force you to repeat yourself.

I was thinking about this and we might be able to add a method that fixes this. By wrapping the value passed in a method we can send the value and map the error.

compose(B, different_field: is(:phone))

The is method (I'm not in love with that name) would pass an object as the value for :different_field. It would contain the name of the filter to map to, :phone, and the value of that filter. Then compose passes the value on, gets back any errors, and maps them to the appropriate field before returning them to the wrapped interaction.

Thoughts?

[antulik]

I like the proposed solution. The is method probably need to be renamed to something more meaningful. E.g. linked_input or compose_field or link_errors

It also should be possible to pass values manually, but I think the method can accept the optional second parameter.

compose(B, different_field: link_errors(:phone, formatted_phone))

[AaronLasseigne]

Version 4.0.0 does away with the idea of automatically linking errors for you. Instead they'll all need to be manually linked. There are two ways to do this. One is like we've discussed above using a method called link.

class I < ActiveInteraction::Base
  float :a, :b

  def execute
    compose(Add, x: link(:a), y: link(:b))
  end
end

The second is a shortcut for when interactions share a variable name and you want them linked. It creates a hash of the appropriate links.

class I < ActiveInteraction::Base
  float :x, :y

  def execute
    compose(Add, autolink(:x, :y))
  end
end

This will take care of the mapping filters when composing interactions.

[antulik]

Version 4.0.0 does away with the idea of automatically linking errors for you

After thinking about that i think it should be opt-out rather than opt-in. And here is why

If I don't want autolinking I just use Add.run(). Currently autolinking is the only reason to use compose method, if not the only one.
Also i'm not clear on the new behaviour with autolinking disabled. If Add (child) interaction fails, then compose will return and stop the parent execution, but there wouldn't be any errors. As a result the operation will silently fail. Am i missing anything?

On the second note. Does link(:a) support passing of custom value? I see a common use case when the parent interaction cleans the data and need to pass cleaned version, but still map the errors. This feature adds a lot of flexibility when needed, so would be good to have that feature now, rather than releasing 4.0 and then adding it later after few issues on github.

[AaronLasseigne]

Using compose links the errors and halt the execute block if there are any errors. Instead of the errors being added to inputs that have the same name it'll move them all to :base. So, there errors are still there, they're just not linked to fields. If the person uses link and/or autolink then they can attach the errors to specific fields.

compose(I, inputs) # => all errors moved to :base

compose(I, autolink(*inputs.keys)) # => all errors moved to their exactly named counterparts

I think automatically moving errors is probably more bad than good.

Right now I don't have it accepting a custom value. It seems like a reasonable request. I'll give it some thought.

[antulik]

ah, right, that makes sense to merge them to :base. I didn't think of that, all good then 👍

Thanks for the explanation and great gem