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:

• 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 removed tests to cover this change

[kennyadsl]

@mamhoff thanks!

I think that your reasoning makes total sense, it took me a while to get it only reading the PR description because of a mistake:

Why not remove the join table entry directly?

Should be

Why not remove the record from the join table entry directly?

I think that fixing it could help readers from the future to avoid confusion.

In terms of PR's content, I also can't find a reason not to remove the record, instead of resurrecting an old one but I think we should deprecate the usage of archived instead of removing the column, which in my opinion is too invasive and gives for granted that people are not using that field with some custom code.

I also considered using Discard (which at least would be consistent with the rest of the code) + remove the "resurrecting" logic but honestly, I'd avoid it here since it's just a record on a join table and I can't see any point where we may have some value in soft-deleting.

[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

wrong PR! :P

[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 still a little bit scared about this. We are assuming people are only using this field in the way we think but it's not guaranteed.

Maybe we can keep the archived column (ignoring it on the Rails side) and use the bin/rails g solidus:update task to be sure people are understanding that those records will be deleted from their database. What do you think?

[kennyadsl]

@waiting-for-dev correct me if I'm wrong here, but if I recall correctly it's a matter of adding a new method in the update generator, isn't it?

[mamhoff]

I really can't imagine that - and if we leave the field, we'll have to deal with it in every upgrade from now on (similar to the Spree::Shipment#deprecated_address_id field. I'd rather add a large changelog entry, and be done with it. People who use the archived flag for something will still be able to edit the migration on their project to not delete the records / column.

[kennyadsl]

Using ignored_columns I think it will be pretty transparent for us.

In my opinion, it's not a matter of imagining how the feature could be used, but more of a formal process of making this software as compliant with SemVer as possible. And removing a table column in a minor release is definitely a breaking change from all possible points of view.

For Spree::Shipment#deprecated_address_id (#4333), it is a bit different because we are not using that field in the code from 2016 as you mentioned in the PR, so it's very unlikely that it is being used. BTW, I think that PR should go in a major release as well. I didn't comment because I just missed it. 🙂

[waiting-for-dev]

@waiting-for-dev correct me if I'm wrong here, but if I recall correctly it's a matter of adding a new method in the update generator, isn't it?

Do you mean adding a message? Yes, we could do that by adding a new method. However, the actual code is already redirecting people to the Changelog. I think it'd be better to add any information there and keep using it as the unique source of truth for upgrades info.

[waiting-for-dev]

Basically, this way we won't have to remember removing the new message before the next Solidus version 😉

[tvdeyen]

I think we should be less strict on semver with this change, because this change has never been visible to users and or admins. It is completely hidden from any UI. We even do not have an address book UI. The only potential users of this "feature" would have been developers and or data analysts. But I bet this is not used by anyone. This causes a lot of confusion and complex code.

What this does is, that it mimics discard where it is not necessary, because addresses are not actually removed, but just removed from the users addresses. This causes lots of confusion.

We recently migrated a store from spree to solidus and were very confused still seeing deleted addresses in the users address books.

I am for removing this even in a minor version. 👍🏻

[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]

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

[waiting-for-dev]

Non-blocking, but ideally, we shouldn't have any diff with the Changelog.

[mamhoff]

Fixed

[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.

[tvdeyen]

@mamhoff can you please rebase this PR? 🙏

[tvdeyen]

@kennyadsl I strongly believe that this is good as is. Not necessary to add deprecations for imo. This is a pressing issue and it has been pushed back for too long now.

[mamhoff]

@mamhoff can you please rebase this PR? 🙏

Done!

[kennyadsl]

I'm ok merging this.

[codecov]

Codecov Report

Attention: Patch coverage is 63.63636% with 4 lines in your changes missing coverage. Please review.

Project coverage is 88.67%. Comparing base (b367f99) to head (6fc2564).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
core/app/models/spree/user_address.rb	42.85%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3852      +/-   ##
==========================================
- Coverage   88.70%   88.67%   -0.03%     
==========================================
  Files         829      829              
  Lines       17992    17995       +3     
==========================================
- Hits        15959    15957       -2     
- Misses       2033     2038       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tvdeyen]

🎉 💯