I can't see anywhere in the committed code, where I can set the copying
to be the "default action" - and that would IMHO be the normal approach
(is on almost every site I've shopped at) - where the customer
"unchecks" the "my billing is the same as the shipping address" box - if
he/she wants to give a seperate address.

Not the other way around.

The code looks for: '#default_value' =>
isset($order->data['profile_copy'][$checkout_pane['pane_id']]['status'])
? $order->data['profile_copy'][$checkout_pane['pane_id']]['status'] :
FALSE,

I can't see anywhere in the patch, supplying an option to reverse that
logic - but shouldn't it default to TRUE in the above (that works for me
- so the billing fields isn't shown by default - and the checkbox is
ticked - and the order gets the correct billing AND shipping info).

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

designbysoil’s picture

I was also looking for a way to do this. Should be the default.

rszrama’s picture

Title: Default to only ask for 1 address » Add the option for address copying checkout panes to make copying the default

Thanks to klavs for opening this follow-up issue to #1410022: Improve copy address functionality - create a more generic solution. There is no option to do this right now, so that must be the starting point - then we can talk about what the default should be. ; )

klavs’s picture

IMHO the simplest, and most correct option, would be to simply add a checkbox, next to the added option stating: "Allow this profile to use values from another profile, where applicable. "

This checkbox would then just say "default to use the value from the other profile, where applicable" or something.

klavs’s picture

Just noticed, that it's not enough to change FALSE to TRUE - for anonymous users - the field being copied to - is still shown - and isn't hidden, until the button is unchecked and then checked again :(

I'll investigate what javascript actually hides the fields and figure out what's up - AFAIK the js wasn't part of the patch that enabled this feature.. so it must be something generic (hints appreciated ;)

delta’s picture

It's not enough to just pass option default value to TRUE. But the problem isn't coming from javascript.

In commerce/modules/customer/includes/commerce_customer.checkout_pane.inc

I change line 124:

'#default_value' => isset($order->data['profile_copy'][$checkout_pane['pane_id']]['status']) ? $order->data['profile_copy'][$checkout_pane['pane_id']]['status'] : FALSE,

to

'#default_value' => isset($order->data['profile_copy'][$checkout_pane['pane_id']]['status']) ? $order->data['profile_copy'][$checkout_pane['pane_id']]['status'] : TRUE,

like described in this issue description, but if we pass this to default, we need to hide the customer profile field when form is created, the same way it is done when the checkbox is checked (via #ajax) so i add this statement on line 146 :

 } elseif (empty($form_state['build_info']['args'][0]->data)) { // ... after some vardump .. i see this is empty when the form is created for the first time.. i'm sure there's a better way to do that
        foreach ($pane_form as $field_name => $field) {
          if(strpos($field_name,'#') !== 0 && $field_name != 'customer_profile') { //sure there's a better way to get only the fields to hide from this array
            $pane_form[$field_name][LANGUAGE_NONE][0]['#access'] = FALSE;
          }
        }
      }

tested this with lastest commerce release and this is working well.

For the hint, when the checkbox is checked the javascript rebuild the customer profile billing or shipping form (depending on your settings) and this code in the form implementation hide the fields
line 136:

// If the order data has reference to fields that were copied over, hide
      // them so we don't confuse the user by still allowing them to edit values.
      if (!empty($order->data['profile_copy'][$checkout_pane['pane_id']]['status']) && isset($order->data['profile_copy'][$checkout_pane['pane_id']]['elements'])) {
        foreach ($order->data['profile_copy'][$checkout_pane['pane_id']]['elements'] as $field_name => $field) {
          foreach ($field as $langcode => $items) {
            foreach ($items as $delta => $item) {
              $pane_form[$field_name][$langcode][$delta]['#access'] = FALSE;
            }
          }
        }

}

klavs’s picture

I finally got some time to try this out.

The change at line 146 worked wonders (except for an extra } at the end :)

I would like to convert this into a patch - but would should the orig be?

I am currently running latest stable + http://drupal.org/node/1410022 + the above two snippets.

rszrama’s picture

To make a patch, you'd need to do it against the latest 7.x-1.x-dev. You should be able to follow the instructions at http://drupal.org/node/707484 to make this using Git.

klavs’s picture

FileSize
1.47 KB

Here's a patch of the two changes.

This just defaults to enabling copying of pane information (if the admin has selected that this should be a possibility) - and also makes it work for anonymous users.

It doesn't have an option, for the admin to choose default state (enabled/disabled) - but logic (mine atleast) tells me, that if you selected that you want data from 1 field, to be copied from another - you'd want this to be the default - so IMHO it's a fair change.

klavs’s picture

Status: Active » Needs review
delta’s picture

Thanks to update this as a patch, i'm agree if your check for enabling copy address feature this would be the default. i think..

hope get this is in futur release.

i'm testing it with latest dev release and this is working like a charm

vasike’s picture

i reworked the #8 patch and added an option for the checkout pane settings for the default copy option.
reviews needed

Status: Needs review » Needs work

The last submitted patch, default_copy_address-1637674-10.patch, failed testing.

vasike’s picture

Status: Needs work » Needs review
FileSize
3.73 KB

new one. it seems the previous wasn't "so good".

Status: Needs review » Needs work

The last submitted patch, default_copy_address-1637674-13.patch, failed testing.

rszrama’s picture

No worries; the testbot appears to be acting up. I'll ask rfay about it.

rszrama’s picture

I agree with vasike that this should be optional; no sense not to bake it into the checkout pane settings like this. Let's go ahead and update the test for address copying, though, to test for this behavior for copying being the default vs. not the default.

checker’s picture

Thank you vasike. Patch #13 looks good. I can't find problems after applying.

checker’s picture

Status: Needs work » Needs review

status should be needs review?

rszrama’s picture

Status: Needs review » Needs work

I had left it in needs work until we got the reverse tests in that would ensure it worked well with address copying as the default.

checker’s picture

After some testing i guess there a still problems with this patch. It is hard to debug.
- It could be possible that the checkbox is selected by default but the address fieldset is still open.
- Sometimes there are errors about empty address fields although the copy address checkbox is selected.

I am making an journey for the next week maybe someone else can have an eye on this.

bojanz’s picture

Issue tags: +kickstart blocker

Tagging.

andyg5000’s picture

Status: Needs work » Needs review
FileSize
5.73 KB

The existing patch hides all fields on the destination profile even if they don't exist on the source profile.

Here's an updated patch that does the following:

Settings:
Adds a fieldset to the options pane that contains profile copy options when profile copying is enabled (Fieldset is hidden when disabled).
Moves the source profile select box into the fieldset.
Creates a checkbox (TRUE by default) in fieldset for setting profile copy as the default action in checkout.

Checkout
Sets the default value of the copy profile checkbox on checkout to the default action if not set elsewhere.
If profile copy action is set as the default and has not been altered by the customer, it hides all fields that are the same between the source and destination profile.

Tests
I've also updated the test to set the default copy action to TRUE and removed the ajax call that previously copied the profiles.

Carlitus’s picture

#22 works for me except that the checkbox (Copy profile by default. ) has to be disabled to be enabled in checkout pane by default.

rszrama’s picture

Status: Needs review » Needs work

Dropping back to needs work while I give this a little time. I actually meant for us to add a test to check the alternate configuration, not simply change the test that was in there. Wanna make sure we have coverage for both. : )

Carlitus’s picture

#23 It now works as expected...i don't know why...

Lukas von Blarer’s picture

Works for me.

checker’s picture

Tested #22 with 7.x-1.3+40-dev (2012-Sep-13) without problems.

cvangysel’s picture

Status: Needs work » Needs review
FileSize
7.11 KB

Re-rolled and reviewed patch against HEAD of 7.x-1.x, including tests for both cases.

rszrama’s picture

Letting test bot have another stab at this with some changed labels; nothing should've changed substantively since cvangysel's last test, but I couldn't get the tests to work at all locally.

rszrama’s picture

Component: Checkout » Customer
Status: Needs review » Fixed

Alrighty, not sure what was going on locally. Committed.

Automatically closed -- issue fixed for 2 weeks with no activity.