Refactor address state validation #3129

cedum · 2019-03-01T15:29:33Z

Description

Deprecates and refactors state validation by extracting out the normalization and validation logic into a separate service-like object, which might be also user-provided, for example:

Spree::Address.state_validator_class = UserProvided::Address::StateValidator

The following private methods have been deprecated from Spree::Address model:

state_validate
validate_state_matches_country

This PR introduces the following configuration flag Spree::Config.use_legacy_address_state_validator that enables to switch from the old state validation behavior to the new in a smooth way.
For newly generated applications, this flag is set to false, which means it will use the new state validator class.
For existing applications upgrading to Solidus v2.11 it is set to true, hence keeping the same behavior as in the previous Solidus versions.

Checklist:

Write a recap in this PR description with the normalization and validation rules
I have followed Pull Request guidelines
I have added a detailed description into each commit message
I have updated Guides and README accordingly to this change (if needed)
I have added tests to cover this change (if needed)

aitbw

I like this approach and I can understand the reasoning behind it 👌

That said, this is definitely a breaking change and should be included for Solidus v3.0+

ericsaupe

I really like this! Definitely a breaking change as @aitbw mentioned. Thanks!

cedum · 2019-03-20T17:00:31Z

Moved the PR back to WIP 🚧. It requires some more work. I'll try to make it less breaking. Besides this, by digging deeper into it I see I misinterpreted some state behaviours and also introduced a few bugs 😅

core/spec/models/spree/address/state_validator_spec.rb

kennyadsl

I like how this is coming along and I left a couple of comments. I'm also curious how and if it makes sense to deprecate the old validation methods instead of removing them.

core/app/models/spree/address.rb

core/app/models/spree/address/state_validator.rb

kennyadsl · 2019-06-13T13:25:16Z

core/app/models/spree/address/state_normalizer.rb

+      @address = address
+    end
+
+    def perform!


Personal preference, but I'd use def call here. Did you have a specific reason to use perform! instead?

I have followed the convention of other service-like objects in Solidus that are using perform as their main action method. I have no preferences though.
Probably it's worth defining a project-wise convention for this kind of objects

cedum · 2019-06-13T16:21:49Z

Ready for a second review 🙏

@kennyadsl I've researched a few methods to deprecate them by showing a warning message only if they have been monkey patched, but the final solution was quite hacky and not working in all cases since there're multiple ways to patch them.
Being both private methods, you think it's worth the effort to deprecate them before removing? I've invested some time into it but I wasn't able to find a +/- clean way to deprecate them.

kennyadsl

👍

kennyadsl · 2020-09-09T15:56:32Z

@cedum we discussed this in the core meeting and we agreed that it would be much better if we have the fix for #2417 and #3098 in a different commit, just for documentation in case someone cannot update and wants to just understand what changed to backport that in their own application. Do you think it's doable?

cedum · 2020-09-11T12:10:29Z

@cedum we discussed this in the core meeting and we agreed that it would be much better if we have the fix for #2417 and #3098 in a different commit, just for documentation in case someone cannot update and wants to just understand what changed to backport that in their own application. Do you think it's doable?

Yep, makes sense! I'm on it

core/app/models/spree/address.rb

The Address custom state validation methods are being extracted and refactored into a separate, configurable class.

cedum · 2020-10-02T10:54:03Z

I've extracted the fix for #3098 into a separate PR (merged into master), this way it should be easier to backport the fix on stores unable to upgrade.
I've just rebased also this PR and I think it's ready for another review 🙏

kennyadsl

👍 Thanks @cedum !

aldesantis

@cedum just left a comment on a deprecation message, but other than that it's good to go!

core/lib/spree/core/engine.rb

Refactors and extract Address state normalization and validation logic into a separate and configurable service object. This enables the developers to customize the state validation in a more clean way by instead of monkey patching private methods.

When saving a credit card, pre-create the country and state as address attributes instead of passing the id "1". By having the objects pre-created explicitly, it will cause less surprises.

spaghetticode

@cedum thank you 👍

cedum force-pushed the cedum/refactor-state-validation branch 5 times, most recently from 7532102 to 47b207c Compare March 1, 2019 18:19

cedum marked this pull request as ready for review March 1, 2019 18:20

jacobherrington added the Code Review Needed label Mar 4, 2019

aitbw reviewed Mar 4, 2019

View reviewed changes

ericsaupe approved these changes Mar 5, 2019

View reviewed changes

jacobherrington added the Needs Core Team Decision label Mar 6, 2019

cedum changed the title ~~Refactor address state validation~~ [WIP] Refactor address state validation Mar 19, 2019

jacobherrington removed the Needs Core Team Decision label Apr 3, 2019

kennyadsl added the WIP label Apr 3, 2019

jacobherrington removed the Code Review Needed label Apr 3, 2019

cedum force-pushed the cedum/refactor-state-validation branch 2 times, most recently from 6fc26f2 to b15ae65 Compare April 5, 2019 15:17

cedum force-pushed the cedum/refactor-state-validation branch from b15ae65 to f136357 Compare May 17, 2019 14:56

houndci-bot reviewed May 17, 2019

View reviewed changes

core/spec/models/spree/address/state_validator_spec.rb Outdated Show resolved Hide resolved

cedum force-pushed the cedum/refactor-state-validation branch 2 times, most recently from 8b49693 to 2d0c586 Compare May 17, 2019 16:26

cedum force-pushed the cedum/refactor-state-validation branch 2 times, most recently from 46cdfb3 to 104af58 Compare June 13, 2019 11:54

mdesantis approved these changes Jun 13, 2019

View reviewed changes

cedum force-pushed the cedum/refactor-state-validation branch from 80e110a to a771b33 Compare June 13, 2019 13:01

cedum changed the title ~~[WIP] Refactor address state validation~~ Refactor address state validation Jun 13, 2019

cedum force-pushed the cedum/refactor-state-validation branch from a771b33 to c7aded8 Compare June 13, 2019 13:03

kennyadsl reviewed Jun 13, 2019

View reviewed changes

cedum force-pushed the cedum/refactor-state-validation branch from c7aded8 to 1e3eb5f Compare June 13, 2019 16:08

cedum force-pushed the cedum/refactor-state-validation branch 3 times, most recently from 1727830 to ea4a79b Compare July 10, 2020 15:30

kennyadsl removed the release:major Breaking change on hold until next major release label Aug 28, 2020

kennyadsl approved these changes Sep 8, 2020

View reviewed changes

cedum mentioned this pull request Sep 18, 2020

Fix address validation having a country w/o states #3763

Merged

5 tasks

kennyadsl added Needs Work changelog:solidus_core Changes to the solidus_core gem labels Sep 30, 2020

cedum force-pushed the cedum/refactor-state-validation branch from ea4a79b to 5ff6913 Compare October 2, 2020 09:48

houndci-bot reviewed Oct 2, 2020

View reviewed changes

core/app/models/spree/address.rb Show resolved Hide resolved

Deprecate Address custom state validation methods

daae0a4

The Address custom state validation methods are being extracted and refactored into a separate, configurable class.

cedum force-pushed the cedum/refactor-state-validation branch from 5ff6913 to 6c89b24 Compare October 2, 2020 09:59

kennyadsl approved these changes Oct 2, 2020

View reviewed changes

kennyadsl requested a review from a team October 2, 2020 11:06

kennyadsl added Needs Core Team Review and removed Needs Work labels Oct 2, 2020

aldesantis requested changes Oct 2, 2020

View reviewed changes

core/lib/spree/core/engine.rb Outdated Show resolved Hide resolved

cedum added 2 commits October 2, 2020 15:42

Refactor address state validation

cb90601

Refactors and extract Address state normalization and validation logic into a separate and configurable service object. This enables the developers to customize the state validation in a more clean way by instead of monkey patching private methods.

Pre-create explicitly a country in credit card spec

ada6fa3

When saving a credit card, pre-create the country and state as address attributes instead of passing the id "1". By having the objects pre-created explicitly, it will cause less surprises.

cedum force-pushed the cedum/refactor-state-validation branch from 6c89b24 to ada6fa3 Compare October 2, 2020 13:42

kennyadsl requested a review from aldesantis October 7, 2020 08:31

spaghetticode self-requested a review October 7, 2020 16:25

spaghetticode approved these changes Oct 7, 2020

View reviewed changes

aldesantis approved these changes Oct 8, 2020

View reviewed changes

kennyadsl merged commit 3a04398 into solidusio:master Oct 8, 2020

kennyadsl deleted the cedum/refactor-state-validation branch October 8, 2020 07:11

cedum mentioned this pull request Nov 22, 2020

Remove legacy address state validation logic #3847

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor address state validation #3129

Refactor address state validation #3129

cedum commented Mar 1, 2019 •

edited

Loading

aitbw left a comment

ericsaupe left a comment

cedum commented Mar 20, 2019 •

edited

Loading

kennyadsl left a comment

kennyadsl Jun 13, 2019

cedum Jun 13, 2019

cedum commented Jun 13, 2019 •

edited

Loading

kennyadsl left a comment

kennyadsl commented Sep 9, 2020

cedum commented Sep 11, 2020

cedum commented Oct 2, 2020

kennyadsl left a comment

aldesantis left a comment

spaghetticode left a comment

Refactor address state validation #3129

Refactor address state validation #3129

Conversation

cedum commented Mar 1, 2019 • edited Loading

aitbw left a comment

Choose a reason for hiding this comment

ericsaupe left a comment

Choose a reason for hiding this comment

cedum commented Mar 20, 2019 • edited Loading

kennyadsl left a comment

Choose a reason for hiding this comment

kennyadsl Jun 13, 2019

Choose a reason for hiding this comment

cedum Jun 13, 2019

Choose a reason for hiding this comment

cedum commented Jun 13, 2019 • edited Loading

kennyadsl left a comment

Choose a reason for hiding this comment

kennyadsl commented Sep 9, 2020

cedum commented Sep 11, 2020

cedum commented Oct 2, 2020

kennyadsl left a comment

Choose a reason for hiding this comment

aldesantis left a comment

Choose a reason for hiding this comment

spaghetticode left a comment

Choose a reason for hiding this comment

cedum commented Mar 1, 2019 •

edited

Loading

cedum commented Mar 20, 2019 •

edited

Loading

cedum commented Jun 13, 2019 •

edited

Loading