Prepare Solidus Multi Domain for Solidus 3.0

[kennyadsl]

Update this extension so that it works with Solidus 3.0 by making sure the specs pass.

[afdev82]

I'd like to help with this one too!

[afdev82]

I'm not going to finish this today (and not in the next days), so I will remove myself from the card.
The specs are passing till 2.11, so I will leave the card where it is now.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[afdev82]

I opened a PR to track the work on that.
I get two deprecation warnings and I'm not sure on the direction to take to fix them:

DEPRECATION WARNING: Spree::Store.current is DEPRECATED
DEPRECATION WARNING: by_url is deprecated and will be removed from Solidus 3.0 (Spree::Store.by_url is DEPRECATED)

I found that this PR is related: solidusio/solidus#1993
In the description I read:

move out of core the logic that checks requests info to determine if another store has to be used. This will be moved to solidus_multi_domain.

But I think it was never done.
In my own app I'm already using a custom Spree::CurrentStoreSelector to select the store based on the given domain.
Should I just copy my custom class and set the preference using an initializer like in my own app?
Or use the code linked in the description?

[afdev82]

I've found a commit that could fix this issue, but it never landed here.

[afdev82]

I think the problem is in the tests that are still using the old (deprecated) by_url and current methods.
What should be the fix here? Just remove the tests? Because they are tests for methods not defined in the extension itself.
I really don't know at this point...
In the meantime I found also other PRs:
solidusio/solidus#2041
solidusio/solidus#2022

So I think it should be clarified the way that I should follow here, because I read that this extension should be deprecated in favor of the solidus core including the same functionality.

[kennyadsl]

I'm not sure all the features of this extension have been ported in core but yes, the idea was to remove from here what is already provided by Solidus and keep the extension only for the rest of the features.

[afdev82]

Hm... yes, but as I said I was confused by the other PR to simplify the store selection: solidusio/solidus#1993

Since the description said

move out of core the logic that checks requests info to determine if another store has to be used. This will be moved to solidus_multi_domain.

I don't know what should be done now to fix these deprecation warnings, just remove the tests?
I don't know how to change them to be able to pass the CI and since the by_url and current methods are removed, I think it's fine to do it.
The selection of the current store was delegated to the store selector classes: https://github.com/solidusio/solidus/tree/master/core/app/models/spree/store_selector, so I don't know if it's needed to provide a store selector here.

[cpfergus1]

I was working on this the other day and ended up just implementing that deprecated functionality in the store decorator:

          scope :by_url, lambda { |url| where("url like ?", "%#{url}%") }

          def self.current(store_key)
            current_store = ::Spree::Store.find_by(code: store_key) || ::Spree::Store.by_url(store_key).first if store_key
            current_store || ::Spree::Store.default
          end

          private

          def store_key
            @request.headers['HTTP_SPREE_STORE'] || @request.env['SERVER_NAME']
          end
        end
      end

This passed the test's through 2.11 with no deprecations, but I then ran into an error when running 3.0:

 
  1) Store promotion rule Can add a store rule to a promotion
     Failure/Error: select2_search store.name, from: "Choose Stores"
     
     Capybara::ElementNotFound:
       Unable to find css ".select2-container" within #<Capybara::Node::Element tag="div" path="/HTML/BODY[1]/DIV[5]/DIV[1]/DIV[1]/DIV[1]/DIV[1]/FIELDSET[1]/FORM[2]/FIELDSET[1]/DIV[2]/DIV[2]/DIV[1]">
     # ./spec/features/admin/promotion_rule_store_spec.rb:18:in `block (2 levels) in <top (required)>'

I have not investigated this error too much yet and was hoping to get out a fix by the end of the week.

[afdev82]

I don't know if it's useful to re-implement deprecated methods that were in the core just to find the current store.
Since there is a legacy class that does the same without the need of the methods.

[kennyadsl]

I agree with @afdev82. We need to update the gem to use the core functionalities instead of re-implementing the deprecated ones.

The selection of the current store was delegated to the store selector classes: https://github.com/solidusio/solidus/tree/master/core/app/models/spree/store_selector, so I don't know if it's needed to provide a store selector here.

The default store selector provided in core already select the current store based on the domain of the request: https://github.com/solidusio/solidus/blob/6c0da5d618a6d04d13ef50ec01ae17c3b06f6259/core/app/models/spree/store_selector/by_server_name.rb. Maybe it's just a matter to use this without re-implementing any new selector?

[afdev82]

I could just remove these lines and see if the tests pass.

[afdev82]

I can confirm the tests fail with Solidus v3 as already said by @cpfergus1

[cpfergus1]

The 401 you encountered is that the tests are using X-Spree-Token which is deprecated instead of Authentication: Bearer <token>
that will help you push past that one!

[afdev82]

Yes, you are right. I found also a PR that was not merged regarding this issue: #106

[afdev82]

Now only the failing test with capybara should be fixed.
@cpfergus1 if you've done already some investigation about that, I will leave it to you. I don't know why it's failing at the moment.

[afdev82]

I think we can forget about the error and remove the feature from this extension.
The feature was already ported to solidus: solidusio/solidus#2552
And I think the extension was breaking that, because in my Store I don't see the select for the stores in the admin interface...

[cpfergus1]

I have not found a solution for it at this time, I am open to it if @kennyadsl feels it is appropriate

[afdev82]

I just removed that feature in my PR.
I think it's fine, since the feature was already ported to Solidus v2.6 (I bumped the min required version to v2.6).
Let's wait @kennyadsl opinion about that.

[kennyadsl]

I merged the PR, thanks @afdev82 and @cpfergus1 !