Added Ability to Delete Provider

[Martyn Taylor]

This patch adds ability for deleting providers.

As discussed with sseago, this patch leaves any images removes all associated objects, but leaves any built images in limbo state.

Currently the provider can only be deleted, provided its associated cloud accounts contain no instances.  Since removing instances is not yet implemented, deleting provider is only managable when no associated instances have been started.

This patch will need to be readressed once multiple providers are supported, in order to address the issues with limbo images.

[PATCH] BZ#641401 Added Ability to Delete Provider

[Martyn Taylor]

From: Martyn Taylor

---
 src/app/controllers/provider_controller.rb      |   27 +++++++++------
 src/app/models/provider.rb                      |   16 ++++++++-
 src/app/views/provider/_providers.haml          |    2 +
 src/features/provider.feature                   |   19 ++++++++++-
 src/features/step_definitions/provider_steps.rb |   41 +++++++++++++++++++++-
 src/features/support/custom.rb                  |   12 +++---
 src/spec/factories/replicated_image.rb          |    5 +++
 7 files changed, 101 insertions(+), 21 deletions(-)
 create mode 100644 src/spec/factories/replicated_image.rb

diff --git a/src/app/controllers/provider_controller.rb b/src/app/controllers/provider_controller.rb
index da5c9bb..8d6ff1f 100644
--- a/src/app/controllers/provider_controller.rb
+++ b/src/app/controllers/provider_controller.rb
@@ -99,19 +99,24 @@ class ProviderController < ApplicationController
   end

def destroy
-    if request.post?
-      @provider = Provider.find(params[:provider][:id])
-      require_privilege(Privilege::PROVIDER_MODIFY, p)
-      if @provider.destroy and @provider.destroyed?
-        redirect_to :action => "index"
-      else
-        flash[:error] = {
-          :summary => "Failed to delete Provider",
-          :failures => @provider.errors.full_messages,
-        }
-        render :action => 'show'
+    if request.post? || request.delete?
+      @provider = Provider.find(params[:id])
+      require_privilege(Privilege::PROVIDER_MODIFY, @provider)
+      if @provider.destroyable?
+        if @provider.destroy and @provider.destroyed?
+          redirect_to :action => "index"
+          flash[:notice] = "Provider Deleted"
+          return
+        end
       end
+
+      flash[:error] = {
+        :summary => "Failed to delete Provider",
+        :failures => @provider.errors.full_messages,
+      }
+      render :action => 'show'
     end
+    render :action => 'show'
   end

def hardware_profiles
diff --git a/src/app/models/provider.rb b/src/app/models/provider.rb
index 6ec3cc3..c4450b6 100644
--- a/src/app/models/provider.rb
+++ b/src/app/models/provider.rb
@@ -38,7 +38,14 @@ class Provider < ActiveRecord::Base
            :include => [:role],
            :order => "permissions.id ASC"

-  before_destroy :destroyable?
+  before_destroy { |provider| ReplicatedImage.destroy_all "provider_id = #{provider.id}" }
+
+  before_destroy { |provider| HardwareProfile.destroy_all "provider_id = #{provider.id}" }
+
+  before_destroy { |provider| Realm.destroy_all "provider_id = #{provider.id}" }
+
+  before_destroy { |provider| CloudAccount.destroy_all "provider_id = #{provider.id}" }
+

# there is a destroy dependency for a cloud accounts association,
   # but a cloud account is silently not destroyed when there is
@@ -126,4 +133,11 @@ class Provider < ActiveRecord::Base
   def valid_framework?
     connect.nil? ? false : true
   end
+
+  def delete_associated_objects
+    replicated_images.each |ri| ri.destoy
+    hardware_profiles.each |hwp| hwp.destroy
+    realms.each |r| r.destroy
+    cloud_accounts.each |ca| ca.destroy
+  end
 end
diff --git a/src/app/views/provider/_providers.haml b/src/app/views/provider/_providers.haml
index f051692..f4eeef9 100644
--- a/src/app/views/provider/_providers.haml
+++ b/src/app/views/provider/_providers.haml
@@ -11,3 +11,5 @@
     = submit_tag t(:edit), :disabled => ('disabled' unless @provider and ['show', 'accounts'].include? controller.action_name)
   - form_tag({:controller => 'provider', :action => 'new'}, {:method => :get , :class => 'buttononly'}) do
     %input{ :type => 'submit', :value => t(:add), :disabled => ('disabled' unless @providers.length == 0) }
+  - form_tag({:controller => 'provider', :action => 'destroy', :id => @provider}, {:method => :post , :class => 'buttononly'}) do
+    = submit_tag 'delete', :disabled => ('disabled' unless @provider and ['show', 'accounts'].include? controller.action_name)
\ No newline at end of file
diff --git a/src/features/provider.feature b/src/features/provider.feature
index c332667..12047bb 100644
--- a/src/features/provider.feature
+++ b/src/features/provider.feature
@@ -85,4 +85,21 @@ Feature: Manage Providers
     And I fill in "cloud_account[password]" with "incorrect_password"
     And I fill in "cloud_account[account_number]" with "12345678"
     And I press "test_account"
-    Then I should see "Test Connection Failed: Invalid Account Details"
\ No newline at end of file
+    Then I should see "Test Connection Failed: Invalid Account Details"
+
+  Scenario: Delete a provider
+    Given I am on the homepage
+    And there is a provider named "provider1"
+    And this provider has 5 replicated images
+    And this provider has 5 hardware profiles
+    And this provider has a realm
+    And this provider has a cloud account
+    When I go to the providers page
+    And I follow "provider1"
+    And I press "delete"
+    Then I should see "Provider Deleted"
+    And there should not exist a provider named "provider1"
+    And there should not be any replicated images
+    And there should not be any hardware profiles
+    And there should not be a cloud account
+    And there should not be a realm
\ No newline at end of file
diff --git a/src/features/step_definitions/provider_steps.rb b/src/features/step_definitions/provider_steps.rb
index e593d43..abc4662 100644
--- a/src/features/step_definitions/provider_steps.rb
+++ b/src/features/step_definitions/provider_steps.rb
@@ -1,7 +1,12 @@
-Given /^there is not a provider named "([^"]*)"$/ do |name|
+Given /^there should not exist a provider named "([^"]*)"$/ do |name|
   Provider.find_by_name(name).should be_nil
 end

+Given /^there is not a provider named "([^"]*)"$/ do |name|
+  provider = Provider.find_by_name(name)
+  if provider then provider.destroy end
+end
+
 Given /^there is a provider named "([^"]*)"$/ do |name|
   @provider = Factory(:mock_provider, :name => name)
 end
@@ -20,9 +25,41 @@ When /^I delete provider$/ do
   click_button "Delete provider"
 end

-
 Given /^there are these providers:$/ do |table|
   table.hashes.each do |hash|
     Factory(:mock_provider, :name => hash['name'])
   end
 end
+
+Given /^this provider has (\d+) replicated images$/ do |number|
+  number.to_i.times { |i| Factory(:replicated_image, :provider => @provider) }
+end
+
+Given /^this provider has (\d+) hardware profiles$/ do |number|
+  number.to_i.times { |i| Factory(:mock_hwp1, :provider => @provider) }
+end
+
+
+Given /^this provider has a realm$/ do
+  Factory(:realm, :provider => @provider)
+end
+
+Given /^this provider has a cloud account$/ do
+  Factory(:mock_cloud_account, :provider => @provider)
+end
+
+Then /^there should not be any replicated images$/ do
+  ReplicatedImage.find(:all, :conditions => { :provider_id => @provider.id} ).size.should == 0
+end
+
+Then /^there should not be any hardware profiles$/ do
+  HardwareProfile.find(:all, :conditions => { :provider_id => @provider.id} ).size.should == 0
+end
+
+Then /^there should not be a cloud account$/ do
+  CloudAccount.find(:all, :conditions => { :provider_id => @provider.id} ).size.should == 0
+end
+
+Then /^there should not be a realm$/ do
+  Realm.find(:all, :conditions => { :provider_id => @provider.id} ).size.should == 0
+end
diff --git a/src/features/support/custom.rb b/src/features/support/custom.rb
index 6e4e1ef..b5440ae 100644
--- a/src/features/support/custom.rb
+++ b/src/features/support/custom.rb
@@ -34,12 +34,12 @@ CloudAccount.class_eval do
   def generate_cloud_account_key
   end

-  def instance_key
-    @key = mock('Key', :null_object => true)
-    @key.stub!(:pem).and_return("PEM")
-    @key.stub!(:id).and_return("1_user")
-    @key   
-  end
+#  def instance_key
+#    @key = mock('Key', :null_object => true)
+#    @key.stub!(:pem).and_return("PEM")
+#    @key.stub!(:id).and_return("1_user")
+#    @key
+#  end
 end

RepositoryManager.class_eval do
diff --git a/src/spec/factories/replicated_image.rb b/src/spec/factories/replicated_image.rb
new file mode 100644
index 0000000..f989380
--- /dev/null
+++ b/src/spec/factories/replicated_image.rb
@@ -0,0 +1,5 @@
+Factory.define :replicated_image do |ri|
+  ri.association :image
+  ri.association :provider
+  ri.sequence(:provider_image_key) { |n| "provider_image_key#(n)" }
+end
\ No newline at end of file

[PATCH] BZ#641401 Added Ability to Delete Provider

Comments below

On Nov 8, 2010, at 2:36 PM, [e-mail] wrote:
> From: Martyn Taylor
>
> src/app/controllers/provider_controller.rb      |   27 +++++++++------
> src/app/models/provider.rb                      |   16 ++++++++-
> src/app/views/provider/_providers.haml          |    2 +
> src/features/provider.feature                   |   19 ++++++++++-
> src/features/step_definitions/provider_steps.rb |   41 +++++++++++++++++++++-
> src/features/support/custom.rb                  |   12 +++---
> src/spec/factories/replicated_image.rb          |    5 +++
> 7 files changed, 101 insertions(+), 21 deletions(-)
> create mode 100644 src/spec/factories/replicated_image.rb
>
> diff --git a/src/app/controllers/provider_controller.rb b/src/app/controllers/provider_controller.rb
> index da5c9bb..8d6ff1f 100644
> --- a/src/app/controllers/provider_controller.rb
> +++ b/src/app/controllers/provider_controller.rb
> @@ -99,19 +99,24 @@ class ProviderController < ApplicationController
>   end
>
> def destroy
> if request.post?
>  @provider = Provider.find(params[:provider][:id])
>  require_privilege(Privilege::PROVIDER_MODIFY, p)
>  if @provider.destroy and @provider.destroyed?
>    redirect_to :action => "index"
>  else
>    flash[:error] = {
>      :summary => "Failed to delete Provider",
>      :failures => @provider.errors.full_messages,
>    }
>    render :action => 'show'
>
> if request.post? || request.delete?
>  @provider = Provider.find(params[:id])
>  require_privilege(Privilege::PROVIDER_MODIFY, @provider)
>  if @provider.destroyable?
>    if @provider.destroy and @provider.destroyed?
>      redirect_to :action => "index"
>      flash[:notice] = "Provider Deleted"
>      return
>    end
> end
>  flash[:error] = {
>    :summary => "Failed to delete Provider",
>    :failures => @provider.errors.full_messages,
>  }
>  render :action => 'show'
>
> end
>
> render :action => 'show'
> end
>
> def hardware_profiles
>
> diff --git a/src/app/models/provider.rb b/src/app/models/provider.rb
> index 6ec3cc3..c4450b6 100644
> --- a/src/app/models/provider.rb
> +++ b/src/app/models/provider.rb
> @@ -38,7 +38,14 @@ class Provider < ActiveRecord::Base
>            :include => [:role],
>            :order => "permissions.id ASC"
> before_destroy :destroyable?
> before_destroy { |provider| ReplicatedImage.destroy_all "provider_id = #{provider.id}" }
> before_destroy { |provider| HardwareProfile.destroy_all "provider_id = #{provider.id}" }
> before_destroy { |provider| Realm.destroy_all "provider_id = #{provider.id}" }
> before_destroy { |provider| CloudAccount.destroy_all "provider_id = #{provider.id}" }

I have tested this and you don't have to do before_destroy with all this code because every has_many
relation has :dependent => :destroy which will do what you do here.
> # there is a destroy dependency for a cloud accounts association,
>   # but a cloud account is silently not destroyed when there is
> @@ -126,4 +133,11 @@ class Provider < ActiveRecord::Base
>   def valid_framework?
>     connect.nil? ? false : true
>   end
> def delete_associated_objects
> replicated_images.each |ri| ri.destoy
> hardware_profiles.each |hwp| hwp.destroy
> realms.each |r| r.destroy
> cloud_accounts.each |ca| ca.destroy
> end

Didn't find any use of this method. Any reason to have it?
> end
> diff --git a/src/app/views/provider/_providers.haml b/src/app/views/provider/_providers.haml
> index f051692..f4eeef9 100644
> --- a/src/app/views/provider/_providers.haml
> +++ b/src/app/views/provider/_providers.haml
> @@ -11,3 +11,5 @@
>     = submit_tag t(:edit), :disabled => ('disabled' unless @provider and ['show', 'accounts'].include? controller.action_name)
> form_tag({:controller => 'provider', :action => 'new'}, {:method => :get , :class => 'buttononly'}) do
> %input{ :type => 'submit', :value => t(:add), :disabled => ('disabled' unless @providers.length == 0) }
> form_tag({:controller => 'provider', :action => 'destroy', :id => @provider}, {:method => :post , :class => 'buttononly'}) do
> = submit_tag 'delete', :disabled => ('disabled' unless @provider and ['show', 'accounts'].include? controller.action_name)
>
> \ No newline at end of file
> diff --git a/src/features/provider.feature b/src/features/provider.feature
> index c332667..12047bb 100644
> --- a/src/features/provider.feature
> +++ b/src/features/provider.feature
> @@ -85,4 +85,21 @@ Feature: Manage Providers
>     And I fill in "cloud_account[password]" with "incorrect_password"
>     And I fill in "cloud_account[account_number]" with "12345678"
>     And I press "test_account"
> Then I should see "Test Connection Failed: Invalid Account Details"
>
> \ No newline at end of file
> Then I should see "Test Connection Failed: Invalid Account Details"
> Scenario: Delete a provider
> Given I am on the homepage
> And there is a provider named "provider1"
> And this provider has 5 replicated images
> And this provider has 5 hardware profiles
> And this provider has a realm
> And this provider has a cloud account
> When I go to the providers page
> And I follow "provider1"
> And I press "delete"
> Then I should see "Provider Deleted"
> And there should not exist a provider named "provider1"
> And there should not be any replicated images
> And there should not be any hardware profiles
> And there should not be a cloud account
> And there should not be a realm
>
> \ No newline at end of file
> diff --git a/src/features/step_definitions/provider_steps.rb b/src/features/step_definitions/provider_steps.rb
> index e593d43..abc4662 100644
> --- a/src/features/step_definitions/provider_steps.rb
> +++ b/src/features/step_definitions/provider_steps.rb
> @@ -1,7 +1,12 @@
> -Given /^there is not a provider named "([^"]*)"$/ do |name|
> +Given /^there should not exist a provider named "([^"]*)"$/ do |name|
>   Provider.find_by_name(name).should be_nil
> end
>
> +Given /^there is not a provider named "([^"]*)"$/ do |name|
> provider = Provider.find_by_name(name)
> if provider then provider.destroy end
>
> +end
>
> Given /^there is a provider named "([^"]*)"$/ do |name|
>   @provider = Factory(:mock_provider, :name => name)
> end
> @@ -20,9 +25,41 @@ When /^I delete provider$/ do
>   click_button "Delete provider"
> end
>
> Given /^there are these providers:$/ do |table|
>   table.hashes.each do |hash|
>     Factory(:mock_provider, :name => hash['name'])
>   end
> end
>
> +Given /^this provider has (\d+) replicated images$/ do |number|
> number.to_i.times { |i| Factory(:replicated_image, :provider => @provider) }
>
> +end
>
> +Given /^this provider has (\d+) hardware profiles$/ do |number|
> number.to_i.times { |i| Factory(:mock_hwp1, :provider => @provider) }
>
> +end
>
> +Given /^this provider has a realm$/ do
> Factory(:realm, :provider => @provider)
>
> +end
>
> +Given /^this provider has a cloud account$/ do
> Factory(:mock_cloud_account, :provider => @provider)
>
> +end
>
> +Then /^there should not be any replicated images$/ do
> ReplicatedImage.find(:all, :conditions => { :provider_id => @provider.id} ).size.should == 0
>
> +end
>
> +Then /^there should not be any hardware profiles$/ do
> HardwareProfile.find(:all, :conditions => { :provider_id => @provider.id} ).size.should == 0
>
> +end
>
> +Then /^there should not be a cloud account$/ do
> CloudAccount.find(:all, :conditions => { :provider_id => @provider.id} ).size.should == 0
>
> +end
>
> +Then /^there should not be a realm$/ do
> Realm.find(:all, :conditions => { :provider_id => @provider.id} ).size.should == 0
>
> +end
> diff --git a/src/features/support/custom.rb b/src/features/support/custom.rb
> index 6e4e1ef..b5440ae 100644
> --- a/src/features/support/custom.rb
> +++ b/src/features/support/custom.rb
> @@ -34,12 +34,12 @@ CloudAccount.class_eval do
>   def generate_cloud_account_key
>   end
> def instance_key
> @key = mock('Key', :null_object => true)
> @key.stub!(:pem).and_return("PEM")
> @key.stub!(:id).and_return("1_user")
> @key
> end
>
> +#  def instance_key
> +#    @key = mock('Key', :null_object => true)
> +#    @key.stub!(:pem).and_return("PEM")
> +#    @key.stub!(:id).and_return("1_user")
> +#    @key
> +#  end
> end
>
> RepositoryManager.class_eval do
> diff --git a/src/spec/factories/replicated_image.rb b/src/spec/factories/replicated_image.rb
> new file mode 100644
> index 0000000..f989380
> --- /dev/null
> +++ b/src/spec/factories/replicated_image.rb
> @@ -0,0 +1,5 @@
> +Factory.define :replicated_image do |ri|
> ri.association :image
> ri.association :provider
> ri.sequence(:provider_image_key) { |n| "provider_image_key#(n)" }
>
> +end
> \ No newline at end of file
> -- 
> 1.7.2.3

Also this patch breaks unit tests for spec/model/provider_spec.rb with:

'Provider (using stubbed out connect method) should not destroy provider if deletion of associated cloud account fails' FAILED expected true to be false
./spec/models/provider_spec.rb:81:

-- Ladislav