shoulda

Comments and changes to this ticket

• 

  Ryan McGeary September 2nd, 2008 @ 09:01 PM

  • → Tag changed from “” to “active_record_helper should_protect_attributes”

  Mike,

  Do you still feel like this is an issue? Could you elaborate some more? http://github.com/thoughtbot/sho...

  Given a particular scenario, what error message do you see, and what do you expect to see?

  (I see that this method was mostly patched by you) http://github.com/thoughtbot/sho...

• 

  Mike Boone September 3rd, 2008 @ 06:08 PM

  I did submit that earlier patch (#25), however the method was done by another fellow and I just added a test and packaged it up.

  Anyway, the current code appears to handle the error messages correctly.

  However, I discovered there's an issue if you use attr_protected and then run a should_protect_attributes on an attribute that was not included in attr_protected. As the code is, the test will pass.

  For example, change line 33 in test/unit/user_test.rb to this:

  should_protect_attributes :password, :ssn

  If you run ruby test/unit/user_test.rb, it will pass, and it should fail, because the User model only specifies password.

  I pushed a patch to my GitHub fork. With this patch, the test will fail as expected.

  http://github.com/boone/shoulda/...

  In the tests for Shoulda itself, is there a way to write a test to make sure that Shoulda raises an error when it's supposed to?

• 

  Ryan McGeary September 3rd, 2008 @ 06:23 PM

  Thanks. I understand the problem now.

  In the tests for Shoulda itself, is there a way to write a test to make sure that Shoulda raises an error when it's supposed to?

  Good question, let me think about that. The Shoulda tests could definitely benefit from testing these kinds of failure scenarios.

• 

  Ryan McGeary September 3rd, 2008 @ 08:51 PM

  • → Assigned user changed from “Tammer Saleh” to “Ryan McGeary”

  I have a branch for this ticket: http://github.com/rmm5t/shoulda/...

  I added a should_fail macro solely for testing failure scenarios within the core shoulda tests: http://github.com/rmm5t/shoulda/...

  The magic comes from this alias_method_chain hack: http://github.com/rmm5t/shoulda/...

  ...which allows for shoulda core tests like this:

  
  should_fail do
    should_require_attributes :comments
    should_protect_attributes :name
  end
  

  I'm going to sleep on this before pushing into shoulda core, but I think the ability to test failure scenarios within the core shoulda tests is a good thing. I'm just not sure if this is the most elegant way to do it.

  Tammer, Mike, Please chime in with your thoughts, especially if can think of a better way to handle this.

• 

  Mike Boone September 4th, 2008 @ 12:31 AM

  My ruby-fu is not sufficient to comment on how you created the should_fail macro. But I like the idea and it works from my minimal testing.

  With regard to using should_fail for should_protect_attributes, it should also be used on the Tag unit test. User uses attr_protected and Tag uses attr_accessible.

  I forget how to push a non-master branch to GitHub, so instead I'll attach a patchfile that adds the should_fail test to the Tag unit test.

  • 0001-Added-should_fail-test-for-should_protect_attributes.patch 681 Bytes
• 

  Ryan McGeary September 14th, 2008 @ 09:22 PM

  • → State changed from “new” to “committed”

  Sorry for the delay, but this fix is in place along with a should_fail macro for internal shoulda testing.

  Thank you very much Mike.

  http://github.com/thoughtbot/sho...

  http://github.com/thoughtbot/sho...

• 

  Mike Boone September 15th, 2008 @ 08:44 AM

  Looks good. Can we add the should_fail test to the Tag unit test as well? (see patch from the 4th)

  That was we can make sure that should_protect_attributes is working properly on attr_accessible and attr_protected.

• 

  Ryan McGeary September 15th, 2008 @ 09:01 AM

  Mike, Thanks for the reminder. I just pushed that extra failure scenario test.

  http://github.com/thoughtbot/sho...