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

raise not found error on DELETE #17

Merged
merged 1 commit into from
Sep 8, 2017
Merged

raise not found error on DELETE #17

merged 1 commit into from
Sep 8, 2017

Conversation

jntullo
Copy link

@jntullo jntullo commented Aug 10, 2017

manageiq-api version of ManageIQ/manageiq#15736

DELETE /api/authentications/:id should raise a not found for invalid authentications. It is currently sending back a 204 as if it's a success due to the catching of the exception and returning it as an action result.

This is inconsistent with other areas of the API where DELETE returns a not found for invalid records.

BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1476869

@miq-bot add_label bug
@miq-bot assign @abellotti

@abellotti
Copy link
Member

@jntullo can you take care of the codeclimate issues ?

@jntullo
Copy link
Author

jntullo commented Sep 7, 2017

@abellotti I'm not too sure I agree with codeclimate here. While this code does have some duplication, it's not something I feel should be abstracted

@jntullo
Copy link
Author

jntullo commented Sep 7, 2017

@abellotti also noticed I didn't even touch the lines it's complaining about 😕

@imtayadeway
Copy link
Contributor

Hi! It looks like your PR has been affected by the recently merged #40, and as a result will need to be rebased. First, I apologize for any inconvenience caused. After rebasing, you'll need to update some specs in order for them to pass. In particular you'll need to change any path helpers, applying the following pattern:

vms_url => api_vms_url
vms_url(vm.id) => api_vm_url(nil, vm) 
vms_url(vm.compressed_id) => api_vm_url(nil, vm.compressed_id)
"#{vms_url(vm.id)}/tags" => api_vm_tags_url(nil, vm)
"#{vms_url(vm.compressed_id}/tags" => api_vm_tags_url(nil, vm.compressed_id)
"#{vms_url(vm.id)}/tags/#{tag.id}" => api_vm_tag_url(nil, vm, tag)
"#{vms_url(vm.compressed_id)}/tags/#{tag.compressed_id}" => api_vm_tag_url(nil, vm.compressed_id, tag.compressed_id)

If you run into any issues, please ping me and I will try to help you out.

Many thanks!
Tim

@miq-bot
Copy link
Member

miq-bot commented Sep 8, 2017

Checked commit jntullo@46aa4e2 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. 🏆

@abellotti
Copy link
Member

Thanks @jntullo for updating to use the api rails url helper. will merge when 🍏

@abellotti abellotti added this to the Sprint 69 Ending Sep 18, 2017 milestone Sep 8, 2017
@abellotti
Copy link
Member

LGTM!!

@abellotti abellotti merged commit 286a681 into ManageIQ:master Sep 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants