Add reverse charge fields to address

[fthobe]

Warnings

Important

Blocks #6169

Summary

Description:

This pull request introduces enhancements to the address handling logic in both the admin and API components by incorporating additional fields: email, VAT ID, and reverse charge status. These changes aim to improve the completeness of address data and enhance tax handling capabilities.

Key Changes:

1. Admin Interface Updates:

   • Added email, VAT ID, and reverse charge status fields to the address forms.
   • Updated form partials and views to handle these new fields.
   • Modified the JavaScript to process the new fields, ensuring proper functionality.

2. API Enhancements:

   • Updated Spree::ApiConfiguration to include email, VAT ID, and reverse charge status in the address attributes.
   • Modified Spree::PermittedAttributes to handle the new fields.
   • Enhanced request specs (address_books_spec.rb and checkouts_spec.rb) to verify the handling of the new attributes.

3. Address Model Improvements:

   • Introduced the new attributes (email, VAT ID, and reverse charge status) to the Address model.
   • Added an enum for reverse charge status to allow users to specify whether the status is enabled, disabled, or not validated.

• Enhanced Data Completeness: Storing email and VAT ID in the address model ensures that all relevant information is captured within the address record.
• Improved Tax Handling: The reverse charge status enum provides better handling and flexibility for tax-related settings, aligning with various tax requirements and user needs.

These changes enhance the functionality and completeness of the address handling logic by capturing additional relevant information and improving tax handling capabilities. The modifications ensure a more robust and flexible approach to managing address data within both the admin interface and the API.

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

• I agree that my PR will be published under the same license as Solidus.
• I have written a thorough PR description.
• I have kept my commits small and atomic.
• I have used clear, explanatory commit messages.

The following are not always needed:

• 📖 I have updated the README to account for my changes.
• 📑 I have documented new code with YARD.
• 🛣️ I have opened a PR to update the guides.
• ✅ I have added automated tests to cover my changes.
• 📸 I have attached screenshots to demo visual changes.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.80%. Comparing base (ed645d4) to head (f5eb41f).
Report is 5 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6168   +/-   ##
=======================================
  Coverage   88.80%   88.80%           
=======================================
  Files         842      842           
  Lines       18234    18235    +1     
=======================================
+ Hits        16192    16193    +1     
  Misses       2042     2042           

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[jarednorman]

We need to find a solution for storing user-specific data on addresses. As it stands, this PR is incompatible with the existing address handling behaviour and could expose the stored data to other users who possessed only another user's name and address. I believe that:

1. The immutable address system has made address management in Solidus more difficult.
2. The immutable address system solved real problems around historical order data integrity and management.

Ultimately, I think our "immutable" addresses were a mistake, but I don't think the solution is trivial either. Curious to hear the thoughts of the rest of @solidusio/core-team.

[jarednorman]

Not opposed to this functionality, but there are security implications due to how our "immutable" addresses work.

[fthobe]

Not opposed to this functionality, but there are security implications due to how our "immutable" addresses work.

There are some good news about that:
all data added is actually public data.

I see where you come from:
while social security numbers are private, vat ids in europe are public, you are obligated to publish them, so right now this extension would not expose data more or less than before.

Removal of the persistent address is a pain. We have a plan for that iterating:

• to a fully fletched address book for users
• from there to keep the immutable addresses only for completed orders

But I don't expect that before September / October.

Question is which of the two approaches we want to take here:

• we can either implement this on our side and you decide and take the responsibility on backports later;
• you can follow us on the migration path and support us here.

Right now we are struggling with the middle way as the PRs are stuck here much longer than the three days (which I can understand), but if we want to move together we need to find a different rhythm here.

@jarednorman Is there anything you find more worrysome than before regarding the addresses with this change? For you in the US the way addresses currently work is technically difficult to handle, but I didn't see any security issues or compliance issues.

[tvdeyen]

Thanks @JustShah well done PR!

Fabian is right, VAT IDs are not PII, but public - even mandatory - information for companies in the EU. I would like to put this feature behind a flag, because the vast majority of our stores will never use this feature. I like that we support this now, though.

[JustShah]

Dependency Notes:

1. Configuration dependancy: Add reverse charge status to stores #6136

[tvdeyen]

We need to find a solution for storing user-specific data on addresses. As it stands, this PR is incompatible with the existing address handling behaviour and could expose the stored data to other users who possessed only another user's name and address.

@jarednorman I just double checked and the reverse_charge_status attribute is part of Spree::Address#value_attributes what we use to compare addresses. I cannot think of an scenario where we expose stored data to another user with the same name and address. Can you elaborate on what scenario you were thinking of?

I believe that:

1. The immutable address system has made address management in Solidus more difficult.
2. The immutable address system solved real problems around historical order data integrity and management.

Ultimately, I think our "immutable" addresses were a mistake, but I don't think the solution is trivial either. Curious to hear the thoughts of the rest of @solidusio/core-team.

I think so as well.

Simple idea™️: Only allow addresses being altered/deleted if not associated with completed/shipped orders.

Most correct idea™️: Introduce a Spree::Invoice model and save a legal record of completed/shipped orders. If this is what the cause for "real problems around historical order data integrity" was. An immutable, legal copy of the order is maybe a better solution for that.

The crazy idea™️: Remove the immutability all together and let the accounting system (all businesses have anyway) deal with the legal regulations and let Solidus just be a order management system which only job is to collect orders and does not care about keeping historical data indefinitely. Maybe too crazy? Worth a thought at least.

[fthobe]

Simple idea™️: Only allow addresses being altered/deleted if not associated with completed/shipped orders.

I would opt for that, it also allows compatibility with progressive invoicing as in most jurisdictions mandatory.

Simple idea™️: Only allow addresses being altered/deleted if not associated with completed/shipped orders.

+1 need to change a shipped order, reimburse and work on a copy as almost all ERPs

Most correct idea™️: Introduce a Spree::Invoice model and save a legal record of completed/shipped orders. If this is what the cause for "real problems around historical order data integrity" was. An immutable, legal copy of the order is maybe a better solution for that.

I'd focus on having solidus more pro on eCommerce and literally focus on that, I think if we find an agreement on how to handle taxes compatible to real life scenarios we have a solution that's relatively easy to integrate and allows people to attach their own system to import orders and handle updates.

[fthobe]

Stupid solution with added benefit:
leave addresses as they are and introduce address book to copy addresses over into the order as immutable object.

In that way we get added benefits such as an out of the box address management for users.

[tvdeyen]

Stupid solution with added benefit: leave addresses as they are and introduce address book to copy addresses over into the order as immutable object.

In that way we get added benefits such as an out of the box address management for users.

With that you mean Address Book management UI features for User account and Admin sections?

I am asking, because we have an address book. All user addresses are stored in their address book.

Can you elaborate on what you mean exactly?

[fthobe]

AFAIK immutable addresses become complex to remove where we touch orders. We can simply separate the user addresses from the Order addresses and we are good for another 10 years as any completed Order must be conserved anyway for tax purposes.

What I would do is this:

• create a separate address book for users sharing the same table schema without flagging them as immutable;
• copy addresses over to the immutable addresses upon selecting it for an order.

Am I missing anything or is the current address book saving only one billing and one shipping address?

Instantly compliant and relatively painless to do (though some additional maintenance effort as the fields need to exist twice) and we could try to solve this more permanently at any point between the year 2025 and 2034 (probably 31/12/2034 knowing us;).

[tvdeyen]

Great work @JustShah

I just have on least request, before this can be merged.

[tvdeyen]

Thanks @fthobe @JustShah there are failing specs related to this change. Mind taking a look please?

[fthobe]

Thanks @fthobe @JustShah there are failing specs related to this change. Mind taking a look please?

Sure, we will try to get to it this week.

[fthobe]

@tvdeyen
Remaining failing spec is:
rspec ./spec/features/admin/products/products_spec.rb:93 # Products as admin user searching products should be able to search deleted products

holy seal of approval?

[tvdeyen]

Thanks

[tvdeyen]

@JustShah I fixed (hopefully most of) the flaky specs in latest main. Do you mind to rebase?

@jarednorman last chance for raising concerns before merging

[fthobe]

@tvdeyen rebased

I trust @tvdeyen.

[tvdeyen]

We need a final rebase (with conflict resolving). I also have a minor non-mandatory nit about the vat_id field.