Fix Country factory states_required attribute #4272

RyanofWoods · 2022-02-15T13:28:49Z

Description
Resolves issue: #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:

I have followed Pull Request guidelines
I have added a detailed description into each commit message

RyanofWoods · 2022-02-15T16:32:29Z

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.

waiting-for-dev · 2022-02-16T04:16:20Z

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

RyanofWoods · 2022-02-16T17:29:35Z

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)

waiting-for-dev · 2022-02-17T04:32:32Z

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.

RyanofWoods · 2022-02-18T09:05:34Z

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

kennyadsl · 2022-02-21T16:25:59Z

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.

RyanofWoods · 2022-02-21T20:14:43Z

That's a good point @kennyadsl 🙂 I don't think it is for the reasons you mentioned. The following issue touches upon this:
#3842

I can look into this more 👍

waiting-for-dev · 2022-02-22T03:44:12Z

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:

solidus/core/db/default/spree/countries.rb

Line 16 in a6c2cee

states_required = connection.quote country.subregions?

waiting-for-dev · 2022-03-09T17:08:16Z

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?

waiting-for-dev · 2022-06-07T10:01:26Z

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

Resolves issue: solidusio#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.

Improves the readability of the next commit.

For some countries such as Italy, its states are nested within regions, and not at the first level of subregions like 'US'. `country > region > states` `Carmen::Country.coded('IT').subregions.first.type` Now nested subregions exist, it will use this instead. This will be useful for when using the state factory, but also necessary for the upcoming commit, where if a state is not given, but a country is, for the address factory, it can automatically use/create the first state of that country.

Two rationales for this change: 1. Now that the states_required attribute is now functioning in the country factory, some specs for codebases might start breaking, due to them using an address factory, not giving a state, but using a country that requires a state. This should prevent many breaking tests * 2. Less setup is now needed for specs. Only a state or country is needed for a valid non-US (default) address * But not all, for example address API endpoint specs might break, which happened for Solidus: cf84832

RyanofWoods · 2022-07-07T13:06:32Z

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?

core/lib/spree/testing_support/factories/state_factory.rb

waiting-for-dev

Thanks, @RyanofWoods. I think now the logic is better covered. However, I'd like another review by someone with more experience than me dealing with this part of Solidus. cc @solidusio/core-team

kennyadsl

👍 Thanks Ryan, this is a nice improvement!

Due to the following PR, the Country factory no longer always returns true for `states_required`. It can be true for the appropriate countries that require a state in shipping addresses. Because of this, addresses using a country where `states_required` is true, such as the US, need a state in the Spree::Address record to be valid. solidusio/solidus#4272

In a recent change to core, an `after(:build)` hook was added to set the state on the address record for countries which require states. This interferes with the spec setup by ignoring the state attribute we were setting to `nil`. Instead we have moved that to a `before` block so that we can correctly setup the test address to not have a state. Relevant change upstream - solidusio/solidus#4272 Co-authored-by: Andrew Stewart <andrew@super.gd>

Country factory was changed to assign a realistic states_required field. We update our test suite to account for this change. See solidusio/solidus#4272 [skip ci]

RyanofWoods added 2 commits July 6, 2022 16:46

Fix Country factory states_required attribute

cf84832

Resolves issue: solidusio#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.

Wrap address_factory_spec in a context

31dd14f

Improves the readability of the next commit.

RyanofWoods force-pushed the ryanofwoods/fix-country-factory-states-required-attribute branch from 0efc121 to 02e6c5c Compare July 6, 2022 15:13

RyanofWoods force-pushed the ryanofwoods/fix-country-factory-states-required-attribute branch 2 times, most recently from 60af8a8 to 1906f9c Compare July 7, 2022 09:14

RyanofWoods force-pushed the ryanofwoods/fix-country-factory-states-required-attribute branch from 1906f9c to bd8ad62 Compare July 7, 2022 09:44

RyanofWoods commented Jul 7, 2022

View reviewed changes

core/lib/spree/testing_support/factories/state_factory.rb Show resolved Hide resolved

waiting-for-dev approved these changes Jul 18, 2022

View reviewed changes

kennyadsl added the type:bug Error, flaw or fault label Aug 23, 2022

kennyadsl approved these changes Aug 23, 2022

View reviewed changes

kennyadsl merged commit a7de972 into solidusio:master Aug 23, 2022

waiting-for-dev added changelog:solidus_api Changes to the solidus_api gem changelog:solidus_core Changes to the solidus_core gem labels Dec 13, 2022

forkata mentioned this pull request Jan 20, 2023

Update spec setup to reset state in before block SuperGoodSoft/solidus_taxjar#211

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Country factory states_required attribute #4272

Fix Country factory states_required attribute #4272

RyanofWoods commented Feb 15, 2022

RyanofWoods commented Feb 15, 2022

waiting-for-dev commented Feb 16, 2022

RyanofWoods commented Feb 16, 2022

waiting-for-dev commented Feb 17, 2022

RyanofWoods commented Feb 18, 2022

kennyadsl commented Feb 21, 2022

RyanofWoods commented Feb 21, 2022

waiting-for-dev commented Feb 22, 2022

waiting-for-dev commented Mar 9, 2022 •

edited

Loading

waiting-for-dev commented Jun 7, 2022

RyanofWoods commented Jul 7, 2022

waiting-for-dev left a comment

kennyadsl left a comment

Fix Country factory states_required attribute #4272

Fix Country factory states_required attribute #4272

Conversation

RyanofWoods commented Feb 15, 2022

RyanofWoods commented Feb 15, 2022

waiting-for-dev commented Feb 16, 2022

RyanofWoods commented Feb 16, 2022

waiting-for-dev commented Feb 17, 2022

RyanofWoods commented Feb 18, 2022

kennyadsl commented Feb 21, 2022

RyanofWoods commented Feb 21, 2022

waiting-for-dev commented Feb 22, 2022

waiting-for-dev commented Mar 9, 2022 • edited Loading

waiting-for-dev commented Jun 7, 2022

RyanofWoods commented Jul 7, 2022

waiting-for-dev left a comment

Choose a reason for hiding this comment

kennyadsl left a comment

Choose a reason for hiding this comment

waiting-for-dev commented Mar 9, 2022 •

edited

Loading