Allow overriding the routes proxy in the ResourceController

[mamhoff]

Summary

Within Solidus, we always want to use the spree routes proxy as that is where all routes are defined. However, many people use the Resource controller as a parent controller in their main app, and because of this they need to override all the URL generation methods in the Resource Controller.

This allows one to specify the routes proxy in a custom controller as follows:

module MyApp
  class WidgetsController < Spree::Admin::ResourceController
  private

  def routes_proxy
    main_app
  end
end

Engine Developers can use this for engines, as well:

module MyEngine
  class WidgetsController < Spree::Admin::ResourceController
  private

  def routes_proxy
    my_engine
  end
end

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

• I have written a thorough PR description.
• I have kept my commits small and atomic.
• I have used clear, explanatory commit messages.

The following are not always needed:

• 📖 I have updated the README to account for my changes.
• 📑 I have documented new code with YARD.
• 🛣️ I have opened a PR to update the guides.
• ✅ I have added automated tests to cover my changes.
• 📸 I have attached screenshots to demo visual changes.

[codecov]

Codecov Report

Merging #5219 (3957b5e) into main (2f59fa0) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #5219   +/-   ##
=======================================
  Coverage   88.67%   88.67%           
=======================================
  Files         564      564           
  Lines       13893    13895    +2     
=======================================
+ Hits        12319    12321    +2     
  Misses       1574     1574           

Impacted Files	Coverage Δ
...app/controllers/spree/admin/resource_controller.rb	98.80% <100.00%> (+0.01%)	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[kennyadsl]

Thanks @mamhoff, excellent work as always. I left some initial questions.

[kennyadsl]

What do you think about duplicating these specs so that we also have some for Spree, instead of replacing them?

[mamhoff]

I think the spree routes proxy is implicitly tested in all of Solidus' admin controllers, so I believe that's not really necessary. If you insist though I can go that route.

[waiting-for-dev]

Thanks, @mamhoff! That will be helpful! I am only concerned about the chosen test strategy, where we're modifying the Dummy app routes mounting an engine that it's later removed. From Solidus's point of view, this is more of a refactor, so I don't think there's the need to change anything. At most, we can just test the new routes_proxy public method.

[mamhoff]

I am only concerned about the chosen test strategy, where we're modifying the Dummy app routes mounting an engine that it's later removed. From Solidus's point of view, this is more of a refactor, so I don't think there's the need to change anything. At most, we can just test the new routes_proxy public method.

Hm, I did develop the feature using those tests. If we aren't concerned about "how the sausage was made", I'll happily drop the spec change. Y'all decide. :)

[kennyadsl]

@mamhoff We discussed internally and we think we can drop those specs changes. The setup complexity/impact ratio is probably not worth it. As @waiting-for-dev suggested, probably we can just unit test the new method.

On a side note, I checked existing extensions and I found some occurrences of using the AdminResource controller without the need for this change (e.g. https://github.com/solidusio-contrib/solidus_content/blob/da2de159fa3c14c139f9f2aedff80636a5398ff8/app/controllers/solidus_content/resource_controller.rb#L4). Do you know why it's not needed there? Maybe it's just because of this?

[mamhoff]

On a side note, I checked existing extensions and I found some occurrences of using the AdminResource controller without the need for this change (e.g. https://github.com/solidusio-contrib/solidus_content/blob/da2de159fa3c14c139f9f2aedff80636a5398ff8/app/controllers/solidus_content/resource_controller.rb#L4). Do you know why it's not needed there? Maybe it's just because of this?

In most Solidus extensions, we create routes within the Spree engine. I have one that isolates the namespace to SolidusFriendlyPromotions, and there it just won't work. Besides, in that example, all the relevant methods (new_object_url, edit_object_url etc.) are overridden, thus sending to whatever the local resource proxy is in that controller, rather than directly addressing a resource proxy.

[kennyadsl]

Besides, in that example, all the relevant methods (new_object_url, edit_object_url etc.) are overridden, thus sending to whatever the local resource proxy is in that controller, rather than directly addressing a resource proxy.

Yep, I saw that but reading the git history, I think it's for another reason, more specific to that extension's purpose and would happen anyway.

In most Solidus extensions, we create routes within the Spree engine. I have one that isolates the namespace to SolidusFriendlyPromotions, and there it just won't work.

Isolating the engine is legit in the Rails world, even if maybe not a common practice in the Soldius' extensions ecosystem. I think it's more than legit for us to support this use case. Thanks for adding some context. 👍

[waiting-for-dev]

Thanks, @mamhoff!