Added support to delete Generic Object Definitions using REST API DELETE #2262

AparnaKarve · 2017-09-28T23:00:54Z

The C-R-U for Generic Object Definitions was implemented in #2137.

This PR implements the D (Delete) using REST API.

https://www.pivotaltracker.com/n/projects/1613907/stories/147627789

to block the Delete operation in the `show` screen if there are instances associated with a record

The observable in the toolbar listens for `delete` and then deletes a single record (i.e. applicable from the `show` screen only) using API

Only records with 0 instances would be deleted For records with > 0 instances a warning message would be displayed indicating why they could not be deleted

post a delete operation

AparnaKarve · 2017-09-29T00:04:57Z

@martinpovolny Please take a look at the codeclimate report here.

IMO, the Similar code found in 1 other location errors here look completely absurd.
Plus not to mention the fact that despite the clean and well-structured code in the JS, codeclimate has decided to grade it a C -- which is not reasonable at all.
Now hypothetically, if I did not have the other file (physical-server-toolbar.js /cloud_volume_form_controller.js ) in the picture, the grade would have been an A, which makes the whole CC grading extremely questionable.

We should definitely disable these errors by tweaking the mass value in CC settings if we can.

Are you able to understand how CC came up with those mass value numbers -
(mass = 112) and (mass = 41)?
(I'm finding it hard to interpret those values)

/cc @dclarizio

gmcculloug · 2017-09-29T15:57:58Z

@lfu Please review

gmcculloug

Working as expected for me. 👍

lfu · 2017-09-29T17:59:59Z

LGTM 👍

martinpovolny · 2017-10-01T15:42:46Z

app/helpers/application_helper/button/generic_object_definition_delete_button.rb

@@ -0,0 +1,17 @@
+class ApplicationHelper::Button::GenericObjectDefinitionDeleteButton < ApplicationHelper::Button::Basic
+  def visible?


def visible?
@display != 'generic_objects'
end

Done in 446f7c4

martinpovolny · 2017-10-01T15:43:17Z

app/helpers/application_helper/button/generic_object_definition_delete_button.rb

+  end
+
+  def disabled?
+    if @record.generic_objects.count > 0


def disabled?
@record.generic_objects.count.zero?
end

Done in 446f7c4

martinpovolny · 2017-10-01T15:43:57Z

app/views/generic_object_definition/show_list.html.haml

+  %generic-object-definition-toolbar#generic_object_definition_show_list_form
+
+:javascript
+  miq_bootstrap('#generic_object_definition_show_list_form');


@himdel : we discussed this as an anti-pattern. Do we have a replacement yet?

Why would miq_bootstrap on the angular component be an anti-pattern?
@himdel Pray tell me how.

This is an anti-pattern simply because a GTL show-list should not have any other components .. what is this generic-object-definition-toolbar actually for? Should it not be an actual toolbar?

Also, it's a component without a template - sounds like something is very wrong there :).

Aah.. and it is an invisible toolbar because you're actuall trying to do things right and call the API for all the toolbar actions, am I right? :)

Sounds maybe we could just move this outside the DOM, since it has no effect on any actual html...

And we can actually listen for Rx messages even outside Angular code.. so maybe this should be a simple file that just starts listening for messages wherever - it means we would have the listeners on every screen but... so what :).

We just need the messages to be discoverable - but genericObjectDefinitionToolbarController pretty much solves that.

I mean.. instead of using a component (and having a non-standard show_list), this could just be a js file which just starts the listeners directly, without anything angulary.

GTL show-list should not have any other components

Seems very restrictive for no real reason.
Why wouldn't be more components allowed?

generic-object-definition-toolbar

generic-object-definition-toolbar is the controller for the toolbar commands.
BTW, there are no groundbreaking changes here.
Something similar has already been implemented for Physical Servers - #1380

Also, it's a component without a template - sounds like something is very wrong there

I have one simple answer to that - Delete operation does not need a form/template. A flash message is good enough
Nothing went wrong ;)

I mean.. instead of using a component (and having a non-standard show_list), this could just be a js file which just starts the listeners directly, without anything angulary.

(Looks like I missed this message since I did not refresh the page as I was typing mine)

It cannot be non-angulary because I'm using the API service, which is available only if you use angular.

Also there is some advantage to using the the controller/component approach that I see.
All messages that are triggered by the Generic Object Definition toolbar would only be directed to the observable in the generic-object-definition-toolbar component.
It's like I'm channeling the messages in a particular area of interest and not going in the common application-wide listeners pool.

@AparnaKarve : You are correct, there's no other way to do this right now. It is 100% ok for this PR.

The comment was ment more as a note that this is something we have to deal with and ping to @himdel to look for a solution ;-)

I am looking at all the "extra" content in the "show" partials because I would like to delete all the "show" partials on our way towards more declarative and client-side UI.

@martinpovolny Thanks for that clarification.

himdel · 2017-10-02T14:48:59Z

app/views/generic_object_definition/show.html.haml

+  %generic-object-definition-toolbar#generic_object_definition_show_form{'generic-object-definition-id' => @record.id,
+                                                                         'redirect-url'                 => "/#{controller_name}/show_list",}
+:javascript
+  miq_bootstrap('#generic_object_definition_show_form');


And here.. I'll say fine for this PR :)...

So just FYI Aparna..

we're trying to remove all those specific things from these show partials, so that miq_bootstrap will have to go as well..
but the idea is that instead, the controllers would have a method which gets called from all the show partials, and can create that miq_bootstrap in a declarative way, maybe ideally with the option to create that component element as well.

But that's really just an idea for now, without any specifics worked out.

It's in the TODO list that is growing...

martinpovolny · 2017-10-02T17:39:19Z

@AparnaKarve : will you, please, replace the 2 methods with corresponding one-liners so that we can merge this?

miq-bot · 2017-10-02T18:13:55Z

Checked commits AparnaKarve/manageiq-ui-classic@7fd40c9~...446f7c4 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
4 files checked, 0 offenses detected
Everything looks fine. ⭐

AparnaKarve · 2017-10-02T18:21:06Z

@martinpovolny I have simplified the 2 methods with simple one-liners as you suggested. Thanks.

AparnaKarve force-pushed the generic_obj_defn_D branch from cd827d7 to 9d92678 Compare September 28, 2017 23:12

AparnaKarve added 5 commits September 28, 2017 16:22

Create a button class GenericObjectDefinitionDeleteButton

7fd40c9

to block the Delete operation in the `show` screen if there are instances associated with a record

Create a component for Generic Object Definition Toolbar

652932b

The observable in the toolbar listens for `delete` and then deletes a single record (i.e. applicable from the `show` screen only) using API

Add Multiple Delete support in the show_list screen

01daf4e

Only records with 0 instances would be deleted For records with > 0 instances a warning message would be displayed indicating why they could not be deleted

Add Reload capability to reflect the true state of the listview

e3eec94

post a delete operation

Fix Rubocop

a86f34c

AparnaKarve force-pushed the generic_obj_defn_D branch from 9d92678 to a86f34c Compare September 28, 2017 23:24

Fix toobar spec for Generic Object Definition Delete

6afa999

dclarizio added enhancement fine/no generic objects labels Sep 29, 2017

dclarizio requested review from martinpovolny and gmcculloug September 29, 2017 13:50

dclarizio self-assigned this Sep 29, 2017

gmcculloug approved these changes Sep 29, 2017

View reviewed changes

martinpovolny reviewed Oct 1, 2017

View reviewed changes

himdel reviewed Oct 2, 2017

View reviewed changes

PR feedback - simplify the visible? and disabled? checks

446f7c4

martinpovolny merged commit ea7e1c0 into ManageIQ:master Oct 3, 2017

martinpovolny added this to the Sprint 71 Ending Oct 16, 2017 milestone Oct 3, 2017

himdel mentioned this pull request Oct 20, 2017

Add vm security group operations #1997

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support to delete Generic Object Definitions using REST API DELETE #2262

Added support to delete Generic Object Definitions using REST API DELETE #2262

AparnaKarve commented Sep 28, 2017

AparnaKarve commented Sep 29, 2017 •

edited

Loading

gmcculloug commented Sep 29, 2017

gmcculloug left a comment

lfu commented Sep 29, 2017

martinpovolny Oct 1, 2017

AparnaKarve Oct 2, 2017

martinpovolny Oct 1, 2017

AparnaKarve Oct 2, 2017

martinpovolny Oct 1, 2017

AparnaKarve Oct 2, 2017

himdel Oct 2, 2017

himdel Oct 2, 2017

himdel Oct 2, 2017

AparnaKarve Oct 2, 2017

AparnaKarve Oct 2, 2017

martinpovolny Oct 2, 2017

AparnaKarve Oct 2, 2017

himdel Oct 2, 2017

martinpovolny Oct 2, 2017

martinpovolny commented Oct 2, 2017

miq-bot commented Oct 2, 2017

AparnaKarve commented Oct 2, 2017

		@@ -0,0 +1,17 @@
		class ApplicationHelper::Button::GenericObjectDefinitionDeleteButton < ApplicationHelper::Button::Basic
		def visible?

Added support to delete Generic Object Definitions using REST API DELETE #2262

Added support to delete Generic Object Definitions using REST API DELETE #2262

Conversation

AparnaKarve commented Sep 28, 2017

AparnaKarve commented Sep 29, 2017 • edited Loading

gmcculloug commented Sep 29, 2017

gmcculloug left a comment

Choose a reason for hiding this comment

lfu commented Sep 29, 2017

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

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

martinpovolny commented Oct 2, 2017

miq-bot commented Oct 2, 2017

AparnaKarve commented Oct 2, 2017

AparnaKarve commented Sep 29, 2017 •

edited

Loading