Fix solidus4 compatibility #172

shahmayur001 · 2025-01-21T13:39:27Z

Summary

This pull request introduces structural changes and enhancements to support the integration of the Solidus starter frontend with multi-domain functionality. It includes updates to controllers, test suite scripts, and test cases to ensure seamless compatibility with the starter frontend.

Key Changes

Structural Changes to Support the Starter Frontend:
- Removed Spree namespacing from controller decorators to align with the starter frontend controllers.
- Ensured compatibility by adapting multi-domain logic to work seamlessly with the new frontend structure.
Add Scripts to Run Test Suites:
- Introduced scripts to automate the creation of the dummy-app required for running test suites.
- Ensured that the testing environment aligns with the starter frontend's requirements.
Fix Test Cases:
- Updated test cases to prevent failures when executed alongside the starter frontend.
- Improved test reliability by addressing issues related to multi-domain configurations and starter frontend dependencies.

ikraamg

Hi @shahmayur001. Thank you so much for modernizing this codebase. It was very outdated!

I just left one requested change to modernize the search override.

An additional thought is that since the new solidus starter frontend functions by copying over all the controller and views into the app that is utilizing the gem,
I would recommend we find a solution to have the overrides you have created here also be copied be over to the app.

My rough idea is to leave them as decorators but instead convert them to templates that are copied over when running the install generator. In this way we can still test the functionality here via the spec generated app whilst handing the code over to the main application that uses this gem.

Let me know what you think!

app/models/spree/search/multi_domain.rb

shahmayur001 · 2025-01-22T10:55:15Z

Hi @shahmayur001. Thank you so much for modernizing this codebase. It was very outdated!

I just left one requested change to modernize the search override.

An additional thought is that since the new solidus starter frontend functions by copying over all the controller and views into the app that is utilizing the gem, I would recommend we find a solution to have the overrides you have created here also be copied be over to the app.

My rough idea is to leave them as decorators but instead convert them to templates that are copied over when running the install generator. In this way we can still test the functionality here via the spec generated app whilst handing the code over to the main application that uses this gem.

Let me know what you think!

@ikraamg Thanks for the feedback and the suggestion!

I believe the starter frontend is developed differently, as you mentioned. It seems to be more of a template where files are copied into the main application. If we were to copy additional files, don’t you think it could lead to a lot of unnecessary clutter in the main application?

I noticed that other extensions are also handling things in a similar way to what we’ve done with this current extension.

Let me know your thoughts!

ikraamg · 2025-01-22T11:21:57Z

I noticed that other extensions are also handling things in a similar way to what we’ve done with this current extension.

If this is the case with how other extensions deal with the new starter frontend, then that is okay 👍
It feels like this goes against the ethos of the template-like approach to the new starter but I won't position myself against it just for this work.

If we were to copy additional files, don’t you think it could lead to a lot of unnecessary clutter in the main application?

That is precisely what my vision was. For it to be an "in-your-face" installation so you can integrate the changes into your (assumably) already customized controllers/views from the starter 😄

shahmayur001 · 2025-01-22T12:24:37Z

That is precisely what my vision was. For it to be an "in-your-face" installation so you can integrate the changes into your (assumably) already customized controllers/views from the starter 😄

Maybe in that case, we can introduce a new generator command or propose the question later on to users, giving them the option to include it in the main application. That way, it becomes a choice

shahmayur001 · 2025-01-27T09:32:46Z

@kennyadsl You were right about this change, it is affecting the PR as well

solidusio/solidus#6062

shahmayur001 · 2025-01-28T14:18:15Z

@kennyadsl You were right about this change, it is affecting the PR as well

solidusio/solidus#6062

@kennyadsl I’ve fixed the issue. Feel free to review it.

fthobe · 2025-01-30T11:56:23Z

@ikraamg Can you review the changes?

ikraamg · 2025-01-30T15:15:25Z

@fthobe Sure. The PR will still need a write-access reviewer before merging but my review can add to the confidence of that final review.

fthobe · 2025-01-30T15:16:42Z

@ikraamg so you are just a poser having us do your work 😂

ikraamg

This looks great!

The comments are not related to functional blockers but general improvements to further modernize this codebase and also reduce the amount of decorators/overrides.

ikraamg · 2025-01-30T16:36:58Z

app/controllers/spree/admin/stores_controller.rb

This controller turns out to be mostly a duplicate as it was probably implemented before solidus_backend had it own stores controller/UI. It can pretty much be ripped out a long with its permission sets etc. The view looks slightly different but does not seem to be missing anything essential aside from perhaps the @payment_methods and @shipping_methods

ikraamg · 2025-01-30T16:45:30Z

app/decorators/controllers/solidus_multi_domain/home_controller_decorator.rb

+    def index
+      @searcher = build_searcher(params)
+      @products = @searcher.retrieve_products
+      @taxonomies = get_taxonomies


I believe the only change to this is get_taxonomies to store the @taxons.
It would be more ideal to not override this file but instead override: https://github.com/solidusio/solidus_starter_frontend/blob/main/templates/app/controllers/concerns/taxonomies.rb

which is made available in the store controller (parent of this one): https://github.com/solidusio/solidus_starter_frontend/blob/652caa6355253c969248fb66f1703bfe9879e927/templates/app/controllers/store_controller.rb#L6

Right, there's an helper method called taxonomies now, which should return the same thing. If we want to keep backward compatibility we can still set the instance variables in a before_action. By the way, why are we overwriting this controller? It doesn't seem like it's doing anything around multi stores.

ikraamg · 2025-01-30T16:46:11Z

app/decorators/controllers/solidus_multi_domain/products_controller_decorator.rb

+                  display_includes.
+                  with_prices(current_pricing_options).
+                  includes([:option_values, :images])
+      @taxonomies = get_taxonomies


As before, it would be more ideal to not override this file but instead override: https://github.com/solidusio/solidus_starter_frontend/blob/main/templates/app/controllers/concerns/taxonomies.rb

which is made available in the store controller (parent of this one) already: https://github.com/solidusio/solidus_starter_frontend/blob/652caa6355253c969248fb66f1703bfe9879e927/templates/app/controllers/store_controller.rb#L6

ikraamg · 2025-01-30T16:48:38Z

app/decorators/controllers/solidus_multi_domain/products_controller_decorator.rb

+      @taxonomies = get_taxonomies
+      @product_properties = @product.product_properties.includes(:property)
+      @taxon = Spree::Taxon.find(params[:taxon_id]) if params[:taxon_id]
+      @similar_products = @product.similar_products.select { |product| product.stores.include?(current_store) }


In this case, it would be better to override the similar products method in the product model. Making both changes would mean that this override is not needed AFAIK.

Agree, that's the place! @shahmayur001 what do you think about changing similar_products instead?

ikraamg · 2025-01-30T17:06:57Z

app/decorators/controllers/solidus_multi_domain/taxons_controller_decorator.rb

+module SolidusMultiDomain
+  module TaxonsControllerDecorator
+    def show
+      @taxon = ::Spree::Taxon.find_by_store_id_and_permalink!(current_store.id, params[:id])


This change can be implemented on the load_taxon method to avoid overriding the show method.

ikraamg · 2025-01-30T17:08:34Z

app/decorators/controllers/solidus_multi_domain/taxons_controller_decorator.rb

+      return unless @taxon
+      @searcher = build_searcher(params.merge(taxon: @taxon.id))
+      @products = @searcher.retrieve_products
+      @taxonomies = get_taxonomies


This would be solved in the same way as the previous comments regarding taxonomy loading.

ikraamg · 2025-01-30T17:13:29Z

app/decorators/helpers/solidus_multi_domain/spree/products_helper_decorator.rb

This file can be removed as the get_taxons method was removed: solidusio/solidus@fc491a7

ikraamg · 2025-01-30T17:19:42Z

lib/solidus_multi_domain/multi_domain_helpers.rb

+      @taxonomies ||= if current_store.present?
+                        ::Spree::Taxonomy.where(["store_id = ?",
+                                                 current_store.id])
+                      else
+                        ::Spree::Taxonomy
+                      end
      @taxonomies = @taxonomies.includes(root: :children)
      @taxonomies
    end


I think this is the method that is available in the new starter frontend through the taxon helper so not required here anymore.

fthobe · 2025-01-30T17:50:16Z

hey @ikraamg
thank you very much for your dettailed feedback.

We appreciate the work you put in the comments, but as far as I understood sometimes opinions here vary on what should be removed and what not.

Is the goal here a major release or having the app restored to a working state?
If we rip stuff out we might break things for users of previous versions of solidus and might require a major release which is something very difficult to obtain on solidus as you might see from the last version bump (from I believe 10 years ago).

I am happy to dedicate mayur to do that, but I would like a defined outcome giving that we delegating our company ressources to open source beyond a scope that provides what we need.

Are going for a major release here that will be maintained by somebody or making making cosmetics for the main repo?

ikraamg · 2025-01-30T21:28:40Z

@fthobe I don't have anything against merging this as is since it contains everything to get this working again.

I can perhaps find some time in the upcoming weeks to make the remaining improvements in a seperate PR.

fthobe · 2025-01-30T22:34:36Z

@ikraamg that would be absolutely great!

kennyadsl

I agree about all of the @ikraamg improvement proposals, but yes, most of them were already there before, and might be out of scope for this PR. Still, it would be great to leave the extension in a better place and remove some technical debt if anyone has will to do it.

kennyadsl · 2025-01-31T07:31:24Z

app/decorators/controllers/solidus_multi_domain/home_controller_decorator.rb

+    def index
+      @searcher = build_searcher(params)
+      @products = @searcher.retrieve_products
+      @taxonomies = get_taxonomies


Right, there's an helper method called taxonomies now, which should return the same thing. If we want to keep backward compatibility we can still set the instance variables in a before_action. By the way, why are we overwriting this controller? It doesn't seem like it's doing anything around multi stores.

kennyadsl · 2025-01-31T07:32:15Z

app/decorators/components/solidus_multi_domain/taxons_tree_component_decorator.rb

+        def tree(root_taxon:, item_classes:, current_item_classes:, max_level:)
+          return if max_level < 1 || root_taxon.children.empty?
+
+          filtered_taxons = root_taxon.children.joins(:taxonomy).where("spree_taxonomies.store_id = ?",


is this the only change made in this class?

kennyadsl · 2025-01-31T07:33:55Z

app/decorators/controllers/solidus_multi_domain/products_controller_decorator.rb

+      @taxonomies = get_taxonomies
+      @product_properties = @product.product_properties.includes(:property)
+      @taxon = Spree::Taxon.find(params[:taxon_id]) if params[:taxon_id]
+      @similar_products = @product.similar_products.select { |product| product.stores.include?(current_store) }


Agree, that's the place! @shahmayur001 what do you think about changing similar_products instead?

fthobe · 2025-01-31T08:02:10Z

@kennyadsl we can do that in a second run by end of march. We are currently giving priority to a Running System.

ikraamg reviewed Jan 21, 2025

View reviewed changes

app/models/spree/search/multi_domain.rb Outdated Show resolved Hide resolved

shahmayur001 force-pushed the fix-solidus4-compatibility branch from dcb4cdf to e40485a Compare January 27, 2025 08:42

shahmayur001 force-pushed the fix-solidus4-compatibility branch from 11daf38 to aac1289 Compare January 28, 2025 12:03

Update gem to support the latest Solidus version

bb655e7

shahmayur001 force-pushed the fix-solidus4-compatibility branch from aac1289 to bb655e7 Compare January 28, 2025 13:58

ikraamg approved these changes Jan 30, 2025

View reviewed changes

kennyadsl approved these changes Jan 31, 2025

View reviewed changes

kennyadsl merged commit b174409 into solidusio-contrib:main Jan 31, 2025
6 checks passed

fthobe mentioned this pull request Jan 31, 2025

Remove deprecated / obsolete code from multi-domain #174

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix solidus4 compatibility #172

Fix solidus4 compatibility #172

shahmayur001 commented Jan 21, 2025

ikraamg left a comment •

edited

Loading

shahmayur001 commented Jan 22, 2025

ikraamg commented Jan 22, 2025

shahmayur001 commented Jan 22, 2025

shahmayur001 commented Jan 27, 2025

shahmayur001 commented Jan 28, 2025

fthobe commented Jan 30, 2025

ikraamg commented Jan 30, 2025 •

edited

Loading

fthobe commented Jan 30, 2025

ikraamg left a comment

ikraamg Jan 30, 2025

ikraamg Jan 30, 2025

kennyadsl Jan 31, 2025

ikraamg Jan 30, 2025

ikraamg Jan 30, 2025

kennyadsl Jan 31, 2025

ikraamg Jan 30, 2025

ikraamg Jan 30, 2025

ikraamg Jan 30, 2025

ikraamg Jan 30, 2025 •

edited

Loading

fthobe commented Jan 30, 2025

ikraamg commented Jan 30, 2025

fthobe commented Jan 30, 2025

kennyadsl left a comment

kennyadsl Jan 31, 2025

kennyadsl Jan 31, 2025

kennyadsl Jan 31, 2025

fthobe commented Jan 31, 2025

Fix solidus4 compatibility #172

Fix solidus4 compatibility #172

Conversation

shahmayur001 commented Jan 21, 2025

Summary

Key Changes

ikraamg left a comment • edited Loading

Choose a reason for hiding this comment

shahmayur001 commented Jan 22, 2025

ikraamg commented Jan 22, 2025

shahmayur001 commented Jan 22, 2025

shahmayur001 commented Jan 27, 2025

shahmayur001 commented Jan 28, 2025

fthobe commented Jan 30, 2025

ikraamg commented Jan 30, 2025 • edited Loading

fthobe commented Jan 30, 2025

ikraamg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikraamg Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

fthobe commented Jan 30, 2025

ikraamg commented Jan 30, 2025

fthobe commented Jan 30, 2025

kennyadsl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fthobe commented Jan 31, 2025

ikraamg left a comment •

edited

Loading

ikraamg commented Jan 30, 2025 •

edited

Loading

ikraamg Jan 30, 2025 •

edited

Loading