Steve Linabery wrote:
> Also, add 'create and add new cloud account from portal pool view'.
> Added link on provider view to see existing accounts & add new accounts.
> The permissions checking in this patch is woefully inadequate, but should be
> easy to add in before end of sprint!
> With the functionality added by this patch, I'm able to start Mock instances.
> Handy for demos!
> ---
> src/app/controllers/cloud_accounts_controller.rb | 86
> ++++++++++++++++++++
> src/app/controllers/portal_pool_controller.rb | 10 +++
> src/app/views/cloud_accounts/_form.erb | 13 +++
> .../cloud_accounts/accounts_for_pool.html.erb | 36 ++++++++
> src/app/views/cloud_accounts/new.html.erb | 14 +++
> .../views/cloud_accounts/new_from_pool.html.erb | 11 +++
> src/app/views/portal_pool/show.html.erb | 6 +-
> 7 files changed, 175 insertions(+), 1 deletions(-)
> create mode 100644 src/app/controllers/cloud_accounts_controller.rb
> create mode 100644 src/app/views/cloud_accounts/_form.erb
> create mode 100644 src/app/views/cloud_accounts/accounts_for_pool.html.erb
> create mode 100644 src/app/views/cloud_accounts/new.html.erb
> create mode 100644 src/app/views/cloud_accounts/new_from_pool.html.erb
>
> diff --git a/src/app/controllers/cloud_accounts_controller.rb
> b/src/app/controllers/cloud_accounts_controller.rb
> new file mode 100644
> index 0000000..96a0e0f
> --- /dev/null
> +++ b/src/app/controllers/cloud_accounts_controller.rb
> @@ -0,0 +1,86 @@
> +#
> +# Copyright (C) 2010 Red Hat, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; version 2 of the License.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write to the Free Software
> +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
> +# MA 02110-1301, USA. A copy of the GNU General Public License is
> +# also available at http://www.gnu.org/copyleft/gpl.html.
> +
> +# Filters added to this controller apply to all controllers in the
> application.
> +# Likewise, all the methods added will be available for all controllers.
> +
> +class CloudAccountsController < ApplicationController
> + before_filter :require_user
> +
> + def new
> + @cloud_account = CloudAccount.new
> + @providers = []
> + all_providers = Provider.all
> + all_providers.each {|provider|
> + begin
> + require_privilege(Privilege::PROVIDER_VIEW,provider)
> + @providers << provider
> + rescue
> + end
> + }
> + end
> +
>
I don't think you need the providers list here, as for the default 'new'
operation you'll be getting a provider ID in the url
You also need 'require_privilege(Privilege::ACCOUNT_MODIFY,@provider)'
somewhere in this method
> + def new_from_pool
> + @pool = PortalPool.find(params[:id])
>
You might use pool_id here since raw 'id' in the
cloud_accounts_controller would normally be an account ID
You also need 'require_privilege(Privilege::ACCOUNT_ADD,@pool)'
> + @cloud_account = CloudAccount.new
> + @providers = []
> + all_providers = Provider.all
> + all_providers.each {|provider|
> + begin
> + require_privilege(Privilege::PROVIDER_VIEW,provider)
> + @providers << provider
> + rescue
> + end
> + }
> + end
> +
>
Instead of calling require_privilege and swallowing the exception, you
can just call authorized? -- require_privilege is the exception-throwing
version of authorized?
Even better would be to filter the query on permissions. oVirt does this
in a few places, and I believe jayg is doing this already in his nav
patch for providers.
> +
> + def create
> + @cloud_account = CloudAccount.new(params[:cloud_account])
> + @provider = Provider.find(params[:provider][:id])
> + @cloud_account.provider = @provider
> + @cloud_account.save!
> + end
> +
>
Hmm. If the select list is in the form as provider_id, then you
shouldn't require any special handling of the association here
You also need 'require_privilege(Privilege::ACCOUNT_MODIFY,@provider)'
before saving anything
> + def create_from_pool
> + @pool = PortalPool.find(params[:pool][:id])
> + @cloud_account = CloudAccount.new(params[:cloud_account])
> + @provider = Provider.find(params[:provider][:id])
> + @cloud_account.provider = @provider
>
Hmm. If the select list is in the form as provider_id, then you
shouldn't require any special handling of the association here
You also need 'require_privilege(Privilege::ACCOUNT_ADD,@pool)' before
saving anything
> + @cloud_account.save!
> + @pool.cloud_accounts << @cloud_account unless
> @pool.cloud_accounts.map{|x| x.id}.include?(@cloud_account.id)
> + @pool.save!
> + redirect_to :controller => "portal_pool", :action => 'show', :id =>
> @pool.id
> + end
> +
> + def accounts_for_pool
> + @pool = PortalPool.find(params[:id])
> + @cloud_accounts = []
> + all_accounts = CloudAccount.all
> + all_accounts.each {|account|
> + begin
> + require_privilege(Privilege::ACCOUNT_VIEW,account)
> + require_privilege(Privilege::ACCOUNT_ADD,account)
> + @cloud_accounts << account unless @pool.cloud_accounts.map{|x|
> x.id}.include?(account.id)
> + rescue
> + end
> + }
> +
> + end
> +
>
I'm assuming this is to generate the form for adding an existing account
to a pool. If so, where's the action that does the actual save
operation? Also you need to make sur ethat the user has ACCOUNT_ADD on
the pool.
Same comment above on require_privilege and rescue. In addition is this
something that can be done with a query as well? It might make less
sense as a single query since we need to filter out currently-used accounts.
> +end
> diff --git a/src/app/controllers/portal_pool_controller.rb
> b/src/app/controllers/portal_pool_controller.rb
> index 90df8a5..be9d774 100644
> --- a/src/app/controllers/portal_pool_controller.rb
> +++ b/src/app/controllers/portal_pool_controller.rb
> @@ -66,4 +66,14 @@ class PortalPoolController < ApplicationController
>
> def delete
> end
> +
> + def add_account
> + @portal_pool = PortalPool.find(params[:portal_pool])
> + @cloud_account = CloudAccount.find(params[:cloud_account])
> + @portal_pool.cloud_accounts << @cloud_account unless
> @portal_pool.cloud_accounts.map{|x| x.id}.include?(@cloud_account.id)
> + @portal_pool.save!
> + @portal_pool.populate_realms_and_images([...@cloud_account])
> + redirect_to :action => 'show', :id => @portal_pool.id
> + end
> +
> end
>
Hmm. it seems a little odd to have the form generation method in one
controller and the save in another -- I think in this case both
operations belong to the pool controller.
> diff --git a/src/app/views/cloud_accounts/_form.erb
> b/src/app/views/cloud_accounts/_form.erb
> new file mode 100644
> index 0000000..78b3510
> --- /dev/null
> +++ b/src/app/views/cloud_accounts/_form.erb
> @@ -0,0 +1,13 @@
> +<ul>
> + <li>
> + <%= form.label :username, "Choose a username" %>
> + <%= form.text_field :username %>
> + </li>
> + <li>
> + <%= form.label :password, form.object.new_record? ? "Choose a password"
> : "Change password" %>
> + <%= form.password_field :password %>
> + </li>
>
Hmm. the "choose a username/password" implies that the user is creating
an account -- but what's really going on is that the user is entering
credentials for an already-existing provider account. Perhaps replace
'choose a' with 'enter your' or something similar
> +<li>
> + <%= select("provider", "id", @providers.map{|p| [p.name,p.id]} , {
> :include_blank => false }) %>
> +</li>
>
The provider part should only be shown for 'add to pool' -- otherwise we
just need a hidden provider_id field with the provider_id from the URL vars.
For the provider selection you should set the provider_id attribute of
the cloud_account object so you won't need to handle the association
separately in the submit handling
> +</ul>
> diff --git a/src/app/views/cloud_accounts/accounts_for_pool.html.erb
> b/src/app/views/cloud_accounts/accounts_for_pool.html.erb
> new file mode 100644
> index 0000000..e273551
> --- /dev/null
> +++ b/src/app/views/cloud_accounts/accounts_for_pool.html.erb
> @@ -0,0 +1,36 @@
> +<% if @pool.cloud_accounts.size > 0 %>
> +<h1>These Cloud Accounts are already attached to this pool</h1>
> +<table>
> +<thead>
> +<tr>
> +<th scope="col">Provider Name</th>
> +<th scope="col">Cloud Account User Name</th>
> +</tr>
> +<% @pool.cloud_accounts.each {|a| %>
> +<tr>
> +<td><%= a.provider.name %></td>
> +<td><%= a.username %></td>
> +</tr>
> +<% } %>
> +</table>
> +<% end %>
> +
> +<% if @cloud_accounts.size == 0 %>
> +<h1>There are no existing Cloud Accounts available to add</h1>
> +<% else %>
> +<h1>These Cloud Accounts are available to add</h1>
> +<table>
> +<thead>
> +<tr>
> +<th scope="col">Provider Name</th>
> +<th scope="col">Cloud Account User Name</th>
> +</tr>
> +<% @cloud_accounts.each {|a| %>
> +<tr>
> +<td><%= a.provider.name %></td>
> +<td><%= a.username %> <%= link_to "Add this account", {:controller=>
> "portal_pool",
> + :action => "add_account", :portal_pool => @pool, :cloud_account => a},
> :class => "actionlink" %></td>
> +</tr>
> +<% } %>
> +</table>
> +<% end %>
> diff --git a/src/app/views/cloud_accounts/new.html.erb
> b/src/app/views/cloud_accounts/new.html.erb
> new file mode 100644
> index 0000000..40f32cb
> --- /dev/null
> +++ b/src/app/views/cloud_accounts/new.html.erb
> @@ -0,0 +1,14 @@
> +<% if @providers.size == 0 %>
> +<h1>No Providers available to associate with a new cloud account</h1>
> +<% else %>
> +<h2 class="greeting">New Cloud Account</h2>
> +
>
For the 'new' action we should be getting provider_id via a URL var
> +<div class="dcloud_form">
> + <% form_for @cloud_account, :url => { :action => "create" } do |f| %>
> + <%= f.error_messages %>
> + <%= render :partial => "form", :object => f %>
> + <%= f.submit "Create Cloud Account", :class => "submit" %>
> + <% end %>
> + <%= link_to "Cancel", root_path, :class => 'actionlink' %>
> +</div>
> +<% end %>
> diff --git a/src/app/views/cloud_accounts/new_from_pool.html.erb
> b/src/app/views/cloud_accounts/new_from_pool.html.erb
> new file mode 100644
> index 0000000..747df03
> --- /dev/null
> +++ b/src/app/views/cloud_accounts/new_from_pool.html.erb
> @@ -0,0 +1,11 @@
> +<h2 class="greeting">New Cloud Account</h2>
> +
> +<div class="dcloud_form">
> + <% form_for @cloud_account, :url => { :action => "create_from_pool" } do
> |f| %>
> + <%= f.error_messages %>
> + <%= render :partial => "form", :object => f %>
> + <%= hidden_field :pool, :id %>
> + <%= f.submit "Create Cloud Account", :class => "submit" %>
> + <% end %>
> + <%= link_to "Cancel", root_path, :class => 'actionlink' %>
> +</div>
> diff --git a/src/app/views/portal_pool/show.html.erb
> b/src/app/views/portal_pool/show.html.erb
> index d649199..9d4e36c 100644
> --- a/src/app/views/portal_pool/show.html.erb
> +++ b/src/app/views/portal_pool/show.html.erb
> @@ -35,4 +35,8 @@
> </tbody>
> </table>
> <% end %>
> -<%= link_to "Add a new instance", {:controller => "instance", :action =>
> "new", :id => @pool}, :class=>"actionlink"%>
> +<%= link_to "Add a new instance", {:controller => "instance", :action =>
> "new", :id => @pool}, :class=>"actionlink"%><br/>
> +<%= link_to "View/Add Existing Cloud Accounts", {:controller =>
> "cloud_accounts",:action => "accounts_for_pool", :id => @pool},
> :class=>"actionlink" %><br/>
>
> +<%= link_to "Add a New Cloud Account", {:controller =>
> +"cloud_accounts",:action => "new_from_pool", :id => @pool},
>
You should probably use pool_id instead of id here -- as suggested above
on the controller comments.
> +:class=>"actionlink" %>
>
_______________________________________________
deltacloud-devel mailing list
[email protected]
https://fedorahosted.org/mailman/listinfo/deltacloud-devel
