solidus icon indicating copy to clipboard operation
solidus copied to clipboard

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

Open seand7565 opened this issue 3 years ago • 6 comments

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.

seand7565 avatar Jun 07 '21 22:06 seand7565

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

aldesantis avatar Jun 16 '21 10:06 aldesantis

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.

iba-1 avatar Jul 26 '21 16:07 iba-1

@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?

cpfergus1 avatar Sep 14 '21 18:09 cpfergus1

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

tvdeyen avatar Dec 16 '21 09:12 tvdeyen

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

tvdeyen avatar Dec 16 '21 09:12 tvdeyen

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

tvdeyen avatar Dec 16 '21 10:12 tvdeyen

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

waiting-for-dev avatar Sep 05 '22 08:09 waiting-for-dev