"Set as default" AJAX is broken when there are no previously default addresses [#1951056]

Steps to reproduce

update to commerce_addressbook 7.x-2.0-rc4
as a user with some addresses, none of which are default, visit user/UID/addressbook
click "set as default" on one of the addresses

The AJAX request completes, then the address disappears. Refreshing the page causes the default address to be displayed correctly.

I guess this is a regression after #1781548: Active Adress boxes, when the DIV #commerce-addressbook-billing-default was removed for users with no default addresses.

Comment	File	Size	Author
#4	commerce_addressbook-fix-set-as-default-address-1951056-4.patch	2 KB	jsacksick
#2	commerce_addressbook-fix-set-as-default-address-1951056-2.patch	1.98 KB	jsacksick
#1	commerce_addressbook-fix-set-as-default-address-1951056-1.patch	1.97 KB	jsacksick

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

jsacksick CreditAttribution: jsacksick commented 25 March 2013 at 11:02

Status:

Active

» Needs review

File	Size
commerce_addressbook-fix-set-as-default-address-1951056-1.patch	1.97 KB

You're right, this is a regression coming from #1781548: Active Adress boxes, the attached patch should fix the issue.

File	Size
commerce_addressbook-fix-set-as-default-address-1951056-2.patch	1.98 KB

Comment #3

pjcdawkins CreditAttribution: pjcdawkins commented 25 March 2013 at 13:07

Status:

Needs review

» Needs work

+ // Check if the user already have defaults addresses before setting one.

The comment should have "has default addresses", and it's a bit confusing anyway (not sure how to fix).

+ $defaults_html = '<div id="commerce-addressbook-' . $customer_profile->type . '-default">'

Perhaps use drupal_html_id() for this sort of thing. It's not clear whether $customer_profile->type is sanitized.

Other than that the patch works for me - thanks!

Comment #4

jsacksick CreditAttribution: jsacksick commented 25 March 2013 at 13:12

Status:

Needs work

» Needs review

File	Size
commerce_addressbook-fix-set-as-default-address-1951056-4.patch	2 KB

I updated the comment, I'm not sure we need to change anything for the $defaults_html, this doesn't need to be passed by drupal_html_id.

Comment #5

pjcdawkins CreditAttribution: pjcdawkins commented 25 March 2013 at 14:01

Status:

Needs review

» Reviewed & tested by the community

Fine with me.

Comment #6

jsacksick CreditAttribution: jsacksick commented 25 March 2013 at 14:24

Status:

Reviewed & tested by the community

» Needs work

This patch doesn't fix all the potential usecases.
Let me explain why :
You could break it by creating two addresses, then you'll end up with one default address and an other non default one.
Now if you remove the default address and set the other as "default" then you'll have an empty #commerce-addressbook-($customer_profile->type)-list div, we could potentially avoid that situation by forcing users to have a default address, so when you'd remove your default address we would set an other as default. I'm not 100% sure about that though.