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

[WIP] Bang Validations #10

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

davesims
Copy link

@davesims davesims commented Apr 19, 2017

🚧 Don't raise exceptions on validation failures by default 🚧

This PR adds bang methods for the three validation types to indicate that they will raise exceptions when they fail, and will convert the existing non-bang validation methods so that they accumulate errors in an errors array and then aggregate them on the context's message, rather than raise exceptions.

The new 'bang' validations will behave exactly like the former non-bang validations:

  • validates_before!
  • validates_after!
  • validates_around!

The existing validation methods will be converted so that they accumulate validation error messages rather than raise exceptions:

  • validates_before
  • validates_after
  • validates_around

The rationale here is to reduce coupling to exception classes and eliminate the need for callsites to control flow with exception handling. So for instance in a controller action that uses an ActionTask with any of the default validation methods:

class FooActionTask
  include ActionLogic::ActionTask
  validates_before foo_id: {}

  def call
    [...]
  end
end

...instead of a begin/rescue to trap for ActionLogic errors like this:

 def show
   begin
     result = FooActionTask.execute(model_id: 1)
     if result.success?
       flash[:notice] = result.message
       redirect_to happy_path
     else
       flash[:error] = result.message
       redirect_to sad_path
     end
   rescue ActionLogic::MissingAttributeError => ex #  ActionLogic::MissingAttributeError: context: Class message: foo_id attributes are missing
     flash[:error] = ex.message
     redirect_to sad_path
   end
 end

The validation error messages would be accumulated in the result/context object's message:

 def show
   result = FooActionTask.execute(model_id: 1)
   if result.success?
     flash[:notice] = result.message
     redirect_to happy_path
   else
     flash[:error] = result.message # "foo_id attributes are missing"
     redirect_to sad_path
   end
 end

If the callsite needs to trap for validation errors, the new bang methods can be used:

class FooActionTask
  include ActionLogic::ActionTask
  validates_before! foo_id: {}

  def call
    [...]
  end
end

...and any validation errors encountered during an execute call will result in an ActionLogic error.

@@ -6,27 +6,31 @@ module ActionLogic
module ActionValidation
module ClassMethods
def validates_before(args)
@validates_before = args
@validates_before = args.merge(raise_action_logic_exception: true)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will eventually eliminate this dupe, but for now I just wanted to see what it looked like to use the validations hash to signal to the validation classes whether to raise an exception or not.

@codecov-io
Copy link

codecov-io commented Apr 19, 2017

Codecov Report

Merging #10 into master will increase coverage by 0.07%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #10      +/-   ##
==========================================
+ Coverage    99.1%   99.17%   +0.07%     
==========================================
  Files          22       22              
  Lines         668      729      +61     
==========================================
+ Hits          662      723      +61     
  Misses          6        6
Impacted Files Coverage Δ
lib/action_logic/action_validation.rb 100% <100%> (ø) ⬆️
.../action_logic/action_validation/type_validation.rb 100% <100%> (ø) ⬆️
spec/action_logic/active_use_case_spec.rb 100% <100%> (ø) ⬆️
...on_logic/action_validation/attribute_validation.rb 100% <100%> (ø) ⬆️
spec/action_logic/action_task_spec.rb 100% <100%> (ø) ⬆️
...ion_logic/action_validation/presence_validation.rb 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d4599fc...4172fd4. Read the comment docs.

return unless validation_rules.values.find { |expected_validation| expected_validation[:presence] }
errors = presence_errors(validation_rules, context)
raise ActionLogic::PresenceError.new(errors) if errors.any?
tmp_rules = validation_rules.clone
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have to clone a copy of validation_rules here instead of deleting the :raise_action_logic_exception directly, since for 'around' validations, validation_rules will be accessed twice, once before and once after.

end

def validates_after(args)
@validates_after = args
@validates_after = args.merge(raise_action_logic_exception: true)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how I feel about using an extra attribute to signal the validation classes. This is just a rough draft. If this is the approach to take, it needs to be a very specific name to avoid potential collisions, hence "raise_action_logic_exception".

end

def validates_around(args)
@validates_around = args
@validates_around = args.merge(raise_action_logic_exception: true)
Copy link
Author

@davesims davesims Apr 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WIP: eventually these will be false on all of the non-bang validation methods.

end

def get_validates_before
@validates_before ||= {}
@validates_before ||= { raise_action_logic_exception: true }
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: fix dupe.

@rewinfrey
Copy link
Owner

@davesims friendly ping 😄 let me know if you'd like to keep pushing on this thread, and maybe we can 🍐 on this or talk through it again?

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

Successfully merging this pull request may close these issues.

3 participants