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

Add update_with_only_comment option #327

Merged

Conversation

jestemkarol
Copy link
Contributor

Added on f2de4e6
Currently there is a possibility to update an audit if only audit comment is present:

assert_difference('post.audits.count', 1) { post.update_attributes(audit_comment: 'Only comment') }

In my opinion, updating an audit with only comment given is purposeless, as in fact there are no changes at all

@domcleal
Copy link
Collaborator

Since this seems to be a matter of opinion, would you mind implementing this as a configuration option on Audited to control the behaviour?

@jestemkarol jestemkarol force-pushed the audit-comments-for-update branch from a1f83ea to d5022a2 Compare February 11, 2019 15:52
@jestemkarol jestemkarol changed the title Do not allow update audits if only an audit comment is present Add update_with_only_comment option Feb 11, 2019
@jestemkarol
Copy link
Contributor Author

I've updated PR with an implementation of the configuration option, but it seems like Travis cached bundler and it needs to be rebuilt somehow, I guess.

PS. Sorry for late response 😄

@domcleal
Copy link
Collaborator

PR looks good to me! 👍

I've opened a PR to fix the test failures, it's been a while since they ran. Once we've got that fixed then we can merge back and get this PR in.

PS. Sorry for late response 😄

No worries, I'm sure I've done worse 😁

@domcleal
Copy link
Collaborator

That's now fixed - would you mind doing git merge origin/master (or git rebase origin/master if you prefer) and pushing please? That'll pick up the test fixes I hope.

…if option is set to false and audit_comment is only supplied
@jestemkarol jestemkarol force-pushed the audit-comments-for-update branch from d5022a2 to 83b8f14 Compare February 16, 2019 19:50
@jestemkarol
Copy link
Contributor Author

Yup, all green right now 👌

@domcleal domcleal merged commit 66490bb into collectiveidea:master Feb 18, 2019
@domcleal
Copy link
Collaborator

Thanks @jestemkarol 👍

@Marri
Copy link

Marri commented Aug 7, 2019

@jestemkarol thank you for this! We had hacked around this and our code is much cleaner now.

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