Add and start populating spree_addresses.name field
#3962
Conversation
Rspec already provides `described_class`, so we don't need to define that extra `subject`.
spaghetticode
force-pushed
the
spaghetticode/always-use-name-field
branch
from
aff9765 to
f24c008
Compare
Given the field is there, and it's going to fully replace `firstname` and `lastname` from Solidus 3.0, it makes sense to start using it early so the migration process will be easier.
Even when the field `name` is not explicitly used yet (for example when the preference `use_combined_first_and_last_name_in_address` is set to false) we're going to backfill the DB when saving new records starting from combining `firstname` and `lastname`, in order to provide a smooth upgrade process to 3.0.
When comparing addresses, in order to be compatible with all possible combinations, we should not use legacy attributes such as `firstname` and `lastname` in the comparison, as they may end up generating false negatives. Instead, using `name` should be safer. For example: Spree::Address.new(name: 'Mary Jane Watson') should be equal to: Spree::Address.new(firstname: 'Mary Jane', lastname: 'Watson') This should happen only when the application has already started moving towards the unique `name` field, i.e. when the preference `Spree::Config.use_combined_first_and_last_name_in_address` is set to `true`. Differently, `firstname` and `lastname` should be accounted for when checking equality, so that: Spree::Address.new(firstname: 'Mary Jane', 'Watson') won't be equal to: Spree::Address.new(firstname: 'Mary', 'Jane Watson')
spaghetticode
force-pushed
the
spaghetticode/always-use-name-field
branch
from
f24c008 to
7741d82
Compare
|
@spaghetticode thanks! I tried this locally in a store with existing addresses in the old format and seems to work well, I just found one unrelated problem (already present in v2.11.4) that I documented here. |
| remove_index :spree_addresses, :name | ||
| remove_column :spree_addresses, :name | ||
| end | ||
| end |
There was a problem hiding this comment.
Does it still count as a patch update if I need to run bin/rails railties:install:migrations?
There was a problem hiding this comment.
Hey! Yes, unfortunately we had to do this change at a patch level because it is fixing the migration process for switching to the new addresses fields schema. We decided to consider this a patch instead of a new feature for this reason. Theoretically, a new migration doesn't always represent a new feature, although it's strange to have. We are sorry if this is causing issues and we are open to improving the documentation to help users upgrading. Let us know your thoughts.
There was a problem hiding this comment.
I think having a more detailed CHANGELOG could have helped me to find the issue. I miss the gap between > 2.11.0 and < 3.0.0.
There was a problem hiding this comment.
You are right. I'm preparing a PR that updates the Changelog here. Anyway, you can find details of each release in https://github.com/solidusio/solidus/releases.
Description
In order to prepare apps for the Solidus 3.0 migration where
firstnameandlastnamefields are completely removed from the DB, this PR introduces the fieldnameand starts using it when creating new records, so backfilling will be required only for historical data prior to incorporating this change.Also, this PR fixes a possible issue when comparing new addresses with double first names to historical records when the preference
use_combined_first_and_last_name_in_addressis set to true.Checklist: