Instead of depending on client-side JS to copy a shipping address from a billing address, I'd prefer a server-side solution that operates through the #ajax system in D7's FAPI. The other approach has been implemented in #1243846: Reuse billing information for shipping profile fields, but this issue will be to replace it in the 2.x branch with the server-side approach. This will have the added benefit of working across multiple checkout pages and allowing the Shipping module to alter the forms such that "copied" elements are not visible while fields that differ between the various profile types remain visible.

I realized re-reading the related issue that I never put the specification in writing in this queue. I had talked with bojanz about it in IRC before he posted his first patch I believe, but I obviously never updated it in this queue or did it in the Commerce queue on a separate issue. I can't remember which.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joachim’s picture

What about a totally server-side Form API version as in my patch at http://drupal.org/node/1243846#comment-5197298? That worked across different checkout pages.

googletorp’s picture

Status: Active » Closed (works as designed)

Shame on you Ryan for not doing your homework.

The functionality happen server side, the javascript is purely to improve UA.

rszrama’s picture

Assigned: Unassigned » rszrama
Status: Closed (works as designed) » Needs review

Well, without a patch for easy review, I couldn't quickly confirm. ; )

The functionality was added across multiple patches, right? I'm finding the initial code here and follow-up code here, but maybe I've missed some?

I'd still like to leave this open for review in 2.x. I don't think working solely through Address Field like you have is the only viable option, and I'm not convinced it's the best. If I read the code right, you have a hard dependency on only copying data from the customer_shipping profile to the customer_billing profile. I'd like it to at least be bi-directional (i.e. not dependent on a certain order of things in the form) and work across multiple checkout pages. Additionally, you're only copying address data regardless of what other fields the two profile types have in common. You're also only copying the data on form submission and hiding form elements that are present on the page instead of actually altering and re-rendering that part of the form via #ajax so the form elements are no longer present in the HTML unless they represent fields that exist on the one profile but not the other.

Again, I could be reading it incorrectly because it's spread across multiple commits, but I see plenty of room here for another approach that simply adds a checkbox or submit button inside the customer profile fieldset, triggers an #ajax submit and refresh of the form where data is even actually created or updated, and rerenders the form elements for the customer profile fieldset such that "copied" fields aren't even in the HTML any more.

You can feel free to ignore this issue if you think I'm barking up the wrong tree, but I'd like to have at least something to remind me to review this before rolling 2.0.

googletorp’s picture

Title: Use server-side address copying via #ajax » Improve copy address functionality - create a more generic solution

I'm sure there is room for improvement. The solution I implemented landed somewhere between ideal and easy to implement. I went for an optional add-on that solves the problem for 90% (have same address on same page). I could have made the solution more generic, but ended up hardcoding some options for now.

I don't think AJAX submit is a good solution, since it would save data, and I don't think that would be a good UX. It also makes it impossible for the user to undo his selection. That is the reason why I hide the form fields instead of remove them entirely - in my first approach I actually remove the fields from the HTML, but found that doing so deleted the data entered. This could of cause be solved by saving the data in $form state, but like I said this landed somewhere between ideal and easy to implement.

I'm not sure it would be a good idea to copy arbitrary field data between profiles. I would call that out of scope for commerce shipping. Some might make good use of it, but I don't feel like that belongs here. The address is important for shipping calculation, and we make the address field. That is the only reason to include the functionality in shipping and that is also the limit of the scope.

So in short I'm open for improvements, but not at the cost of existing functionality.

googletorp’s picture

Status: Needs review » Needs work

Changing status. Ryan has some ideas on how to improve for 2.x

agoradesign’s picture

Hi,
this feature is exactly what I was looking for. Only one thing disturbs me a bit: when the shipping address is hidden, its legend is still present. But it should be hidden also, of course.

I was looking into the copy_billing_address.inc code, in order to modify it to my needs. Although it's should be an easy task, I must admit that I failed. Can you plz help me out, how I can get this to work?

thx, Andi

ibot’s picture

Same problem with the fieldset-legend here.

Since the current solution is working with/replacing the inner of the fieldset it seems not so easy to change its behaviour.
- Maybe playing around with the #ajax-wrapper could help as a starting point.

(copy_billing_address.inc in line 26)
'wrapper' =>'copy-billing-address',

Best,
T.

agoradesign’s picture

Thx, I'll try on monday and report back any success

Anonymous’s picture

I have multiple problems with the dev version. The fieldset is still visible as mentioned, and mine does not copy the data from shipping info at all (testing as an anonymous user if that makes any difference).

IckZ’s picture

I also have multiple problems :(

The fieldset is still visible as mentioned
If using the addressbook module it is also still visible instead of hiding all other shipping details

The checkbox should be checked by default so shipping would be hidden by default.. I think customers want the shortest and easiest way..

I also ask me if there is a reason why billing is the primary adress? I would think to fill in shipping and click "billing is same as shipping.."

btw: The patch from http://drupal.org/node/1243846#comment-5340398 is in my opinion the best way but it should also create both profiles and not only the billding..

cheers!

IckZ’s picture

Hey!
Perhaps it is better to use the integrated solution and modifiy it a bit..

What do you think about:

- making the billing address coppied by default
- setting the form weight of the checkbox to the top of the fieldset (i was not able to get this over the adressbook dropdown menu (if you have installed adressbook)
- animating the checkbox to copy the address like a toggling collapsible fieldset.. -> by default it should be collapsed..

Optional it would maybe be better to set the shipping address to the default address.. (or make the ability to choose from which address it should be coppied) so that the billing is coppied from the shipping adress

cheers!

timodwhit’s picture

Just re-sharing: #1373134: Copy billing address to shipping address UI feature because I feel it is closer to what Ryan was suggesting. I personally like this solution because of the ability to save each profile, instead of voiding the shipping profile completely. But, as googletorp mentions though once the data is set, there is no way to clear it, which makes the UX a little odd.

but nonetheless, just resharing since DO is so big! :-)

Best.

helior’s picture

Project: Commerce Shipping » Commerce Core
Version: 7.x-2.x-dev » 7.x-1.x-dev
Component: User interface » Customer
Status: Needs work » Needs review
FileSize
12.11 KB

Implementing this patch on Drupal Commerce directly. Here's a list of what it does:
- Adds a setting to each customer profile checkout pane to allow it to copy values from any other profile (not just billing to shipping)
- Copies all similar fields between profiles (not just addressfield)
- Works even when customer profile forms exist in different checkout pages.
- Copies values in the back-end without saving profile data, and allows copied values to be cleared if checkbox is unchecked.
- Hides only copied fields, while leaving inapplicable profile fields editable by user.

diakon333’s picture

#13: copy-profile.patch queued for re-testing.

helior’s picture

FileSize
12.07 KB

Minor updates:
- simplified ajax callback.
- check for inconsistent empty field values in copy helper function.

joemoraca’s picture

tested #15 against commerce 7.x-1.3 and worked well. allowed me to pick either address type to copy to the other. when I added a new field to the shipping address it filled in the shipping address from the billing address but allowed me to fill in the "extra" field. If I unchecked the box it cleared the fields.

rszrama’s picture

I see you checking an $order->data['profiles'] array but don't actually see any place in this patch where that variable gets set. Is it just leftover code from a prior iteration where that array was used to store profile data?

Regarding your @fixme, the reason you can't use the wrapper is we aren't actually declaring any entity data properties in our .info.inc files. I don't actually recall why, as it should technically be possible to declare these as structs like the Price field does for its data column... but perhaps there was a technical reason not to make this available through the wrapper. : ?

Also, if I'm not mistaken, it looks like you were able to accomplish the patch without requiring any pre-submission save of profile data. I may be getting my edge cases mixed up between this and the other shipping refresh patch, but in any case, good job. I'm going to do some development with this patch on to see if it trips me up anywhere.

timodwhit’s picture

@helior: great patch, works like a charm!

The only thing that might cause an issue is if you have Commerce Address Book and this patch installed. When selecting an address from a previous address on file, if the customer selects "My shipping information is the same as my billing information" the profile is copied to the addressbook, even if it already exists.

Is there a way to prevent this? If not, the patch is still awesome.

helior’s picture

@rszrama
I found this code in commerce_customer.checkout_pane.inc

  // If the associated order field has been set...
  if ($field_name = variable_get('commerce_' . $checkout_pane['pane_id'] . '_field', '')) {
    $profile = $wrapper->{$field_name}->value();
  }
  else {
    // Or try the association stored in the order's data array if no field is set.
    if (!empty($order->data['profiles'][$checkout_pane['pane_id']])) {
      $profile = commerce_customer_profile_load($order->data['profiles'][$checkout_pane['pane_id']]);
    }
  }

So I followed the same pattern in my patch to make sure I'm not breaking backwards compatibility. I probably should have consulted with you on that though, because I don't see where the pane ID is ever saved to the order's data array().

Regarding the @fixme, I figured that much but didn't know the reason behind not declaring that property in hook_entity_property_info(). Code consistency was my biggest concern here.

@timodwhit
I suspected there was a bug here, thanks for confirming it! Looking into it now..

helior’s picture

Found the issue: I was wiping out reference to the profile_id while unsetting profile values, which caused the entity controller to create new profiles. So I removed the logic to "clear" values when unchecking the box. Not a huge deal as this was just a very minor convenience feature anyway.

timodwhit’s picture

Thanks for the patch. Works like a charm!

RowboTony’s picture

Hello, I applied this patch, cleaned caches, and don't see any of this functionality on my checkout form. Is there something I must change in manage fields/display fields within the customer-profiles for shipping and billing? I would be very grateful if someone can provide a quick howto and me understand how to implement this functionality?

rszrama’s picture

Nope, should just have to turn it on in the checkout pane settings.

RowboTony’s picture

Thanks, I got it now, seems that I incorrectly patched the module in the profiles directory/profiles/commerce_kickstart/modules/commerce/modules/customer, when in fact my site is using /sites/all/modules/commerce/modules/customer/, it works properly now! Thanks a bunch!

rszrama’s picture

Ahh, well, make sure you don't leave two copies of the code lying around. Go ahead and delete the one that's not in use to prevent conflict in the future.

ghilsman’s picture

Do I need to apply all patches in order (#13 #15 #20), or just the latest one (#20)?
Thank you!!

rszrama’s picture

Always just the last one, the one in comment #20.

ibot’s picture

Thank you for the patch!

Since i'm setting up a site in german language i'm depending on correct capital letters.
So i removed the strtolower() in line 124 of commerce_customer.checkout_pane.inc

        '#title' => t('My @target is the same as my @source', array('@target' => strtolower($target_profile_type['name']), '@source' => strtolower($source_profile_type['name']))),

gets

       '#title' => t('My @target is the same as my @source', array('@target' => $target_profile_type['name'], '@source' => $source_profile_type['name'])),

- could it make sense to update the patch?

best,
T.

rszrama’s picture

I was actually reviewing this and changed it yesterday from strtolower() to drupal_strtolower(). Does that resolve the issue for you?

rszrama’s picture

Assigned: rszrama » Unassigned
Status: Needs review » Fixed

Alrighty, finally gave this an in depth review. I replaced strtolower() with drupal_strtolower(), brought the throbber back but was able to tweak the CSS so just the image showed to the left of the checkbox, and added a test for it to the Customer UI test class.

I also tried to hack this by altering the source profile's state select HTML to include an invalid value before attempting to copy that to the destination profile, and the invalid selection was picked up just fine, preventing the save of a profile with invalid data.

Many thanks to Helior for doing the bulk of the work on this patch.

Commit: http://drupalcode.org/project/commerce.git/commitdiff/8d5c51f

klavs’s picture

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).

klavs’s picture

Status: Fixed » Needs work

forgot to change status.

rszrama’s picture

Status: Needs work » Fixed

Yeah, that needs to be a separate feature request. I thought about adding it into this patch, but it wasn't worth holding up this functionality. I meant to open a separate issue for it, so feel free to go ahead and do it.

Status: Fixed » Closed (fixed)

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

corniquet’s picture

Category: feature » bug
Status: Closed (fixed) » Needs work

Thanks for the patch Helior.
I try it with different profiles with extra fields and it works like a charm except one point.

I added an extra field in the billing and shipping profile. This field is an option field where the user can choose a civility between Miss, Mister...
This field is not hidden when i click the checkbox "My informations of shipping is the same as my informations of billing".

Is it because this field is an option field and not a text field ?

corniquet’s picture

Status: Needs work » Patch (to be ported)

I have found a way to fix the problem when the extra field is an option one.

In commerce_customer.checkout_customer.inc line 141, I have replaced the following:

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 $lang => $items) {
  					foreach ($items as $delta => $item) {
  						$pane_form[$field_name][$lang][$delta]['#access'] = FALSE;
  					}
  				}
  			}
  		}

with this new code:

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 $lang => $items) {
  					foreach ($items as $delta => $item) {
  						$pane_form[$field_name][$lang][$delta]['#access'] = FALSE;
                                                                                    
  						// special treatment for option field
  						$pane_form[$field_name][$lang]['#access'] = FALSE;
  					}
  				}
  			}
  		}

I'm sorry, i don't know how to make patch. Peharps, Helior, you could add this 2 lines in a new patch ?

rszrama’s picture

Category: bug » feature
Status: Patch (to be ported) » Closed (fixed)

Thanks for the report, corniquet, but you should really post it as a separate issue. This issue was for the initial feature request and shouldn't be modified for any follow-up bug reports. When you post your follow-up bug report, please provide the precise steps for reproducing the error - I need to know exactly what field type you mean by "option."

willieseabrook’s picture

Just noting here for others, with various solutions floating around (in commerce shipping, and multiple in commerce), that I was able to apply the patch in #30 from http://drupalcode.org/project/commerce.git/patch/8d5c51f to a Commerce 1.3 install and the feature is working nicely, after adjusting the settings in the billing/shipping checkout pane configuration. It's a very nice solution.

I applied the patch because I was a little too nervous to upgrade to dev.

rszrama’s picture

fwiw, there's no reason to be nervous about upgrading to dev at this point. Just run update.php and you'll be fine. We've had at least one follow-up patch to this functionality through a separate issue, so you're going to have an incomplete feature on the site w/ just that patch.

Anonymous’s picture

Status: Closed (fixed) » Needs work

drupal_strtolower() also replaces the capital 'L' of Lieferadresse into lieferadresse, which is wrong in German. Should I create a separate bug report?

rszrama’s picture

Status: Needs work » Closed (fixed)

No need; you can just review #1718958-1: Update the profile copy checkbox text to work better internationally if you have a minute. : )

brephraim’s picture

Component: Customer » Order
Status: Closed (fixed) » Active

Can we get this functionality integrated with admin/commerce/order/add?

joshmiller’s picture

Component: Order » Customer
Status: Active » Closed (fixed)

brephraim: Close to what I suggested :) How about you create a new "feature request" that links to this already completed issue...

Josh

joshmiller’s picture

Spoke too soon ... this the issue that brephraim opened:

#1850842: Enable "Profile Copying" for Administration Interface

docans’s picture

Issue summary: View changes

I will like to ask how this can also be done for the manual order page at www.mysite.com/admin/commerce/orders/add