Cleanup - Specify Spree:: namespace in all Product calls

[fylooi]

Some calls to Product are not namespaced with Spree::. This leads to a collision if the including application has an existing Product model defined.

[flyfy1]

I don't quite understand.. if we have code like:

class Product
  def m
    puts "Product::m"
  end
end


module Spree
  class Product
    def m
      puts "Spree::Product::m"
    end
  end

  class Caller
    def call_product
      Product.new.m
    end
  end
end

Spree::Caller.new.call_product

shouldn't the Spree::Product to be called by the Caller by default?

[fylooi]

@flyfy1
Yes, that would be the case if the constants were all in one file.
In this case, it calls my preexisting Product model. Suspect Rails autoloading might have something to do with it. Specifying the namespace results in correct behaviour and aligns with the rest of the code.

[cbrunsdon]

I'm with @flyfy1 on this one in that I'm not sure under what circumstances the wrong product will be called, could it be something like code you've copy-pasted into a decorator that is causing the issue? I tried a local repo and couldn't recreate the undesired behavior.

[fylooi]

Hmm. Moved past that code and can't dig up the point of reproduction. I'll reopen this PR if I can get a handle on it.

[flyfy1]

@fylooi When I was reading Rails Docs upon module loading today, at section 6.2, it mentioned sth below (cannot copy with style so I just include the screenshot):

I think it's something related to your issue (and which also means, seems to make ur pull request valid)

[fylooi]

@flyfy1
The problem is actually a combination of Rails autoloading and development code reloading. Have experienced a couple of other model name conflicts but still unable to reproduce this in a test app. Disabling Spring and setting config.eager_load = true in development appears to prevent these conflicts, with a slightly degraded dev experience.

Ensuring that all models in controllers are consistently invoked with the Spree:: namespace should solve this. The current code is a mix of namespaced and non-namespaced model calls.

If you guys are okay with this, I can do the changes and update this PR.

[flyfy1]

@fylooi I haven't looked through the documentation yet.. nor do I sure if u should continue with the PR. probably need comments from the core team. Meanwhile, I think it's definitely helpful to create a sample App and surface the problem..

[tvdeyen]

I'm ok with adding the model module namespace.

It has no downsides for us having them (besides bytecount ^_^).

I think it even makes the code better understandable. Not everybody is super aware of Ruby/Rails module constant lookup. And if this helps avoiding super hard to track down bugs, even better.

From me 👍

[tvdeyen]

Here we have one Product missing its module namespace

[fylooi]

Updated the PR to cover all model invocations in controllers and models.

[mamhoff]

I'm also fine with this. Quoting the Zen of Python:

Namespaces are one honking great idea -- let's do more of those!

;)

[cbrunsdon]

Good by me, thanks for seeing this through

[jhawthorn]

Thank you

[fylooi]

Managed to reproduce one case. Documenting the steps for reference.

This is as of v1.4.

Assuming you have a Country model defined in your host application:

1. Add a reference to Country in an around_action in ApplicationController. yield after calling a method on Country
2. Visit CustomerDetailsController#edit

      def edit
          country_id = Country.default.id
          @order.build_bill_address(country_id: country_id) if @order.bill_address.nil?
          @order.build_ship_address(country_id: country_id) if @order.ship_address.nil?

          @order.bill_address.country_id = country_id if @order.bill_address.country.nil?
          @order.ship_address.country_id = country_id if @order.ship_address.country.nil?
        end

The Country.default.id call will call the host Country class, and will fail if the host class doesn't have default defined.