Convert the order balance computed field to a base field

+1 for this.

Patch attached, adds:
1. Adds to views data so that order balance can be seen in order views
2. Adds global order balance field that can be used in summaries, consistent with the order summary global field

Note: column is viewable, but can't be used as a filter or sort, as that would be too expensive. We would need to convert balance to an actual field to support filtering and sorting.

Thanks so much for the patch! Applied cleanly to Drupal core 10.1.1, Commerce Core 8.x-2.36.
+1

I don't see an actual computed field being declared? What am I missing?

Actually applying this patch caused the following issue:
🐛 TypeError: CommerceGuys\Intl\Formatter\CurrencyFormatter::format(): Argument #1 ($number) must be of type string, null given, Closed: cannot reproduce

Creating an order with no order items causes a crash in the UI when viewing the Orders view due to no total price number.

TypeError: CommerceGuys\Intl\Formatter\CurrencyFormatter::format(): Argument #1 ($number) must be of type string, null given, called in /var/www/html/web/modules/contrib/commerce/modules/price/src/Plugin/Field/FieldFormatter/PriceDefaultFormatter.php on line 143 in CommerceGuys\Intl\Formatter\CurrencyFormatter->format() (line 93 of /var/www/html/vendor/commerceguys/intl/src/Formatter/CurrencyFormatter.php).

steps to reproduce.
Create an order with no items, and filter the Orders view to drafts so that the order is rendered in the views table. The order total field in the view must be set to default for it to crash.

Seeing this when adding the field:

Also seeing these in the logs:
Warning: Undefined array key "balance" in commerce_promotion_form_views_ui_config_item_form_alter() (line 106 of /var/www/html/web/modules/contrib/commerce/modules/promotion/commerce_promotion.module)

And also this:
Drupal\Core\Entity\Sql\SqlContentEntityStorageException: Column information not available for the 'balance' field. in Drupal\Core\Entity\Sql\DefaultTableMapping->getFieldColumnName() (line 440 of /var/www/html/web/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php).

Not sure how I got the field added to the view prior without the crash.

I think this patch is missing the field definition.

Field definition is not missing. "balance" is a base field: https://git.drupalcode.org/project/commerce/-/blob/8.x-2.x/modules/order...

The core EntityViewsData skips computed fields.

views_data for computed fields must be supplied by each module, as noted in the Change Record referenced in the Issue summary.

There are two changes in this patch:
1. Very simple addition to add the field so it shows up in views
2. More complex addition so that you can also add the balance as a global field, similar to the order total summary field. Maybe it makes more sense to make balance an additional component of that field, rather than introducing a new one.

(We should also consider making balance a true field, rather than a computed one, that way we could support filtering and sorting, too. That would obviously be a more involved change.)

Oh I forgot that we added that, maybe we need to return early if the balance is empty.

Simplified patch that only creates the basic view field for balance, and not the global view field that is more complex.

Additionally, it fixes two issues that were made readily visible from adding this computed view field:
1. In OrderBalanceFieldItemList, we are creating a list item, even if getBalance() returns NULL, which is what causes 🐛 TypeError: CommerceGuys\Intl\Formatter\CurrencyFormatter::format(): Argument #1 ($number) must be of type string, null given, Closed: cannot reproduce , mentioned in comment #3343679-8: Views integration for Order Balance (Computed field) → . A fix is included that doesn't add a list item with a NULL price entity.

2. The first warning mentioned in comment #3343679-9: Views integration for Order Balance (Computed field) → , specifically "Warning: Undefined array key "balance" in commerce_promotion_form_views_ui_config_item_form_alter() (line 106 of /var/www/html/web/modules/contrib/commerce/modules/promotion/commerce_promotion.module)"

would happen for ANY computed field that has been defined for views, not specific to the balance field. There aren't many implementations out there, which is why this might not be obvious.

The fix here is two fold: First, the alter hook in the promotion module is intended only to address 2 specific fields on the Promotion entity. It now short circuits earlier if the entity type is NOT commerce_promotion.

This is also a performance benefit, since we will no longer be loading the entity_field.manager service and the field definitions every time the view ui config form is opened. (It will now only happen if the field's entity_type_id is actually commerce_promotion, which was previously being checked later in the code.)

It also exits if it can't find the field_storage_definition for the field, which will always be the case for a computed field.

Patch works.
There is still this in the logs upon adding the field to the view, prior to saving the view:
Drupal\Core\Entity\Sql\SqlContentEntityStorageException: Column information not available for the 'balance' field. in Drupal\Core\Entity\Sql\DefaultTableMapping->getFieldColumnName() (line 440 of /var/www/html/web/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php).

You can also see a browser error message:
Oops, something went wrong. Check your browser's developer console for more details.

Hi @tonytheferg,

Thanks for the quick check and feedback.

I'm not seeing this error message at all.

I would suspect that it might be a module you have enabled in your instance that I don't have. Similar to the issue with the promotion alter that surfaced. It might be another commerce module, or it might be another contrib module you have.

If we can identify which module it is, that would help, though depending on which one it is, we might not be able to address it in this patch.

Can you provide the full stack trace for that error?

I have aggregation on the view. Maybe I can check a cleaner environment and see what the factor is.

Doesn't come up with the error on a cleaner environment. Maybe views data export, but those are disabled on the view in question.

Presuming you are pulling the message from the watchdog("Recent log messages") view, you can do the following:
1. Edit the watchdog view
2. Add the "variables" column to the view

You don't need to save this...you can just do it temporarily.

In the variables column, you should see a pretty long array of values in that variables column, one of which is the "@backtrace_string" property. That usually has the full stack trace.

Looks like the issue was aggregation.

I've added a custom views handler so that it doesn't attempt to aggregate a computed field.

Changes in #19 fix the errors in #14.

The patch works good. It would be really nice to have the ability to sort the order view based on the order balance to show any orders that have an outstanding balance. That could be a subsequent issue I suppose.

New patch with a different approach:
1. This converts the base field definition of balance from a computed field to an actual stored value.
2. This allows the field to show up in views, without any special views_data handling.
3. Since it is a true field, it can be filtered and sorted, just like total_price.
4. Like total_price, it is still a read-only field.
5. Minimal change in code. Besides the updated base field definition, there is a recalculateBalance() method added that is called by recalculatePrice() and setTotalPaid(). (the computed fielditemlist class is no longer needed.)
6. Besides installing the field, a db revision also backfills the number/currency_code columns for the balance field, all all existing orders will have the correct data.
7. Test Coverage already exists for getBalance().

Note: We should update the title and issue summary, if this approach is acceptable. Did not update them yet, pending review/feedback.

Enhanced patch from #22, with the following small changes:
1. Separate the SQL updates from the field update into a separate hook_update_n().
2. Combine the two SQL updates (number and currency) into a single statement for better performance.
3. Add an optional state variable ("commerce_order_balance_backfill") to skip the backfill, if you wish to do so manually. If you set this to false, then during the database update, it will skip the backfill and provide the SQL statement to you.

Note: local env performance is ~1s per 100k rows. Specifically:
100k - 855ms
1m - 9.411s
5m - 46.008s
12m - 124.195s

A few notes:

We used settings not state for commerce_log:

  if (!Settings::get('commerce_log_skip_update_8202', FALSE)) {

What happens with the following:

 balance__number = total_price__number - total_paid__number

Shouldn't we ad WHERE total_paid__number IS NOT NULL?

And... Your query is missing an execute(), it should be query()->execute() instead.

Would feel more comfortable committing this after gathering some feedback from the community :).

1. State Changed to Setting to be consistent with commerce_log.

2a. A where isn't strictly necessary, because if either total_price__number or total_paid_number is NULL, then the result of the math will be NULL. I've gone ahead and added a WHERE clause, as it doesn't seem to affect performance, even with 12m rows.

2b. The update now outputs the number of orders whose balance were calculated during the backfill.

2c. If the update were re-run for some reason, 2a would only update the rows with a balance of NULL. 2b would output only the newly calculated rows.

3. The missing execute() has been added.

I'll switch patches, run the update and ping back. 👍

Update went well:

drush updb
 ---------------- ----------- --------------- ------------------------------------------------------------------- 
  Module           Update ID   Type            Description                                                        
 ---------------- ----------- --------------- ------------------------------------------------------------------- 
  commerce_order   8217        hook_update_n   8217 - Add the 'balance' field to 'commerce_order' entities.       
  commerce_order   8218        hook_update_n   8218 - Backfill the 'balance' field of 'commerce_order' entities.  
 ---------------- ----------- --------------- ------------------------------------------------------------------- 


 Do you wish to run the specified pending updates? (yes/no) [yes]:
 > 

>  [notice] Update started: commerce_order_update_8217
>  [notice] Update completed: commerce_order_update_8217
>  [notice] Update started: commerce_order_update_8218
>  [notice] Balance calculated for 2226 orders
>  [notice] Update completed: commerce_order_update_8218
 [success] Finished performing updates.

However, orders that are paid the balance__number is properly 0.000000, but for orders that are not paid, the balance__number is NULL.

+1 for RTBC

Retitling, for clarity.

I'm not really comfortable with this change as this feels like a big one...
Since the main issue we're trying to fix is sorting, I'd think we could fix this differently by providing a views field handler that does the subtraction in SQL.

Could we attempt implementing this approach instead?

Setting this to "needs work" as I'd favor the approach described in comment #30 if possible (i.e. the custom views field handler approach).

It is not just sorting, but also filtering.

Unfortunately, a SQL calculation is neither performant nor scalable for filtering and sorting.

@jsacksick Can we proceed then in light of #32?

@tonytheferg: Still not really comfortable with this change but in any case, not planning to include this for sure in the next release that should be out in the upcoming days.

Needs a reroll against 2.37

Reroll of 25. I think I got it right.

Missed a bracket... disregard #36

Reroll of the patch.

Went ahead and committed the patch. Thanks Tom!

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