add flavors create, delete to api #14

alexander-demicev · 2017-08-10T09:49:00Z

Add flavors actions(create,delete) to api

jntullo

Hi @alexander-demichev, can you also add some specs to this? You will also need to update the config/api.yml. Thanks!

jntullo · 2017-08-10T13:11:03Z

app/controllers/api/flavors_controller.rb

+ end
+
+ def validate_flavor_attrs(data)
+ data.dup


not sure this method is necessary if no validation is being done. If you aren't manipulating the data, you shouldn't need to dup it either

jntullo · 2017-08-10T13:13:41Z

app/controllers/api/flavors_controller.rb

+
+ def delete_resource(type, id, _data = {})
+ flavor = resource_search(id, type, collection_class(:flavors))
+ raise "Delete not supported for #{flavor_ident(flavor)}" unless flavor.respond_to?(:delete_flavor_queue)


is this supposed to be delete_in_provider_queue? it's checking a different method than is used below

@alexander-demichev I may be incorrect, but is this method delete_flavor_queue the correct method? I don't see any flavor descendants responding to it

@jntullo I made comment with link to flavors actions PR.

Why don't all flavors respond to this? How does the user know which ones can be deleted? Is it possible that flavors could utilize the supports_feature? feature?

All flavors support delete. I should delete this line of code.

alexander-demicev · 2017-08-12T11:50:47Z

Thank you for review, @jntullo. I added some specs.

alexander-demicev · 2017-08-15T12:19:28Z

Flavors actions are implemented here ManageIQ/manageiq#15552

alexander-demicev · 2017-09-06T14:30:42Z

@jntullo Hi, can you review it again?

imtayadeway · 2017-09-07T17:07:07Z

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

alexander-demicev · 2017-09-11T14:59:37Z

@imtayadeway Hi, one of the tests is failing, can you take a look at it? Thanks. :-)

jntullo · 2017-09-11T15:41:10Z

spec/requests/flavors_spec.rb

+
+ api_basic_authorize
+
+ run_delete api_flavors_url(nil, flavor)


I believe you want api_flavor_url(nil, flavor) here

@jntullo Silly mistake :) Thank you, now it`s ok!

aufi · 2017-09-20T14:55:02Z

Looks good to me, looking forward to see this merged :-)

abellotti · 2017-09-20T15:12:38Z

Maybe add 2 tests for delete single flavor (POST /api/flavors/:id - action edit) and do a bulk delete (POST /api/flavors - action edit).

Otherwise looks good,

@imtayadeway and @jntullo your 👀 when you get a chance. Thanks!!

jntullo

LGTM! ✨ Minor comment about removing compressed_id and just passing in the flavor object

jntullo · 2017-09-20T15:28:22Z

spec/requests/flavors_spec.rb

+ 'count' => 1,
+ 'subcount' => 1,
+ 'name' => 'flavors',
+ 'resources' => [hash_including('href' => a_string_matching(api_flavors_url(nil, flavor.compressed_id)))]


I wouldn't use compressed_id here, and instead just pass in flavor

alexander-demicev · 2017-09-21T18:34:51Z

@jntullo @imtayadeway Hi, can you take a look, I edited tests and during CI it's throwing 'NoMethodError', but it works if I run test on my machine.

jntullo · 2017-09-21T18:53:05Z

hi @alexander-demichev, if you rebase and update to use get instead of run_get, etc, you should be good to go 👍

alexander-demicev · 2017-09-27T06:06:04Z

Added some tests :) @imtayadeway @jntullo

jntullo · 2017-09-27T11:52:42Z

app/controllers/api/flavors_controller.rb

@@ -1,4 +1,24 @@
 module Api
 class FlavorsController < BaseController
+ def create_resource(_type, _id, data)
+ task_id = Flavor.create_flavor_queue(User.current_user.id, EmsCloud.find(data['ems']['id']), data)


Rather than EmsCloud.find here, I think you may want to use resource_search(:id, :providers, collection_class(:providers). Also, what do you think about supporting both id and href? Something along the lines of
{ "provider": { "href" : "/api/providers/:id" } or { "provider": { "id": ":id" } }. This example may help

jntullo · 2017-10-06T11:46:21Z

app/controllers/api/subcollections/flavors.rb

+ action_result(false, err.to_s)
+ end
+
+ def delete_resource_snapshots(_parent, type, id, _data)


I believe this should be flavors_delete_resource

alexander-demicev · 2017-10-09T09:02:03Z

@jntullo @imtayadeway Can I ask you for another round of review? 😄

jntullo · 2017-10-09T12:11:25Z

config/api.yml

@@ -861,20 +861,30 @@
 :description: Flavors
 :identifier: flavor
 :options:
- - :collection
- :verbs: *gp
+ - :subcollection


I didn't realize the collection already exists, I am assuming we'll still want to keep the collection actions? And just add in the subcollection actions?

👍 Yeah I think if it already exists we can preserve the ability to retrieve flavors using only their id

imtayadeway

LGTM, but as @jntullo suggested I think it would be better to leave the existing routes for flavor retrieval intact.

alexander-demicev · 2017-10-17T08:55:39Z

@imtayadeway @jntullo Is it ok now?

jntullo · 2017-10-17T10:53:55Z

config/api.yml

 :klass: Flavor
- :collection_actions:
+ :subcollection_actions:


if you just add the collection actions back in, should be good to go 👍

jntullo · 2017-10-17T10:54:19Z

spec/requests/collections_spec.rb

@@ -114,11 +114,6 @@ def test_collection_bulk_query(collection, collection_url, klass, id = nil)
 test_collection_query(:features, api_features_url, MiqProductFeature)
 end

- it "query Flavors" do


and we'll want these once you add the collections actions back into the api.yml

jntullo

LGTM! Thanks @alexander-demichev! ✨

abellotti · 2017-10-18T23:40:00Z

@alexander-demichev can you add the test for bulk deletes ? i.e. POST /api/providers/:id/flavors action "delete" with 2 flavors ?

Thanks.

miq-bot · 2017-10-19T08:24:44Z

Checked commit alexander-demicev@eb40c9e with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks fine. 🍪

alexander-demicev · 2017-10-19T08:48:41Z

@abellotti Added tests for bulk delete.

abellotti · 2017-10-19T12:58:14Z

Thanks @alexander-demichev for the added test and the API enhancement. 👍

jntullo suggested changes Aug 10, 2017

View reviewed changes

alexander-demicev force-pushed the flavors-api branch from cafe415 to fdb9783 Compare August 12, 2017 11:50

alexander-demicev force-pushed the flavors-api branch 5 times, most recently from 6b98a40 to f408fab Compare August 12, 2017 13:52

alexander-demicev force-pushed the flavors-api branch from f408fab to f98e512 Compare September 6, 2017 14:13

alexander-demicev force-pushed the flavors-api branch 2 times, most recently from 452a2a1 to 25c5983 Compare September 11, 2017 14:57

jntullo reviewed Sep 11, 2017

View reviewed changes

alexander-demicev force-pushed the flavors-api branch 2 times, most recently from 2902981 to febf9f1 Compare September 12, 2017 17:32

abellotti self-assigned this Sep 15, 2017

jntullo reviewed Sep 20, 2017

View reviewed changes

alexander-demicev force-pushed the flavors-api branch 2 times, most recently from 7e54907 to 5f8895a Compare September 21, 2017 18:18

alexander-demicev force-pushed the flavors-api branch from 5f8895a to db4a574 Compare September 21, 2017 21:05

jntullo reviewed Sep 27, 2017

View reviewed changes

alexander-demicev force-pushed the flavors-api branch from db4a574 to 2a6ff2f Compare September 27, 2017 13:50

jntullo reviewed Oct 6, 2017

View reviewed changes

alexander-demicev force-pushed the flavors-api branch 6 times, most recently from 020b5a3 to 8425e11 Compare October 9, 2017 08:16

jntullo reviewed Oct 9, 2017

View reviewed changes

imtayadeway suggested changes Oct 16, 2017

View reviewed changes

alexander-demicev force-pushed the flavors-api branch from 8425e11 to 132082d Compare October 17, 2017 08:55

jntullo reviewed Oct 17, 2017

View reviewed changes

alexander-demicev force-pushed the flavors-api branch 4 times, most recently from b8ada28 to e3bdf15 Compare October 17, 2017 12:43

jntullo approved these changes Oct 17, 2017

View reviewed changes

imtayadeway approved these changes Oct 17, 2017

View reviewed changes

alexander-demicev force-pushed the flavors-api branch from e3bdf15 to 4d73097 Compare October 19, 2017 08:04

add flavors actions to api

eb40c9e

alexander-demicev force-pushed the flavors-api branch from 4d73097 to eb40c9e Compare October 19, 2017 08:19

abellotti merged commit c5cc04a into ManageIQ:master Oct 19, 2017

abellotti added this to the Sprint 72 Ending Oct 30, 2017 milestone Oct 19, 2017

abellotti added the enhancement label Oct 19, 2017

add flavors create, delete to api #14

add flavors create, delete to api #14

Conversation

alexander-demicev commented Aug 10, 2017

jntullo left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-demicev commented Aug 12, 2017 • edited Loading

alexander-demicev commented Aug 15, 2017

alexander-demicev commented Sep 6, 2017

imtayadeway commented Sep 7, 2017

alexander-demicev commented Sep 11, 2017

jntullo Sep 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aufi commented Sep 20, 2017

abellotti commented Sep 20, 2017

jntullo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-demicev commented Sep 21, 2017

jntullo commented Sep 21, 2017

alexander-demicev commented Sep 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-demicev commented Oct 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imtayadeway left a comment

Choose a reason for hiding this comment

alexander-demicev commented Oct 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jntullo left a comment

Choose a reason for hiding this comment

abellotti commented Oct 18, 2017

miq-bot commented Oct 19, 2017

alexander-demicev commented Oct 19, 2017

abellotti commented Oct 19, 2017

jntullo left a comment •

edited

Loading

alexander-demicev commented Aug 12, 2017 •

edited

Loading

jntullo Sep 11, 2017 •

edited

Loading