Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adding additional steps in wrap #61

Open
Quantenradierer opened this issue Nov 29, 2021 · 12 comments
Open

adding additional steps in wrap #61

Quantenradierer opened this issue Nov 29, 2021 · 12 comments

Comments

@Quantenradierer
Copy link

Hey,

We have a rails engine which defines business processes. A few other projects use this engine and make minor changes to the business processes, depending on the specific application.

Now the engine defines a wrap around a few steps, while a project wants to add a step in this wrap:

# defined in the engine:
module Concerns::TestOperation
    extend ActiveSupport::Concern
    
    included do
      step Wrap(Transaction) {
        step :load_something
        step :save_something
      }
    end
end

# defined in the project:
class TestOperation < Trailblazer::Operation
  include Concerns::TestOperation

  step :change_something, after: :load_something
end 

This of course won't work when :load_something is wrapped.

I already looked into various documentation about after, before and even subprocess patching but I can't find anything how to add/replace/delete steps in a Wrap?

Thank you

@yogeshjain999
Copy link
Member

Hey @Quantenradierer!!

The reason before, after etc doesn't work with Wrap is because passing set of steps to Wrap creates isolated anonymous operation and injects it into your operation, similar to what Subprocess does.

So Wrap is like an shortcut to define and inject nested operation with addition to calling wrapper . In above case, after searches steps which are present in current operation and not in nested or wrapped operation.

How about you use inheritance here to achieve similar thing ?

# in engine
class BaseOperation < Trailblazer::Operation
  step :load_something
  step :save_something
end

# in your project
class TestOperation < BaseOperation
  step :change_something, after: :load_something
end

# in your operation
step Wrap(Transaction) {
  step Subprocess(TestOperation)
}

@Quantenradierer
Copy link
Author

Thank you for the response. I have thought about this as well and in the few cases for transactions or exceptions this is okay.

But for metrics which shall document if a operation failed/succeeded/raised an error it would add one additional class since we need to wrap all operations.

This is our current implementation for metrics, which overwrites the call from operations:

      def call(*, **kwargs)
        result = super
        metrics(success: result.success?, error: nil, **kwargs)
        result
      rescue StandardError => e
        metrics(success: false, error: e, **kwargs)
        raise
      end

Is there a reason why after and before do not support 'chained'-ids?

like:

step Wrap(Transaction), id: 'wrap1' {
        step :load_something
        step :save_something
      }

step :change_something, after: {wrap1: :load_something}

I am not familiar enough with the code to implement this yet but maybe in a year or so there will be a PR, if you think this belongs into trailblazer :)

Can be closed once you answer.

@yogeshjain999
Copy link
Member

Good point about the "chained" ids!! But I think patching is more explicit and configurable. What if you want to add a step in nested operation which is 2 level deeper using after ? 😉

Current implementation of id has been kept simple to use plain objects and use patching instead to explicitly modify given operation without mutation.

If you want to avoid defining extra classes for Wrap, how about below example taken from patching documentation and extended to use Wrap ?

class BaseOperation < Trailblazer::Operation
  step :load_something
  step :save_something
end

class TestOperation < Trailblazer::Operation
  # Defining this as class method in order to access it in BaseOperation scope
  def self.change_something(ctx, **)
  end

  step Wrap(Transaction) {
    step Subprocess(BaseOperation, patch: -> { step TestOperation.method(:change_something), after: :load_something })
  }
end

@apotonick
Copy link
Member

Doesn't patching work here? https://trailblazer.to/2.1/docs/activity.html#activity-dsl-options-patching This implements "chained IDs" the way @Quantenradierer need it.

@Quantenradierer
Copy link
Author

Quantenradierer commented Dec 1, 2021

@yogeshjain999 I don't think this is suitable for us.

One of our requirements is that the engine must define a whole usable process.


I have tried the patching @apotonick but it does not work for wraps. The class is being copied and modified (via Class.new) but the wrap (Wrapped) is an instance and can't be copied .

I also tried to modify the wrap activity without copying it and it worked.

This was just for testing purposes, don't use this code:

def customize in helper.rb

                if activity.is_a?(Trailblazer::Macro::Wrap::Wrapped)
                  patched_activity = activity.instance_variable_get(:@operation)
                else
                  patched_activity = Class.new(activity)
                end

I also looked into before/after parameters.def find_index(sequence, insert_id) in linear.rb returns an index to an id.

Allowing insert_id to be an array would also require to return an array of indices. This of course needs a lot of changes in other code.

@apotonick
Copy link
Member

So, do we agree we have a patching bug here, then?

Regarding the insert_id points, we have the chance now to push something into the new dsl minor release. Let me think about it a bit.

@apotonick
Copy link
Member

@Quantenradierer How would you use the insertion logic with an array? You mean you want to insert a bunch of sequential steps at a certain point at once?

@apotonick
Copy link
Member

apotonick commented Dec 1, 2021

One of our requirements is that the engine must define a whole usable process.

Maybe you're misunderstanding patching? It is possible and designed to insert/alter as many steps as you need into nested operations, isn't that what you need? 😂

@Quantenradierer
Copy link
Author

@Quantenradierer How would you use the insertion logic with an array? You mean you want to insert a bunch of sequential steps at a certain point at once?

I would expect that

step Wrap(Transaction), id: 'wrap1' {
        step :load_something
        step :save_something
      }

step :change_something, after: [:wrap1, :load_something]

results in this:

step Wrap(Transaction), id: 'wrap1' {
        step :load_something
        step :change_something
        step :save_something
      }

@Quantenradierer
Copy link
Author

One of our requirements is that the engine must define a whole usable process.

Maybe you're misunderstanding patching? It is possible and designed to insert/alter as many steps as you need into nested operations, isn't that what you need? 😂

Let's take the example:

class BaseOperation < Trailblazer::Operation
  step :load_something
  step :save_something
end

class TestOperation < Trailblazer::Operation
  # Defining this as class method in order to access it in BaseOperation scope
  def self.change_something(ctx, **)
  end

  step Wrap(Transaction) {
    step Subprocess(BaseOperation, patch: -> { step TestOperation.method(:change_something), after: :load_something })
  }
end

This won't do it, cause I need to define the wrap in the BaseOperation. And then I can't modify the inside of it in the TestOperation

@apotonick
Copy link
Member

apotonick commented Dec 3, 2021

Thanks - that clarifies it! You'd use patch as follows to add/alter the nested OP (pseudo code!!!).

class TestOperation < Trailblazer::Operation
  step Wrap(Transaction), patch: [:base_operation] -> { step added_step, after ... }]

We need to test this, though. Anyway, the idea is to be able to change internals from TestOperation and not somewhere deeper. I think that's what you need, right?

@apotonick
Copy link
Member

Yo @Quantenradierer, moin!

I am envisioning a more intuitive syntax for patching, how about this?

class TestOperation < Trailblazer::Operation
  patch: ["wrap/Transaction", :base_operation] -> { step added_step, after ... }]

Something like that, so you don't have to repeat step (Wrap(Transaction)) and instead provide its ID.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants