When using the price selection block to set the price role when creating an administrative order (see http://drupal.org/node/1872860), if the administrator possesses a priced role but chooses the 'Default" setting in the price selection block, the Default setting is ignored, and the role pricing applicable to the administrative user is applied instead.

This is a bug; function uc_price_per_role_find_price() should return FALSE in such circumstances, instead of returning a role price.

The attached patch, made against 7x.-1.x-dev fixes the issue.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

DanZ’s picture

Status: Active » Needs review

Thanks for the patch! I'll look into this when I get a chance.

rjlang’s picture

Same patch, but now in proper git format.

rjlang’s picture

I do apologize for the multiple patches and this lengthy comment, but I have been thinking more about the price selection block and how uc_price_per_role_find_prices() behaves (or ought to), and I think there's a fundamental ambiguity in the meaning of the "Default" setting. There are really two different behaviors that could be considered "Default":

(1) "Default price." Use the default price that is set for the product, i.e., Product field "sell_price", no matter what the user's combination of roles may be.

(2) "Default for user." Use the default price that would be served up to the current user with his/her assigned roles.

Current behavior (before my last patch): "Default" behaved as "Default for user".

After my patch: "Default" behaves as "Default price"

I have realized that there are use cases that would call for both types of "Default". For example, if default prices are retail and a role price is wholesale, an admin would likely want to see what the catalog looks like to a retailer, then to a wholesaler, by switching between Default price and Wholesale role.

On the other hand, anyone who was used to the previous "Default for user" behavior might still want that behavior and doesn't want the behavior to change underneath them.

So, I think we should have *both* settings as options. But, to make them clear, call them "Default price" and "Default for user", rather than simply "Default". And use the original key '' for "Default for user" so existing users see no change in behavior if they update during a session.

Another issue is what happens if we set the price selection block to a particular role, but the product does not have a price for that role. In the current code, the logic falls through to the normal handling of user roles in order of weights. But I think that it shouldn't behave this way, because two different users with two different sets of roles would experience different prices, even though they've both set the price selection block to the same setting. It seems like if you've set the price selection block to anything other than "Default for user," the behavior should be entirely independent of the user's own roles.

I see two possible choices: if we try to force a role but the product doesn't have a price at that role, go through the hierarchy of *all* enabled roles in order of weight, or just drop immediately to the default price. In the interest of simplicity, I went with the latter in what I'm about to present.

So, I now attach a new patch, made against 7.x-1.x-dev, which supercedes the patch above. This new patch puts both "Default for user" and "Default price" into the price selection block and uc_price_per_role_find_prices() exhibits the following behavior based on the block setting:

"Default for user": use the price that fits the user's combination of roles. (Previous module behavior.) uc_price_per_role_find_prices() falls through to the default handling.

"Default price": use the default price for the product. uc_price_per_role_find_prices() should return FALSE.

specific role: return the price that would apply if the user had ONLY that role, i.e., the role price if there is one, default price otherwise.
uc_price_per_role_find_prices() returns $prices[$rid] if price exists, FALSE otherwise.

DanZ’s picture

I was about to reject your earlier patch for exactly the reasons you stated. Looks like you beat me to it.

There does indeed need to be at least a "default for user" option, as exists now, because the admin user may well have valid roles that should be used.

Yes, there should also be some sort of "non-role price" option. Calling this "Default price" is too vague and confusing.

if we try to force a role but the product doesn't have a price at that role, go through the hierarchy of *all* enabled roles in order of weight, or just drop immediately to the default price.

Of these two, I'd take the second.

In fact, I don't like any of the names so far, and I'm not inspired enough right now. Here's what I have so far (and I don't like any of them).

  • Role-less price
  • Non-role price
  • Default price
  • User default price
  • Standard price
  • Default price
  • Non-block-modified price

All that considered, the right way to do this is to get rid of the role drop-down and have the role selection block have a checkbox for each and every role that can affect prices. The price should then be calculated as it would be for a user with exactly those checked roles. Again, there needs to be a way to distinguish between the role-less price and the current user's price. That would probably be a couple radio buttons. A little AJAX could even make the role list appear and disappear based on the radio button status.

rjlang’s picture

I will work on this. Agreed on the structure (and desirability of AJAX).

DanZ’s picture

Title: Respect 'Default' price selection setting on administrative orders » Improve role price selection block
Category: bug » feature

Thank you very much for your work on this! Ubercart and its many support modules need far more attention than can be provided by the available developers.

Also, note that it is not just the admin order screen. This module modifies the price in a hook_node_load() implementation, so it changes the price everywhere.

If you're feeling ambitious once this is done, you can have a look at some things that really need attention: #1937764: Master Issue: Overhaul shipping, an effort to make Ubercart fulfilment not suck and #821986: Evaluate conditions on list items, to fill a gaping hole in Rules conditions (and allow conditions on lists of products in orders).

rjlang’s picture

OK, third time's a charm, I hope.

This new patch re-implements the price selection block per #6. The block has two radio buttons:

- Default
- Override

Default gives the normal behavior for the user. If Override is selected, AJAX opens up a list of all of the enabled roles as checkboxes, in their weighted order, and the user can choose which roles to consider, and then the behavior is exactly as if the user had that set of roles. As before, the selection takes effect when the form is submitted; the selection is stored in the $_SESSION array (now under $_SESSION['uc_price_per_role'] to be namespace-friendly).

I've tested it with a bunch of different user/role/price combinations (e.g., with mutiple priced roles, not all roles having prices assigned, etc.), and it seems to handle all the corner cases.

I (like you) was not crazy about "Default" as a label in the original configuration, but here I think "Default" versus "Override" is pretty clear as to meaning.

Hope this is useful.

Robert

P.S. I'll look at other open Ubercart issues to see if there's any I can take on, but I'm still trying to wrap my head around Rules API, et al.

DanZ’s picture

Wow, great! I'll try to test this over the weekend. Anyone else here should test it, too.

There are some minor style issues:

+  $roles = $user->roles;
   foreach ($weights as $rid => $weight) {
-    if (isset($user->roles[$rid]) && $enabled[$rid] && isset($prices[$rid])) {
+    if (isset($roles[$rid]) && $enabled[$rid] && isset($prices[$rid])) {

I can only see $roles being used once, so I don't see any point to this change. It was actually more understandable without a temporary variable.

+    $form['price_roles'] = array(
+      '#prefix' => '<div id="price-roles">',
+      '#suffix' => '</div>',

To be really nitpicky, the DIV should probably prepend the name of the module to make it unique on the page. Although I can't imagine that anyone else would use that name, I have seen collisions before, and they're not pretty to troubleshoot.

 /**
+ * Callback for switch form price selection
+ */
+function uc_price_per_role_switch_form_js($form, $form_state) {

Comments need to end in a period. $form_state should always be called by reference, even though you don't use it here. The name of this function should probably be uc_price_per_role_switch_form_callback().

See https://drupal.org/node/1354 for comment standards, including for callback functions. See https://drupal.org/coding-standards/docs for more standards.

+  $show_checkboxes = isset($form_state['values']['price_selection']) ?
+    $form_state['values']['price_selection'] :
+    $price_selection;

I remember from somewhere that operators at line breaks are supposed to go at the beginning of the second line, not the end of the first line. However, I can't find that doc, so maybe I'm imagining that.

You can run the changes through pareview.sh to do an automated standards check. Note that this entire module could use some cleanup, but that's for another issue.

rjlang’s picture

I can only see $roles being used once, so I don't see any point to this change. It was actually more understandable without a temporary variable.

OK.

To be really nitpicky, the DIV should probably prepend the name of the module to make it unique on the page

Agreed. And I was so careful with the $_SESSION variable...

$form_state should always be called by reference, even though you don't use it here. The name of this function should probably be uc_price_per_role_switch_form_callback().

Actually, on this one I followed Drupal 7 developer docs, see here:

https://api.drupal.org/api/drupal/developer%21topics%21forms_api_referen...

In Drupal core, both field_add_more_js() and poll_choice_js() call by value, not by reference:
https://api.drupal.org/api/drupal/modules%21field%21field.form.inc/funct...
https://api.drupal.org/api/drupal/modules%21poll%21poll.module/function/...

Also function ajax_example_simplest_callback(), given as an example in ajax.inc.

For the others, do you want me to tweak and submit a revised patch, or wait for further review/testing?

Robert

DanZ’s picture

Ok, I couldn't find any documented standards for AJAX callbacks. Therefore, the next three paragraphs are just my opinions.

Nevertheless, I'm going to stick my neck way out here and say that I'm right and the documented examples you found aren't as good. The examples should be changed, and a standard should be documented.

$form_state should always be called by reference. The whole nature of that array is that it's something that can change at any time (even if doing so is a bad idea). Besides, calling by reference saves copying the big thing into a stack frame.

Appending _js on an AJAX callback is misleading. It doesn't let you know that it's a callback, and it doesn't contain any actual Javascript.

There are some standards for callbacks at https://drupal.org/coding-standards/docs. They don't fit perfectly for AJAX, but are worth a look, I guess.....

However, the standards for form functions and submit functions are thoroughly documented there, and should be followed. This means some @see and @ingroup directives, among other things.

Fix it now or later? Up to you. I keep getting too busy to test this properly, but I'll get to it eventually.

rjlang’s picture

[assorted style comments]

Makes sense, and you da boss. New patch attached conforming with the above, except:

I remember from somewhere that operators at line breaks are supposed to go at the beginning of the second line, not the end of the first line. However, I can't find that doc, so maybe I'm imagining that.

https://drupal.org/node/318#linelength shows the opposite, with the operator at the end of the line.

Also:

the standards for form functions and submit functions are thoroughly documented [at https://drupal.org/coding-standards/docs], and should be followed. This means some @see and @ingroup directives, among other things...

Among the "other things" mentioned in the coding standards are documentation of all parameters (@param) and all return values (@return), which would seem to be needed for many more functions in this module than the couple I've added (for which I tried to follow the prevailing practice within the module). I'm happy to take a crack at adding the whole shmear (and fixing a few other things that Coder flagged for this module), but perhaps doing the documentation and stylistic cleanup should be a wholly separate patch?

If you would like me to take a sweep through, I'll do that.

DanZ’s picture

perhaps doing the documentation and stylistic cleanup should be a wholly separate patch?

Yes, that's exactly how you do it. Each patch should fix one issue at a time.

There are a few issues left to take care of before the next release of this module (Views, migration/import). After that, it's time for a full style/doc cleanup. This gets it done all at once. The obvious times to do this are when going to a beta or going from a beta to a release candidate.

I'll create an issue for this and put it in "postponed" status until it's time.

Until then, just make sure the new code you add follows the standards. That will make the mess smaller later.

rjlang’s picture

OK. I've subscribed to the new docs issue and will work on it when it's poked.

Meanwhile, I have one more change to this patch. Having tried this block out for a bit, I realized that for most use cases, the admin is going to choose either "Default" or "Override" (and in the latter case, a set of roles) and then keep it that way for the duration of the session and doesn't need to see all the gory details on every page where the block shows up. So I wrapped everything in a collapsible/collapsed fieldset that shows the status (either "Default" or "Override") in the fieldset title. So in most cases, the admin can see the status but it takes up minimal room on the page, and if the admin wants to change status, he/she expands the fieldset, then makes the appropriate changes.