Problem/Motivation

Sometimes you just want to update the record.

Proposed resolution

Provide a checkbox to force update even if the data is saved on an order, effectively bypassing commerce_order_commerce_customer_profile_presave()

Remaining tasks

User interface changes

New checkbox for admin to save order.

API changes

Data model changes

CommentFileSizeAuthor
#21 explanation-text.png53.85 KBrszrama
#17 interdiff-allow_forced_update_of-2528654-1-17.txt19 bytessmccabe
#17 allow_forced_update_of-2528654-17.patch2.82 KBsmccabe
#11 interdiff-2528654-1-7.diff2.73 KBmglaman
#7 allow_forced_update_of-2528654-7.patch2.82 KBjnicola
#4 allow_forced_update_of-2528654-4.patch2.3 KBjnicola
#3 allow_forced_update_of-2528654-2.patch2.24 KBjnicola
#1 allow_forced_update_of-2528654-1.patch2.42 KBjoelpittet

Comments

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Issue tags: +sprint
StatusFileSize
new allow_forced_update_of-2528654-1.patch2.42 KB

UX could use some love but seems to do the trick.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Active » Needs review

Status!

jnicola’s picture

jnicola commented
Priority: Normal » Minor
StatusFileSize
new allow_forced_update_of-2528654-2.patch2.24 KB

Reviewed code. Works as stated. No bugs, no errors, no warnings, all is well for me!

Updated patch to include the following:
1- Better label/title for check box.
2- A description explaining the implication
3- A permission check for "Edit any profile". May be wiser to change to be relevant to orders as this has a far reaching implication on orders possibly and is most relevant to that.

Only other thing I think I'd suggest on this is adding an entity field query to see if there are even orders out there containing this profile ID. If so, display the option, if not, do not.

Leaving as needs review so as others can chime in on the changes I've made and/or recommended.

jnicola’s picture

jnicola commented
StatusFileSize
new allow_forced_update_of-2528654-4.patch2.3 KB

Actually... thinking on it further it just makes sense to check if user can edit (essentially, affect) any order. Checking this box will in turn affect orders. I kept that check along with edit any profile check. Feel free to strike the edit any profile check.

Also I forgot to mention I downgraded this to a minor issue. It would seem the need for this in Drupal Commerce is up for debate.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

Interdiff would be nice between #1 and #3 and #3 and #4.

@see https://www.drupal.org/documentation/git/interdiff

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Priority: Minor » Normal
Status: Needs review » Needs work

@jnicola It's not a minor issue, it causes a crap load of confusion.

+++ b/modules/customer/includes/commerce_customer_profile.forms.inc
@@ -140,13 +147,16 @@ function commerce_customer_customer_profile_form_validate($form, &$form_state) {
-  global $user;
-
...
+  // If we are forcing override, tell the entity that.
+  if (!empty($form_state['values']['force_override'])) {
+    $profile->force_override = TRUE;
+  }
+

Where did this go?

+++ b/modules/order/commerce_order.module
@@ -502,6 +502,12 @@ function commerce_order_commerce_customer_profile_can_delete($profile) {
+  if (!empty($profile->force_override)) {

I can't see how this entity is now getting this property since it's transfer is now missing.

jnicola’s picture

jnicola commented
StatusFileSize
new allow_forced_update_of-2528654-7.patch2.82 KB

Hmmm you're right. That did drop out of my patches somehow. How strange!

Included. I'll let the actual maintainers of this decide to move this around or prioritize as they want. I can see why it's confusing, but I can also see why this was done the way it was.

I personally think a different approach all together might be wiser. Perhaps do something in the validation handler that checks for the conflict, and if the conflict exists and the user has appropriate permissions, prompt the user with a choice of what they desire to do.

This does resolve the issue though, and with a proper description and title makes it's purpose clear. Adding in permissions checks ensures we aren't showing something highly technical to casual user roles... you're in a good spot. Further review should quickly open this up for acceptance if the maintainers desire to include this.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs work » Needs review

Please add interdiffs with your changes so we know what you've changed and what you've left in the patch. It's helpful for anybody reviewing.

Thanks for the doc update. Not sure on the permission around that field yet, may be unnessasary since they wouldn't get to that edit form without those permissions to start with?

jnicola’s picture

jnicola commented

A user may be able to edit profiles, and not orders. Adjusting a profile and forcing it to update relevant orders will in turn affect orders.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

Interesting point. Not directly editing the order just indirectly editing the order... hmmm.

So we shouldn't need user_access('edit any commerce_customer_profile entity') check to my point still?

Maybe we need the other one, but better would be to check which order type it belongs to and check the permissions on that type?

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
StatusFileSize
new interdiff-2528654-1-7.diff2.73 KB

Posting interdiff as requested in #8

Since updating existing profiles adjusts orders I could see needing order edit permission. How does Commerce Address book work? Doesn't that edit existing entries?

Status: Needs review » Needs work

The last submitted patch, 11: interdiff-2528654-1-7.diff, failed testing.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

Based on the interdiff:

  1. +++ b/modules/customer/includes/commerce_customer_profile.forms.inc
@@ -97,12 +97,19 @@
-    '#title' => t('Force Override?'),
...
+      '#title' => t('Force Profile Update'),

    This may be a better title change but maybe lower case the words after, isn't that the convention? Sorry that was my fault too. 

  2. +++ b/modules/customer/includes/commerce_customer_profile.forms.inc
@@ -97,12 +97,19 @@
+  // Check permission to edit ANY profile, and if so display force option.
+  if (user_access('edit any commerce_customer_profile entity')
+    && user_access('edit any commerce_order entity')
+  ) {

    Still not convinced we need both or any of these permission checks. 

  3. +++ b/modules/customer/includes/commerce_customer_profile.forms.inc
@@ -97,12 +97,19 @@
+      '#description' => t('When you edit a user profile associated with an order, instead of updating all relevant orders and possibly causing issues, Drupal Commerce takes the updated profile and instead makes a new profile. Checking this box will prevent creating a new profile and update this profile directly along with all associated orders. This can have far reaching implications so be aware.'),

    Thanks for adding the description. 

  4. +++ b/modules/customer/includes/commerce_customer_profile.forms.inc
@@ -139,6 +146,11 @@
+  // If we are forcing override, tell the entity that.
+  if (!empty($form_state['values']['force_override'])) {
+    $profile->force_override = TRUE;
+  }

@@ -147,16 +159,13 @@
   // Save default parameters back into the $profile object.
   $profile->status = $form_state['values']['status'];
 
-  // If we are forcing override, tell the entity that.
-  if (!empty($form_state['values']['force_override'])) {
-    $profile->force_override = TRUE;
-  }

    Condidering the $profile->status was done in the other function, that's why I put it there, why the move? 

  5. +++ b/modules/customer/includes/commerce_customer_profile.forms.inc
@@ -147,16 +159,13 @@
+  global $user;
+

    Why was this global added if it's not used?

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

re #11 from what I recall it disables and creates a new one, or maybe it just endlessly duplicates things:)

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Issue tags: -sprint +Commerce Sprint
joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

I'm still using patch #1 in production, btw.

smccabe’s picture

smccabe commented
Status: Needs work » Needs review
StatusFileSize
new allow_forced_update_of-2528654-17.patch2.82 KB
new interdiff-allow_forced_update_of-2528654-1-17.txt19 bytes

Hey @joelpittet

I took the changes to title and description from #13 and rolled them into your original #1 patch. I didn't take anything else from the other patches as they didn't make sense to me either. I think this patch is good to go now.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

Thanks this looks RTBC to me:) It's docs updates to #1 for better clarity.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
Status: Needs review » Reviewed & tested by the community

Looks good to me, with #17. I'll mark RTBC and try to ping rszrama before commit.

jnicola’s picture

jnicola commented

Woohoo! Given that last commit contains some of my work I look forward to having authorship on a commit to commerce :)

rszrama’s picture

rszrama commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new explanation-text.png53.85 KB

The good vibes are almost making me feel bad about being a pedant ... but still. : D

It seems a better title for the form element and process here is exactly what the issue title stays, force_update / "Force update". Additionally, I don't think we should have such a long description. In general, checkboxes shouldn't require descriptions and should just use a sufficiently descriptive title. (I'm sure I read about that from Nielsen once but am having a hard time finding the article now ... I just never see checkboxes with descriptions outside of Drupal. : )

In any event, I think this is too much explanation:

We can probably just shorten it to the title being "Force update this profile without creating a copy if necessary." or else put it behind a vertical tab if we truly need something longer. Or if we want, a longer checkbox title could be used in a settings form that enables this feature, which brings us to...

I do agree with the order permission check for the same reasons Jesse pointed out above. Since you are potentially indirectly editing other orders, it makes sense to protect this feature. That said, since we allow customer profile updating already within the context of an order edit form, we probably don't need the customer profile check ... but maybe we can replace that with a check against a variable called commerce_customer_profile_allow_force_update.

In fact, at first I was going to suggest we don't need that variable to be editable outside of code, but now I'm kinda thinking we should make it available and default it to FALSE. That will ensure sites that update and get this feature don't automatically have it appear but essentially have to opt-in from the order settings form.