Skip to content

fix(pivot-grid): Handle currency pivot values with count aggregator - 19.2.x #15428

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Jun 27, 2025

Conversation

IMinchev64
Copy link
Contributor

@IMinchev64 IMinchev64 commented Feb 28, 2025

Closes #15148

Additional information (check all that apply):

  • Bug fix
  • New functionality
  • Documentation
  • Demos
  • CI/CD

Checklist:

  • All relevant tags have been applied to this PR
  • This PR includes unit tests covering all the new code (test guidelines)
  • This PR includes API docs for newly added methods/properties (api docs guidelines)
  • This PR includes feature/README.MD updates for the feature docs
  • This PR includes general feature table updates in the root README.MD
  • This PR includes CHANGELOG.MD updates for newly added functionality
  • This PR contains breaking changes
  • This PR includes ng update migrations for the breaking changes (migrations guidelines)
  • This PR includes behavioral changes and the feature specification has been updated with them

@IMinchev64 IMinchev64 added ❌ status: awaiting-test PRs awaiting manual verification grid: pivot labels Feb 28, 2025
@IMinchev64 IMinchev64 changed the title fix(pivot-grid): Handle currency pivot values with count aggregator fix(pivot-grid): Handle currency pivot values with count aggregator - master Feb 28, 2025
@IMinchev64 IMinchev64 requested a review from MayaKirova March 14, 2025 09:42
@@ -1338,6 +1338,8 @@ export interface PivotGridType extends GridType {
excelStyleFilterMinHeight: string;
valueChipTemplate: TemplateRef<any>;
rowDimensionHeaderTemplate: TemplateRef<IgxColumnTemplateContext>;
/** @hidden @internal */
currencyColumnSet: Set<string>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to store the current currency columns? It doesn't seem to matter what their individual dataType is as we need to change them all anyway based on the IPivotValue's dataType and current selected aggregator.

  • pivot value with dataType = "currency" + selected aggregator with aggregatorName: "COUNT" => All columns associated with it need to set dataType = "number" (doesn't matter what their state was before that).

  • pivot value with dataType = "currency" + selected aggregator with aggregatorName: "Not COUNT"=> All columns associated with it need to set dataType = "currency" (doesn't matter what their state was before that).

Also there's a third numeric data type: GridColumnDataType.Percent which we should also consider. It should also change the column types to number for Count.

private handleCountAggregator() {
const valueMember = this.value.member;
const columns = this.grid.columns;
const isCountAggregator = this.value.aggregate.key.toLowerCase() === 'count';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The key is user set and will not necessarily match with count. Would be better to check aggregatorName or the actual aggregator.


columns.forEach(column => {
const isRelevantColumn = column.field?.includes(valueMember);
const isCurrencyColumn = column.dataType === GridColumnDataType.Currency;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't care about the individual columns dataType. Just if the value's dataType is currency or percent.

  • If it is and has count aggregator -> all column become number.
  • If it does not have a count aggregator -> all columns become currency.

No need to store and manage column state.

this.grid.pipeTrigger++;
}
}

private handleCountAggregator() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a method with the same name and same code in the pivot selector. Perhaps set it somewhere where it can be re-used. Maybe in the pivot grid component since both should have access to it or in as a common util function?

@ChronosSF ChronosSF changed the base branch from master to 19.2.x April 16, 2025 17:04
@ChronosSF ChronosSF changed the title fix(pivot-grid): Handle currency pivot values with count aggregator - master fix(pivot-grid): Handle currency pivot values with count aggregator - 19.2.x Apr 16, 2025

if ((value.dataType === GridColumnDataType.Currency || value.dataType === GridColumnDataType.Percent) && isCountAggregator) {
columns.forEach(column => {
console.log(column.field?.includes(value.member))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove console log.


if ((value.dataType === GridColumnDataType.Currency || value.dataType === GridColumnDataType.Percent) && isCountAggregator) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These checks are a bit repetitive as is the code in the ```if``'s. The same ones are in updateColumnDataTypeByAggregator and in `generateColumnHierarchy`. Maybe just extract it in a common util method that gets the column type for a given PivotValue and reuse it. It would also make the code a bit more readable:

public static updateColumnTypeByAggregator(columns: ColumnType[], value: IPivotValue, isSingleValue: boolean): void {
  const targetColumnType = getColumnDataTypeForValue(value);   
   columns.forEach(column => {
                  if (column.field?.includes(value.member) || isSingleValue) {
                      column.dataType = targetColumnType;
                  }  
              });
}

@IMinchev64 IMinchev64 requested a review from MayaKirova June 18, 2025 15:16
MayaKirova
MayaKirova previously approved these changes Jun 19, 2025
@MarielaTihova MarielaTihova added ✅ status: verified Applies to PRs that have passed manual verification and removed ❌ status: awaiting-test PRs awaiting manual verification labels Jun 27, 2025
@dkamburov dkamburov merged commit 233b5b4 into 19.2.x Jun 27, 2025
5 checks passed
@dkamburov dkamburov deleted the iminchev/fix-15148 branch June 27, 2025 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
grid: pivot version: 19.2.x ✅ status: verified Applies to PRs that have passed manual verification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pivot Grid data type currency is applied on COUNT aggregator as well
4 participants