solidusio / solidus

🛒 Solidus, the open-source eCommerce framework for industry trailblazers.
https://solidus.io
Other
4.97k stars 1.29k forks source link

(doc/guides) Unclear how to monkey patch e.g. Address to not require_phone? #3651

Closed fwolfst closed 2 years ago

fwolfst commented 4 years ago

Is your feature request related to a problem? Please describe. I want Adresses not to require the phone number (besides other things) and was happy to find kind of official documentation about it immediately: https://guides.solidus.io/developers/users/addresses.html

However, it is unclear to me where to put this code such that it gets loaded at the right time (probably related to Zeitwerk? just guessing). I spew it around at lib, app/lib, config/initializers/solidus_overrides.rb, config/initializers/spree.rb, to no avail. I still see the star in the billing adress form indicating that the phone number input is a required field and in rails c a Spree::Address responds with true to require_phone?.

Describe the solution you'd like The guides could tell me a preferred (and working ;) ) location for the code.

Once somebody provided me with the insights, I can draft a PR for the guides.

aldesantis commented 4 years ago

Hey @fwolfst, sorry about the confusion! The best place to put that code is in a decorator such as the following:

# app/decorators/your_app/spree/address/make_phone_optional.rb
module YourApp::Spree::Address::MakePhoneOptional
  def require_phone?
    false
  end

  Spree::Address.prepend self
end

(Just to be clear, the exact name of the decorator doesn't really matter as long as the namespace and module name match the file's path, in order to play nice with Zeitwerk. The name I'm using here is just the convention we adopt at Nebulab and in extensions.)

The current guides are a bit outdated. In fact, we're working on a new version of the guides that explains these concepts much better, so there's no need to update the existing ones imo.

fwolfst commented 4 years ago

Thanks @aldesantis , unfortunately that one doesnt work either.

aldesantis commented 4 years ago

@fwolfst can you clarify? What do you see when you open a console and type Spree::Address.ancestors?

fwolfst commented 4 years ago

To be clear what else I tried:

# app/models/spree/address/optional_phone.rb
module Spree::Address::OptionalPhone
  def require_phone?
    false
  end

  Spree::Address.prepend self
end
$ pkill -f spring # just be sure
$ rails c
# ...
2.7.1 :001 > Spree::User.first.addresses.new.require_phone?
   (0.4ms)  SELECT sqlite_version(*)
  Spree::User Load (1.0ms)  SELECT "spree_users".* FROM "spree_users" WHERE "spree_users"."deleted_at" IS NULL ORDER BY "spree_users"."id" ASC LIMIT ?  [["LIMIT", 1]]
 => true 
fwolfst commented 4 years ago

@fwolfst can you clarify? What do you see when you open a console and type Spree::Address.ancestors?

[Spree::Address (call 'Spree::Address.connection' to establish a connection), Spree::Address::GeneratedAssociationMethods, Spree::Address::GeneratedAttributeMethods, Spree::Base(abstract), Spree::RansackableAttributes, Spree::Preferences::Preferable, Kaminari::ConfigurationMethods, Kaminari::ActiveRecordModelExtension, Spree::Base::GeneratedAssociationMethods, Spree::Base::GeneratedAttributeMethods, ActiveRecord::Base, ActionText::Attribute, ActiveStorage::Reflection::ActiveRecordExtensions, ActiveStorage::Attached::Model, Paperclip::Schema, Paperclip::Validators::HelperMethods, Paperclip::Validators, Paperclip::Callbacks::Running, Paperclip::Callbacks, Paperclip::Glue, GlobalID::Identification, Spree::Core::Permalinks, Kaminari::ActiveRecordExtension, FriendlyId::UnfriendlyUtils, CanCan::ModelAdditions, ActiveRecord::Suppressor, ActiveRecord::SecureToken, ActiveRecord::Store, ActiveRecord::Serialization, ActiveModel::Serializers::JSON, ActiveModel::Serialization, ActiveRecord::Reflection, ActiveRecord::NoTouching, ActiveRecord::TouchLater, ActiveRecord::Transactions, ActiveRecord::NestedAttributes, ActiveRecord::AutosaveAssociation, ActiveModel::SecurePassword, ActiveRecord::Associations, ActiveRecord::Timestamp, ActiveRecord::Callbacks, ActiveRecord::AttributeMethods::Serialization, ActiveRecord::AttributeMethods::Dirty, ActiveModel::Dirty, ActiveRecord::AttributeMethods::TimeZoneConversion, ActiveRecord::AttributeMethods::PrimaryKey, ActiveRecord::AttributeMethods::Query, ActiveRecord::AttributeMethods::BeforeTypeCast, ActiveRecord::AttributeMethods::Write, ActiveRecord::AttributeMethods::Read, ActiveRecord::Base::GeneratedAssociationMethods, ActiveRecord::Base::GeneratedAttributeMethods, ActiveRecord::AttributeMethods, ActiveModel::AttributeMethods, ActiveModel::Validations::Callbacks, ActiveRecord::DefineCallbacks, ActiveRecord::Locking::Pessimistic, ActiveRecord::Locking::Optimistic, ActiveRecord::AttributeDecorators, ActiveRecord::Attributes, ActiveRecord::CounterCache, ActiveRecord::Validations, ActiveModel::Validations::HelperMethods, ActiveSupport::Callbacks, ActiveModel::Validations, ActiveRecord::Integration, ActiveModel::Conversion, ActiveRecord::AttributeAssignment, ActiveModel::AttributeAssignment, ActiveModel::ForbiddenAttributesProtection, ActiveRecord::Sanitization, ActiveRecord::Scoping::Named, ActiveRecord::Scoping::Default, ActiveRecord::Scoping, ActiveRecord::Inheritance, ActiveRecord::ModelSchema, ActiveRecord::ReadonlyAttributes, ActiveRecord::Persistence, ActiveRecord::Core, ActiveSupport::Dependencies::ZeitwerkIntegration::RequireDependency, ActiveSupport::ToJsonWithActiveSupportEncoder, Object, PP::ObjectMixin, FriendlyId::ObjectUtils, JSON::Ext::Generator::GeneratorMethods::Object, ActiveSupport::Tryable, ActiveSupport::Dependencies::Loadable, Kernel, BasicObject]

fwolfst commented 4 years ago

Probably I have a typo in a file name or something, but then I probably have to go to an optician.

If I execute Spree::Address.prepend Soshop::Spree::Address::MakePhoneOptional in a Rails console, afterwards .ancestors and require_phone? behave as wished.

fwolfst commented 4 years ago

What I understand:

Now, I did find a way to modify Spree::Address, as follows:

# app/models/spree/address/optional_phone.rb
module Spree::Address::OptionalPhone
  def require_phone?
    false
  end

  # (does not work)
  # Spree::Address.prepend self
end

and

# app/overrides/optional_phone_for_address.rb
module OptionalPhoneForAddress
  Spree::Address.prepend Spree::Address::OptionalPhone
end

Afterwards, I see desired change in the frontend and rails r "p Spree::User.first.addresses.new.require_phone?" prints false. But I doubt that this is something that should go into the documentation or which I would still understand in a couple of months. I'll take a look a the edgeguides to read up about overrides.

softr8 commented 4 years ago

Can you verify you're loading your decorators?

Solidus injects something like this to application.rb:

    initializer 'spree.decorators' do |app|
      config.to_prepare do
        Dir.glob(Rails.root.join('app/**/*_decorator*.rb')) do |path|
          require_dependency(path)
        end
      end
    end

You need to tell rails to load decorators folder as well:

    config.to_prepare do
      Dir.glob(File.join(File.dirname(__FILE__), '../app/**/decorators/**/*.rb')) do |klass|
        require_dependency(klass)
      end
    end
fwolfst commented 4 years ago

You need to tell rails to load decorators folder as well:

    config.to_prepare do
      Dir.glob(File.join(File.dirname(__FILE__), '../app/**/decorators/**/*.rb')) do |klass|
        require_dependency(klass)
      end
    end

I added the snippet in the beginning of the config block in config/environments/development.rb - no change.

Then I picked up that it probably has to be '../../app/**#....' - that worked!

I only then saw that you referenced config/application.rb, which looks like the following already:

 12     initializer 'spree.decorators' do |app|                                      
 13       config.to_prepare do                                                       
 14         Dir.glob(Rails.root.join('app/**/*_decorator*.rb')) do |path|            
 15           require_dependency(path)                                               
 16         end                                                                      
 17       end                                                                        
 18     end   

Now what this tells us is ... the original advice by @aldesantis was great, and would have worked without modifications if the file name (and the module name) would have been *_decorator.rb / ...Decorator . :)

I am sorry - this is most likely mentioned in the guides somewhere (pretty sure I've seen this), but it should be made explicit in all code examples throughout the guides, imho.

As I had no time to read through the edgeguides, I leave it up to you if this issue can be closed.

Thanks a lot @aldesantis and @softr8 , that was a timely and professional response.

seki93 commented 3 years ago

@aldesantis @softr8

Hi, I need help with this. I want to override the logo method from Spree::BaseHelper and I added the category_tree method.

I added a folder under the app called decorators then the folder with app_name then spree and finally my class with an overridden method.

So path is app/decorators/app_name/spree/my_class.rb

Inside that class now I have 2 methods, logo ( should override logo method from Spree::BaseHelper) and category_tree.

In _header.html.erb I add

and that gave me this error: ActionView::Template::Error (Nil location provided. Can't build URI.):

In application file I added:

config.to_prepare do Dir.glob(File.join(File.dirname(FILE), '../app//decorators//*.rb')) do |klass| require_dependency(klass) end end

Can someone point me to the next step or what did I do wrong? Thanks.

aldesantis commented 3 years ago

@seki93 can you please direct your support request to the Solidus Slack? GitHub is only for reporting actual bugs.

jarednorman commented 3 years ago

Sounds like the desired outcome of the original discussion here might be that this guides page might need to be updated to be clearer about naming convention requirements. Is that fair?

elia commented 2 years ago

This seems to be solved, and the overrides section in the new guides has updated instructions https://edgeguides.solidus.io/customization/customizing-the-core#using-overrides.

@jarednorman @kennyadsl you think we can close this?

kennyadsl commented 2 years ago

Yes, going to move it to Troubleshooting to keep track of it.