On 05/10/2011 05:59 PM, Matt Wagner wrote:
---
src/app/controllers/application_controller.rb | 12 ++++
src/app/controllers/deployments_controller.rb | 38 ++++++++++++
src/app/controllers/instances_controller.rb | 40 +++++++++++-
src/app/controllers/pools_controller.rb | 19 ++++++
src/app/models/deployment.rb | 2 +
src/app/models/instance.rb | 4 +
src/app/views/deployments/edit.haml | 11 ++++
src/app/views/instances/edit.haml | 10 +++
src/config/locales/en.yml | 31 +++++++++-
src/config/navigation.rb | 4 +-
src/features/deployment.feature | 9 +++
src/features/instance.feature | 8 +++
src/features/support/paths.rb | 12 ++++
.../controllers/deployments_controller_spec.rb | 39 ++++++++++++
src/spec/controllers/instance_controller_spec.rb | 64 ++++++++++++++++++++
src/spec/controllers/pools_controller_spec.rb | 38 ++++++++++++
16 files changed, 332 insertions(+), 9 deletions(-)
create mode 100644 src/app/views/deployments/edit.haml
create mode 100644 src/app/views/instances/edit.haml
create mode 100644 src/spec/controllers/deployments_controller_spec.rb
diff --git a/src/app/controllers/application_controller.rb
b/src/app/controllers/application_controller.rb
index 6bda765..746811b 100644
--- a/src/app/controllers/application_controller.rb
+++ b/src/app/controllers/application_controller.rb
@@ -142,6 +142,18 @@ class ApplicationController< ActionController::Base
flash[:notice] = "The record you tried to access does not exist, it may have
been deleted"
end
+ # Returns an array of ids from params[:id], params[:ids].
+ def ids_list(other_attrs=[])
+ other_attrs.each do |attr_key|
+ return params[attr_key].to_a if params.include?(attr_key)
+ end
+ if params[:id].present?
+ return params[:id].to_a
+ elsif params[:ids].present?
+ return params[:ids].to_a
+ end
+ end
+
private
def json_error_hash(msg, status)
json = {}
diff --git a/src/app/controllers/deployments_controller.rb
b/src/app/controllers/deployments_controller.rb
index 6bfb78b..9159daa 100644
--- a/src/app/controllers/deployments_controller.rb
+++ b/src/app/controllers/deployments_controller.rb
@@ -1,6 +1,7 @@
class DeploymentsController< ApplicationController
before_filter :require_user
before_filter :load_deployments, :only => [:index, :show]
+ before_filter :load_deployment, :only => [:edit, :update]
def index
end
@@ -61,6 +62,39 @@ class DeploymentsController< ApplicationController
end
end
+ def edit
+ require_privilege(Privilege::MODIFY, @deployment)
+ end
+
+ # TODO - This should eventually support updating multiple objects
+ def update
+ attrs = {}
+ params[:deployment].each_pair{|k,v| attrs[k] = v if
Deployment::USER_MUTABLE_ATTRS.include?(k)}
+ if check_privilege(Privilege::MODIFY, @deployment) and
@deployment.update_attributes(attrs)
+ flash[:success] = t('deployments.updated', :count => 1, :list =>
@deployment.name)
+ redirect_to @deployment
+ else
+ flash[:error] = t('deployments.not_updated', :count => 1, :list =>
@deployment.name)
+ render :action => :edit
+ end
+ end
+
+ def destroy
+ destroyed = []
+ failed = []
+ Deployment.find(ids_list).each do |deployment|
+ if check_privilege(Privilege::MODIFY, deployment)&&
deployment.destroyable?
+ deployment.destroy
+ destroyed<< deployment.name
+ else
+ failed<< deployment.name
+ end
+ end
+ flash[:success] = t('deployments.deleted', :list => destroyed, :count
=> destroyed.size) if destroyed.present?
+ flash[:error] = t('deployments.not_deleted', :list => failed, :count
=> failed.size) if failed.present?
+ redirect_to deployments_url
+ end
+
def multi_stop
notices = ""
errors = ""
@@ -112,6 +146,10 @@ class DeploymentsController< ApplicationController
)
end
+ def load_deployment
+ @deployment = Deployment.find(params[:id])
+ end
+
def init_new_deployment_attrs
@pools = Pool.list_for_user(@current_user, Privilege::CREATE, :target_type =>
Deployment)
@realms = FrontendRealm.all
diff --git a/src/app/controllers/instances_controller.rb
b/src/app/controllers/instances_controller.rb
index 6e675df..f44e3af 100644
--- a/src/app/controllers/instances_controller.rb
+++ b/src/app/controllers/instances_controller.rb
@@ -1,6 +1,6 @@
class InstancesController< ApplicationController
before_filter :require_user, :except => [:can_start, :can_create]
- before_filter :load_instance, :only => [:show, :key]
+ before_filter :load_instance, :only => [:show, :key, :edit, :update]
before_filter :set_view_vars, :only => [:show, :index]
def index
@@ -76,9 +76,6 @@ class InstancesController< ApplicationController
end
end
- def edit
- end
-
def show
load_instances
@url_params = params.clone
@@ -95,6 +92,41 @@ class InstancesController< ApplicationController
end
end
+ def edit
+ require_privilege(Privilege::MODIFY, @instance)
+ end
+
+ def update
+ # TODO - This USER_MUTABLE_ATTRS business is because what a user and app components
can do
+ # will be greatly different. (e.g., a user shouldn't be able to change an
instance's pool,
+ # since it won't do what they expect). As we build this out, this logic will
become more complex.
+ attrs = {}
+ params[:instance].each_pair{|k,v| attrs[k] = v if
Instance::USER_MUTABLE_ATTRS.include?(k)}
This could be written as:
attrs = params[:instance].reject {|k,v| not
Instance::USER_MUTABLE_ATTRS.include?(k)}
I find it cleaner but feel free to keep your way.
+ if check_privilege(Privilege::MODIFY, @instance) and
@instance.update_attributes(attrs)
+ flash[:success] = t('instances.updated', :count => 1, :list =>
@instance.name)
+ redirect_to @instance
+ else
+ flash[:error] = t('instances.not_updated', :count =>1, :list =>
@instance.name)
+ render :action => :edit
+ end
+ end
+
+ def destroy
+ destroyed = []
+ failed = []
+ Instance.find(ids_list).each do |instance|
+ if check_privilege(Privilege::MODIFY, instance)&& instance.destroyable?
+ instance.destroy
+ destroyed<< instance.name
+ else
+ failed<< instance.name
+ end
+ end
+ flash[:success] = t('instances.deleted', :list => destroyed.to_sentence,
:count => destroyed.size) if destroyed.present?
+ flash[:error] = t('instances.not_deleted', :list => failed.to_sentence,
:count => failed.size) if failed.present?
+ render :action => :show
+ end
+
def key
unless @instance.instance_key.nil?
send_data @instance.instance_key.pem,
diff --git a/src/app/controllers/pools_controller.rb
b/src/app/controllers/pools_controller.rb
index 7706902..0ccbf78 100644
--- a/src/app/controllers/pools_controller.rb
+++ b/src/app/controllers/pools_controller.rb
@@ -81,6 +81,25 @@ class PoolsController< ApplicationController
end
end
+ def destroy
+ destroyed = []
+ failed = []
+ error_messages = []
+ Pool.find(ids_list('pools_selected')).each do |pool|
+ if pool.id == MetadataObject.lookup("self_service_default_pool").id
+ error_messages<< "The default pool cannot be deleted"
+ elsif check_privilege(Privilege::MODIFY, pool)&& pool.destroyable?
+ pool.destroy
+ destroyed<< pool.name
+ else
+ failed<< pool.name
+ end
+ end
+ flash[:success] = t('pools.index.pool_deleted', :list =>
destroyed.to_sentence, :count => destroyed.size) if destroyed.present?
+ flash[:error] = t('pools.index.pool_not_deleted', :list =>
failed.to_sentence, :count => failed.size) if failed.present?
+ redirect_to instances_url
+ end
+
def multi_destroy
destroyed = []
failed = []
diff --git a/src/app/models/deployment.rb b/src/app/models/deployment.rb
index 291e351..9f382c0 100644
--- a/src/app/models/deployment.rb
+++ b/src/app/models/deployment.rb
@@ -68,6 +68,8 @@ class Deployment< ActiveRecord::Base
SEARCHABLE_COLUMNS = %w(name)
+ USER_MUTABLE_ATTRS = ['name']
+
def object_list
super<< pool
end
diff --git a/src/app/models/instance.rb b/src/app/models/instance.rb
index c9c5c7f..ef38263 100644
--- a/src/app/models/instance.rb
+++ b/src/app/models/instance.rb
@@ -124,6 +124,10 @@ class Instance< ActiveRecord::Base
before_destroy :destroyable?
+ # A user should only be able to update certain attributes, but the API may permit
other attributes to be
+ # changed if called from another Aeolus component, so attr_protected isn't quite
what we want:
+ USER_MUTABLE_ATTRS = ['name']
+
def validate
if assembly and template
errors.add(:assembly, "Please specify either template or assembly, but not
both")
diff --git a/src/app/views/deployments/edit.haml b/src/app/views/deployments/edit.haml
new file mode 100644
index 0000000..72c8e60
--- /dev/null
+++ b/src/app/views/deployments/edit.haml
@@ -0,0 +1,11 @@
+= error_messages_for 'deployment'
+%h2 Edit deployment
+- form_for @deployment do
+ = hidden_field :deployment, :deployable_id
+ %ul
+ %li
+ = label :deployment, :name
+ = text_field :deployment, :name
+
+ = submit_tag 'Cancel', :name => 'cancel'
+ = submit_tag 'Save', :name => 'save'
diff --git a/src/app/views/instances/edit.haml b/src/app/views/instances/edit.haml
new file mode 100644
index 0000000..dfce76f
--- /dev/null
+++ b/src/app/views/instances/edit.haml
@@ -0,0 +1,10 @@
+= error_messages_for 'instance'
+%h2 Edit instance
+- form_for @instance do
+ %ul
+ %li
+ = label :instance, :name
+ = text_field :instance, :name
+
+ = submit_tag 'Cancel', :name => 'cancel'
+ = submit_tag 'Save', :name => 'save'
diff --git a/src/config/locales/en.yml b/src/config/locales/en.yml
index b8455bc..859bd9e 100644
--- a/src/config/locales/en.yml
+++ b/src/config/locales/en.yml
@@ -1,6 +1,5 @@
en:
dashboard: Dashboard
- instances: Instances
users: Users
settings: Settings
about: About
@@ -67,8 +66,34 @@ en:
choose_treatment: Choose Treatment
apply: Apply
resource_management: Resource Management
- deployments: Deployments
- instances: Instances
+ deployments:
+ deployments: Deployments
+ updated:
+ one: "The deployment %{list} was successfully updated."
+ other: "The deployments %{list} were successfully updated."
+ not_updated:
+ one: "The deployment %{list} could not be updated."
+ other: "The deployments %{list} could not be updated."
+ deleted:
+ one: "The deployment %{list} was successfully deleted."
+ other: "The deployments %{list} were succesfully deleted."
+ not_deleted:
+ one: "The deployment %{list} could not be deleted."
+ other: "The deployments %{list} could not be deleted."
+ instances:
+ instances: Instances
+ updated:
+ one: "The instance %{list} was successfully updated."
+ other: "The instances %{list} were successfully updated."
+ not_updated:
+ one: "The instance %{list} could not be updated."
+ other: "The instances %{list} could not be updated."
+ deleted:
+ one: "The instance %{list} was successfully deleted."
+ other: "The instances %{list} were successfully deleted."
+ not_deleted:
+ one: "The instance %{list} could not be deleted."
+ other: "The instances %{list} could not be deleted."
searches: Searches
image_factory: Image Factory
assemblies: Assemblies
diff --git a/src/config/navigation.rb b/src/config/navigation.rb
index 2abd7d0..794c2b6 100644
--- a/src/config/navigation.rb
+++ b/src/config/navigation.rb
@@ -3,8 +3,8 @@ SimpleNavigation::Configuration.run do |navigation|
navigation.items do |first_level|
first_level.item :resource_management, t(:resource_management), pools_path,
:highlights_on => /^\/$/ do |second_level|
second_level.item :pools, t('pools.index.pools'), pools_path
- second_level.item :deployments, t(:deployments),deployments_path, :highlights_on
=> /^\/$|\/deployments/
- second_level.item :instances, t(:instances), instances_path
+ second_level.item :deployments,
t('deployments.deployments'),deployments_path, :highlights_on =>
/^\/$|\/deployments/
+ second_level.item :instances, t("instances.instances"), instances_path
end
first_level.item :image_factory, t(:image_factory), templates_path, :highlights_on
=> /\/image_factory/ do |second_level|
second_level.item :templates, t('templates.templates'), templates_path
diff --git a/src/features/deployment.feature b/src/features/deployment.feature
index 22a8748..b98505d 100644
--- a/src/features/deployment.feature
+++ b/src/features/deployment.feature
@@ -40,3 +40,12 @@ Feature: Manage Deployments
When I am on the operational status of deployment page
Then I should see "Operational status of testdeployment"
And I should see "Uptime"
+
+ Scenario: Edit deployment name
+ Given there is a deployment named "Hudson" belonging to "QA
Infrastructure" owned by "joe"
+ When I go to Hudson's edit deployment page
+ Then I should see "Edit deployment"
+ When I fill in "name" with "Jenkins"
+ And I press "Save"
+ Then I should be on Jenkins's deployment page
+ And I should see "Jenkins"
diff --git a/src/features/instance.feature b/src/features/instance.feature
index 3b4e000..8ba3251 100644
--- a/src/features/instance.feature
+++ b/src/features/instance.feature
@@ -146,3 +146,11 @@ Feature: Mange Instances
And I am on the the instances page
When I follow "mock1"
Then I should see "Properties for mock1"
+
+ Scenario: Edit an instance name
+ Given there is a "Tomct" instance
+ And I am on Tomct's edit instance page
+ And I fill in "name" with "Tomcat"
+ And I press "Save"
+ Then I should be on Tomcat's instance page
+ And I should see "Tomcat"
\ No newline at end of file
diff --git a/src/features/support/paths.rb b/src/features/support/paths.rb
index e26e611..6718403 100644
--- a/src/features/support/paths.rb
+++ b/src/features/support/paths.rb
@@ -122,6 +122,18 @@ module NavigationHelpers
when /^(.*)'s deployable page$/
deployable_path(Deployable.find_by_name($1))
+ when /^(.*)'s edit deployment page$/
+ edit_deployment_path(Deployment.find_by_name($1))
+
+ when /^(.*)'s edit instance page$/
+ edit_instance_path(Instance.find_by_name($1))
+
+ when /^(.*)'s instance page$/
+ instance_path(Instance.find_by_name($1))
+
+ when /^(.*)'s deployment page$/
+ deployment_path(Deployment.find_by_name($1))
+
# Add more mappings here.
# Here is an example that pulls values out of the Regexp:
#
diff --git a/src/spec/controllers/deployments_controller_spec.rb
b/src/spec/controllers/deployments_controller_spec.rb
new file mode 100644
index 0000000..87bc349
--- /dev/null
+++ b/src/spec/controllers/deployments_controller_spec.rb
@@ -0,0 +1,39 @@
+require 'spec_helper'
+
+describe DeploymentsController do
+ fixtures :all
+ before(:each) do
+ @admin_permission = Factory :admin_permission
+ @admin = @admin_permission.user
+ activate_authlogic
+ end
+
+ it "should allow RESTful delete of a single deployment" do
+ UserSession.create(@admin)
+ deployment = nil
+ lambda do
+ deployment = Factory(:deployment)
+ deployment.owner = @admin
+ deployment.save!
+ end.should change(Deployment, :count).by(1)
+ lambda do
+ delete :destroy, :id => deployment.id
+ end.should change(Deployment, :count).by(-1)
+ end
+
+ it "should allow RESTful delete of multiple deployments" do
+ UserSession.create(@admin)
+ deployment1 = nil
+ deployment2 = nil
+ lambda do
+ deployment1 = Factory(:deployment, :owner => @admin)
+ deployment1.save!
+ deployment2 = Factory(:deployment, :owner => @admin)
+ deployment2.save!
+ end.should change(Deployment, :count).by(2)
+ lambda do
+ delete :destroy, :ids => [deployment1.id, deployment2.id]
+ end.should change(Deployment, :count).by(-2)
+ end
+
+end
\ No newline at end of file
diff --git a/src/spec/controllers/instance_controller_spec.rb
b/src/spec/controllers/instance_controller_spec.rb
index 2837338..2971013 100644
--- a/src/spec/controllers/instance_controller_spec.rb
+++ b/src/spec/controllers/instance_controller_spec.rb
@@ -80,4 +80,68 @@ describe InstancesController do
:hardware_profile_id => hwp.id }
response.flash[:warning].should == "Failed to launch instance: Pool is not
enabled"
end
+
+ it "should respond to restful delete for a single instance" do
+ UserSession.create(@admin)
+ pool = Factory(:pool)
+ template = Factory(:template)
+ hwp = Factory(:mock_hwp1)
+ post :create, :instance => { :name => 'mockinstance2',
+ :pool_id => pool.id,
+ :template_id => template.id,
+ :hardware_profile_id => hwp.id }
+ instance = Instance.find_by_name('mockinstance2')
+ # An instance in 'new' state is not destroyable, so we have to fake it:
+ instance.update_attribute(:state, Instance::STATE_CREATE_FAILED)
+ lambda do
+ delete :destroy, :id => instance.id
+ end.should change(Instance, :count).by(-1)
+ response.should be_success
+ end
+
+ it "should respond to restful delete for multiple instances" do
+ UserSession.create(@admin)
+ pool = Factory(:pool)
+ template = Factory(:template)
+ hwp = Factory(:mock_hwp1)
+ lambda do
+ post :create, :instance => { :name => 'mockinstance3',
+ :pool_id => pool.id,
+ :template_id => template.id,
+ :hardware_profile_id => hwp.id }
+ end.should change(Instance, :count).by(1)
+ lambda do
+ post :create, :instance => { :name => 'mockinstance4',
+ :pool_id => pool.id,
+ :template_id => template.id,
+ :hardware_profile_id => hwp.id }
+ end.should change(Instance, :count).by(1)
+ instance1 = Instance.find_by_name('mockinstance3')
+ instance2 = Instance.find_by_name('mockinstance4')
+ # An instance in 'new' state is not destroyable, so we have to fake it:
+ instance1.update_attribute(:state, Instance::STATE_CREATE_FAILED)
+ instance2.update_attribute(:state, Instance::STATE_CREATE_FAILED)
+ lambda do
+ delete :destroy, :ids => [instance1.id, instance2.id]
+ end.should change(Instance, :count).by(-2)
+ response.should be_success
+ end
+
+ it "should not allow editing of protected attributes by users" do
+ UserSession.create(@admin)
+ pool = Factory(:pool)
+ template = Factory(:template)
+ hwp = Factory(:mock_hwp1)
+ lambda do
+ post :create, :instance => { :name => 'mockinstance5',
+ :pool_id => pool.id,
+ :template_id => template.id,
+ :hardware_profile_id => hwp.id }
+ end.should change(Instance, :count).by(1)
+ instance = Instance.find_by_name('mockinstance5')
+ put :update, :instance => {:state => 'running', :name =>
'mockinstance6'}, :id => instance.id
+ instance.reload
+ instance.state.should == "new"
+ instance.name.should == "mockinstance6"
+ end
end
diff --git a/src/spec/controllers/pools_controller_spec.rb
b/src/spec/controllers/pools_controller_spec.rb
index 01c8d8d..f5ee0cf 100644
--- a/src/spec/controllers/pools_controller_spec.rb
+++ b/src/spec/controllers/pools_controller_spec.rb
@@ -33,4 +33,42 @@ describe PoolsController do
response.should redirect_to(pool_path(id))
end
+ it "should allow RESTful delete of a single pool" do
+ UserSession.create(@admin)
+ lambda do
+ post :create, :pool => {
+ :name => 'pool1',
+ :pool_family_id => PoolFamily.find_by_name('default').id,
+ :enabled => true
+ }
+ end.should change(Pool, :count).by(1)
+ pool = Pool.find_by_name('pool1')
+ lambda do
+ delete :destroy, :id => pool.id
+ end.should change(Pool, :count).by(-1)
+ end
+
+ it "should allow RESTful delete of multiple pools" do
+ UserSession.create(@admin)
+ lambda do
+ post :create, :pool => {
+ :name => 'pool1',
+ :pool_family_id => PoolFamily.find_by_name('default').id,
+ :enabled => true
+ }
+ end.should change(Pool, :count).by(1)
+ lambda do
+ post :create, :pool => {
+ :name => 'pool2',
+ :pool_family_id => PoolFamily.find_by_name('default').id,
+ :enabled => true
+ }
+ end.should change(Pool, :count).by(1)
+ pool1 = Pool.find_by_name('pool1')
+ pool2 = Pool.find_by_name('pool2')
+ lambda do
+ delete :destroy, :ids => [pool1.id, pool2.id]
+ end.should change(Pool, :count).by(-2)
+ end
+
end
ACK with one small comment inline.
Thomas
--
No trees were killed to send this message, but a large number of
electrons were terribly inconvenienced.