Problem/Motivation

Noticed this, while working on a "Table with aggregation options" with commerce_price fields, "Apply column function" SUM and "Column aggregation row applies to" set to "the entire result set".

Steps to reproduce

If the first result (node with a few commerce_price fields in my case) in a view configured like above, has no value set (Field is empty), the aggregated total won't be shown. This is because of this condition in https://git.drupalcode.org/project/views_aggregator/-/blob/2.0.x/src/Plu...

if (isset($field->currency_code)) {
  $field->number = $raw_value;
}
else {
  $field->value = $raw_value;
}

if (isset($field->currency_code)) { will evaluate to FALSE if the field value is empty. So it will set $field->value = $raw_value; on a price field, which won't work since the field excepts the value in $field->number.

Proposed resolution

See MR.

Remaining tasks

This also should respect the "Column aggregation row applies to" setting set to "the entire result set". I noticed that if the view is paged, it will only process the first page in https://git.drupalcode.org/project/views_aggregator/-/blob/2.0.x/src/Plu....

Could use the same as in: https://git.drupalcode.org/project/views_aggregator/-/blob/2.0.x/src/Plu...

User interface changes

API changes

-

Data model changes

-

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

daveiano created an issue. See original summary.

daveiano’s picture

Status: Active » Needs review

tr made their first commit to this issue’s fork.

tr’s picture

Version: 2.0.x-dev » 2.1.x-dev
tr’s picture

Status: Needs review » Postponed (maintainer needs more info)
Issue tags: +Needs tests

I'm confused, I don't see anything wrong with the MR, but how does this change correspond to what you said in the issue summary? The block of code you quoted in the summary and the block of code you're changing in the MR are separated by about 400 lines of code.

None of the Commerce-specific code is currently being tested at all. It is not maintainable. If we are going to make exceptions for Commerce fields then we really really really need test cases for those exceptions. Can you write a simple test for this, hopefully without requiring the test to have a full install of Commerce?

Also, under your "Remaining Tasks", those things sound like different problems that should be handled in their own issue. The pager issue might be #3350966: Column aggregation row applies to entire result set incompatible with Views pager #3447012: Does not work with data aggregation + paging - can you take a look at those and contribute there if that is the problem you are seeing? Or if you have a different problem please open a new issue.