As we discussed, this will need to be rebased once the portalPool naming patch has been pushed, but also, there are some whitespace errors that prevent it from applying properly. You can manually fix them, but to save effort for next time, I suggest enabling some hooks in portal/.git/hooks. Here are the errors:
warning: 6 lines add whitespace errors. 81a935a3404789ec778428916440f3a425a2834e src/app/controllers/portal_pool_controller.rb:114: trailing whitespace. + src/app/views/portal_pool/new.html.erb:10: space before tab in indent. + <li><label>Name<span>Provide a descriptive name for this pool.</span></label><%= text_field :portal_pool, :name %></li> src/config/routes.rb:34: trailing whitespace. + src/config/routes.rb:35: trailing whitespace. + src/config/routes.rb:37: trailing whitespace. + src/config/routes.rb:47: trailing whitespace.
Remaining feedback inline.
On Wed, Apr 7, 2010 at 2:27 PM, mtaylor mtaylor@redhat.com wrote:
src/app/controllers/portal_pool_controller.rb | 8 +++----- src/app/views/portal_pool/new.html.erb | 2 +- src/app/views/portal_pool/show.html.erb | 2 +- src/config/routes.rb | 8 ++++++-- 4 files changed, 11 insertions(+), 9 deletions(-)
diff --git a/src/app/controllers/portal_pool_controller.rb b/src/app/controllers/portal_pool_controller.rb index 69208fa..cacc451 100644 --- a/src/app/controllers/portal_pool_controller.rb +++ b/src/app/controllers/portal_pool_controller.rb @@ -84,9 +84,8 @@ class PortalPoolController < ApplicationController def delete end
- def images
- @pool = PortalPool.find(params[:portal_pool])
- @pool = PortalPool.find(params[:id]) require_privilege(Privilege::POOL_VIEW, @pool) end
@@ -112,6 +111,5 @@ class PortalPoolController < ApplicationController end redirect_to :action => 'show', :id => @portal_pool.id end
-end
+end \ No newline at end of file diff --git a/src/app/views/portal_pool/new.html.erb b/src/app/views/portal_pool/new.html.erb index e98fc4b..29201f5 100644 --- a/src/app/views/portal_pool/new.html.erb +++ b/src/app/views/portal_pool/new.html.erb @@ -7,7 +7,7 @@ <% form_tag :action => 'create' do -%> <legend>Pool</legend> <ul>
<li><label>Name<span>Provide a descriptive name for this
pool.</span></label><%= text_field :portal_pool, :name %></li>
<li><label>Name<span>Provide a descriptive name for thispool.</span></label><%= text_field :portal_pool, :name %></li>
This ^ is the one that triggered the tab error. All indents shoudl be spaces only, our standard is 2 spaces per level.
</ul> </fieldset> <%= submit_tag "Save", :class => "submit" %>diff --git a/src/app/views/portal_pool/show.html.erb b/src/app/views/portal_pool/show.html.erb index 31e2d7c..cc9b30d 100644 --- a/src/app/views/portal_pool/show.html.erb +++ b/src/app/views/portal_pool/show.html.erb @@ -39,5 +39,5 @@ <%= link_to "Back end Accounts", {:action => "accounts", :id => @pool.id}, :class=>"actionlink"%> <%= link_to "User access", {:controller => "permissions", :action => "list", :portal_pool_id => @pool.id}, :class=>"actionlink" if has_view_perms? %> <%= link_to "Hardware Profiles", {:action => "hardware_profiles", :id => @pool.id}, :class=>"actionlink"%> -<%=link_to "View Images", {:controller => "portal_pool", :action => "images", :portal_pool => @pool}, :class => "actionlink" %> +<%=link_to "View Images", {:controller => "portal_pool", :action => "images", :id => @pool.id}, :class => "actionlink" %> <%= link_to "Realms", {:action => "realms", :id => @pool.id},
you only need to pass @pool here, even though you changed the param name - it still gets you the id.
:class=>"actionlink"%>
diff --git a/src/config/routes.rb b/src/config/routes.rb index 38e9ce4..053d61f 100644 --- a/src/config/routes.rb +++ b/src/config/routes.rb @@ -31,6 +31,10 @@ ActionController::Routing::Routes.draw do |map|
# You can have the root of your site routed by hooking up '' # -- just remember to delete public/index.html.
map.resources :portal_pool
map.connect '', :controller => 'provider'
map.login 'login', :controller => "user_sessions", :action => "new"
@@ -40,11 +44,11 @@ ActionController::Routing::Routes.draw do |map| map.resource :account, :controller => "users" map.resources :users map.root :login
- # Temporarily disable this route, provider stuff is not restful yet. # Will be re-enabled in upcoming patch # map.resources :provider
- # Allow downloading Web Service WSDL as a file with an extension # instead of a file named 'wsdl' map.connect ':controller/service.wsdl', :action => 'wsdl'
@@ -52,4 +56,4 @@ ActionController::Routing::Routes.draw do |map| # Install the default route as the lowest priority. map.connect ':controller/:action/:id.:format' map.connect ':controller/:action/:id' -end +end \ No newline at end of file --
Lastly, a general patch style note. Subjects should be short enough to be easily readable. There is no limit on how much detail you can put in the commit message (though you want to explain motivation for doing things, not the code itself). Try to make the subject a reasonable summary line, then insert a blank line followed by whatever detail you want. This will put the detail in the body of the email when you use git send-email. Something like this.
Set up RESTful routes for pools.
Lots of words here.
If there is a group like this and you want to give a summary, you can use the --compose flag at the end of your send-email command. This will allow you to compose an introduction which will be sent first, and your patches will follow as 1/3, 2/3, 3/3.