Remove Spree::UserAddress#archived flag

[mamhoff]

Description

This flag is not very useful and gives rise to bugs.

It is not very useful, as we only need it to "resurrect" addresses that the user inputs newly, but that match an address that has been removed from the address book in the past for some reason. Why not remove the join table entry directly? Much simpler, and less code to worry about.

The bug we found that exists only because of the existence of this column is the following: If an address is added to the address book that matches an existing user address that is archived, there is a chance of it throwing a uniqueness validation error, because the uniqueness check on user/address on the Spree::UserAddress model does not take into account the "archived" flag.

The third reason why I think this should go is for data protection reasons: If I call "delete" on a record, I want it to be gone, not half-gone. And in the case of a join table representing which addresses a user wants to be presented with again, I very much want it gone.

Not archived.

Note also: Removing this complexity removes explanatory comments in otherwise straight-forward API endpoints.

Checklist:

• [x] I have followed Pull Request guidelines
• [x] I have added a detailed description into each commit message
• [x] I have updated Guides and README accordingly to this change (if needed)
• [x] I have ~added~ removed tests to cover this change

[mamhoff]

Rebased this PR on the current master branch. I've also added deprecation warnings for the scopes Spree::UserAddress.archived and Spree::UserAddress.all_historical.

[mamhoff]

@kennyadsl I changed the wording of the commit message.

I've also kept the scopes to return all records instead of their custom behavior. If stores make use of that archived Boolean (I really doubt it), they are free to empty the migration on upgrading and overriding the new to the old behavior in their stores. I have a really, really hard time imagining this was used for anything anywhere.

[mamhoff]

From @kennyadsl : Check if default billing / default shipping addresses are preserved with this change

[kennyadsl]

@mamhoff looks good but I don't understand why the commit states "Behavior change". I'm not sure to follow which behavior changed. Are you saying that at the moment, when we are "changing" an address that was set as the default, the resulting new immutable address won't have the default (ship or bill) boolean set? If yes, this is probably worth a separate PR?

[mamhoff]

Yes, i think that is what happens :) I'll provide a separate PR

[mamhoff]

@kennyadsl Extracted the behavior change to #4332, care to re-review? :)

[kennyadsl]

I'm ok removing the feature entirely in a minor version. Just saying that maybe we can postpone the database column removal to the next major by just using ignored_columns on the model. Any eventual confusion will be gone because that field is explicitly ignored, and we can keep any unexpected usage of that column safe.

On the other hand, when archived = true the record will be removed from the database, so if someone is using that column for some reason, they will lose a lot of those records anyway, unless they change the migration manually, but at that point, they can also change the removal of the field if needed.

I will bring this to the core team meeting later today and will get back with a shared decision. Thanks again!

[mamhoff]

I'm ok removing the feature entirely in a minor version. Just saying that maybe we can postpone the database column removal to the next major by just using ignored_columns on the model. Any eventual confusion will be gone because that field is explicitly ignored, and we can keep any unexpected usage of that column safe.

If we use ignored_columns, then anyone actually using the column would have to metaprogram themselves out of us having invoked it. Not sure that would improve the (imo extremely unlikely) situation.

[kennyadsl]

With metaprogram themselves out of us having invoked it do you mean decorating the model and removing the field from the ignored_columns list, or there's anything more complex to do that I am missing? I thought it was just a matter of doing that but maybe I'm not considering some other things.

By the way, even if it's just that, it's not less complex than manually changing the migration so that nothing happens.

[kennyadsl]

As is, I'd push this as well in the next major. Since it's not that far I think it's a good idea to avoid removing columns from the database in a minor.

[mamhoff]

Should I remove the deprecated scopes here entirely then, or deprecate for Solidus 5?

[kennyadsl]

If there's a way of deprecating this without removing the database column (eg. as with ignored_columns), I think we can merge this immediately, and only remove the column with Solidus 4. Makes sense?

[mamhoff]

Rebased fromn master, removed the changelog entries (as I assume they're added back in when y'all release).

[kennyadsl]

Again, sorry for raising this only now, but we were thinking about alternative paths to deprecate removing this column. We were thinking about this approach but we are not 100 sure it's the best approach yet. Leaving here for documentation.

In the first minor:

1. create a preference called something like delete_user_addresses_instead_of_archiving
2. deprecate having the above preference at the false value
3. in remove_from_address_book method, conditionally destroy records instead of archiving

In a subsequent minor (to be sure that code has been deployed):

5. add another preference called ignore_active_scope_for_user_addresses. This requires users to have deleted the archived: true records after setting the above preference (delete_user_addresses_instead_of_archiving) to true.
6. deprecate using the active state, saying in the warning to turn the second preference on.

On the next major, we can remove deprecated code and the column safely.