[PATCH conductor] Fixing return_path in permissions#set_permission_object

Imre Farkas ifarkas at redhat.com
Mon Dec 19 14:01:03 UTC 2011


On 12/16/2011 02:30 PM, Jan Provazník wrote:
> On 12/16/2011 02:03 PM, Jan Provazník wrote:
>> On 12/14/2011 04:25 PM, ifarkas at redhat.com wrote:
>>> From: Imre Farkas<ifarkas at redhat.com>
>>>
>>> https://bugzilla.redhat.com/show_bug.cgi?id=766473
>>>
>>> The original parameterization of polymorphic_path doesn't work
>>> correctly with nested resources.
>>> To handle this @polymorphic_path_extras is used by
>>> to_polymorphic_path_param method of the model.
>>> ---
>>> src/app/controllers/application_controller.rb | 6 ++++--
>>> src/app/controllers/deployables_controller.rb | 2 +-
>>> src/app/controllers/permissions_controller.rb | 10 ++++++----
>>> src/app/models/deployable.rb | 5 +++++
>>> src/app/views/permissions/_permissions.html.haml | 4 ++++
>>> 5 files changed, 20 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/src/app/controllers/application_controller.rb
>>> b/src/app/controllers/application_controller.rb
>>> index 053a87b..368411e 100644
>>> --- a/src/app/controllers/application_controller.rb
>>> +++ b/src/app/controllers/application_controller.rb
>>> @@ -280,15 +280,17 @@ class ApplicationController<
>>> ActionController::Base
>>> %w[asc desc].include?(params[:order_dir]) ? params[:order_dir] : "asc"
>>> end
>>>
>>> - def add_permissions_inline(perm_obj, path_prefix = "")
>>> + def add_permissions_inline(perm_obj, path_prefix = '',
>>> polymorphic_path_extras = {})
>>> @permission_object = perm_obj
>>> @path_prefix = path_prefix
>>> + @polymorphic_path_extras = polymorphic_path_extras
>>> @roles = Role.find_all_by_scope(@permission_object.class.name)
>>> set_permissions_header
>>> end
>>>
>>> - def add_permissions_tab(perm_obj, path_prefix = "")
>>> + def add_permissions_tab(perm_obj, path_prefix = '',
>>> polymorphic_path_extras = {})
>>> @path_prefix = path_prefix
>>> + @polymorphic_path_extras = polymorphic_path_extras
>>> if "permissions" == params[:details_tab]
>>> require_privilege(Privilege::PERM_VIEW, perm_obj)
>>> @permission_object = perm_obj
>>> diff --git a/src/app/controllers/deployables_controller.rb
>>> b/src/app/controllers/deployables_controller.rb
>>> index 7271695..ba7795d 100644
>>> --- a/src/app/controllers/deployables_controller.rb
>>> +++ b/src/app/controllers/deployables_controller.rb
>>> @@ -55,7 +55,7 @@ class DeployablesController< ApplicationController
>>> save_breadcrumb(catalog_deployable_path(@catalog, @deployable),
>>> @deployable.name)
>>> @providers = Provider.all
>>> @catalogs_options = Catalog.list_for_user(current_user,
>>> Privilege::VIEW).select {|c| !@deployable.catalogs.include?(c)}
>>> - add_permissions_inline(@deployable)
>>> + add_permissions_inline(@deployable, '', {:catalog_id => @catalog.id})
>>> @images_details = @deployable.get_image_details
>>> images = @deployable.fetch_images
>>> uuids = @deployable.fetch_image_uuids
>>> diff --git a/src/app/controllers/permissions_controller.rb
>>> b/src/app/controllers/permissions_controller.rb
>>> index a5c5bb7..86787de 100644
>>> --- a/src/app/controllers/permissions_controller.rb
>>> +++ b/src/app/controllers/permissions_controller.rb
>>> @@ -185,7 +185,7 @@ class PermissionsController< ApplicationController
>>> def set_permission_object (required_role=Privilege::PERM_SET)
>>> obj_type = params[:permission_object_type]
>>> id = params[:permission_object_id]
>>> - @path_prefix = params[:path_prefix]
>>> + @polymorphic_path_extras = params[:polymorphic_path_extras]
>>> @use_tabs = params[:use_tabs]
>>> unless obj_type or id
>>> @permission_object = BasePermissionObject.general_permission_scope
>>> @@ -196,10 +196,12 @@ class PermissionsController< ApplicationController
>>> @return_path = permissions_path
>>> set_admin_users_tabs 'permissions'
>>> else
>>> - @return_path = send("#{@path_prefix}polymorphic_path",
>>
>> @path_prefix should be kept, it's used at least in
>> app/controllers/providers_controller.rb
>>
>>> - @permission_object,
>>> + @return_path = send("polymorphic_path",
>>> + @permission_object.respond_to?(:to_polymorphic_path_param) ?
>>> +
>>> @permission_object.to_polymorphic_path_param(@polymorphic_path_extras) :
>>> + @permission_object,
>>> @use_tabs == "yes" ? {:details_tab => :permissions,
>>> - :only_tab => true} : {})
>>> + :only_tab => true} : {})
>>> end
>>> require_privilege(required_role, @permission_object)
>>> end
>>> diff --git a/src/app/models/deployable.rb b/src/app/models/deployable.rb
>>> index ebd230c..33f844c 100644
>>> --- a/src/app/models/deployable.rb
>>> +++ b/src/app/models/deployable.rb
>>> @@ -193,6 +193,11 @@ class Deployable< ActiveRecord::Base
>>> end
>>> end
>>>
>>> + def to_polymorphic_path_param(polymorphic_path_extras)
>>> + catalog = polymorphic_path_extras.has_key?(:catalog_id) ?
>>> catalogs.find(polymorphic_path_extras[:catalog_id]) : catalogs.first
>>> + [catalog, self]
>>> + end
>>> +
>>
>> I'm not sure it's good idea to return first catalog if 'catalog_id' is
>> not passed in params. Why not return only [self] instead of [catalog,
>> self] in this case?

If we would return the array without a catalog then polymorphic_path 
would return a path like "/deployables/37", which does not match to any 
route.
The point of this patch is to be able to generate the correct path for 
nested resources and both parent and child object has to be passed to 
polymorhic_path. Otherwise it wouldn't create the correct path for the 
nested resource.

>>
>>> private
>>>
>>> def self.apply_search_filter(search)
>>> diff --git a/src/app/views/permissions/_permissions.html.haml
>>> b/src/app/views/permissions/_permissions.html.haml
>>> index 8846dd8..3ac19de 100644
>>> --- a/src/app/views/permissions/_permissions.html.haml
>>> +++ b/src/app/views/permissions/_permissions.html.haml
>>> @@ -5,6 +5,10 @@
>>> %li= restful_submit_tag t('edit'), "multi_update",
>>> multi_update_permissions_path, 'POST', :id => 'perm_edit_button',
>>> :class => 'hidden'
>>> = hidden_field_tag :permission_object_type,
>>> @permission_object.class.name
>>> = hidden_field_tag :permission_object_id, @permission_object.id
>>> +
>>> + - @polymorphic_path_extras.each do |key, value|
>>> + = hidden_field_tag("polymorphic_path_extras[#{key}]", value)
>>> +
>>> = hidden_field_tag :path_prefix, @path_prefix
>>> = hidden_field_tag :use_tabs, @use_tabs ? @use_tabs : (@tabs ? 'yes' :
>>> 'no')
>>>
>>
>>
>> Hi Imre,
>> looks good, but there is a couple of notes inline.
>>
>> Jan
>
> And one more thing, it would be really great to attach a simple rspec
> test which checks that a user is redirect to proper page.
>
> Jan

I am sending the patch again with the test soon.
Imre



More information about the aeolus-devel mailing list