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

Remove dead href parsing paths #13684

Merged

Conversation

imtayadeway
Copy link
Contributor

@imtayadeway imtayadeway commented Jan 27, 2017

If hrefs were supplied in the request body, they should have already
been parsed by BaseController#update_multiple_collections and given to
these methods as the id for the resource they represent. So these parts
of the code should been unreachable, and can be replaced by an empty
hash in the event that all other attempts to parse the resource at hand
have failed.

Follow up to #13671

@abellotti no rush on this, just wanted to make this PR so I don't forget about it.

@miq-bot add-label api, technical debt
@miq-bot assign @abellotti

/cc @jntullo

✂️ ✂️ ✂️

If hrefs were supplied in the request body, they should have already
been parsed by `BaseController#update_multiple_collections` and given to
these methods as the id for the resource they represent. So these parts
of the code should been unreachable, and can be replaced by an empty
hash in the event that all other attempts to parse the resource at hand
have failed.

Follow up to ManageIQ#13671
@miq-bot
Copy link
Member

miq-bot commented Jan 27, 2017

Checked commit imtayadeway@207fd39 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks good. ⭐

@abellotti
Copy link
Member

I think this is fine, manually tested with tags and profiles sub-collections.

Can you verify we have good test coverage for both these sub-collections on both assign and unassign via href's ? If not extend as needed. Thanks.

@imtayadeway
Copy link
Contributor Author

@abellotti we should be good WRT coverage - there are some holes in areas like what happens if assigning/unassigning a policy fails, etc.., but all things that are post-parsing

@abellotti
Copy link
Member

Thanks @imtayadeway for checking. 🎵

@abellotti abellotti merged commit 5d51b97 into ManageIQ:master Feb 16, 2017
@abellotti abellotti added this to the Sprint 55 Ending Feb 27, 2017 milestone Feb 16, 2017
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