factlink / pavlov Goto Github PK

https://travis-ci.org/Factlink/pavlov/jobs/8690041

We seem to have compatibility issues with 1.9.2. Shall we just drop support? I don't think anyone uses it still.

@marten @jjoos do you guys agree?

Why should we want to type query :user_by_id instead of query :UserById, while the class is called UserById? It makes it hard when searching through the code as both formats have to be searched.

This can probably be partly extracted from the backend branch. Let's first introduce a Context and then later on add the Backend.

Most gems I know don't do this, and I'll often open up the Github page for a gem to see if it's still under active development. I would probably simply assume that Pavlov has been dead for 5 months, and not look at potential other branches.

I understand this is an artefact of using git-flow, but maybe this is an unforeseen consequence.

The Virtus gem used by DataMapper Ruby Object Mapper implements a lot of what we have for defining arguments. RoQua uses this for various entities.

We might want to evaluate using that library inside of Pavlov, instead of something home grown. There's also some alternatives to Virtus.

Right now RoQua adds callbacks to interactors, which are used to communicate result state and execute bits of controller logic (redirect_to @post for success vs render action: :edit for failure):

class PostsController
  def update
    interaction = PostUpdate.new(params)
    interaction.on_success { redirect_to interaction.post }
    interaction.on_failure { render action: :edit, locals: {post: interaction.post} }
    interaction.call
  end
end

class PostUpdate < Interactor
  def on_success(&block)
    @on_success = block
  end
  # same for on_failure

  def execute
    if rand > 0.5
      @on_success.call
    else
      @on_failure.call
    end
  end
end

We could add it like that to Pavlov, but another way would be to support returning "status objects" from interactors:

class PostsController
  def update
    result = PostUpdate.new(params).call
    result.on_success { redirect_to interaction.post }
    result.on_failure { render action: :edit, locals: {post: result.post} }
  end
end

class PostUpdate < Interactor
  class PostUpdateStatus < Struct.new(:status, :post)
    def on_success
      yield if status == :success
    end
    # same for on_failure
  end

  def execute
    if rand > 0.5
      PostUpdateStatus.new(:success, post)
    else
      PostUpdateStatus.new(:failure, post)
    end
  end
end

For us, both would work equally well. Which would be your preference?

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!

Engines, like many gems, scope all their stuff within a module named after the gem. Pavlov expects Interactors to be defined top-level, and this is incompatible.

RubyGems.org doesn't report a license for your gem. This is because it is not specified in the gemspec of your last release.

via e.g.

spec.license = 'MIT'
# or
spec.licenses = ['MIT', 'GPL-2']

Including a license in your gemspec is an easy way for rubygems.org and other tools to check how your gem is licensed. As you can image, scanning your repository for a LICENSE file or parsing the README, and then attempting to identify the license or licenses is much more difficult and more error prone. So, even for projects that already specify a license, including a license in your gemspec is a good practice. See, for example, how rubygems.org uses the gemspec to display the rails gem license.

There is even a License Finder gem to help companies/individuals ensure all gems they use meet their licensing needs. This tool depends on license information being available in the gemspec. This is an important enough issue that even Bundler now generates gems with a default 'MIT' license.

I hope you'll consider specifying a license in your gemspec. If not, please just close the issue with a nice message. In either case, I'll follow up. Thanks for your time!

Appendix:

If you need help choosing a license (sorry, I haven't checked your readme or looked for a license file), GitHub has created a license picker tool. Code without a license specified defaults to 'All rights reserved'-- denying others all rights to use of the code.
Here's a list of the license names I've found and their frequencies

p.s. In case you're wondering how I found you and why I made this issue, it's because I'm collecting stats on gems (I was originally looking for download data) and decided to collect license metadata,too, and make issues for gemspecs not specifying a license as a public service :). See the previous link or my blog post aobut this project for more information.

The actual API is also up for debate, so feel free to have better ideas than just using ActiveModel::Errors. The new README proposes:

  def validate
    errors.add(:id, "can't contain spaces") if id.include?(" ")
  end

• This needs to be implemented.
• Remove the old validations module from alpha_compat?