[PATCH aggregator] BZ #644050 - instances launched by non-admin users are listed as started by Admin

[Jan Provazník]

From: Jan Provaznik <jprovazn(a)redhat.com&gt;

User object was assigned to id. Also moved name generation into helper.
---
 src/app/controllers/instance_controller.rb |    2 +-
 src/app/helpers/instance_helper.rb         |   10 ++++++++++
 src/app/views/instance/index.haml          |    4 ++--
 3 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/src/app/controllers/instance_controller.rb
b/src/app/controllers/instance_controller.rb
index e0aaf3b..ad90404 100644
--- a/src/app/controllers/instance_controller.rb
+++ b/src/app/controllers/instance_controller.rb
@@ -92,7 +92,7 @@ class InstanceController < ApplicationController
 
     @instance = Instance.new(params[:instance])
     @instance.state = Instance::STATE_NEW
-    @instance.owner_id = current_user
+    @instance.owner = current_user
 
     require_privilege(Privilege::INSTANCE_MODIFY,
                       Pool.find((a)instance.pool_id))
diff --git a/src/app/helpers/instance_helper.rb b/src/app/helpers/instance_helper.rb
index e0b6e98..fea3cf4 100644
--- a/src/app/helpers/instance_helper.rb
+++ b/src/app/helpers/instance_helper.rb
@@ -20,4 +20,14 @@
 # Likewise, all the methods added will be available for all controllers.
 
 module InstanceHelper
+  def owner_name(inst)
+    return '' unless inst.owner
+    # if last_name is set, use full name,
+    # else use login
+    if inst.owner.last_name.blank?
+      inst.owner.login
+    else
+      "#{inst.owner.first_name} #{inst.owner.last_name}"
+    end
+  end
 end
diff --git a/src/app/views/instance/index.haml b/src/app/views/instance/index.haml
index 8024a2f..83690b7 100644
--- a/src/app/views/instance/index.haml
+++ b/src/app/views/instance/index.haml
@@ -6,7 +6,7 @@
   {:name => 'TEMPLATE', :sort_attr => 'templates.name'},         
|
   {:name => 'PUBLIC ADDRESS', :sort_attr => 'public_addresses'}, 
|
   {:name => 'PROVIDER', :sortable => false},                      |
-  {:name => 'STARTED BY', :sort_attr => 'users.last_name'},      
|
+  {:name => 'CREATED BY', :sort_attr => 'users.last_name'},      
|
 ]                                                                 |
 
 - pool_columns = [                                                                |
@@ -124,4 +124,4 @@
                 %td= inst.template.name
                 %td= inst.public_addresses
                 %td= inst.cloud_account ? inst.cloud_account.provider.name : ''
-                %td= "#{inst.owner.first_name} #{inst.owner.last_name}" # TODO,
there is "started by" in comps pdf, but we don't save this info
+                %td= owner_name(inst)
-- 
1.7.2.3

[Jason Guiditta]

On Tue, 2010-10-26 at 15:07 +0200, jprovazn(a)redhat.com wrote:
>  From: Jan Provaznik <jprovazn(a)redhat.com&gt;
>  
>  User object was assigned to id. Also moved name generation into helper.
>  ---
>   src/app/controllers/instance_controller.rb |    2 +-
>   src/app/helpers/instance_helper.rb         |   10 ++++++++++
>   src/app/views/instance/index.haml          |    4 ++--
>   3 files changed, 13 insertions(+), 3 deletions(-)
>   
Please add a test verifying the bug before applying this patch, and
leave this as the 2nd patch, showing you have corrected the bug.

-j

[PATCH aggregator 1/2] BZ #644050 - instances launched by non-admin users are listed as started by Admin

[Jan Provazník]

From: Jan Provaznik <jprovazn(a)redhat.com&gt;

User object was assigned to id. Also moved name generation into helper.
---
 src/app/controllers/instance_controller.rb |    2 +-
 src/app/helpers/instance_helper.rb         |   10 ++++++++++
 src/app/views/instance/index.haml          |    4 ++--
 3 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/src/app/controllers/instance_controller.rb
b/src/app/controllers/instance_controller.rb
index 7f7bc0e..6855635 100644
--- a/src/app/controllers/instance_controller.rb
+++ b/src/app/controllers/instance_controller.rb
@@ -92,7 +92,7 @@ class InstanceController < ApplicationController
 
     @instance = Instance.new(params[:instance])
     @instance.state = Instance::STATE_NEW
-    @instance.owner_id = current_user
+    @instance.owner = current_user
 
     require_privilege(Privilege::INSTANCE_MODIFY,
                       Pool.find((a)instance.pool_id))
diff --git a/src/app/helpers/instance_helper.rb b/src/app/helpers/instance_helper.rb
index e0b6e98..fea3cf4 100644
--- a/src/app/helpers/instance_helper.rb
+++ b/src/app/helpers/instance_helper.rb
@@ -20,4 +20,14 @@
 # Likewise, all the methods added will be available for all controllers.
 
 module InstanceHelper
+  def owner_name(inst)
+    return '' unless inst.owner
+    # if last_name is set, use full name,
+    # else use login
+    if inst.owner.last_name.blank?
+      inst.owner.login
+    else
+      "#{inst.owner.first_name} #{inst.owner.last_name}"
+    end
+  end
 end
diff --git a/src/app/views/instance/index.haml b/src/app/views/instance/index.haml
index 8024a2f..83690b7 100644
--- a/src/app/views/instance/index.haml
+++ b/src/app/views/instance/index.haml
@@ -6,7 +6,7 @@
   {:name => 'TEMPLATE', :sort_attr => 'templates.name'},         
|
   {:name => 'PUBLIC ADDRESS', :sort_attr => 'public_addresses'}, 
|
   {:name => 'PROVIDER', :sortable => false},                      |
-  {:name => 'STARTED BY', :sort_attr => 'users.last_name'},      
|
+  {:name => 'CREATED BY', :sort_attr => 'users.last_name'},      
|
 ]                                                                 |
 
 - pool_columns = [                                                                |
@@ -124,4 +124,4 @@
                 %td= inst.template.name
                 %td= inst.public_addresses
                 %td= inst.cloud_account ? inst.cloud_account.provider.name : ''
-                %td= "#{inst.owner.first_name} #{inst.owner.last_name}" # TODO,
there is "started by" in comps pdf, but we don't save this info
+                %td= owner_name(inst)
-- 
1.7.2.3

[PATCH aggregator 1/2] BZ #644050 - instances launched by non-admin users are listed as started by Admin

[Jason Guiditta]

On Wed, 2010-10-27 at 15:18 +0200, jprovazn(a)redhat.com wrote:
>  From: Jan Provaznik <jprovazn(a)redhat.com&gt;
>  
>  User object was assigned to id. Also moved name generation into helper.
>  ---
>   src/app/controllers/instance_controller.rb |    2 +-
>   src/app/helpers/instance_helper.rb         |   10 ++++++++++
>   src/app/views/instance/index.haml          |    4 ++--
>   3 files changed, 13 insertions(+), 3 deletions(-)
>   
ACK and pushed, this does indeed fix the problem.

[PATCH aggregator 2/2] Instance controller rspec test

[Jan Provazník]

From: Jan Provaznik <jprovazn(a)redhat.com&gt;

---
 src/spec/controllers/instance_controller.rb |   41 +++++++++++++++++++++++++++
 1 files changed, 41 insertions(+), 0 deletions(-)
 create mode 100644 src/spec/controllers/instance_controller.rb

diff --git a/src/spec/controllers/instance_controller.rb
b/src/spec/controllers/instance_controller.rb
new file mode 100644
index 0000000..b474518
--- /dev/null
+++ b/src/spec/controllers/instance_controller.rb
@@ -0,0 +1,41 @@
+require 'spec_helper'
+
+describe InstanceController do
+  fixtures :all
+  before(:each) do
+    @admin_permission = Factory :admin_permission
+    @admin = @admin_permission.user
+    activate_authlogic
+  end
+
+  it "should provide ui to create new instance" do
+     UserSession.create(@admin)
+     get :new
+     response.should be_success
+     response.should render_template("new")
+  end
+
+  it "should fail to grant access to new pool ui for unauthenticated user" do
+     get :new
+     response.should_not be_success
+  end
+
+  it "should allow admin to launch instance" do
+     @inst_user_permission = Factory :instance_creator_and_user_permission
+     @inst_user = @inst_user_permission.user
+     UserSession.create(@inst_user)
+     pool = Permission.first(:conditions => {:permission_object_type =>
'Pool', :user_id => @inst_user.id}).permission_object
+     template = Factory.build(:template)
+     template.save!
+     hwp = Factory.build(:mock_hwp1)
+     hwp.save!
+     lambda do
+       post :create, :instance => { :name => 'mockinstance',
+                                    :pool_id => pool.id,
+                                    :template_id => template.id,
+                                    :hardware_profile_id => hwp.id }
+     end.should change(Instance, :count).by(1)
+     inst = Instance.find(:first, :conditions => ['name = ?',
'mockinstance'])
+     inst.owner_id.should == @inst_user.id
+  end
+end
-- 
1.7.2.3

[PATCH aggregator 2/2] Instance controller rspec test

[Jason Guiditta]

On Wed, 2010-10-27 at 15:18 +0200, jprovazn(a)redhat.com wrote:
>  From: Jan Provaznik <jprovazn(a)redhat.com&gt;
>  
>  ---
>   src/spec/controllers/instance_controller.rb |   41 +++++++++++++++++++++++++++
>   1 files changed, 41 insertions(+), 0 deletions(-)
>   create mode 100644 src/spec/controllers/instance_controller.rb
>   
Two minor nits, then ACK.  First,this file should be named
instance_controller_spec.rb to keep consistency.  2nd is inline.

>  diff --git a/src/spec/controllers/instance_controller.rb
> b/src/spec/controllers/instance_controller.rb
>  new file mode 100644
>  index 0000000..b474518
>  --- /dev/null
>  +++ b/src/spec/controllers/instance_controller.rb
>  @@ -0,0 +1,41 @@
>  +require 'spec_helper'
>  +
>  +describe InstanceController do
>  +  fixtures :all
>  +  before(:each) do
>  +    @admin_permission = Factory :admin_permission
>  +    @admin = @admin_permission.user
>  +    activate_authlogic
>  +  end
>  +
>  +  it "should provide ui to create new instance" do
>  +     UserSession.create(@admin)
>  +     get :new
>  +     response.should be_success
>  +     response.should render_template("new")
>  +  end
>  +
>  +  it "should fail to grant access to new pool ui for unauthenticated user" do
>  +     get :new
>  +     response.should_not be_success
>  +  end
>  +
>  +  it "should allow admin to launch instance" do 
Shouldn't the above be 'allow instance creator to launch instance',
rather than admin, since that is the user the test is verifying?


>  +     @inst_user_permission = Factory
> :instance_creator_and_user_permission
>  +     @inst_user = @inst_user_permission.user
>  +     UserSession.create(@inst_user)
>  +     pool = Permission.first(:conditions => {:permission_object_type =>
> 'Pool', :user_id => @inst_user.id}).permission_object
>  +     template = Factory.build(:template)
>  +     template.save!
>  +     hwp = Factory.build(:mock_hwp1)
>  +     hwp.save!
>  +     lambda do
>  +       post :create, :instance => { :name => 'mockinstance',
>  +                                    :pool_id => pool.id,
>  +                                    :template_id => template.id,
>  +                                    :hardware_profile_id => hwp.id }
>  +     end.should change(Instance, :count).by(1)
>  +     inst = Instance.find(:first, :conditions => ['name = ?',
> 'mockinstance'])
>  +     inst.owner_id.should == @inst_user.id
>  +  end
>  +end

[PATCH aggregator 2/2] Instance controller rspec test

[Jan Provazník]

Dne 27.10.2010 19:46, Jason Guiditta napsal(a):
>  On Wed, 2010-10-27 at 15:18 +0200, jprovazn(a)redhat.com wrote:
> > From: Jan Provaznik<jprovazn(a)redhat.com>
> >
> > ---
> >   src/spec/controllers/instance_controller.rb |   41 +++++++++++++++++++++++++++
> >   1 files changed, 41 insertions(+), 0 deletions(-)
> >   create mode 100644 src/spec/controllers/instance_controller.rb
> >
>  Two minor nits, then ACK.  First,this file should be named
>  instance_controller_spec.rb to keep consistency.  2nd is inline.
>
> > diff --git a/src/spec/controllers/instance_controller.rb
> b/src/spec/controllers/instance_controller.rb
> > new file mode 100644
> > index 0000000..b474518
> > --- /dev/null
> > +++ b/src/spec/controllers/instance_controller.rb
> > @@ -0,0 +1,41 @@
> > +require 'spec_helper'
> > +
> > +describe InstanceController do
> > +  fixtures :all
> > +  before(:each) do
> > +    @admin_permission = Factory :admin_permission
> > +    @admin = @admin_permission.user
> > +    activate_authlogic
> > +  end
> > +
> > +  it "should provide ui to create new instance" do
> > +     UserSession.create(@admin)
> > +     get :new
> > +     response.should be_success
> > +     response.should render_template("new")
> > +  end
> > +
> > +  it "should fail to grant access to new pool ui for unauthenticated user"
> do
> > +     get :new
> > +     response.should_not be_success
> > +  end
> > +
> > +  it "should allow admin to launch instance" do
>
>  Shouldn't the above be 'allow instance creator to launch instance',
>  rather than admin, since that is the user the test is verifying?
>  
Both fixed and pushed. Thanks.

Jan