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 href on create #15005

Merged
merged 3 commits into from
Jun 9, 2017
Merged

Return href on create #15005

merged 3 commits into from
Jun 9, 2017

Conversation

jntullo
Copy link

@jntullo jntullo commented May 4, 2017

The {:add_href => true} is not being sent to normalize_hash as it is looping through the results on create. As a result, the href was not being added to the resources. It does not appear that :add_href is actually serving any purpose.

Removing the condition on :add_href appropriately adds the href to the resources and maintains other functionality.

@miq-bot add_label api, enhancement
@miq-bot assign @abellotti

Pivotal ticket

@abellotti
Copy link
Member

probably some type/collection check missing, href should have reflect tags here.

POST /api/tags
{
  "name" : "hi",
  "description" : "Hello",
  "category" : { "id" : 40 }
}
{
  "results": [
    {
      "href": "http://localhost:3000/api/results/164",
      "id": 164,
      "name": "/managed/department/hi"
    }
  ]
}

@abellotti
Copy link
Member

probably also need tests checking href on creating subcollections, and simply fetching subcollections via expand and these are returned with proper href. tested ok by hand, but should be in rspecs if not there. Thanks.

Copy link
Member

@abellotti abellotti left a comment

Choose a reason for hiding this comment

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

Hi @jntullo, minor changes below. However, I'm still getting results with creating tags, should be getting /api/tags/# href, can you look into this ? Thanks.

{
  "name" : "hi",
  "description" : "Hello",
  "category" : { "id" : 40 }
}

{
  "results": [
    {
      "href": "http://localhost:3000/api/results/165",
      "id": 165,
      "name": "/managed/department/hi"
    }
  ]
}

@@ -128,8 +128,8 @@

expected = {
"results" => a_collection_containing_exactly(
a_hash_including("name" => "foo", "description" => "bar"),
a_hash_including("name" => "baz", "description" => "qux")
a_hash_including("name" => "foo", "description" => "bar", "href" => a_string_including("/api/results")),
Copy link
Member

Choose a reason for hiding this comment

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

would prefer using results_url instead of "/api/results", same below.

tag_category = Category.find(tag.category.id)
expect(tag_category).to eq(category)

expect(result["href"]).to include("/api/results/#{tag.id}")
Copy link
Member

Choose a reason for hiding this comment

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

using results_url(tag.id) instead of "/api/results/#{tag.id}"

@@ -43,10 +43,15 @@
it "can create a quota from a tenant" do
api_basic_authorize action_identifier(:quotas, :create, :subcollection_actions, :post)

expected = {
'results' => [
a_hash_including('href' => a_string_including('api/results/'))
Copy link
Member

Choose a reason for hiding this comment

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

results_url

@miq-bot
Copy link
Member

miq-bot commented May 22, 2017

Checked commits jntullo/manageiq@a2caa5a~...b0fec4e with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
5 files checked, 0 offenses detected
Everything looks fine. 🏆

@chessbyte
Copy link
Member

@abellotti bump

@abellotti
Copy link
Member

yeay, happy tags href returned.

Thanks @jntullo for this enhancement 😍

@abellotti abellotti merged commit 330bbd1 into ManageIQ:master Jun 9, 2017
@abellotti abellotti added this to the Sprint 63 Ending Jun 19, 2017 milestone Jun 9, 2017
@jntullo jntullo deleted the enhancement/return_href_on_create branch November 28, 2017 19:42
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.

4 participants