Add support for v3.1 #6
Conversation
jtapia
force-pushed
the
chore/add-support-v3.1
branch
from
71ec3f6 to
46419cc
Compare
jtapia
force-pushed
the
chore/add-support-v3.1
branch
from
46419cc to
184cb34
Compare
There was a problem hiding this comment.
Thanks @jtapia
Most changes seems good, just have a few doubts and comments
|
|
||
| task default: %i(rubocop extension:specs) | ||
| task default: 'extension:specs' |
There was a problem hiding this comment.
I prefer to keep rubocop in default task, that way is more easier notice when is failing
| @@ -5,6 +5,6 @@ class Locality < Spree::Base | |||
| belongs_to :state, class_name: 'Spree::State' | |||
| has_one :country, through: :state | |||
|
|
|||
| validates :name, presence: true, uniqueness: { scope: :state_id } | |||
| validates :name, presence: true | |||
There was a problem hiding this comment.
Not sure why need to remove this validation? I think we should keep to ensure don't have duplicated cities for state
| @@ -24,7 +24,7 @@ | |||
| locality do |address| | |||
| state = address.state | |||
| Spree::Locality.find_by(name: city_name, state: state) || | |||
| create(:city, name: city_name, state: state) | |||
| create(:locality, name: city_name, state: state) | |||
There was a problem hiding this comment.
Our factory is named :city right now, If we plan to use :locality instead should update the factory definition too.
| spec.version = SolidusZonesByCity::VERSION | ||
| spec.authors = ['César Carruitero'] | ||
| spec.email = 'ccarruitero@protonmail.com' | ||
| Gem::Specification.new do |s| |
There was a problem hiding this comment.
I think is unnecessary rename the block variable
| s.authors = ['César Carruitero'] | ||
| s.email = 'ccarruitero@protonmail.com' |
There was a problem hiding this comment.
This was added when bootstrap the extension, but should be updated to Magma Labs
| spec.license = 'BSD-3-Clause' | ||
| s.summary = 'Allow zones at city level' | ||
| s.description = s.summary | ||
| s.homepage = 'http://github.com/magma-labs/solidus_zones_by_city' |
There was a problem hiding this comment.
I prefer to keep https in urls
| s.add_dependency 'solidus_core', ['>= 2.5', '< 4'] | ||
| s.add_dependency 'solidus_support', '~> 0.5' | ||
|
|
||
| s.add_development_dependency 'autoprefixer-rails', '~> 10.2.5' |
There was a problem hiding this comment.
Until I can see in their repository, is for add browser prefix in our css. I think we should avoid use browser specific css and instead use standard css.
| s.add_dependency 'solidus_support', '~> 0.5' | ||
|
|
||
| s.add_development_dependency 'autoprefixer-rails', '~> 10.2.5' | ||
| s.add_development_dependency 'rspec-activemodel-mocks' |
There was a problem hiding this comment.
Looking into gem repository doesn't found a good reason to add. What is the advance of using this gem over use double and allow/receive for mock and stub?
| @@ -20,8 +18,8 @@ | |||
| # in spec/support/ and its subdirectories. | |||
| Dir["#{__dir__}/support/**/*.rb"].sort.each { |f| require f } | |||
|
|
|||
| # Requires factories defined in lib/solidus_zones_by_city/factories.rb | |||
| require 'solidus_zones_by_city/factories' | |||
| # Requires factories defined in lib/solidus_related_products/testing_support/factories.rb | |||
No description provided.