Re: [PATCH conductor 2/2] Created new deployables controller/views

[Scott Seago]

On 06/03/2011 05:05 AM, jprovazn(a)redhat.com wrote:
>  From: Jan Provaznik<jprovazn(a)redhat.com>
>
>  task https://www.aeolusproject.org/redmine/issues/1604
>  Simple UI for managing deployables (for now mapping is done
>  against LegacyDeployable as we don't have real deployables in warehouse)
>  ---
>    src/app/controllers/application_controller.rb     |    9 +++
>    src/app/controllers/deployables_controller.rb     |   77 +++++++++++++++++++++
>    src/app/views/deployables/_form.haml              |   14 ++++
>    src/app/views/deployables/edit.haml               |    3 +
>    src/app/views/deployables/index.haml              |   12 +++
>    src/app/views/deployables/new.haml                |    3 +
>    src/app/views/deployables/show.haml               |   14 ++++
>    src/config/locales/en.yml                         |    3 +
>    src/config/navigation.rb                          |    1 +
>    src/config/routes.rb                              |    1 +
>    src/features/deployable.feature                   |   54 ++++++++++++++
>    src/features/step_definitions/deployable_steps.rb |   22 ++++++
>    12 files changed, 213 insertions(+), 0 deletions(-)
>    create mode 100644 src/app/controllers/deployables_controller.rb
>    create mode 100644 src/app/views/deployables/_form.haml
>    create mode 100644 src/app/views/deployables/edit.haml
>    create mode 100644 src/app/views/deployables/index.haml
>    create mode 100644 src/app/views/deployables/new.haml
>    create mode 100644 src/app/views/deployables/show.haml
>    create mode 100644 src/features/deployable.feature
>    create mode 100644 src/features/step_definitions/deployable_steps.rb
>
>  diff --git a/src/app/controllers/application_controller.rb
> b/src/app/controllers/application_controller.rb
>  index 812cf2d..41234d9 100644
>  --- a/src/app/controllers/application_controller.rb
>  +++ b/src/app/controllers/application_controller.rb
>  @@ -261,4 +261,13 @@ class ApplicationController<  ActionController::Base
>      def js_for_xhr
>        request.format = :js if request.xhr?
>      end
>  +
>  +  def require_admin
>  +    # TODO: any better way how to check that user 'is admin'?
>  +    unless current_user and current_user.permissions.first(:conditions =>  {:role_id
> =>  Role.find_by_name('Administrator').id,
>  +                                                           :permission_object_id => 
> BasePermissionObject.general_permission_scope.id})
>  +      flash[:notice] = "You must be admin to access this page" 
We should
never assume anything about the roles a user may be assigned 
to, since those can be renamed/restructured/etc. What matters is what 
privileges they have.

So we need to work out what privilege the user ought to have and then 
require that. Since this is a new model that needs permission to add it, 
we should probably add this object type to the permissions system:
- SuggestedDeployable should include PermissionedObject (I know we'd 
said this wouldn't be needed, but now I think we need it in order to 
manage edit permissions)
- Add "SuggestedDeployable => ACTIONS" to the Privilege::TYPES hash
- Add an appropriate belongs_to association in permission.rb for 
suggested_deployable on the foreign key permission_object_id
- Add  'SuggestedDeployable Administrator' and 'SuggestedDeployable 
User' roles on BasePermissionObject (basically copy what's done w/ 
'Template Administrator' and instead of 'Template Creator', the User 
role will only have view and user -- not modify)
- Add SuggestedDeployable privileges to the main Administrator role 
(again use Template as a model here)
- Add user/owner roles on SuggestedDeployable model (like we do for 
Template)
- Again using templates as an analogy, we need to grant self-service 
users access to view/use suggested deployables -- look at seeds.rb ad 
how we do this for template creation with 
self_service_default_template_obj and self_service_default_template_role

Once that's done, the UI for this model can be treated like any other -- 
list page filters by permission (but open to all). Any edit/action links 
are available only to users with modify permissions, and 'new' links are 
only available if the user has create permissions.
>  +      redirect_to root_url
>  +    end
>  +  end
>    end
>  diff --git a/src/app/controllers/deployables_controller.rb
> b/src/app/controllers/deployables_controller.rb 
This will need to be renamed too
>  new file mode 100644
>  index 0000000..fba2dd3
>  --- /dev/null
>  +++ b/src/app/controllers/deployables_controller.rb
>  @@ -0,0 +1,77 @@
>  +class DeployablesController<  ApplicationController
>  +  before_filter :require_user
>  +  before_filter :require_admin, :except =>  [:index, :show] 
Instead of this,
once we've added roles/privileges for this model we can 
handle permissions checking just like everywhere else.
>  +  layout 'application'
>  +
>  +  def top_section
>  +    :administer
>  +  end
>  +
>  +  def index
>  +    @deployables = Deployable.all
>  +    set_header
>  +  end
>  +
>  +  def new
>  +    load_deployables
>  +    @deployable = Deployable.new(params[:deployable])
>  +  end
>  +
>  +  def show
>  +    @deployable = Deployable.find(params[:id])
>  +  end
>  +
>  +  def create
>  +    if params[:cancel]
>  +      redirect_to deployables_path
>  +      return
>  +    end
>  +
>  +    @deployable = Deployable.new(params[:deployable])
>  +    if @deployable.save
>  +      flash[:notice] = 'Deployable added'
>  +      redirect_to deployables_path
>  +    else
>  +      load_deployables
>  +      render :new
>  +    end
>  +  end
>  +
>  +  def edit
>  +    @deployable = Deployable.find(params[:id])
>  +    load_deployables
>  +  end
>  +
>  +  def update
>  +    @deployable = Deployable.find(params[:id])
>  +
>  +    if @deployable.update_attributes(params[:deployable])
>  +      flash[:notice] = 'Deployable updated successfully!'
>  +      redirect_to deployables_url
>  +    else
>  +      load_deployables
>  +      render :action =>  'edit'
>  +    end
>  +  end
>  +
>  +  def multi_destroy
>  +    Deployable.destroy(params[:deployables_selected])
>  +    redirect_to deployables_path
>  +  end
>  +
>  +  private
>  +
>  +  def load_deployables
>  +    # TODO: pull warehouse deployables once it's possible, for now we fetch
>  +    # legacy deployables
>  +    @all_deployables = LegacyDeployable.all 
I don't think we need this. For
now we'll just require the admin to 
enter the URL to the deployable.
>  +  end
>  +
>  +  def set_header
>  +    @header = [
>  +      { :name =>  '', :sortable =>  false },
>  +      { :name =>  t("deployables.index.name"), :sort_attr =>  :name },
>  +      { :name =>  t("deployables.index.url"), :sortable =>  :url }
>  +    ]
>  +  end
>  +end
>  diff --git a/src/app/views/deployables/_form.haml b/src/app/views/deployables/_form.haml
>  new file mode 100644
>  index 0000000..9316f94
>  --- /dev/null
>  +++ b/src/app/views/deployables/_form.haml
>  @@ -0,0 +1,14 @@
>  += error_messages_for 'deployable'
>  +%ul
>  +  %li
>  +    = form.label :name
>  +    = form.text_field :name
>  +  %li
>  +    = form.label :description
>  +    = form.text_area :description
>  +  %li
>  +    = form.label :deployable
>  +    = form.select :url, @all_deployables.map {|p| [ p.name, p.id ]}, { :include_blank
> =>  false } 
So this should just be a textfield to enter the URL
>  +
>  += submit_tag 'Cancel', :name =>  'cancel'
>  += submit_tag 'Save', :name =>  'save'
>  diff --git a/src/app/views/deployables/edit.haml b/src/app/views/deployables/edit.haml
>  new file mode 100644
>  index 0000000..e3af0ee
>  --- /dev/null
>  +++ b/src/app/views/deployables/edit.haml
>  @@ -0,0 +1,3 @@
>  +%h2 Edit deployable
>  +- form_for @deployable, :url =>  deployable_path(@deployable), :html =>  { :method
> =>  :put } do |f|
>  +  = render :partial =>  "form", :locals =>  { :form =>  f }
>  diff --git a/src/app/views/deployables/index.haml b/src/app/views/deployables/index.haml
>  new file mode 100644
>  index 0000000..20b3cf5
>  --- /dev/null
>  +++ b/src/app/views/deployables/index.haml
>  @@ -0,0 +1,12 @@
>  += render :partial =>  'layouts/admin_header'
>  +- form_header = capture_haml do
>  +  = link_to "Add Deployable", new_deployable_path, { :class => 
> 'button' }
>  +  = restful_submit_tag "Delete", "destroy",
> multi_destroy_deployables_path, 'DELETE', :id =>  'delete_button'
>  += filter_table(@header, @deployables, :form_header =>  form_header) do |deployable|
>  +  %tr
>  +    %td
>  +      %input{:name =>  "deployables_selected[]", :type => 
> "checkbox", :value =>  deployable.id, :id => 
> "deployable_checkbox_#{deployable.id}" }
>  +    %td
>  +      = link_to deployable.name, deployable_path(deployable.id)
>  +    %td
>  +      = deployable.url
>  diff --git a/src/app/views/deployables/new.haml b/src/app/views/deployables/new.haml
>  new file mode 100644
>  index 0000000..77a0214
>  --- /dev/null
>  +++ b/src/app/views/deployables/new.haml
>  @@ -0,0 +1,3 @@
>  +%h2 Add new deployable
>  +- form_for @deployable, :url =>  deployables_path do |f|
>  +  = render :partial =>  "form", :locals =>  { :form =>  f }
>  diff --git a/src/app/views/deployables/show.haml b/src/app/views/deployables/show.haml
>  new file mode 100644
>  index 0000000..5abeb65
>  --- /dev/null
>  +++ b/src/app/views/deployables/show.haml
>  @@ -0,0 +1,14 @@
>  += render :partial =>  'layouts/admin_header'
>  +%h2= &quot;&#39;#{(a)deployable.name}&#39; Deployable"
>  +%ul
>  +  %li
>  +    = label_tag :name, 'Name'
>  +    %span= @deployable.name
>  +  %li
>  +    = label_tag :description, 'Description'
>  +    %span= @deployable.description
>  +  %li
>  +    = label_tag :url, 'URL'
>  +    %span= @deployable.url
>  +
>  +  = link_to t(:edit), edit_deployable_path(@deployable), :class =>  'button
> formbutton'
>  diff --git a/src/config/locales/en.yml b/src/config/locales/en.yml
>  index 1ca4035..a6198de 100644
>  --- a/src/config/locales/en.yml
>  +++ b/src/config/locales/en.yml
>  @@ -142,6 +142,9 @@ en:
>      deployables:
>        index:
>          deployables: Deployables
>  +      name: Name
>  +      url: URL
>  +      description: Description
>          base_deployable: Base Deployable
>          deleted:
>            one: "Deployable %{list} was deleted."
>  diff --git a/src/config/navigation.rb b/src/config/navigation.rb
>  index caa711d..b23cbec 100644
>  --- a/src/config/navigation.rb
>  +++ b/src/config/navigation.rb
>  @@ -20,6 +20,7 @@ SimpleNavigation::Configuration.run do |navigation|
>          second_level.item :hardware_profiles, t(:cloud_engine_hardware_profiles),
> hardware_profiles_path, :highlights_on =>  /\/hardware_profiles/
>          second_level.item :realms, t(:cloud_engine_realms), realms_path, :highlights_on
> =>  /\/realms/
>          second_level.item :pool_families, t('pool_families.pool_families'),
> pool_families_path, :highlights_on =>  /\/pool_families/
>  +      second_level.item :deployables, t('deployables.index.deployables'),
> deployables_path, :highlights_on =>  /\/deployables/
>          second_level.item :settings, t('settings.settings'), settings_path,
> :highlights_on =>  /\/settings/
>        end
>        first_level.item :dashboard, t(:dashboard), '#'
>  diff --git a/src/config/routes.rb b/src/config/routes.rb
>  index 100e960..367e7b7 100644
>  --- a/src/config/routes.rb
>  +++ b/src/config/routes.rb
>  @@ -56,6 +56,7 @@ ActionController::Routing::Routes.draw do |map|
>      map.resources :pool_families, :collection =>  { :multi_destroy =>  :delete,
> :add_provider_account =>  :post, :multi_destroy_provider_accounts =>  :delete }
>      map.resources :realms, :collection =>  { :multi_destroy =>  :delete }
>      map.resources :realm_mappings, :collection =>  { :multi_destroy =>  :delete }
>  +  map.resources :deployables, :collection =>  { :multi_destroy =>  :delete }
>
>      map.matching_profiles
> '/hardware_profiles/matching_profiles/:hardware_profile_id/provider/:provider_id',
> :controller =>  'hardware_profiles', :action => 
> 'matching_profiles', :conditions =>  { :method =>  :get }
>
>  diff --git a/src/features/deployable.feature b/src/features/deployable.feature
>  new file mode 100644
>  index 0000000..f1e027f
>  --- /dev/null
>  +++ b/src/features/deployable.feature
>  @@ -0,0 +1,54 @@
>  +Feature: Manage Realms
>  +  In order to manage realms
>  +  As an admin
>  +  I want to add/edit/remove realms
>  +
>  +  Background:
>  +    Given I am an authorised user
>  +    And I am logged in
>  +    And There is a mock pulp repository
>  +
>  +  Scenario: Create new deployable
>  +    Given I am on the deployables page
>  +    And there is a legacy deployable "legacydeployable"
>  +    When I follow "Add Deployable"
>  +    Then I should see "Add new deployable"
>  +    When I fill in "deployable[name]" with "test1"
>  +    When I fill in "deployable[description]" with "description"
>  +    And I select "legacydeployable" from "deployable_url"
>  +    And I press "Save"
>  +    Then I should see "Deployable added"
>  +
>  +  Scenario: Change the name
>  +    Given there is a legacy deployable "legacydeployable"
>  +    And a deployable "testdepl" exists
>  +    And a deployable "testdepl" points to "legacydeployable"
>  +    And I am on the deployables page
>  +    When I follow "testdepl"
>  +    And I follow "Edit"
>  +    Then I should see "Edit deployable"
>  +    When I fill in "deployable[name]" with "testdepl-renamed"
>  +    And I press "Save"
>  +    Then I should see "Deployable updated successfully!"
>  +    And I should see "testdepl-renamed"
>  +
>  +  Scenario: Show deployable details
>  +    Given a deployable "testdepl" exists
>  +    And I am on the deployables page
>  +    When I follow "testdepl"
>  +    Then I should see "'testdepl' Deployable"
>  +    And I should see "Name"
>  +    And I should see "Description"
>  +    And I should see "URL"
>  +
>  +  Scenario: Delete deployables
>  +    Given a deployable "testdepl1" exists
>  +    And a deployable "testdepl2" exists
>  +    And I am on the deployables page
>  +    When I check "testdepl1" deployable
>  +    And I check "testdepl2" deployable
>  +    And I press "Delete"
>  +    Then there should be only 0 deployables
>  +    And I should be on the deployables page
>  +    And I should not see "testdepl1"
>  +    And I should not see "testdepl2"
>  diff --git a/src/features/step_definitions/deployable_steps.rb
> b/src/features/step_definitions/deployable_steps.rb
>  new file mode 100644
>  index 0000000..31908f9
>  --- /dev/null
>  +++ b/src/features/step_definitions/deployable_steps.rb
>  @@ -0,0 +1,22 @@
>  +Given /^there is a legacy deployable "([^"]*)"$/ do |arg1|
>  +  Factory :legacy_deployable, :name =>  arg1
>  +end
>  +
>  +Given /^a deployable "([^"]*)" exists$/ do |arg1|
>  +  Factory :deployable, :name =>  arg1
>  +end
>  +
>  +When /^I check "([^"]*)" deployable$/ do |arg1|
>  +  dep = Deployable.find_by_name(arg1)
>  +  check("deployable_checkbox_#{dep.id}")
>  +end
>  +
>  +Then /^there should be only (\d+) deployables$/ do |arg1|
>  +  Deployable.count.should == arg1.to_i
>  +end
>  +
>  +Given /^a deployable "([^"]*)" points to "([^"]*)"$/ do
> |arg1, arg2|
>  +  dep = Deployable.find_by_name(arg1)
>  +  ldep = LegacyDeployable.find_by_name(arg2)
>  +  dep.url = ldep.id
>  +end