Image controller with interactors

[kevindew]

Trello: https://trello.com/c/vf6xQW7w/739-prototype-images-controller-with-interactors-pattern

As discussed at our meeting last week this is a demo of image controllers implemented with the interactor gem. I'm opening this up for reviews, thoughts, forks and ideas about how we feel/move forward with this pattern.

This PR is relatively quickly done so may have some errors, it also hasn't made efforts to reduce code duplication.

There are two small changes made in this PR to the behaviour of the existing code:

1. We no longer preview on image creation - this always fails because of lack of alt text, so why bother trying?
2. Round the generation of crop_height rather than floor - this made tests slightly easier.

Here's a quick round up on my thoughts on the implementation here:

The good

• The image controller is now comprehensible. Every method seems short enough that you can follow reading the code in one pass
• The interactors offer opportunities to break down the business logic into easier to manage chunks
• Once you know the structure it's pretty easy to map interactor to an action

The bad

• Reading the controller there are no clues where you might find a class such as Images::Create
• I got caught out a few times with the plural naming and accidentally prefixed interactors with Image::
• I got in a bit of a muddle experimenting with the means to write the interactors - more on that later.

The ugly

• Writing the tests was a bit strange. Since we rely on feature tests already I'm not sure if much value is provided by writing them now, they also felt a bit repetitive. However they do feel like they should be part of a future strategy.
• There's quite a lot of code duplication in both the interactors and tests. We may need to use inheritance/concerns/helpers as a means to reduce.

Style to write the interactors

I quickly got myself into a muddle trying to work out the nicest way to write the interactors and how to deal with the big global context variable.

My first approach went for a very readable call method at the expense of the readability of the private methods: https://gist.github.com/kevindew/4ab20f0d861a83267395ff7d0c0d608f. In here you'll notice the various assignments to context and the frequent usage of context. In this way they're similar to our feature tests of using methods to describe a procedure.

In later approaches I started trying to use context less. I went more for the approach that we use delegators to access the context creation arguments (which in hindsight could be just instance variables) and then set a context at a point of completing or exiting. This felt more like we have a defined entry and exit state as opposed to collecting state as progress.

I'm interested in thoughts on which of the styles here is more appealing.

---

Anyway to wrap this up, please provide thoughts or ideas of things we can try to reduce any initial weaknesses with this approach. Even better would be to fork this branch and experiment with tweaks to it. Catch you in a week.

[benthorner]

Great improvement for the ImagesController :-). I agree the tests aren't adding much value, but they could be a great place to move some of the feature testing, such as the transactional behaviour of the code, or the edge case behaviour of certain features - you're right, we need a strategy!

One thing that concerns me is the recent activity on this repo. I've raised an issue/question to get their thoughts on a couple of my comments below, but I notice that the issue responses are looking a little stagnant; that said, it's so un-opinionated that I can't see this being an issue, but I wonder if we've thought about just rolling our own?

[benthorner]

Can we be more specific about the type of failure here? It's unclear why the code within the if block follows from the generic failure?, but I appreciate it's also the only expected failure mode for this particular action. What do you think this would look like for distinct types of failure?

[benthorner]

I also notice the examples in the gem use result.success? in preference to failure?. Probably doesn't make much difference, except that having the success code first communicates the intention of the action faster.

[kevindew]

Yeah I don't mind if success is before failure, I imagine I mostly maintained the existing flow of the code.

What do you think this would look like for distinct types of failure?

Could do something like:

if result.failure_reason == :invalid_state
elsif result failure.reason == :issues
elsif result.success?
end

Though I'd hope we're mostly only dealing with one failure type in the view (for the sake of conditionals) and that other pre-condition ones can be handled with exceptions in a similar way to ActiveRecord::RecordNotFound.

[benthorner]

I'm not too fussed either - I was trying to think of a way of maintaining the clarity of the current controller structure when we have requirements issues. Perhaps just doing raise unless result.issues is enough to express that in result.failure?, as using ifs can still imply there's a third execution path, which is neither a defined success or failure.

[benthorner]

It's a little strange that we can directly ask the result about failure?, but not about the success - we're asking the updater instead. Perhaps this is just a limitation of the interactor gem :-(.

[kevindew]

So for me I was thinking of updater as some of the key data that is part of the success - but it could always be assiged to context, particularly as we do same conditional in the interactor itself for the timeline entry.

[benthorner]

Yeah, I think that would be better. Otherwise, you have to be quite familiar with the code of the interactor; although this is the case for other everything else assigned to the result, at least for domain models, the value of the variable is pretty clear.

One annoyance I found when working on my own approach was the verbosity of using context/result everywhere, as well as the mystery of what's in the result. How would it be if we used something like values_at on result to get them all in one go?

[benthorner]

Could we just use params[:document] here? It's strange to use the context to extract the document, as it's not something that was operated on by the interactor.

[kevindew]

Yeah we could - I found it a bit strange using a param for one argument and result for other. Not something I'm hugely bothered about though.

[benthorner]

I thought we said we'd call these CreateInteractor? That would help with the discoverability issue you mentioned in your comments :-).

[kevindew]

That feels pretty weird because of the class name being a verb, if that seems cool to you and others though can roll with that - is consistent at least.

[benthorner]

This is much easier to follow than the controller version :-).

[benthorner]

The structure of this method is quite different from the Create version, which makes use of private methods. I see you've talked about the different styles in the PR description, but maybe the Create version was forgotten accidentally - it does seem easier to read, though :-).

[kevindew]

Yup, the create one was a lot closer to the feature test style I used at first. Create also has the advantage that not many variables are shared around so it can be quite simple. Whereas here we're passing the updater back.

This one could be along the lines of below, if we're not too concerned about context setting side-effects in methods.

def call
  Edition.find_and_lock_current(document: params[:document]) do |edition|
     remove_image(edition, params[:image_id])
     create_timeline_entry(edition)
     try_create_preview(edition)
  end
  
  def remove_image(edition, image_id)
    context.image_revision = edition.image_revisions.find_by!(image_id: params[:image_id])
    context.updater = Versioning::RevisionUpdater.new(edition.revision, user)

    updater.remove_image(image_revision)
    edition.assign_revision(updater.next_revision, user).save!
  end
end

[benthorner]

I found there are pros and cons to this: within the same file, it's quite easy to understand where all the assignment are coming from, but accessing the values becomes more verbose e.g. you're missing context in updater.remove_image in the above example. This is something I was toying with getting around by using something like delegate_missing_to, but I reckon what you have in your Create example is the best compromise, as it's really clear where the context is getting set. So, I vote for Create :-).

[benthorner]

And now a rival PR (not really) - #914

[kevindew]

One thing that concerns me is the recent activity on this repo. I've raised an issue/question to get their thoughts on a couple of my comments below, but I notice that the issue responses are looking a little stagnant; that said, it's so un-opinionated that I can't see this being an issue

We could have a conditional of if result.error == :error_type_1 and not even check the failure? field since result.error would be nil if only errors used it.

but I wonder if we've thought about just rolling our own?

When it comes to rolling our own, I’m keen for us to try avoid that unless it becomes absolutely necessary. The reasons I feel this way are:

• we’re not solving a bespoke problem, so if we need a bespoke solution we’re likely on a bad path
• It’s a pattern to define/maintain/tweak/document - which can all be handled by a gem
• It’s much harder to bikeshed on someone else’s decisions
• The fact someone else’s stuff is domain agnostic makes it much harder to spill our domain in, as evidenced by command pattern in Publishing API which tried to define consistencies and then blocked re-usability.

One of the things I like about Interactor is that if we reach a point where we have to let it go because it's not right for our needs then we can just re-write an implementation for us. So mostly I'm keen for us to experiment with interactor until we feel that we've either customised it so much it's unrecognisable or that we're doing things with it outside it's purpose.

And now a rival PR (not really) - #914

Woop! nice one putting that together. I'm not really sure whether to comment here or there, but since there's already discussion here I'll comment here briefly, and move over there if it gets more in depth.

So I think I've identified two main differences in your suggestion with what we have with interactors: which are the distinct pre_op, op and post_op steps; and the ability to succeed or fail with a particular reason.

With regards to the distinct steps:

These feel a bit like before and after blocks in Rails terminology. Although it's been useful to talk in terms of post_op for sidekiq I think this pattern here is actually of low meaning due to it's generality. Most of our concerns seem to be if something is running in a lock or not, which I can imagine is where post_op (or after) can make sense.I think the difference of pre_op and op is a greyer area where there aren't defined bounds and it'd be much easier to read code that did the steps of those individually rather than bound into methods.

With regards to success failure reasons:

This does seem valuable - I wonder what ways we can tinker with it. I left a suggestion above in this comment about you can set a variable that'd otherwise be nil.

One of the things I think we lose once we tie things down to particular reasons for failure is the need to even be checking for failure?. E.g. if I take your example here:

content-publisher/app/controllers/images_controller.rb

Lines 14 to 26 in 9f6e34b

	if result.aborted?(:issues)
	flash.now["alert_with_items"] = {
	"title" => I18n.t!("images.index.flashes.upload_requirements"),
	"items" => result.issues.items,
	}
	render :index,
	assigns: { issues: result.issues, edition: result.edition },
	layout: rendering_context,
	status: :unprocessable_entity
	return
	end

- as we're only checking that it failed for issues, then it will try run the success code even if it was marked as failed.

[benthorner]

@kevindew thanks for the thorough comments :-). I think we both agree that the success/failure mechanism provided by interactor is the main thing that's not quite fit-for-purpose. I'm happy to go with your idea of if result.error == ..., or even just if result.issues.

The other change I made in my PR was to try and make working with the context easier, but I think your approach of not using it until we return is much easier to understand. Possibly I just created my own problem and solved it!

Otherwise, I'm still wondering if there's a way to expose the locking/transaction behaviour more clearly. The only idea I have so far is to actually put these calls inside the interactor, which does re-introduce duplication, but makes it easier to see what's going on when we have post op code, and that it's running outside of the lock/transaction.

[kevindew]

Closing in preference of #960