Repository navigation
Conversation
… report logic with the Site Goals widget via a new reports module.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several new Key Metric widgets related to sales performance, including 'Total sales', 'Sales rate', 'Sales engagement rate', and various 'driving sales' breakdowns. The implementation adds new constants, widget configurations, and tile components, along with updated logic for report options and row mapping in the Analytics 4 module. I have reviewed the code and the provided feedback. The reviewer's comment regarding the duplication of primary ecommerce event resolution logic is valid and actionable, as it directly addresses the DRY principle outlined in the project's style guide. However, the comment regarding the columns constant definition is not actionable in the context of the provided code, as the component-specific columns depend on the component's scope and context, and moving them outside would not improve the code's quality or performance in a meaningful way.
| // `getPrimaryEcommerceEvent` derives from `getDetectedEvents` | ||
| // but has no resolver of its own, so resolve the detected | ||
| // events first and read the derived value once they're in. | ||
| await registry | ||
| .resolveSelect( MODULES_ANALYTICS_4 ) | ||
| .getDetectedEvents(); | ||
| const primaryEvent = registry | ||
| .select( MODULES_ANALYTICS_4 ) | ||
| .getPrimaryEcommerceEvent(); |
There was a problem hiding this comment.
The logic for resolving the primary ecommerce event is duplicated across several new key metric tile definitions. This violates the DRY (Don't Repeat Yourself) principle mentioned in the repository's style guide (line 83).
Affected tiles:
KM_ANALYTICS_TOTAL_SALES(line 1861)KM_ANALYTICS_SALES_RATE(line 1908)KM_ANALYTICS_SALES_ENGAGEMENT_RATE(line 1971)KM_ANALYTICS_TOP_TRAFFIC_CHANNELS_DRIVING_SALES_RATE(line 2028)KM_ANALYTICS_SALES_BY_VISITOR_TYPE(line 2074)KM_ANALYTICS_SALES_BY_COUNTRIES(line 2120)KM_ANALYTICS_TOP_AUTHORS_DRIVING_SALES(line 2166)KM_ANALYTICS_TOP_PAGES_DRIVING_SALES(line 2212)
To improve maintainability, you could extract this logic into a helper function. For example:
async function getPrimaryEcommerceEvent( registry ) {
// `getPrimaryEcommerceEvent` derives from `getDetectedEvents`
// but has no resolver of its own, so resolve the detected
// events first and read the derived value once they're in.
await registry
.resolveSelect( MODULES_ANALYTICS_4 )
.getDetectedEvents();
return registry
.select( MODULES_ANALYTICS_4 )
.getPrimaryEcommerceEvent();
}Then, you can use this helper in each getTileData function, for instance:
getTileData: createKeyMetricTileDataLoader(
async ( dates, registry ) => {
const primaryEvent = await getPrimaryEcommerceEvent( registry );
if ( ! primaryEvent ) {
return [];
}
// ...
}
)This will make the code cleaner and easier to maintain.
References
- Cross-file duplication / DRY — identical logic that should be extracted into a shared helper, trait, or utility. (link)
|
Size Change: +7.75 kB (+0.23%) Total Size: 3.43 MB 📦 View Changed
ℹ️ View Unchanged
|
… IDs left over from the reports.ts extraction.
zutigrm
left a comment
zutigrm
left a comment
There was a problem hiding this comment.
Thanks @mehidi258 , few things to align with before it is ready for MR.
The refactor changes the percentage denominator for two of the eight drivers. TopAuthorsGoalDriver used to fetch a separate site-wide total report (analytics-4_site-goals_top-authors-total_${goalType}) and divide by it; TopTrafficChannelsGoalDriver used totalReport ?? sum of rows. Both now always divide by the sum of the returned rows.
The regenerated VRT reference is the proof — same fixture, same authors:
AuthorName1 Before: 30.5% After: 42.7%
AuthorName2 Before: 24.7% After: 34.6%
AuthorName3 Before: 16.2% After: 22.7%
It's a change of meaning, not rounding: "share of all sales" (summing to 71.4% of a site total of 1000) becomes "share of the top-N attributed rows" (summing to 100%). Let's restore the total report for these two drivers and have the Key Metric tiles fetch it too
We are also missing the test that IB mention which would catch the issue above "update the existing goal driver tests to exercise them through it, so the Site Goals widget is proven unchanged by the extraction", no existing goal-driver test was touched. TopAuthorsGoalDriver.test.tsx asserts no percentages, so all 58 tests pass with or without the denominator change.
tofumatt
left a comment
tofumatt
left a comment
There was a problem hiding this comment.
All of the additional stories added here are adding a significant amount of bloat but aren't tested any new visual behaviours/UI elements.
All of the ADDED stories here can be removed; just the diffed ones should remain.
…eedback, and add missing percentage regression coverage for the Top traffic channels goal driver.
…ent/13404-site-goals-selling-products-key-metrics.
|
VRT failure is unrelated to this PR, pre-existing instability on develop. |
tofumatt
left a comment
tofumatt
left a comment
There was a problem hiding this comment.
Most of my comments here apply to all of the components in question, so please have a look through and address the comments in both the files with comments and apply them to similar files as well. 🙂
Please be sure to answer any questions and not just implement changes. I'm curious about the thought process here when asking, so I'd like to understand the original decisions made here even if we do want to change something.
| } ); | ||
|
|
||
| describe( 'getKeyMetricsConversionEventWidgets', () => { | ||
| it( 'includes the Selling products Key Metric tiles under purchase', () => { |
There was a problem hiding this comment.
This test spec is difficult to understand. It reads like all of these tiles are related to "Selling products" under a "Purchase" heading, but "purchase" is an event. How is it "under purchase"? Can you explain/reword this test spec so it more clearly/explicitly defines what's being tested?
There was a problem hiding this comment.
Reworded to 'includes the "Selling products" Key Metrics widgets in the list of widgets mapped to the "purchase" conversion event'.
| function getDates() { | ||
| return registry | ||
| .select( CORE_USER ) | ||
| .getDateRangeDates( { compare: true } ); | ||
| } |
There was a problem hiding this comment.
Conversely, this is borderline overkill, but I'm fine if it stays…
There was a problem hiding this comment.
Refactored to get rid of this now.
| [ | ||
| '.googlesitekit-km-widget-tile__loading', | ||
| '.googlesitekit-km-widget-tile__loading-header', | ||
| '.googlesitekit-km-widget-tile__loading-body', | ||
| ].forEach( ( selector ) => { | ||
| expect( container.querySelector( selector ) ).toBeInTheDocument(); | ||
| } ); |
There was a problem hiding this comment.
This is more lines and less readable than just writing the selectors in the expectations:
| [ | |
| '.googlesitekit-km-widget-tile__loading', | |
| '.googlesitekit-km-widget-tile__loading-header', | |
| '.googlesitekit-km-widget-tile__loading-body', | |
| ].forEach( ( selector ) => { | |
| expect( container.querySelector( selector ) ).toBeInTheDocument(); | |
| } ); | |
| expect( container.querySelector( '.googlesitekit-km-widget-tile__loading' ) ).toBeInTheDocument(); | |
| expect( container.querySelector( '.googlesitekit-km-widget-tile__loading-header' ) ).toBeInTheDocument(); | |
| expect( container.querySelector( '.googlesitekit-km-widget-tile__loading-body' ) ).toBeInTheDocument(); |
But additionally—testing for the header and body is redundant. I suspect this is AI-generated; please audit test assertions like this because:
| [ | |
| '.googlesitekit-km-widget-tile__loading', | |
| '.googlesitekit-km-widget-tile__loading-header', | |
| '.googlesitekit-km-widget-tile__loading-body', | |
| ].forEach( ( selector ) => { | |
| expect( container.querySelector( selector ) ).toBeInTheDocument(); | |
| } ); | |
| expect( container.querySelector( '.googlesitekit-km-widget-tile__loading' ) ).toBeInTheDocument(); |
is better. The MetricTileLoader always outputs those other selectors, so we don't need to test/check for them or its internal layout. Knowing the top-level loading selector is here is enough.
There was a problem hiding this comment.
Applied your suggestion.
| const metricElement = container.querySelector( | ||
| '.googlesitekit-km-widget-tile__metric' | ||
| ); | ||
| expect( metricElement ).toBeInTheDocument(); | ||
| expect( | ||
| // eslint-disable-next-line sitekit/acronym-case | ||
| within( metricElement as HTMLElement ).getByText( '0%' ) | ||
| ).toBeInTheDocument(); |
There was a problem hiding this comment.
Seems better to cast this on definition. (Same elsewhere.)
| const metricElement = container.querySelector( | |
| '.googlesitekit-km-widget-tile__metric' | |
| ); | |
| expect( metricElement ).toBeInTheDocument(); | |
| expect( | |
| // eslint-disable-next-line sitekit/acronym-case | |
| within( metricElement as HTMLElement ).getByText( '0%' ) | |
| ).toBeInTheDocument(); | |
| const metricElement = container.querySelector( | |
| '.googlesitekit-km-widget-tile__metric' | |
| // eslint-disable-next-line sitekit/acronym-case | |
| ) as HTMLElement; | |
| expect( metricElement ).toBeInTheDocument(); | |
| expect( | |
| within( metricElement ).getByText( '0%' ) | |
| ).toBeInTheDocument(); |
| * Builds a fake `select` for exercising a Key Metric tile's display gates | ||
| * without a full registry, mirroring the real selectors' "some events | ||
| * detected" / "every dimension available" combining logic. |
There was a problem hiding this comment.
Why did you create this instead of supplying data to the registry and calling the selectors like we do in all other tests?
There was a problem hiding this comment.
Ahh, Thanks for the flag, It was AI generated i didn't catch it 😔
|
|
||
| const tooltipContent = documentationURL ? ( | ||
| <Fragment> | ||
| { infoTooltip }{ ' ' } |
There was a problem hiding this comment.
This should be localised properly with createInterpolateElement, not a non-translated whitespace character.
See:
site-kit-wp/assets/js/components/ReportErrorActions.js
Lines 159 to 180 in cdf85c8
There was a problem hiding this comment.
Do not add entirely new files as JS. Please check the contents of the PR before submitting for things like this; we should only add new files as TypeScript.
| // The widget-scoped components below are wrapped in `forwardRef`, whose | ||
| // narrow inferred prop type otherwise fails structural checks against the | ||
| // looser prop types TS widget components declare for them (e.g. `WidgetNull`). |
There was a problem hiding this comment.
narrow inferred prop type otherwise fails structural checks against the looser prop types TS widget components declare for them
Can you rephrase this? That's quite technical and difficult to make sense of.
| // Scope widget-specific components to the widget instance so that the | ||
| // component does not need to (re-)specify the widget slug. |
There was a problem hiding this comment.
I have refactored the implementation and updated the come
Hi @tofumatt, thanks for all the feedback here, It's really helpful. Yes, this PR started from AI-generated code, and while I went through and fixed a lot, I clearly missed several things that I should have caught myself before asking for review. Appreciate you taking the time to point them out. |
tofumatt
left a comment
tofumatt
left a comment
There was a problem hiding this comment.
Thanks so much for your work on this PR; I know it was a big one and I think the changes you made are a lot better 👍🏻
Summary
Related issue(s):
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist