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

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

Open
Hirurg103 opened this issue Feb 5, 2020 · 5 comments
Open

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

Hirurg103 opened this issue Feb 5, 2020 · 5 comments

Comments

@Hirurg103
Copy link

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
Copy link
Contributor

bmorrall commented Mar 1, 2020

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
Copy link
Contributor

bmorrall commented Mar 1, 2020

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
Copy link
Author

Hirurg103 commented Mar 4, 2020

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

@bmorrall
Copy link
Contributor

bmorrall commented Mar 6, 2020

This PR should fix that #25

@juliofalbo
Copy link

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.

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