Site-kit-wp: Introduce utility functions `sumObjectListValue` and `averageObjectListValue`

[felixarntz]

There should be new utility functions sumObjectListValue and averageObjectListValue that iterate over an array of objects (or alternatively an array of arrays) and calculate the sum or average respectively over one value in those objects.

---

_Do not alter or remove anything below. The following sections will be managed by moderators only._

Acceptance criteria

• There should be a function sumObjectListValue( list, fieldName ) in assets/js/util/sum-object-list-value.js:
  
  
  
  • It should iterate over the array list. It should be expected that each value in list is an object or array.
  
  
  • For every item item in list it should add item[ fieldName ] (if set) to a variable.
  
  
  • It should return the resulting numeric value.
  
  
• There should be a function averageObjectListValue( list, fieldName ) in assets/js/util/average-object-list-value.js:
  
  
  
  • It should iterate over the array list. It should be expected that each value in list is an object or array.
  
  
  • For every item item in list it should add item[ fieldName ] (if set) to a variable.
  
  
  • Afterwards it should divide the sum by the number of items in list.
  
  
  • It should return the resulting numeric value. Based on the values aggregated, if they are integers, the return value should also be an integer, i.e. rounded so that it doesn't have any decimal point. Otherwise, it should be returned as floating point value as it is.
  
  
• Move extractForSparkline into assets/js/util/extract-for-sparkline.js, and add an optional indexColumn parameter which defaults to 0:
  
  
  
  • Use row[ indexColumn ] instead of row[ 0 ].
  
  
  • If column contains dots, split the string and traverse along row to that property/key (e.g. if column is metrics.0.values.0, look at row[ 'metrics' ][ 0 ][ 'values' ][ 0 ]).
  
  
  • If indexColumn contains dots, split the string and traverse along row to that property/key (e.g. if indexColumn is keys.0, look at row[ 'keys' ][ 0 ]).
  
  
• The Search Console DashboardClicksWidget and DashboardImpressionsWidget components should be updated to use the new functions instead of relying on the legacy function extractSearchConsoleDashboardData:
  
  
  
  • Split data into first half (main values) and second half (values to compare to).
  
  
  • Call sumObjectList for both halves with clicks or impressions respectively for fieldName.
  
  
  • Then call changeToPercent to compare both totals, similarly to how it's currently done in the legacy function extractSearchConsoleDashboardData.
  
  
  • Call extractForSparkline with the first half, clicks or impressions respectively for column and keys.0 for indexColumn.
  
  

Implementation Brief

• Add a new file in assets/js/util/sum-object-list-value.js:
  
  
  
  • Add a function sumObjectListValue( list: array[Object], fieldName: string ):
  
  

const sumObjectListValue = ( list, fieldName ) => {
    return list.reduce( (acc, item) => {
        return item[fieldName] ? acc + item[fieldName] : acc;
    }, 0 )
}

• Add a new file in assets/js/util/average-object-list-value.js:
  
  
  
  • Add a function averageObjectListValue( list: array[Object], fieldName: string ):
  
  

const averageObjectListValue = ( list, fieldName ) => {
    const allIntegers = list.every( (item) => {
        return Number.isInteger(item);
    } );

    const average = sumObjectListValue( list, fieldName ) / list.length;

    if ( allIntegers ) {
        return Math.round(average);
    }

    return average;
}

• Move extractForSparkline from https://github.com/google/site-kit-wp/blob/358ee855a51784aa89a07e542d223c1ae229bf1e/assets/js/util/index.js#L333-L350 into assets/js/util/extract-for-sparkline.js, and add an optional indexColumn parameter which defaults to 0:
  
  
  
  • Use row[ indexColumn ] instead of row[ 0 ].
  
  
  • If column contains dots, split the string and traverse along row to that property/key (e.g. if column is metrics.0.values.0, look at row[ 'metrics' ][ 0 ][ 'values' ][ 0 ]).
  
  
  • If indexColumn contains dots, split the string and traverse along row to that property/key (e.g. if indexColumn is keys.0, look at row[ 'keys' ][ 0 ]).
  
  
• The Search Console DashboardClicksWidget and DashboardImpressionsWidget components should be updated to use the new functions instead of relying on the legacy function extractSearchConsoleDashboardData:
  
  
  
  • Split data into first half (main values) and second half (values to compare to).
  
  
  • Call sumObjectList for both halves with clicks or impressions respectively for fieldName.
  
  
  • Then call changeToPercent to compare both totals, similarly to how it's currently done in the legacy function extractSearchConsoleDashboardData.
  
  
  • Call extractForSparkline with the first half, clicks or impressions respectively for column and keys.0 for indexColumn.
  
  

Test Coverage

• Add tests to ensure these functions behave as outlined in the ACs.

Visual Regression Changes

• No visual changes will result from this issue.

QA Brief

• All Jest tests should pass (npm run test:js) – no manual QA needed as this is required to merge already

Changelog entry

• Add JS utility functions sumObjectListValue, averageObjectListValue, and enhance capabilities of extractForSparkline function.

[tofumatt]

Based on the values aggregated, if they are integers, the return value should also be an integer, i.e. rounded so that it doesn't have any decimal point. Otherwise, it should be returned as floating point value as it is.

This one is a bit odd to me—wouldn't it be better to always provide the most precision and let whatever calling
component/code determine how it wants to handle rounding?

In this case we'd check every value to see if _any_ are floats, and then only return a float. But I don't know what the use-case is for that that wouldn't be solved by calling Math.round() on the results when needed.

I left the solution in the IB for now, but I'm curious why we need to remove the average's precision without the option to keep it.

(Also, the last two points in the ACs also served as good IBs to me, so I just copied them 😆 )

[felixarntz]

@tofumatt IB ✅

Based on the values aggregated, if they are integers, the return value should also be an integer, i.e. rounded so that it doesn't have any decimal point. Otherwise, it should be returned as floating point value as it is.

This one is a bit odd to me—wouldn't it be better to always provide the most precision and let whatever calling
component/code determine how it wants to handle rounding?

That's a fair point. However, due to the nature of these values and the UI in which they're displayed, we should ensure that the average of integer values will also be displayed as integer values. While a floating point may be technically more accurate, to be aligned with Google service UI we should use integers for those too - for example, it would look a bit odd to see something like "Average Impressions: 43.75" when all "Impression" values are generally integers.

[ryanwelcher]

@felixarntz @tofumatt while working on this, I found a couple of things that are not in the IB that I'm going to address.

averageObjectListValue

The implementation above doesn't take into account missing keys for individual items in the list array when calculating the average. For example, if there are 4 items but only three have the fieldName key, the sum would be correct however the average uses the length of the original list which would return an incorrect average as the sum would include 3 numbers but would be divided by 4.

extractForSparkline
The current implementation uses lodash.map. I'll refactor it to use Array.map instead as it's very minimal to do so.

[felixarntz]

@ryanwelcher Good catch, SGTM!

[wpdarren]

@aaemnnosttv the QA brief is All Jest tests should pass (npm run test:js) Just wondering if this is for the engineers to QA? If not, please could you add some direction on how I might run the test. Assuming it's within terminal? Thanks!

[aaemnnosttv]

Thanks for asking @wpdarren – this one doesn't actually need manual QA since the tests are sufficient and those are already a requirement for the PR to be merged in the first place.

Normally I would ask one of the other engineers to take a quick look over this (it should have been marked QA:Eng anyways), but since @tofumatt and I already gave this a thorough review in CR I don't think additional QA is needed.

QA ✅