solidus

solidus copied to clipboard

Description Resolves issue: https://github.com/solidusio/solidus/issues/4270

Some tests were modified to give a state with the address parameters as they used a country which will now get the states_required attribute of true, meaning a state must be present on the address.

Checklist:

• [x] I have followed Pull Request guidelines
• [x] I have added a detailed description into each commit message

This change has potential to break codebase's tests if they have not modified the 'country' factory 'states_required' attribute to be the intended behaviour, because the attribute can be true. If this fix was released on a major release, this would be very confusing, as developers might think it's a breaking change causing the failed tests, and not the factory.

Thanks, @RyanofWoods!

What about a warning in the address factory? Something like:

after(:build) do |address, evaluator|
  carmen_country = Carmen::Country.code(adress.country.iso)
  Spree::Deprecation.warn <<~MSG if address.state.nil? && Spree::Config.address_require_state && carmen_country.subregions? && address.country.states_required == false
    You built an inconsistent country......
    whether use a country that doesn't require subregions (e.g. ?) or add a state to the address
  MSG
end

Thanks, @RyanofWoods!

What about a warning in the address factory? Something like:

after(:build) do |address, evaluator|
  carmen_country = Carmen::Country.code(adress.country.iso)
  Spree::Deprecation.warn <<~MSG if address.state.nil? && Spree::Config.address_require_state && carmen_country.subregions? && address.country.states_required == false
    You built an inconsistent country......
    whether use a country that doesn't require subregions (e.g. ?) or add a state to the address
  MSG
end

@waiting-for-dev I don't think this warner is aligned with the scenario that I mentioned. The problem is that if they used the Solidus Country factory logic, states_required was always false, but now it can become true, which could breaking tests for them. If they used a build_strategy of create, then they will probably get an error of State cannot be blank. But it is not guaranteed and they could use other strategies.

I am not really sure there is something we can do, other than perhaps realise it on a minor release and make a note of it in the changelog 🤔

(I think this warning could be annoying if the have custom logic where some countries's state_required is being set to false even though it has subregions)

Hey @RyanofWoods!

I don't think this warner is aligned with the scenario that I mentioned. The problem is that if they used the Solidus Country factory logic, states_required was always false, but now it can become true, which could breaking tests for them. If they used a build_strategy of create, then they will probably get an error of State cannot be blank.

I'm not familiar with this part of the codebase, but I tried to dig a bit, and it seemed to be that the only place where that could happen is on the address model because of a custom validator.

But it is not guaranteed and they could use other strategies.

AFAIK, after(:build) will only run for the build and creation strategies. I thought using it on : build was a good idea, as they may be asserting that the instance is valid. We could be more localized and use a before(:create).

(I think this warning could be annoying if the have custom logic where some countries's state_required is being set to false even though it has subregions)

It seemed that there's no frontend for states_required, and it's only set during the population of seeds. So users should not be messing with it. If they do not require addresses to have a state when the country is states_required, they must have Spree::Config.address_require_state to false, a condition included in the guard.

WDYT? Let's also wait for the opinion of others.

We can we wait for further opinions @waiting-for-dev 🙂

I'd explore if you are making the right assumption in the first place: are we sure that just having subregions is enough to determine if states are required to make an address valid in a given country? That information doesn't seem to live in Carmen (for a good reason, it's just a repository of geographies, doesn't include addresses concerns), but I guess there are places where we have subregions and they are not mandatory for address validity.

That's a good point @kennyadsl 🙂 I don't think it is for the reasons you mentioned. The following issue touches upon this: https://github.com/solidusio/solidus/issues/3842

I can look into this more 👍

are we sure that just having subregions is enough to determine if states are required to make an address valid in a given country?

@kennyadsl, it seems that's the condition we're checking on our seeds:

https://github.com/solidusio/solidus/blob/a6c2ceeb2ee6bff832080ebb11773f7b6e4c2df0/core/db/default/spree/countries.rb#L16

Hey @RyanofWoods, after talking with the core team, @spaghetticode came up with an idea that we think could solve the concern about breaking tests. What about modifying the address factory so that:

• Check whether its country has states_required.
• If so, check whether a state is given.
• If it's not given, create a state for that country and associate with the address.

WDYT?

Hey, @RyanofWoods! :wave: Did you find the time to think about the proposed solution? Do you think it's something that makes sense?

Hey, @RyanofWoods! 👋 Did you find the time to think about the proposed solution? Do you think it's something that makes sense?

Hi @waiting-for-dev 👋 I added some commits implementing the suggested solution. So it was possible, but the address and state factories changed quite a bit and got a bit more complex. WDYT?