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

Return property method actions on Generic Object subresources #247

Merged
merged 3 commits into from
Dec 20, 2017

Conversation

jntullo
Copy link

@jntullo jntullo commented Dec 12, 2017

Currently when retrieving a generic object subresource, ie:

/api/generic_object_definitions/:c_id/generic_objects/:id

The custom actions / property method actions are not being returned. This change moves code from the generic objects controller into the shared generic objects mixin so that the actions are returned as they should be.

This also fixes another issue where the href being returned for custom actions specifies the subresource href, ie POST /api/generic_object_definitions/:c_id/generic_objects/:id. This is incorrect, because when attempting to call the custom method with this href, it results in:

{"error":{"kind":"bad_request","message":"No actions are supported for generic_objects resource","klass":"Api::BadRequestError"}}

when attempting to call this action. Generic Objects were not the only resource affected by this problem.

@miq-bot add_label blocker, bug, gaprindashvili/yes
Resolves https://bugzilla.redhat.com/show_bug.cgi?id=1518803

@jntullo
Copy link
Author

jntullo commented Dec 12, 2017

@miq-bot assign @imtayadeway

@@ -352,7 +352,7 @@ def add_actions(json, aspecs, type)
def expand_resource_custom_actions(resource, json, type, physical_attrs)
return unless render_actions(physical_attrs) && collection_config.custom_actions?(type)

href = json.attributes!["href"]
href = @req.subcollection.present? ? collection_href(@req.subcollection, resource.id) : json.attributes!["href"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/cc @imtayadeway I think collection_href is gone with 866c813

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this important? What's written there is wanting to force accessing the custom action urls in the top level collection when we just went the opposite direction in that we've 'fixed' things to prefer being nested if at all possible. So I'm wondering from a business perspective if that can/should be nested and I'll just change that here.

Jillian Tullo and others added 3 commits December 20, 2017 13:26
…sub collection by moving more methods into the shared generic objects mixin

Resolves https://bugzilla.redhat.com/show_bug.cgi?id=1518803
…tion href should be returned because custom actions are not supported on sub resources.
To fix hrefs in base controller abstraction land that doesn't have
concrete controllers/serializers for exceptions, we've 'normalized'
hrefs the best we could by deterministically giving an href based on the
request that prefers to be nested when possible - but are now screwed
because it's actually determined by what you can and can't do with
nested collections in the interface.

So, this does the same sort of hacky override that was here, just
without the misleading helper that was present before by constructing
our own URL deliberately.
Copy link
Contributor

@imtayadeway imtayadeway left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, though we /really/ need to revisit this whole href generation thing in 2018 😉

@chrisarcand
Copy link
Member

we /really/ need to revisit this whole href generation thing in 2018

🙃

@abellotti
Copy link
Member

LGTM!! 👍 will merge when 🍏

@abellotti abellotti added this to the Sprint 76 Ending Jan 1, 2018 milestone Dec 20, 2017
@miq-bot
Copy link
Member

miq-bot commented Dec 20, 2017

Checked commits jntullo/manageiq-api@ca8449b~...eaec60d with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
4 files checked, 0 offenses detected
Everything looks fine. 🏆

@abellotti abellotti merged commit 78fc3b6 into ManageIQ:master Dec 20, 2017
simaishi pushed a commit that referenced this pull request Jan 3, 2018
Return property method actions on Generic Object subresources
(cherry picked from commit 78fc3b6)

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1530714
@simaishi
Copy link
Contributor

simaishi commented Jan 3, 2018

Gaprindashvili backport details:

$ git log -1
commit a5cc3452ad42ac74fba09d473fd2f507d40e6475
Author: Alberto Bellotti <abellotti@users.noreply.github.com>
Date:   Wed Dec 20 15:30:34 2017 -0500

    Merge pull request #247 from jntullo/bz_1518803
    
    Return property method actions on Generic Object subresources
    (cherry picked from commit 78fc3b644f2bf70a28ce5c394629001bdea47e0f)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1530714

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.

6 participants