Tasks: delete support #220

himdel · 2017-11-21T01:47:31Z

This adds deletion for MiqTask, both via DELETE and via POST {action:"delete"}, and bulk deletion (POST {action:"delete", resources: [...]}).

Closes #203

(Needed for ManageIQ/manageiq-ui-classic#2708.)

Is it OK to reuse existing identifiers (miq_task_delete) or is that frowned upon? :)

jntullo · 2017-11-21T12:57:47Z

Hi @himdel can you add some specs for this? Is there any reason you didn't add delete on the collection (for bulk delete)? and you can reuse existing identifiers :)

himdel · 2017-11-21T17:05:05Z

@jntullo Sure, will do :) (wasn't quite sure if we wanted to spec methods that aren't technically there, but definitely can't hurt :))

As for bulk delete.. I just didn't think of it :).. but there I have questions.. (I'm not seeing any docs on custom collection actions, sorry :).)

Collection actions are named foo_collection, similar to foo_resource or is there a different convention? I'm not seeing any such delete_collection in the generic code, should I add it there, or should this be strictly for task? Or .. am I misunderstanding and does it magically know to call the resource action on all the resources mentioned?

himdel · 2017-11-21T17:20:28Z

... Never mind :).

It's really that easy, it does seem to call the resource action for each of the resources .. so while I still don't know how to custom collection action, I don't seem to need it right now :).

Addded, on to the specs now :)

himdel · 2017-11-21T18:33:15Z

@jntullo Specs added, WDYT? :)

jntullo

Looking good @himdel !! Just a few minor things/nits 😃

jntullo · 2017-11-21T18:37:44Z

spec/requests/tasks_spec.rb

+
+    delete(url)
+
+    expect(response).to have_http_status(204)


For the http status, could you use :forbidden rather than 204? will help to keep our specs consistent

Probably not :forbidden ... I guess something like :empty?

Turns out :no_content, according to http://billpatrianakos.me/blog/2013/10/13/list-of-rails-status-code-symbols/

That's why I prefer the numerical values, no confusion about what it could be :).

jntullo · 2017-11-21T18:40:39Z

spec/requests/tasks_spec.rb

+
+    post(url, :params => data)
+
+    expect(response).to have_http_status(200)


Similarly, you could use :ok instead of 200

jntullo · 2017-11-21T18:45:14Z

spec/requests/tasks_spec.rb

+      :resources => [
+        {:href => api_task_url(nil, task.id)},
+        {:href => api_task_url(nil, task2.id)},
+      ],


nit, but no , here

fixed :) Funnily enough, in ui, trailing commas are strongly encouradged :).

jntullo · 2017-11-21T18:47:49Z

spec/requests/tasks_spec.rb

+    post(url, :params => data)
+
+    expect(response).to have_http_status(200)
+    expect_deleted(task, task2)


Generally, the API is concerned with the responses rather than validating the objects, something like this

Well, I don't care about the response at all, only that it actually deleted the thing :).

Should I add something like that then?

But.. more and more this looks like I'm testing your generic functions for one specific case, which is identical to all the other specific cases.

Would you be interested in specs for the generic resource_delete instead?

Or at least a set of helpers so that we can write the tests once, and then only give that the things that should be different?

That task_spec should not really have to be more code than something like..

describe 'TasksController' do let(:task) { FactoryGirl.create(:miq_task, :state => MiqTask::STATE_FINISHED) } let(:task2) { FactoryGirl.create(:miq_task, :state => MiqTask::STATE_FINISHED) } test_generic_delete(:methods => [:delete, :post, :bulk], :resources => [:task, :task2]) end

jntullo · 2017-11-21T18:49:54Z

spec/requests/tasks_spec.rb

+  it 'bulk deletes' do
+    api_basic_authorize collection_action_identifier(:tasks, :delete)
+
+    url = api_tasks_url


are you only using this variable in one place? May not be necessary to assign it

jntullo · 2017-11-21T18:50:59Z

spec/requests/tasks_spec.rb

+  it 'deletes on POST' do
+    api_basic_authorize resource_action_identifier(:tasks, :delete)
+
+    url = api_task_url(nil, task.id)


You may not need to assign this to a variable if you're only calling it once, same as above and below

Sure I don't.

But is it not clearer to write..

url = function call with not-so clear params data = create the structure post(url, :params => data)

than to have it all in one blob, like

post(api_task_url(nil, task.id), :params => { :action => 'delete', :resources => [ {:href => api_task_url(nil, task.id)}, {:href => api_task_url(nil, task2.id)}, ], })

?

But of course your repo, your rules, just tell me you're sure you want it that way :).

My two penneth: I think "clearer" is always going to be a bit subjective in these cases. I personally find your counter example clearer.

both via DELETE and via POST {action:"delete"}

``` API.post('/api/tasks', { action: "delete", resources: [ { href: 'http://localhost:3000/api/tasks/10000000054726' }, { href: 'http://localhost:3000/api/tasks/10000000054727' }, ], }); ```

similar to `collection_action_identifier` except for resource actions

himdel · 2017-11-29T15:43:04Z

@jntullo Can you check again please?

I've added the spec for status and message in response, and removed the extra vars.

himdel · 2017-11-29T15:43:38Z

@miq-bot add_label gaprindashvili/yes

apparently miq-bot complains about readable hashes - moving back to vars

himdel · 2017-11-29T15:50:21Z

.. and I can't fix both miq-bot and not using vars for data while still being readable .. undoing that change.

And not touching this again :D.

abellotti · 2017-12-05T18:01:23Z

spec/requests/tasks_spec.rb

+      :action    => 'delete',
+      :resources => [
+        {:href => api_task_url(nil, task.id)},
+        {:href => api_task_url(nil, task2.id)}


Hi @himdel minor, but on all the api_task_url calls, prefer just the object being specified i.e.

api_task_url(nil, task)

abellotti · 2017-12-05T18:02:00Z

LGTM!! minor nit on the rails url helper usage in specs.

himdel · 2017-12-05T18:04:02Z

Thanks @abellotti, updated :)

abellotti · 2017-12-05T18:09:21Z

Thanks @himdel for the update and the API enhancement !! 👍 will merge when 🍏

miq-bot · 2017-12-05T18:10:45Z

Checked commits https://github.com/himdel/manageiq-api/compare/8cc67b298383313f5616d8e518113343a44a8585~...74b36b6d10a7c803f80108e893b4fa05c24455ab with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 👍

Tasks: delete support (cherry picked from commit 85f3153)

simaishi · 2017-12-11T17:31:39Z

Gaprindashvili backport details:

$ git log -1
commit 80834992e9cfb29b48b8cf37febf64d8ecdf2398
Author: Alberto Bellotti <abellotti@users.noreply.github.com>
Date:   Tue Dec 5 13:17:36 2017 -0500

    Merge pull request #220 from himdel/tasks-delete
    
    Tasks: delete support
    (cherry picked from commit 85f315312fff5ba1fb41bbc71fb833766272d49e)

jntullo reviewed Nov 21, 2017

View reviewed changes

himdel added 4 commits November 29, 2017 15:41

Tasks: delete support

8cc67b2

both via DELETE and via POST {action:"delete"}

Tasks - add bulk delete

c1f7bd8

``` API.post('/api/tasks', { action: "delete", resources: [ { href: 'http://localhost:3000/api/tasks/10000000054726' }, { href: 'http://localhost:3000/api/tasks/10000000054727' }, ], }); ```

helpers - add resource_action_identifier

461f4ff

similar to `collection_action_identifier` except for resource actions

Tasks deletion - specs

e602f5d

miq-bot added the gaprindashvili/yes label Nov 29, 2017

tasks_spec - shut up miq-bot

fa77ac0

apparently miq-bot complains about readable hashes - moving back to vars

abellotti self-assigned this Dec 5, 2017

abellotti added this to the Sprint 75 Ending Dec 11, 2017 milestone Dec 5, 2017

abellotti added the enhancement label Dec 5, 2017

abellotti approved these changes Dec 5, 2017

View reviewed changes

abellotti reviewed Dec 5, 2017

View reviewed changes

api_task_url - pass in whole tasks, not just id

74b36b6

abellotti merged commit 85f3153 into ManageIQ:master Dec 5, 2017

himdel deleted the tasks-delete branch December 5, 2017 18:32

simaishi pushed a commit that referenced this pull request Dec 11, 2017

Merge pull request #220 from himdel/tasks-delete

8083499

Tasks: delete support (cherry picked from commit 85f3153)

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Dec 11, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tasks: delete support #220

Tasks: delete support #220

himdel commented Nov 21, 2017 •

edited

Loading

jntullo commented Nov 21, 2017

himdel commented Nov 21, 2017

himdel commented Nov 21, 2017

himdel commented Nov 21, 2017

jntullo left a comment

jntullo Nov 21, 2017

himdel Nov 22, 2017 •

edited

Loading

himdel Nov 22, 2017

himdel Nov 22, 2017

jntullo Nov 21, 2017

jntullo Nov 21, 2017

himdel Nov 22, 2017

jntullo Nov 21, 2017

himdel Nov 22, 2017

himdel Nov 22, 2017 •

edited

Loading

jntullo Nov 21, 2017

jntullo Nov 21, 2017

himdel Nov 22, 2017 •

edited

Loading

imtayadeway Nov 22, 2017

himdel commented Nov 29, 2017

himdel commented Nov 29, 2017

himdel commented Nov 29, 2017

abellotti Dec 5, 2017

abellotti commented Dec 5, 2017

himdel commented Dec 5, 2017

abellotti commented Dec 5, 2017

miq-bot commented Dec 5, 2017

simaishi commented Dec 11, 2017


		post(url, :params => data)

		expect(response).to have_http_status(200)

Tasks: delete support #220

Tasks: delete support #220

Conversation

himdel commented Nov 21, 2017 • edited Loading

jntullo commented Nov 21, 2017

himdel commented Nov 21, 2017

himdel commented Nov 21, 2017

himdel commented Nov 21, 2017

jntullo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himdel Nov 22, 2017 • 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

Choose a reason for hiding this comment

himdel Nov 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himdel Nov 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himdel commented Nov 29, 2017

himdel commented Nov 29, 2017

himdel commented Nov 29, 2017

Choose a reason for hiding this comment

abellotti commented Dec 5, 2017

himdel commented Dec 5, 2017

abellotti commented Dec 5, 2017

miq-bot commented Dec 5, 2017

simaishi commented Dec 11, 2017

himdel commented Nov 21, 2017 •

edited

Loading

himdel Nov 22, 2017 •

edited

Loading

himdel Nov 22, 2017 •

edited

Loading

himdel Nov 22, 2017 •

edited

Loading