Allow users to drupal_alter a checkout pane's enabled field

subscribe

Patch works. Checkout panes still function as expected.

Arski, sorry if I don't fully understand, but I ran across this module http://drupal.org/project/uc_node_panes and thought it seemed similar in function?

@arski: Good catch, I didn't think about the hook overriding the admin's pane settings. Not marking RTBC as I haven't actually tested it, but it looks like it should be fine to me.

@Todd: this patch lets module developers enable, disable or move existing checkout panes from code, for example if you wanted to reorder the checkout panes depending on the items that are in the cart.

@Longwave it just that it seems uc_node_panes must be doing something similar? "A pane is then generated from the node content. Set the weight of the pane in admin/store/settings/checkout/edit/panes. Now watch to see the node displayed within a pane whenever the associated product is in the customer's cart"

Just trying to be helpful, not argumentative.

@Todd: this patch will let developers alter checkout panes from code, without modifying the code of the original module. If I used uc_node_panes in my store to add some new panes to the checkout page, but it didn't quite do exactly what I needed - say I only want the pane to show up on certain days of the week - I could write another module that adds this feature rather than editing the code of uc_node_panes itself. Then if a new release of uc_node_panes comes out that adds some new features, I could upgrade it easily, while keeping my specific changes in place; this would apply to any module that creates checkout panes, including Ubercart itself. Hopefully that explains it a little better :)

@Longwave Thanks for barring with me. I suspect it's a subtle distinction I'm just not seeing. No need to explain further.

@arski Cool. That's how I ended up here, following the progress of your uc_product_panes. Good luck to all.

Hi Arski, I did a short test with your patch and I can confirm everything still works as expected. I hope to do a bit wider test next week, because I didn't test yet to alter the checkout pane's weight with a custom module. I only did a quick test with altering the checkout pane's enabling and didn't encountered any problems on that yet, but I haven't tried all possible use cases.

There was only one (very) minor thing I had discovered when I had looked at your code. In the function _checkout_pane_list() you had added some trailing spaces.

Subscribing.

Second patch looks good to me.

Yes, though I wouldn't get your hopes up; the maintainers have not been very active in committing or commenting on RTBC patches recently, see http://drupal.org/project/issues/ubercart?status=14

I emailed TR about getting some of these patches committed, and did not receive a reply.

@longwave: First, I have never received any e-mails from you, recently or in the past, either through my drupal.org account or through my ubercart.org account.

Second, 16 RTBC is on par with other large Drupal modules, it is not excessive. And some of those aren't really ready - they have unaddressed comments or untested aspects and should really have their status changed. The remainder will be committed in due time when someone has time to do it. RTBC almost never means "no work left to do" - for the patch in #7 for instance, why does the pane weight default to TRUE when the variable isn't set? Weight values are numeric, right? Has anyone tested this patch with E_ALL set, or are we just introducing more errors that will need to be fixed later? What about Coder errors? Be aware that responsibly committing a patch involves a lot more IMO than just typing the cvs command.

Third, with this current issue, it's a little unfair to complain about nobody committing it, since it was only marked RTBC two days ago. In fact, most of the time this issue has been open it's been waiting for someone to review it. Getting someone to actually spend quality time reviewing proposed fixes is the main bottleneck in this queue.

Finally got time to test your patch (v3). I have tested to alter the weight and enabling of one pane and this works as expected on the checkout- and reviewpage.
I have two things to comment:
- In the function _checkout_pane_list() you added some unnecessary extra spaces.
- On the checkout panes admin form (admin/store/settings/checkout/edit/panes) the panes are displayed in the wrong order if a panes' weight is altered.

Trailing spaces
In your patch you add some unnecessary 'trailing' spaces. Look at the line before '// Set the global default values first', the line before '// Allow other modules to alter the panes.' and the line after 'drupal_alter('checkout_pane', $panes);'.

-  // Allow other modules to alter the panes.
-  drupal_alter('checkout_pane', $panes);
-
+  
+  // Set the global default values first
   foreach ($panes as $i => $value) {
     $panes[$i]['enabled'] = variable_get('uc_pane_'. $panes[$i]['id'] .'_enabled', (!isset($panes[$i]['enabled']) ? TRUE : $panes[$i]['enabled']));
     $panes[$i]['weight'] = variable_get('uc_pane_'. $panes[$i]['id'] .'_weight', (!isset($panes[$i]['weight']) ? 0 : $panes[$i]['weight']));
+  }
+  
+  // Allow other modules to alter the panes.
+  drupal_alter('checkout_pane', $panes);
+  
+  // Make sure that all the required attributes are set

Panes are not displayed in the expected order on the checkout panes admin form
An example: if I set the weight of the billing pane to -10 in an implementation of hook_checkout_pane_alter(), the billing pane is displayed above the list, while I had it's saved weight was set to 4. The expected behavior would be to display the billing pane below the delivery pane (which saved weight was set to 3).
I have looked at the code and this might be a good solution:

Add the following in uc_cart_checkout_panes_form() of uc_cart.admin.inc

$form['panes'][$pane['id']]['#weight'] = variable_get('uc_pane_'. $pane['id'] .'_weight', 0);

after

$form['panes'][$pane['id']]['uc_pane_'. $pane['id'] .'_weight'] = array(
  '#type' => 'weight',
  '#default_value' => variable_get('uc_pane_'. $pane['id'] .'_weight', 0),
);

Attached is a new patch where I fixed above things. Might need some review yet.

@TR In the past six months I also had send you an e-mail via your drupal.org account contact form, twice. Got no reply either. Maybe those e-mails got treated as junkmail in your mail application?

@arski Yeah, I probably made that spaces issue to big in my previous post. But because I already had mentioned it in #17 and you didn't react on that, I had assumed I wasn't clear enough, so I explained it a bit wider.

And yes, the coding standards really suggest to delete extra spaces. The coder module says "There should be no trailing spaces" and Drupal.org says "Lines should have no trailing whitespace at the end."
See http://drupal.org/coding-standards#indenting

I agree with you this is a very minor thing, but I thought it would be worth mentioning because you had added them while they previously weren't there. (I hope you don't interpret this as blaming you, that's not how I mean it.)

But enough about this ;). I hope someone else wants to review the latest patch in #28, so this thing can get committed!

subscribing...

Committed, needs porting to 7.x.

Fixed in 7.x.

Great! I hope #970744: Hook to alter order panes also will get in.

@arski: I think you find your answer in #35