Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Solidus 2.11 - it's impossible to update a split address name from the backend #4094

Closed
seand7565 opened this issue Jun 7, 2021 · 7 comments
Assignees
Labels
confirmed Validated report wontfix Confirmed report, but not accepted for some reason
Milestone

Comments

@seand7565
Copy link
Contributor

On Solidus 2.11, theres a bug that prevents you from updating firstname and lastname from admin.

It all starts with the addresses value_attributes class method. It combines the old addresses attributes with the attribute changes that you're trying to make. That way, your changes are layered on top of the existing address information. However, if you've set use_combined_first_and_last_name_in_address to false, you won't be passing in a name attribute (instead using firstname and lastname). But the old address will still have a name attribute.

So the combined attributes would be something like: { firstname: 'Bruce', lastname: 'Wayne', name: 'The Batman' }.

On line 88 of the method, we generate a new name with Spree::Address::Name.from_attributes. But if you pass in a name attribute, it just uses that instead. So even if you pass in the above example, it will disregard the firstname and lastname and just use name instead - which comes from the old address.

Additionally

If that were fixed, there's another issue - on line 97 of the value attributes method, you'll see that we only replace the name attribute with the new generated name only if the name attribute is not present. So even if we did generate the correct name above, the new address would still use the incorrect name.

Last note

The address model knows that it needs to generate a new address when you pass in an updated firstname or lastname, so it does that in this scenario... but those attributes don't change. Meaning, you generate an exact copy of the address you're trying to change every time you attempt to change it.

https://www.loom.com/share/37ab0c961dbd4ab9b40e39e8fe4bfff3

Solidus Version:
2.11

To Reproduce
Start a Solidus store on 2.11 from scratch
Set use_combined_first_and_last_name_in_address config to false
Go to admin, attempt to change an addresses firstname or lastname.

@aldesantis
Copy link
Member

This seems to happen even when not touching the firstname/lastname attributes, even if you just click on "Update" in the Solidus backend.

@iba-1
Copy link

iba-1 commented Jul 26, 2021

Could this lead to being unable to update addresses in checkout?
We came across this issue while looking for a reason why shipping address/billing addresses don't get updated in checkout.
Every time a user is in the "confirm" state of Checkout, gets back to the "address" state and tries to edit shipping or billing address each field but firstname and lastname get updated. Maybe this could be related.

@cpfergus1
Copy link
Contributor

cpfergus1 commented Sep 14, 2021

@seand7565 & @aldesantis,

By removing name prior to the update if the initializer setting is false, it would prevent the address duplication as well as allow the expected operation to continue as intended...

def self.value_attributes(base_attributes, merge_attributes = {})
      base = base_attributes.stringify_keys.merge(merge_attributes.stringify_keys)
      base.delete("name") unless Spree::Config.use_combined_first_and_last_name_in_address
      
      name_from_attributes = Spree::Address::Name.from_attributes(base)
      if base['firstname'].presence || base['first_name'].presence
        base['firstname'] = name_from_attributes.first_name
      end
      if base['lastname'].presence || base['last_name'].presence
        base['lastname'] = name_from_attributes.last_name
      end

      virtual_name = name_from_attributes.value
      if base['name'].blank? && virtual_name.present?
        base['name'] = virtual_name
      end

      excluded_attributes = DB_ONLY_ATTRS + %w(first_name last_name)

      base.except(*excluded_attributes)
    end

Perhaps this may be an acceptable solution?

@tvdeyen tvdeyen self-assigned this Dec 16, 2021
@tvdeyen
Copy link
Member

tvdeyen commented Dec 16, 2021

We came across this issue updating to 2.11 as well this week. Trying to work on a fix.

@tvdeyen
Copy link
Member

tvdeyen commented Dec 16, 2021

@seand7565 thanks for the excellent description
@cpfergus1 trying your fix, but no luck so far. Will investigate further

@tvdeyen tvdeyen added this to the 2.11 milestone Dec 16, 2021
@tvdeyen tvdeyen added confirmed Validated report Needs Backport labels Dec 16, 2021
@tvdeyen
Copy link
Member

tvdeyen commented Dec 16, 2021

Oh wow. Spree::Config.use_combined_first_and_last_name_in_address is set to true in the dumy app. this is wrong

tvdeyen added a commit to tvdeyen/solidus that referenced this issue Dec 16, 2021
If use_combined_first_and_last_name_in_address is false (default) and firstname and lastname are given in the changeset (ie. from a form)
we need to make sure that name is set from it, otherwise
data will be inconsistent.

Closes solidusio#4094
tvdeyen added a commit to tvdeyen/solidus that referenced this issue Dec 16, 2021
In Solidus v2.11 we still use `false` for this setting.
Using `true` in specs per default is giving false positives
and issues like solidusio#4094 where not found.
tvdeyen added a commit to tvdeyen/solidus that referenced this issue Dec 28, 2021
If use_combined_first_and_last_name_in_address is false (default) and firstname and lastname are given in the changeset (ie. from a form)
we need to make sure that name is set from it, otherwise
data will be inconsistent.

Closes solidusio#4094
tvdeyen added a commit to tvdeyen/solidus that referenced this issue Jan 7, 2022
If use_combined_first_and_last_name_in_address is false (default) and firstname and lastname are given in the changeset (ie. from a form)
we need to make sure that name is set from it, otherwise
data will be inconsistent.

Closes solidusio#4094
tvdeyen added a commit to tvdeyen/solidus that referenced this issue Apr 20, 2022
In Solidus v2.11 we still use `false` for this setting.
Using `true` in specs per default is giving false positives
and issues like solidusio#4094 where not found.
@kennyadsl kennyadsl added the wontfix Confirmed report, but not accepted for some reason label Aug 23, 2022
@waiting-for-dev
Copy link
Contributor

Closing, as that preference has been removed and we no longer support v2.11.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed Validated report wontfix Confirmed report, but not accepted for some reason
Projects
None yet
Development

No branches or pull requests

7 participants