Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Spree 1.3 - nested resources on admin screens #67

Merged
merged 1 commit into from Sep 11, 2013

Conversation

Projects
None yet
3 participants
Contributor

nostopbutton commented Mar 8, 2013

Please can you review my attempt to fix the issue with editing nested resources in admin screens seen when using this extension in combination with Spree v1.3 (#61, #60 & #52).

I am quite new to both Rails and Spree (and OS contribution), so apologies in advance if I've missed anything here.

Basically, the issue seems to be incorrect routes urls being created by the link_to_remove_fields method in core Spree 1.3 when used with nested resources e.g.

resources :product_customization_types do
  resources :customizable_product_options do

and

resources :ad_hoc_option_types do
  resources :ad_hoc_option_values do

The extension's routes definition creates fully qualified paths combining both inner and outer resource:

admin_product_customization_type_customizable_product_option
admin_ad_hoc_option_type_ad_hoc_option_value 

however the link_to_remove_fields method in core Spree 1.3 doesn't know anything about the outer resource. There may well be a more graceful way of solving this issue, but as I'm new to Ruby and Rails I created a new helper (link_to_remove_nested_fields) method to cope with the nesting and modified the places that this extension calls link_to_remove_fields.

I hope this is useful.

Owner

jsqu99 commented Mar 8, 2013

I really appreciate your contribution. Unfortunately I'm extremely busy ATM but I will definitely review this ASAP.

Contributor

nostopbutton commented Mar 8, 2013

No worries, thanks for replying. I know what it's like.

Thanks for this really useful extension - without it, I wouldn't be able to get Spree to support the level of product flexibility that I require. So it is very much appreciated!

Pete.

Owner

jsqu99 commented Mar 8, 2013

Thank you so much. I'm really glad you are finding it useful.

I have a site I'm hoping to upgrade to 1.3 and it needs this extension. I'm going to try really hard to dig into to it mid-week next week.

Contributor

ccschmitz commented Sep 11, 2013

@jsqu99 any chance of this getting merged into the spree-1-3-stable branch soon?

jsqu99 added a commit that referenced this pull request Sep 11, 2013

Merge pull request #67 from nostopbutton/master
Spree 1.3 - nested resources on admin screens

@jsqu99 jsqu99 merged commit 34f1639 into jsqu99:master Sep 11, 2013

1 check passed

default The Travis build passed
Details
Owner

jsqu99 commented Sep 11, 2013

Sorry everyone for not revisiting this. I know this extension needs some updating and will hopefully happen soon

Contributor

ccschmitz commented Sep 11, 2013

That was fast - thanks @jsqu99!

Owner

jsqu99 commented Sep 11, 2013

np @ccschmitz - if I merge into the 1-3 branch will you please confirm it works well?

Contributor

ccschmitz commented Sep 11, 2013

Yeah, I actually brought this over to my fork and was running my app off of that. Seems to be working fine for me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment