Add Program Analytics page

[tomalec]

Changes proposed in this Pull Request:

Closes #96, #99, #100.

• Creates Program reports UI as described in Reports > Programs UI #96
• Adds wcadmin_gla_table_sort track event for AppTableCard. ⚠️ It adds it also for Dashboard, Product Feed tables as well.
• Adds wcadmin_gla_datepicker_update track event for the DatePickerFilter. It adds it also for Dashboard

Screenshots:

Detailed test instructions:

1. Go to http://localhost/wordpress/wp-admin/admin.php?page=wc-admin&path=%2Fgoogle%2Freports
2. Check if the UI matches the Figma designs and requirements in Reports > Programs UI #96

   • Filters have required options,

   • Metrics are presented correctly
     You can use via temporary, debugging URL param missingFreeListingsData to switch mocked data, to test 3 scenarios:

     • not set or &missingFreeListingsData= - free and paid listings with no missing data (2022 case)
     • &missingFreeListingsData=na only free listings without some metrics (pre Q4 2021, no paid programs)
     • &missingFreeListingsData=anythingelse free and paid programs with missing data (pre Q4 2021, with paid programs)

   • Compare Programs table looks correctly,

   • Track events are being fired (You can execute localStorage.setItem( 'debug', 'wc-admin*' ); in the browser console to get them logged)

     • wcadmin_gla_table_header_toggle
     • wcadmin_gla_table_sort
     • wcadmin_gla_datepicker_update

Changelog Note:

(Optional) Enter a summary of all changes in this Pull Request to appear in the changelog if it's accepted.

Follow-ups:

• Performance metric's deltas / <SummaryNumber> does not render numbers with given precision SummaryNumber's delta does not render floats. woocommerce-admin#5925
• <CheckboxControl> does collapse its right margin, making it impossible to center it, without knowing the implementation details - issue TBC
• Single and Compare program filters, need to be implemented - follow-up issue TBC, related ones Search component does not accept static options Array in autocomplete woocommerce-admin#6061, FilterPicker does not forward all props to Search component woocommerce-admin#6062
  • Plus track event gla_filter
• Track event for switching chart mode gla_chart_tab_click - <Chart> does not expose a nice way to observe when the char type is switched.
• There is some work pending on the API side. These are marked with a "TODO" comment in the code.

Aditional comments:

• One line still makes eslint unhappy. @return {module:@woocommerce/components~DateRangeFilterPicker}
  I guess it's

  • old version of JSDoc parser
  • @woocommerce/components not providing docs and type.

  We can either leave it as is and wait for the times we will improve our (JS)Docs, remove that line, or create a follow-up.

[tomalec]

It seems to me that Travis failure is not related to this PR https://travis-ci.com/github/woocommerce/google-listings-and-ads/jobs/471402632

[jconroy]

Looks like this line here https://github.com/woocommerce/google-listings-and-ads/pull/109/files#diff-6bce386ac5f573cccc06f4a07af482c6d5101b5b4d4f48ab26eb5ad79c7d4c2dR43

[tomalec]

🤦‍♂️ Thanks, got it, fixed.

[jconroy]

All good :high_five:

[tomalec]

The PR is ready for the review (with a list of followups already started ;) )

[ecgan]

I think this should be props[handlerName] ...?

Personally I prefer not to dynamically call a function like this. When we call decorateHandlerWithTrackEvent('onColumnsChange', recordColumnToggleEvent), the 'onColumnsChange' and recordColumnToggleEvent should always go together anyway, I would create specific function like the previous handleColumnsChange which calls recordColumnToggleEvent and onColumnsChange directly, instead of a generic dynamic `decorateHandlerWithTrackEvent. Dynamic things can make code a lil harder to read, more difficult in static analysis and code editor (e.g. "Find reference" in code editor).

[tomalec]

I think this should be props[handlerName] ...?

Right, good catch, thanks :)

I get your point and agree with

Dynamic things can make code a lil harder to read, more difficult in static analysis and code editor (e.g. "Find reference" in code editor).

However, I'd rather avoid manual, repetitive creation of new functions, as having n handleTicChange, handleTacChange, could blur the picture, and is prone to merge and maintenance bugs.

How would you like, the following case:

function decorateHandlerWithTrackEvent( originalHandler, recordEvent ) {
	  return function decoratedHandler( ...args ) {
		    if ( trackEventReportId ) {
			    recordEvent( trackEventReportId, ...args );
		    }
		    
		    // Call the original handler if given.
		    if ( originalHandler ) {
			    originalHandler( ...args );
		    }
	  };
}

decorateHandlerWithTrackEvent(
	props.onColumnsChange,
	recordColumnToggleEvent
)

The behavior would be a tad different if props will be mutable. This is not the case, is it?

See b24c817 (#109)

[ecgan]

That looks better. 🙂 👍

BTW I think I just spotted a problem. Right now you have this line:

	const { trackEventReportId, ...rest } = props;

which means the onColumnsChange and onSort are inside rest, and then you have:

			<TableCard
				onColumnsChange={ decorateHandlerWithTrackEvent(
					recordColumnToggleEvent,
					props.onColumnsChange
				) }
				onSort={ decorateHandlerWithTrackEvent(
					recordTableSortEvent,
					props.onSort
				) }
				{ ...rest }
			/>

which means the functions inside rest would override those onColumnsChange and onSort that you have coded. Right?

I think you could try this:

	const { trackEventReportId, onColumnsChange = () => {}, onSort = () => {} ,  ...rest } = props;

and also with that you don't need to check if ( originalHandler ) because we specify default empty function.

[tomalec]

Thanks for the catch!

Would you mind

			<TableCard
				{ ...rest }
				onColumnsChange={ decorateHandlerWithTrackEvent(
					recordColumnToggleEvent,
					props.onColumnsChange
				) }
				onSort={ decorateHandlerWithTrackEvent(
					recordTableSortEvent,
					props.onSort
				) }
			/>

So we will not have to sprout those names many times in many places and define no-ops?
I'd prefer one if rather than many defined values, as this is one point to check, and makes it clear that the value is not needed, required nor used specifically.

[ecgan]

Usually I specify the default values for the following main reasons:

1. To convey the expected type / value. By looking at const { onSort = () => {} } = props;, I know onSort is expected to be a function. (Although the on... prefix would already indicate it is a callback function 😄 ) (And yeah JSDoc helps in that regard too 😄 ).
2. More importantly, I don't have to worry about IF checking throughout the whole component or function. I know it is a function, regardless of whether it is supplied by the parent or the default one. Currently we only have one IF check, but if it grows to be more complicated, then we would need to have multiple same if ( handler ), which would then make us wonder "why didn't we specify it as default value in the first place" 😂 .

[tomalec]

1. Yep, I was used to delivering type in JSDoc or in TS. Especially, usually, I need to know the type when I'm outside of the component. Also, default value still does not mean that the function/component cannot accept {Function | Object | Boolean}. To accept object like EventListener with handleEvent (see why it could https://webreflection.medium.com/dom-handleevent-a-cross-platform-standard-since-year-2000-5bf17287fd38)

2. I don't get it. Why would you want to check that? Right now we have a single if, so maybe we can revisit the problem once we will have more? If I'd have more checks probably I'd do it differently. Maybe it's just me and my paranoid avoidance of creating and executing functions to micro optimize performance on any DOMEvent listeners.

[jconroy]

so maybe we can revisit the problem once we will have more?

❤️ the discussion but sounds like we are getting stuck in the weeds a bit. Sounds like a healthy thing to revisit.

[ecgan]

I think we should put trackEventReportId="product-feed" inside the component, do we need to expose it as props?

[tomalec]

I made it specifically here. As described in the commit message:
ba43468 (#109)

To have a single source to provide same trackEventReportId to its descendant components, and a single place to update a hard-coded value

I like to have hard-coded values in one place, not scattered around the codebase. Also, to me ProductFeed, Dashboard, ProgramsReport, ProductReport are the "reports" that should have their Ids.
ProductFeedTableCard, CompareProgramsTableCard, AllProgramsTableCard should use the Id of whatever the report that's employing them.

[ecgan]

I like to have hard-coded values in one place, not scattered around the codebase.

But right now it's still kinda scattered in those TableCard's parent components, no? If we want to centralize in one place, we could have something like report-ids.js that exports something like:

export default {
PRODUCT_FEED: 'product-feed'
}

I'm neutral about this, go ahead if you like to do this or you can ignore it 😄

ProductFeedTableCard, CompareProgramsTableCard, AllProgramsTableCard should use the Id of whatever the report that's employing them.

The way I see it is that those TableCard components are not meant to be shared, they are meant to be used directly by the parent report component, they "assist" the parent report component in rendering, so it's fine for them to have trackEventReportId handled internally. If it helps, we could also rename ProductFeedTableCard to ProductFeedReportTableCard to signify it handles its own trackEventReportId.

Usually we expose a prop because we want them to be reusable and configurable by different parent components. In our case here, they are specific to the parent components and not reusable. Exposing the props just seem to put a bit unnecessary "noise" in the parent component.

[tomalec]

But right now it's still kinda scattered in those TableCard's parent components, no?

To me, it's not scattered there, as TableCard's parent is the actual "report" and reportId is its id.

If we want to centralize in one place, we could have something like report-ids.js that exports something like:

I don't think it will improve "scattering" here. as you will still need to write report-specific variable names anyway.

The way I see it is that those TableCard components are not meant to be shared...

I agree. Also, the noise argument speaks to me.
My rationale was: "Whose id it is?", by the proposed values I assumed it's the id of a Dashboard, ProductFeed, etc. Therefore, the id should be attached there, and child components: *TableCard, AppDateRangeFilterPicker, FilterPicker should utilize it - the same value from the same source.

I could also imagine another instance of ProgramsReport rendered side-by-side to compare two slightly different filter results, to construct a custom "dashboard". Then ProgramReports could get two different id's and we are done.

But maybe my assumption was wrong, and the answer to "Whose id it is?" is "individual widget".
Then instead of using:

• record Id dashboard, for events
  • gla_table_sort
• reports-programs
  • gla_table_sort
  • gla_datepicker_update
  • gla_filter
    Every widget should have a unique id, and we could use
• recordId dashboard-table for gla_table_sort
• recordId reports-programs-date-filter for gla_filter
• recordId reports-programs-program-filter for gla_filter
• recordId reports-programs-table for gla_table_sort

But then I see no point in recordId as it's redundant with event name.

[ecgan]

I dig a lil deeper and just realize where you are coming from after I see this:

The above screenshot makes sense to me.

My original comment in this thread was actually specific to this line:

<ProductFeedTableCard trackEventReportId="product-feed" />

Anyway, with what you have described and the "revelation" I got from the screenshot I posted, I think it's pretty fine, not that of a big deal 😄 👍 .

---

But maybe my assumption was wrong, and the answer to "Whose id it is?" is "individual widget".

Every widget should have a unique id, and we could use
recordId dashboard-table for gla_table_sort
recordId reports-programs-date-filter for gla_filter
recordId reports-programs-program-filter for gla_filter
recordId reports-programs-table for gla_table_sort

Yeah, actually that is closer to my thought. 😄

But then I see no point in recordId as it's redundant with event name.

What do you mean by that? I don't get it.

Examples:

• Event name: gla_table_sort, event properties: { reportId: 'dashboard-table', ... }
• Event name: gla_table_sort, event properties: { reportId: 'reports-programs-table', ... }

How are they redundant? 🤔

[jconroy]

Sorry @tomalec this will need a quick rebase after some other PHP changes that been merged into trunk. Keen to get this one merged and see how the product report is shaping up now we have this baseline.

[tomalec]

Sorry @tomalec this will need a quick rebase after some other PHP changes that been merged into trunk

Thanks for the heads up. Conflict solved and merged.

[ecgan]

After looking through the code here with the availableMetrics function and metricsHeaders, I realize we can actually simplify the code even further and we don't need to have Set and .filter() call here.

Currently the availableMetrics function return an array of keys and then we use that to cross match with metricsHeaders. One problem is the keys are in two separate places, prone to typo error, etc.

An idea in my mind now is we can have a function getAvailableMetricsHeaders that will return the table headers array in the right order. An example:

const getAvailableMetricsHeaders = () => {
	const headers = [ { ... }, { ... }, ... ]  // ALL metrics headers in proper order here.

	// remove some headers by index when query parameter match our condition. Or we can use `filter`.
	if (missingFreeListingsData === 'na' ) {
		headers.splice(1, 1)
	}

	// headers returned are in proper order.
	return headers
}

[tomalec]

That depends on how we divide responsibilities between the frontend and back end.

I made it into to based on the following ideas:

• server-side may provide a variably set o metrics - depending on the filters chosen, user data available, pre- vs. post-2021Q4 timing, or any other reasons why Google may or may not have this data.
• I'd rather not rely on the timing or some "simpler-but-not-direct" conditions/reasons for missing data. That would make us more future-proof. See more at Reports > Programs UI #96 (comment)
• client-side JS code is the one responsible for providing translations - metric labels/titles for in various places around the page,
• client-side JS code is the one responsible for providing metrics order.
  For example: Even though the data stays the same, we may decide that for programs we do ABCD for products we do CBDA, for tiny screens we do CA or whatever.
• client and server agree to use metric keys netSales, itemsSold, ... in their communication.

I believe the above makes us more future-proof, stable for API and requirement changes.

Do you think any of my assumptions are incorrect?

[ecgan]

Do you mean that availableMetrics function would be calling back end API?

I think you raise a good point here - I did not think about back end API call when I was making this comment.

I think it's okay to proceed with what you have here. Let's see what we will have when the back end API comes. 😄

[tomalec]

Yes, I used mocked*.js to mock the API responses.

[tomalec]

@ecgan once again thanks for the detailed review :)
I believe I addressed all your comments, and it's ready for another round of review.

I'd appreciate it if you mark as "Resolved" the comments you find addressed correctly and completely.

[tomalec]

I'm merging as the basic feature is here, and approved. We can continue the remaining discussions here and in followup-issues, to improve it further.