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

[Api] Improve error handling on destroy action #14098

Merged

Conversation

isimluk
Copy link
Member

@isimluk isimluk commented Feb 28, 2017

Minor improvements of the generic resource_delete action

  • get RBAC check closer to the actual action
  • avoid extra select
  • publish errors from validations/callbacks to user

@miq-bot add_label api, enhancement, euwe/yes
@miq-bot assign @abellotti

That way nobody runs the delete without the rbac check.
ActiveRecord.destroy selects the resource to memory to run all the
callbacks and validations anyway.
Sometimes validations disallow user to delete a resource. Let's make
him/her conscious about the reasons.
@miq-bot
Copy link
Member

miq-bot commented Feb 28, 2017

Checked commits isimluk/manageiq@18a0cd0~...d3d5dcd with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks good. 🍰

@abellotti
Copy link
Member

Hi @isimluk I think this is good. Why the euwe/yes flag though ?

@isimluk
Copy link
Member Author

isimluk commented Mar 2, 2017

I have noticed this issue when working on https://bugzilla.redhat.com/show_bug.cgi?id=1414881 I expect the guy behind the bug may happen to notice as well, so I put here euwe/yes.

It can be euwe/no as well.

@abellotti
Copy link
Member

saw your fix for that on #14097, nice !!

yes, would prefer this PR here separate from the Bug, and thus euwe/no. Thanks.

@isimluk
Copy link
Member Author

isimluk commented Mar 2, 2017

Alright. 🐎
@miq-bot remove_label euwe/yes
@miq-bot add_label euwe/no

@miq-bot miq-bot added euwe/no and removed euwe/yes labels Mar 2, 2017
@abellotti
Copy link
Member

Thanks @isimluk for this enhancement. LGTM!!

@abellotti abellotti merged commit 7113841 into ManageIQ:master Mar 2, 2017
@abellotti abellotti added this to the Sprint 56 Ending Mar 13, 2017 milestone Mar 2, 2017
@isimluk isimluk deleted the api-improve-erro-handling-on-destroy branch March 3, 2017 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants