nebulab / simple_command

A simple, standardized way to build and use Service Objects (aka Commands) in Ruby
http://nebulab.it
MIT License
625 stars 55 forks source link

Make SimpleCommand::Errors compatible with ActiveModel::Errors #24

Open Hirurg103 opened 4 years ago

Hirurg103 commented 4 years ago

After update to v0.1.0 from v0.0.9 tests started to fail with

expect(
  get_users(page: '-2')
).to eq(errors: { page: ["must be a positive number"] })
expected: {:errors=>{:page=>["must be a positive number"]}}
     got: {:errors=>{:page=>"must be a positive number"}}

In the controller errors are rendered the following way:

load_users = LoadUsers.(params)
if load_users.success?
  render json: load_users.result
else
  render json: { errors: load_users.errors }
end

The problem is that SimpleCommand::Errors#to_json format differs from ActiveModel::Errors#to_json

# SimpleCommand::Errors
load_users = LoadUsers.new
load_users.errors.add(:page, 'must be a positive number')
load_users.errors.add(:page, 'cannot be blank')
load_users.errors.as_json
=> {"page"=>"cannot be blank"}

# ActiveModel::Errors
user = User.new
user.errors.add(:email, 'is invalid')
user.errors.add(:email, 'cannot be blank')
user.errors.as_json
=> {:email=>["is invalid", "cannot be blank"]}

It would be good to have SimpleCommand::Errors#as_json to be the same format as ActiveModel::Errors#as_json

bmorrall commented 4 years ago

Hi @Hirurg103, I'm not associated with the simple_command project, but I gave this a look.

I am not seeing this problem; I tried looking through the code and it appears to act the way you expect it to work. I wrote an rspec test to verify this behaviour, and it works as expected.

I created a new Rails, wrote a Command that recreates the errors in your example, and it still behaved as expected.

Have you found this issue to be resolved? or have I missed something while looking into it?

bmorrall commented 4 years ago

I have missed something, not including ActiveModel::Validations does produce the errors object you are referencing.

I wrote a small gist that replicates this problem:

# app/commands/authenticate_user.rb
class AuthenticateUser
  prepend SimpleCommand
  # include ActiveModel::Validations

  def initialize(email, password)
    @email = email
    @password = password
  end

  def call
    errors.add(:email, "is invalid")
    errors.add(:email, "is from a blocked domain")
    errors.add(:password, "is too common")
    nil
  end
end

# app/controllers/authenticate_controller.rb
class AuthenticateController < ApplicationController
  def create
    load_users = AuthenticateUser.(params[:email], params[:password])
    if load_users.success?
      render json: load_users.result
    else
      render json: { errors: load_users.errors }
    end
  end
end

Output:

"errors" => {"email"=>"is from a blocked domain", "password"=>"is too common"}
Hirurg103 commented 4 years ago

Hi @bmorrall , yes, this error is reproducible with pure SimpleCommand (without including ActiveModel::Validations)

bmorrall commented 4 years ago

This PR should fix that https://github.com/nebulab/simple_command/pull/25

juliofalbo commented 3 years ago

I had the same issue, thanks for reporting and for the PR, hope they will merge it soon and release a new version. The bump from 0.0.9 would be a MAJOR release since we had break changes and not a minor how it was.