Re-fix honoring of 'except' when used with 'comment_required' #425
Conversation
lib/audited/auditor.rb
Outdated
| end | ||
| end | ||
|
|
||
| def comment_required_state? | ||
| auditing_enabled && |
There was a problem hiding this comment.
Wouldn't it be simpler to add here audited_changes.peresent?
|
ping @jeffdill2, any updates on this? |
|
@tbrisker Sorry, been a busy couple of weeks. I'll get this addressed today. |
|
@tbrisker OK, I've got a test in place now to ensure this doesn't break again in the future. 😄 |
|
@tbrisker Just following up with this one as well. Tests are in place now, so it's ready for review. Thanks! |
lib/audited/auditor.rb
Outdated
| then | ||
| false | ||
| else | ||
| true |
There was a problem hiding this comment.
This is quite a confusing check, returning the opposite result to the condition - perhaps you can just reuse the comment_required_state? function and add the additional condition instead of flipping the entire logic around?
There was a problem hiding this comment.
@tbrisker I think it was mainly just the name comment_required_state? that I wasn't a fan of. To me it seems to indicate whether or not comment_required is enabled, but what it's really doing is adding additional checks for whether or not audit_comment should be validated for presence when comment_required is enabled. I felt that eligible_for_comment_validation? was clearer about what it's actually checking.
And my though process for flipping the logic around a bit was just so that the conditions would fail earlier – meaning that if auditing isn't enabled, we can immediately return false instead of having to test any more conditions. But I can definitely understand how it's a bit confusing. And now that I think about it, my logic was complete non-sense anyways – if !auditing_enabled || and auditing_enabled && will both return false immediately if auditing isn't enabled. So that was just a big derp on my part. 😬
I'll get this fixed today.
|
@tbrisker OK, should be better now. 😄 |
This is a service we no longer use.
I noticed the 4.7.1 changes were missing from the Changelog.md on master. This PR adds them back.
jeffdill2
force-pushed
the
re-fix-honoring-of-except-with-comment-required
branch
from
6599f04
to
34ee9a9
Compare
|
@tbrisker merge conflict resolved now. |
…oring-of-except-with-comment-required
…hub.com:jeffdill2/audited into re-fix-honoring-of-except-with-comment-required
|
@tbrisker I've got |
This was originally fixed here: #419
But it was inadvertently broken when this was merged in: #420