Potential Pavlov enhancements

[jobertabma]

Last week I spent time on Pavlov's code. I looked at the way Pavlov was structured and how it is used. Did a small write-up about this:

Unit tests A while ago I noticed that the authorized? and validate methods cannot be unit tested. This is due to the fact that the initializer of the interactor calls the authorized? and validate method. Right now, unit tests of the authorized? methods look like this:

describe '#authorized?' do
  it 'does not raise an error when the current_user is passed' do
    expect { described_class.new '<<arguments>>', current_user: current_user}.to_not raise_error(Pavlov::AccessDenied)
  end

  it 'raises an error when no current_user is passed' do
    expect { described_class.new '<<arguments>>' }.to raise_error(Pavlov::AccessDenied)
  end
end

The problem here is that it doesn't test the authorized? method itself. The Pavlov::AccessDenied exception could also be raised by the has_access? method which is implemented in a lot of interactors. A better way of testing these methods would be something like:

describe '#authenticated?' do
  it 'should return false when no current_user object is passed' do
    interactor = described_class.new

    expect(interactor.authenticated?).to be_false
  end

  it 'should return true when a current_user object is passed' do
    interactor = described_class.new current_user: current_user

    expect(interactor.authenticated?).to be_true
  end
end

Optional arguments Every once in a while developers want to write a query, interactor or command that can take an optional argument. Right now this is not possible. Optional arguments are typically implemented like this:

# ...
arguments :item_id, :optional_argument

def execute
  if @optional_argument
    # do something
  else
    # do something else
  end

  # some more code
end

A cleaner way to do this would be something like this:

def execute
  something = arg :optional_argument, false
end

In this example the arg method takes a symbol that looks for the :optional_argument argument. If it is not found, the second parameter is returned (in this case false). Retrieving arguments would also clean user's code base, because they are not bound to a specific order of the arguments anymore. A structure below might make more sense because of its backwards compatibility with the current Pavlov though:

class JustSomeInteractor
  include Pavlov::Interactor

  argument :item_id, default: false, validate: :integer_string

  # some more code
end

Code duplication The interactors take an optional options hash. This hash includes additional arguments that can be passed to an interactor. In most cases this hash contains the current_user key. The value of this key contains a User object and is used for doing authentication and authorization checks. Calling an interactor currently looks something like this:

# the following code lives somewhere in a controller
interactor :'module/class', 'first argument', 'second argument', { current_user: current_user }

The third argument are referred to as the 'Pavlov options'. Looking at the way it is used, it is just an argument for the interactor. Given the fact that it is passed through to the interactors in all situations, the arguments could be merged. This would look something like this:

# the following code lives somewhere in a configuration of Pavlov
Pavlov::options = {
  current_user: current_user
}

# the following code lives somewhere in a controller
interactor :'module/class', {
  user_id: 'an ID',
  foo: 'bar'
}

The interactor method merges the Pavlov::options hash and hash that is given to the second parameter of the interactor method. A side-effect of looking at it this way is that the current_user becomes an argument of an interactor (instead of it being a 'special' option).

Inconsistent naming Pavlov has some inconsistent method names. An interactor only contains an authorized? method. This method is typically used for an authentication check. The has_access? method is typically used for implementing the authorization check. The problem is that the has_access? method needs to be called manually in the execute method`. It looks something like this:

class JustSomeInteractor
  include Pavlov::Interactor

  def execute
    raise Pavlov::AccessDenied unless has_access?

    # some additional magic
  end

  def authorized?
    !! @options[:current_user]
  end

  def has_access?
    @options[:current_user].is_admin?
  end
end

People end up testing if the has_access? method is called in the execute method (which is called manually). It'd be better to separate this logic and rename the methods. I'd propose something like this:

class JustSomeInteractor
  include Pavlov::Interactor

  def execute
    # some additional magic
  end

  def authenticated?
    !! @options[:current_user]
  end

  def authorized?
    @options[:current_user].is_admin?
  end
end

The ActiveSupport dependency Currently, Pavlov depends on ActiveSupport inflectors:

# lib/pavlov.rb

def self.get_class_by_string classname
  classname.constantize
end

def self.string_to_classname string
  string.to_s.camelize
end

The constantize and camelize methods are part of ActiveSupport. In terms of decoupling and stop depending on these kind of frameworks, I'd propose to replace these calls using the following code:

# lib/pavlov.rb

def self.camelize term
  term.to_s.gsub(/\/(.?)/) { '::' + $1.upcase }.gsub(/(^|_)(.)/) { $2.upcase }
end

def self.constantize class_name
  class_name.split('::').inject(Kernel) do |scope, const_name|
    scope.const_get(const_name)
  end
end

Obsolete code Looking at the code base it seems that the commands and queries also have an authorized? method. Given the fact that Pavlov's philosophy is partially about centralizing logic into interactors, it doesn't seem like this is the way to go. I'd propose to remove this functionality.

Wanted to start a discussion about these topics – will try to come up with a couple of smaller pull requests to implement the topics above. Let me know what you think!

[markijbema]

Hey Jobert, could we maybe split this up over several issues? I think, that way we can discuss it easier. I have some answers/opinions on some points, but I'm afraid that if we do this in one issue it won't be very usable.

[marten]

Maybe @markijbema could also preemptively close this issue, so it's clear we're not going to do anything in this issue.