[PATCH conductor] Fixing return_path in permissions#set_permission_object

Jan Provaznik jprovazn at redhat.com
Mon Dec 19 14:29:13 UTC 2011


On 12/19/2011 03:01 PM, Imre Farkas wrote:
> 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.

We discussed this with Imre now, here is summary:
"/deployables/37" should actually be regular route (according to 
config/routes), so it would make sense if 'to_polymorphic_path_param' 
returns only [self] if catalog_id is nil.

THere is another bug when accessing "/deployables/id" and error "No 
route matches {:viewstate=>nil, :controller=>"deployables"}" is returned.

> 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