-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 response with error message on failure destroy action #2920
Add response with error message on failure destroy action #2920
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, thanks! Just left a possible improvement. Let me know what do you think!
|
||
def check_destroy_constraints | ||
return unless name == 'undestroyable' | ||
errors.add :base, 'You cant destroy undestroyable things!' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not crazy about You cant destroy undestroyable things!
I think there is a better way to phrase this.
We definitely need to add an apostrophe: You can't destroy undestroyable things!
Also, we probably want to add this to en.yml
instead of just writing the string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This validation is only for testing as you can see. Adding it to en.yml is weird. But if you do not like the phrase itself, then you can offer absolutely any one to your taste.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bitberries That's fair, let's not worry about putting it in yaml then! I know it's picky, but I'd like to at least see the apostrophe :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jacobherrington done
518765f
to
7973a7a
Compare
@bitberryru failures should go away if you rebase against master, thanks! |
7973a7a
to
3e4e7fb
Compare
Thank you, done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @bitberryru, I have left you just one comment I'd like you to address whenever you can. Thanks a lot! 🎉
context 'js format' do | ||
subject { delete :destroy, params: params, format: 'js' } | ||
|
||
it 'responses with error message' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change responses
for responds
? Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aitbw good catch!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks @bitberryru 👍
3e4e7fb
to
582ab65
Compare
Hello again!
Now I noticed bug in admin when validations prevent destroy.
Your resource controller doesn't handle js format when destroy failures but your js code expects error message
Test and fix in commit! :)