Use ActiveModel::Attributes instead of custom implementation about activeinteractor HOT 4 CLOSED

I see that it's the intended behaviour, my point is more wondering aloud if the context should inherit all of the values in the incoming context or if it should only handle named attributes when initializing the context. The context should be modifiable once it has been received by an interactor since that is the intent of the interactor pattern, but should the initial context itself receive all values during initialization? In the patch I provided (which I'm not particularly fond of) I only copy over explicitly named attributes when they are set, rather than all attributes in the context.

Anyways, we can continue the discussion in the bug report. I'll post the example code from here.

from activeinteractor.

I've discovered a bug in this PR that isn't covered by a spec: when you are using an organizer, the context values in the organizer's steps don't get initialized properly. Basic case:

class TestInteractor < ApplicationInteractor
  def perform
    p context
    p context['user']
    p context.user
  end
end

class TestInteractorContext < ApplicationContext
  attributes :user
end

class TestOrganizer < ApplicationInteractorOrganizer
  organize :test_interactor
end

TestOrganizer.perform(user: 'foo')

Output before change:

#<TestInteractorContext user="foo">
"foo"
"foo"
TestOrganizer::Context {
    :user => "foo"
}

Output after change:

#<TestInteractorContext user="foo">
"foo"
nil
TestOrganizer::Context {
    :user => "foo"
}

As we see here, accessing the user attribute via a method call fails and silently returns nil. The reason this is happening goes something like this:

• there is no TestOrganizerContext and we actually end up using a just-in-time TestOrganizer::Context that has no attributes defined.

• in the original code, this worked because the OpenStruct initializer handled copying over all key-pair values from the context, but in the new code, we copy over attributes, which is quite different. In the new code, since no attributes are defined on the just-in-time TestOrganizer::Context class that's created on-the-fly, no attributes are copied over to the new context.

This issue presumably affects any time that a context is being copied, because it now relies upon attributes being specified rather than all values in an enumerable structure being copied by iteration via OpenStruct. To fix the example code, I would have to manually create a context for TestOrganizer and give it a user attribute.

The question here becomes then: should attributes need to be explicit, rather than allowing basically any value to enter the context via OpenStruct? I see that this is what the interactor gem uses, but in that case, they don't have validations or explicitly defined contexts with explicitly defined attributes, so the two philosophies might not be compatible in this case.

I played around with a couple of ideas, and pushed a branch with a test case that works in both the original code and with the adjustments I've made, but I don't particularly like the code I've written (https://github.com/dark-panda/activeinteractor/tree/attribute-merging-fixes):

• it seems flaky, because it requires a call to #to_sym and makes the assumption that context keys are defined as symbols, which may not be the case.

• it has rather high complexity.

• there are likely further edge cases that I'm not accounting for.

This might sort of boil down to philosophy though -- are explicit contexts with implicit key-pair values going to work together in this structure?

from activeinteractor.

@dark-panda the on the fly context is not new functionality and is the intended behavior. See the first paragraph in https://github.com/aaronmallen/activeinteractor/wiki/Context however this is a bug and I think it is because of the private method merge_attribute_values. I thought this spec compensated for this https://github.com/aaronmallen/activeinteractor/blob/master/spec/active_interactor/context/base_spec.rb#L96 but it would appear I was wrong.

Could you do me a favor and open a bug with your use case. I look at it tomorrow.

from activeinteractor.

@dark-panda the feature has been released and is now available in v1.0.1

from activeinteractor.