solidusio-contrib / solidus_active_shipping

:package: Solidus integration for Shopify's active_shipping gem.
8 stars 22 forks source link

stock_location_decorator breaks base seeding scripts #59

Open tblanchard opened 6 years ago

tblanchard commented 6 years ago

Solidus seed scripts create a default stock_location with just a name 'default'. It is now no longer possible to initialize a new solidus instance with this gem bundled because of these validation rules.

This has caused me way more grief than having a stock location without address info would.

forkata commented 6 years ago

Hi @tblanchard, thanks for reporting this issue. Indeed the base seed data does not contain address information and I can see how the validations added by this extension would cause errors. Unfortunately that information is required for building the origin information for shipments.

There are two options I see here, one would be to remove the validations and raise an exception during runtime if a shipment originates from a stock location without address information. This is not ideal because an admin can edit a stock location and remove the necessary address information which would cause getting rates for shipments to start failing for users. The second option would be to make a change to the seed data to add a dummy address, however that doesn't seem correct either.

I will investigate if there is a way for an extension to override the default seeds in order to avoid issues like this.

tblanchard commented 6 years ago

I worked around it with a decorator containing the following:

  # remove validation rules introduced by active_shipping

  %i[address1 city zipcode country_id].each do | ea |
    _validators[ea].find do |v|
      v.is_a? ActiveRecord::Validations::PresenceValidator
    end.attributes.delete(ea)
  end

  def state_id_or_state_name_is_present
    true
  end

but it is rather inconvenient.

Another worthwhile question is to ask why seeding even bothers to create a default stock_location. It is useless and I end up deleting it and making a new one anyhow, but this is perhaps an issue for the core developers.

jchapin commented 6 years ago

I ran into this issue today when my pre-push hook alerted me to a test failure with:

ActiveRecord::RecordInvalid: Validation failed: Street Address can't be blank, City can't be blank, Zip can't be blank, Country can't be blank, State name can't be blank

Another possible solution:

Since the core seed task was part of our current CI / automated deployment I opted to pull the content following out of db/seeds.rb

Spree::Core::Engine.load_seed if defined?(Spree::Core)

and replace it with the content of Solidus' core db seeds file directly.

https://github.com/solidusio/solidus/blob/master/core/db/seeds.rb

I then created a default/spree folder in the project's db directory and filled it with the default seed files from the gem, but I replaced the content in db/default/spree/stock_locations.rb with the information from my client's warehouses. That allowed me to keep the validation rules, setup the store, and not change my continuous integration or continuous deployment plans at the moment.

I might minimally modify some of those copied seed files, though I would rather have left the seeding of the database alone and modified the Store configuration with other purpose built Rake tasks.

It would be nice if the extension could somehow nudge / warn the administrators of the Solidus install that their stock locations need addresses, otherwise shipping calculation won't be possible.