It could be useful to have a new property exposed to Entity API, the price as a float

CommentFileSizeAuthor
#18 1518084-commerce-expose-amount-floatted-18.patch1.39 KBbojanz
#17 1518084-commerce-expose-amount-floatted-17.patch1.66 KBbojanz
#4 1518084-commerce-expose-amount-floatted-4.patch1.86 KBjsacksick
#1 1518084-commerce-expose-amount-floatted-1.patch1.99 KBjsacksick
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jsacksick’s picture

jsacksick CreditAttribution: jsacksick commented
Status: Active » Needs review
FileSize
1518084-commerce-expose-amount-floatted-1.patch1.99 KB

Here is the patch.

rszrama’s picture

rszrama CreditAttribution: rszrama commented

What would you use this for? For other cases where we're doing comparisons, I'd think it'd be better to just ensure that decimal values entered were properly converted to integer amounts.

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Status: Needs review » Needs work

This is mainly useful for exposing the price as a filter or facet for Search API. But it might also help fix the current rules madness we have in which we force people to enter prices in their internal integer representation instead of their external decimal form.

So massive +1 here.

About the patch:

+  if ($name == 'amount_float') {
+    $data['amount'] = commerce_currency_decimal_to_amount($value, $data['currency_code']);
+  }

I don't think the if {} here is necessary at all. We are only using this setter callback in one place, and we don't have this check in the getter callback either.

jsacksick’s picture

jsacksick CreditAttribution: jsacksick commented
FileSize
1518084-commerce-expose-amount-floatted-4.patch1.86 KB

In fact, I wasn't really conviced by the if but because of some examples I saw in other commerce modules, I added it e.g :

/**
 * Callback for setting customer profile properties.
 *
 * @see commerce_customer_entity_property_info()
 */
function commerce_customer_profile_set_properties($profile, $name, $value) {
  if ($name == 'user') {
    $profile->uid = $value;
  }
}

And again in commerce_line_item : 

/**
 * Callback for setting line item properties.
 *
 * @see commerce_line_item_entity_property_info()
 */
function commerce_line_item_set_properties($line_item, $name, $value) {
  switch ($name) {
    case 'order':
      $line_item->order_id = $value;
      break;
  }
}

Anyway I removed it in this new patch.

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Status: Needs work » Needs review
rszrama’s picture

rszrama CreditAttribution: rszrama commented

I'm not sure it's going to be easier for users to use two different tokens and have to ensure they pick the right one that it would be for users to just have a single token with logic behind the scenes that integrates the conversion invisibly. That said, this is obviously a simpler short term solution... just wonder if we shouldn't go ahead and fix it in the actual integrations.

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Status: Needs review » Reviewed & tested by the community

We cannot change a property in 1.x. So for now we are going to need both. It's clearly not optimal but we cannot break all the rules and custom code out there :)

rszrama’s picture

rszrama CreditAttribution: rszrama commented
Status: Reviewed & tested by the community » Needs review
Issue tags: +1.3 blocker

Oh, I'd definitely never vouch for changing the property itself. In a related issue, fago basically outlined how to write the necessary plugins to perform the same decimal -> integer amount conversions we're doing in entity forms with price fields. There's another issue to do the same for Views and then yet another that walked through how to do this in the Search API. I linked that one to Jonathan, but I'm not sure if it's an actual solution or a hack... and of course I closed that IRC session before updating the issue with the link here. :-/

I'll dig it up, but if we can basically solve all the issues without adding this token (which would itself require a tricky update in the future), I'd prefer to go that route. I'll see if I can relocate that issue and then talk with helior about him taking on the long overdue Rules / Views UI integration fixes.

One way or another, we'll get this fixed for 1.3. Tagging it as a release blocker and will chat it over with helior in the morning.

rszrama’s picture

rszrama CreditAttribution: rszrama commented

Child issues:

Simon Georges’s picture

Simon Georges CreditAttribution: Simon Georges commented

Hi,
Do you plan to commit this before 1.4 release?

bojanz’s picture

bojanz CreditAttribution: bojanz commented
Issue tags: +kickstart blocker

Tagging.

jsacksick’s picture

jsacksick CreditAttribution: jsacksick commented

Well that's not really a kickstart blocker as Ryan doesn't want to commit that we could also define an entity_property_info_alter in one of our kickstart modules that defines a new property with a proper getter and setter callbacks

bojanz’s picture

bojanz CreditAttribution: bojanz commented

Well, resolving it in one way or another is still a kickstart blocker :) I'm fine with moving that to Kickstart if Ryan doesn't want it.
I just feel this issue needs to be addressed either way, since it's important for building a Commerce install with search

michfuer’s picture

michfuer CreditAttribution: michfuer commented

I was working with the commerce_multicurrency module and Commerce Kickstart (ver:7.x-2.0-alpha3)
Using the Currency Selector menu block to switch between currencies will fire the Rules action:
commerce_line_item_unit_price_currency_convert(). The following line in that fn was causing me grief,

$wrapper->commerce_unit_price->amount =
commerce_currency_convert($wrapper->commerce_unit_price->amount->value(),
$wrapper->commerce_unit_price->currency_code->value(), $currency_code);

The problem is that commerce_currency_convert() returns a decimal
but "...commerce_unit_price->amount" is of type 'int'. Looking at the latest version of
Commerce Kickstart (7.x-2.0-beta1+58-dev), the problem still exists, however downloading the
latest (7.x-1.x-dev) version of Drupal Commerce it appears the "...commerce_unit_price->amount"
property is of type 'decimal' and the multi-currency selector works as supposed to.

It seems like this might be pertinent to the discussion regarding integer/decimal conversion for
the price property 'amount'.

pcambra’s picture

pcambra
he/him
Primary language Spanish
Location Asturies
CreditAttribution: pcambra commented
Status: Needs review » Needs work

This patch was included in kickstart and affected Commerce Discounts #1789412-19: Product % discount gives fatal error when including a tax as I don't think we can change amount to be an integer just like that.

There are some cases that will end having floats inside the amount value, i.e. when calculating taxes and changing that to integer will affect there.

I think that this is going to be removed from Kickstart
https://code.drupalcommerce.org/#/c/437/

rszrama’s picture

rszrama CreditAttribution: rszrama commented

It needs to be removed sooner rather than later, because as soon as people start building Rules using the property, it's gotta stay whether I commit it to core or not. :-/

bojanz’s picture

bojanz CreditAttribution: bojanz commented
Status: Needs work » Needs review
FileSize
1518084-commerce-expose-amount-floatted-17.patch1.66 KB

Discussed this with DamZ and we agreed that:

1) We should not modify the datatype for "amount", it is too risky at this point in the release cycle, and causes some of the mentioned issues (though the Discount issue from #15 is probably a Discount bug actually).
2) We should still proceed with adding this property to Commerce.
It is a pretty elegant way of fixing Search API integration. Our "map callback alternative" turned out to be really complex: https://code.drupalcommerce.org/#/c/437/3/modules/commerce_kickstart/com...

Attaching rerolled patch. Also changed amount_float to amount_decimal, which is more accurate. I can revert that if needed.
We can also remove the setter callback and have it be a calculated read-only property (related Rules issue: #1807730: Don't show properties that don't support writing in the selector box for 'Set Data Value'), but I don't see what that brings us (though it might be more correct semantically).

bojanz’s picture

bojanz CreditAttribution: bojanz commented
FileSize
1518084-commerce-expose-amount-floatted-18.patch1.39 KB

Rerolled without the setter callback, marked as a computed, as agreed on.

rszrama’s picture

rszrama CreditAttribution: rszrama commented
Status: Needs review » Fixed

Alrighty, committing this as a read only property at least for the time being. We can work out whether or not it needs a setter or to be hidden in the Rules UI entirely in follow-up commits. From a pure Commerce core standpoint, the fact that we now have a "Price comparison" condition basically mitigates the need for this property to be used by "Data comparison" conditions (as the "Price comparison" condition will elegantly convert currencies in comparing), but obviously that doesn't help our integration with Search API. : )

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