Check for existence of product_path instead of Spree::Frontend::Engine

[gsmendoza]

Goal

We want to update the Edit Product page to check for existence of
product_path instead of Spree::Frontend::Engine so that the Edit
Product page would work with a frontend generated by
SolidusStarterFrontend.

Background

This PR is part of an ongoing effort to replace solidus_frontend in Solidus with SolidusStarterFrotntend:

eng-293-product-path.mp4

Requirements

Admin can find a link to the product in the Edit Product page if the app has a frontend

Given I am an admin of a Solidus app
And the app has a product page i.e. it has a frontend
When I go to the Edit Product page
Then I should see a link to the product page

Admin won't find a link to the product in the Edit Product page if the app does not have a frontend

Given I am an admin of a Solidus app
And the app has does not have a product page i.e. it does not have a
frontend
When I go to the Edit Product page
Then I should not see a link to the product page

Manual tests

eng-293-update-solidus-backend-productsedit--demo.mp4

2022-03-02 update

eng-293-update-solidus-backend-productsedit--demo-2.mp4

Checklist:

• I have followed Pull Request guidelines
• I have added a detailed description into each commit message
• N/A: I have updated Guides and README accordingly to this change (if needed)
• N/A: I have added tests to cover this change (if needed)
• I have attached screenshots to this PR for visual changes (if needed)

[waiting-for-dev]

Thanks for your PR, @gsmendoza.

I think it's a great idea to replace the check of the engine's class with something else. However, I'm afraid checking for the presence of a concrete route is not a very robust solution. What about configuring that route in the backend and defaulting to product_path?.

[gsmendoza]

What about configuring that route in the backend and defaulting to product_path?.

Do you mean something like this, @waiting-for-dev ?

# backend/lib/spree/backend_configuration.rb

module Spree
  class BackendConfiguration < Preferences::Configuration
    preference :locale, :string, default: I18n.default_locale

    preference :product_path_method, :string, default: :product_path
  end
end

  <% if spree.respond_to?(Spree::Backend::Config[:product_path_method]) %>
    <li id="view_product_link">
      <%= link_to t('spree.view_product'), spree.public_send(Spree::Backend::Config[:product_path_method], @product), id: 'admin_view_product', class: 'btn btn-default ml-2' %>
    </li>
  <% end %>

[waiting-for-dev]

Do you mean something like this, @waiting-for-dev ?

Yes, but maybe we could be more flexible and allow custom user-defined routes (i.e., not within spree namespace). Something like:

preference :product_path_method, :string, default: Rails.application.routes.url_helpers.spree.product_path

[gsmendoza]

@waiting-for-dev Sounds good! I'll give it a try 👍

[gsmendoza]

preference :product_path_method, :string, default: Rails.application.routes.url_helpers.spree.product_path

@waiting-for-dev I'm still researching if this is possible, but this is what I have completed so far:

1. I still couldn't figure out where to call product_path. Based on frontend/config/routes.rb, it seems I could get it from Spree::Core::Engine.routes. However, when I call Spree::Core::Engine.routes.url_helpers.product_path(Spree::Product.first), I get the following error: NoMethodError Exception: undefined method `product_path' for #Module:0x00007f6624027010.
2. The product_path helper has to be called from the spree ActionDispatch::Routing::RoutesProxy object. If we want to avoid any reference to spree, it seems like the preference would need to store the spree.product_path method itself (and not just the "product_path" string method name). I looked at Spree::Preferences::PreferableClassMethods:: DEFAULT_ADMIN_FORM_PREFERENCE_TYPES and it doesn't have a type for a method or a proc object.

One alternative I am thinking is to extract the view_product_link block into a partial, and add a preference for that partial's path. So, we would have a preference like this:

preference :product_link_partial_path, :string, default: 'spree/admin/products/product_link'

And we'd call it from the view like this:

<% if Spree::Backend::Config[:product_link_partial_path] %>
  <%= render partial: Spree::Backend::Config[:product_link_partial_path] %> 
<% end %>

SolidusStarterFrontend-based apps can provide their own product_link partial.

What do you think of this option?

[waiting-for-dev]

Hmm, I see. However, using a partial for that is rather cumbersome. We can wrap in a proc and provide the view context. Something like:

preference :frontend_product_path, :proc, default: proc { ->(template_context) { template_context.spree.product_path } }

Then, from the template:

<%= Spree::Backend::Config[:frontend_product_path].call(self) %>

Ideally, we should narrow the scope of the argument we're giving to the proc (i.e., only providing access to the route helpers), but I don't know if that's possible from the template.

BTW, the default proc needs to be nested because the outer one is used to manage loading different returned values depending on the loaded Solidus version.

[gsmendoza]

@waiting-for-dev Sounds good! I'll give it a try 👍

[gsmendoza]

@waiting-for-dev Appreciate to head your thoughts on this fixup. Prior to this PR, the behavior of the backend product link is as follows:

1. If the app has a frontend component, the backend would display the link. For instance, this is the behavior of the Solidus sandbox app.
2. If the app does not have a frontend component, the backend would NOT display the product link. This is how the backend feature spec suite behaves, since it does not have an integrated frontend.

I've attempted to maintain this behavior in this default value for the frontend_product_path preference. What do you think? On the upside, it doesn't introduce changes to the feature. However, on the downside, it seems to look a bit complicated for a default value.

The good news is that if an app has to override this frontend_product_path, it does not necessarily have to match the behavior of this default value. In particular, for a Starter FE-based app, the app already knows that it has a frontend, so it can skip the respond_to?(:product_path) guard:

# config/initializers/spree.rb
Spree::Backend::Config.configure do |config|
  config.frontend_product_path = ->(template_context, product) { template_context.spree.product_path(product) }
end

If you feel that the default value for frontend_product_path is too complicated, I think the next best alternative is to set the default value to nil, like this:

# backend/lib/spree/backend_configuration.rb
preference :frontend_product_path, :proc, default: nil

<!-- backend/app/views/spree/admin/products/edit.html.erb  ->
  <% if Spree::Backend::Config[:frontend_product_path] %>
    <li id="view_product_link">
      <%= link_to t('spree.view_product'), Spree::Backend::Config[:frontend_product_path].call(self, @product), id: 'admin_view_product', class: 'btn btn-default ml-2' %>
    </li>
  <% end %>

I think this implementation is straightforward and it allows the backend feature spec suite to pass without any changes. The downside is that this is a breaking change: users would have to override this preference if they would like to continue showing the product link on their backend.

What do you think?

[waiting-for-dev]

Hey @gsmendoza, I see your point. I'd stick with the first one. You're right; it's complicated, but it's what it's giving us the behavior we need 🙂 Nothing guarantees us that the method product_path is present, so, yeah, we need to check it!

[gsmendoza]

@waiting-for-dev Cool! I'll go ahead with implementing the first option 👍

[gsmendoza]

@waiting-for-dev I've updated the PR. Appreciate if you can take a look. You can find my updated manual test at https://user-images.githubusercontent.com/61476/156317692-f7cf2e8f-7f8f-40b3-bc49-1024821a04a2.mp4.

[waiting-for-dev]

Thanks, @gsmendoza!! Great job! I just left a non-blocking suggestion 🙂

[waiting-for-dev]

What are your thoughts about moving it to a helper? I think it's too much logic to be in the template.

[gsmendoza]

@waiting-for-dev Do you mean a helper for the frontend_product_path like this?

# backend/app/helpers/spree/admin/products_helper.rb

module Spree
  module Admin
    module ProductsHelper
      def frontend_product_path(product)
        # Not sure about the `self` here, but I think it will work.
        Spree::Backend::Config[:frontend_product_path].call(self, @product) 
      end
    end
  end
end

<%# backend/app/views/spree/admin/products/edit.html.erb %>

  <% if frontend_product_path(@product) %>
    <li id="view_product_link">
      <%= link_to t('spree.view_product'), frontend_product_path(@product), id: 'admin_view_product', class: 'btn btn-default ml-2' %>
    </li>
  <% end %>

[waiting-for-dev]

Yes, I mean that! But I leave it to your discretion 🙂

[gsmendoza]

@waiting-for-dev Cool! I added the helper :)

[waiting-for-dev]

🙌 🙌 thanks, @gsmendoza!!