Add Analytics pivot report support to the plugin. #8811
Conversation
benbowler
force-pushed
the
enhancement/8484-pivot-reports
branch
from
bb86462
to
24ce52e
Compare
|
Build files for 1d565f6 have been deleted. |
|
Size Change: +35 kB (+2.31%) Total Size: 1.55 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thanks @benbowler nice work on this one. I just left you a few comments
There was a problem hiding this comment.
@benbowler Thanks, LGTM.
Can you just please update QAB to provide an example snippet for the QA to use, so they can test the pivot report. The referenced issue does not have QAB yet, so there is no JS snippet there either. After this is updated you can move it straight to the MR
|
Thanks @zutigrm, I added an example to the QAB. |
| return acc.sort( ( rowA, rowB ) => | ||
| orderbyItem.desc | ||
| ? rowB.metricValues[ metricIndex ].value - | ||
| rowA.metricValues[ metricIndex ].value | ||
| : rowA.metricValues[ metricIndex ].value - | ||
| rowB.metricValues[ metricIndex ].value | ||
| ); |
There was a problem hiding this comment.
This is tough to read; it's technically all one statement with an implicit return and multiline-ternary. It winds up being tough to glance at and figure out. How about:
| return acc.sort( ( rowA, rowB ) => | |
| orderbyItem.desc | |
| ? rowB.metricValues[ metricIndex ].value - | |
| rowA.metricValues[ metricIndex ].value | |
| : rowA.metricValues[ metricIndex ].value - | |
| rowB.metricValues[ metricIndex ].value | |
| ); | |
| return acc.sort( ( rowA, rowB ) => { | |
| if ( orderbyItem.desc ) { | |
| return rowB.metricValues[ metricIndex ].value - rowA.metricValues[ metricIndex ].value; | |
| } | |
| return rowA.metricValues[ metricIndex ].value - rowB.metricValues[ metricIndex ].value; | |
| } ); |
(Arguably you could also just sort it one way, then reverse the array if orderbyItem.desc is set, but technically I guess this is more efficient and it's not any harder to read 😄 )
| const val = | ||
| ANALYTICS_4_DIMENSION_OPTIONS[ dimension ]( i ); | ||
| if ( val ) { | ||
| observer.next( val ); | ||
| } else { | ||
| break; | ||
| } |
There was a problem hiding this comment.
Lets avoid shortened words for things like value. Even better to say what kind of value this is, as variable names like value, data, etc. don't provide any context. At least this is a bit more explicit:
| const val = | |
| ANALYTICS_4_DIMENSION_OPTIONS[ dimension ]( i ); | |
| if ( val ) { | |
| observer.next( val ); | |
| } else { | |
| break; | |
| } | |
| const dimensionValue = | |
| ANALYTICS_4_DIMENSION_OPTIONS[ dimension ]( i ); | |
| if ( dimensionValue ) { | |
| observer.next( dimensionValue ); | |
| } else { | |
| break; | |
| } |
| * @param {Object[]} pivots The pivots to check. | ||
| * @return {boolean} TRUE if pivot definitions are valid, otherwise FALSE. | ||
| */ | ||
| export function isValidPivots( pivots ) { |
There was a problem hiding this comment.
isValidPivots seems a bit vague, because a pivot itself isn't a clear noun 😅
Would isValidPivotsDefinition or something be more clear? Or arePivotDefinitionsValid? 🤷🏻
Maybe not, but this just seems a bit incompletely named 😅
There was a problem hiding this comment.
I went with isValidPivotsObject to match other validation checks such as isValidDateString and isPlainObject.
| // NOTE: The majority of this classes logic has been abstracted to | ||
| // ReportParsers which contains the shared methods for both | ||
| // Report and PivotReport classes. |
There was a problem hiding this comment.
What is even really left of this class? It seems like it isn't implementing anything itself… why keep it around? 🤔
There was a problem hiding this comment.
IMO we should keep this class around for a consistant structure between both report types and in theory we can use this class in future to implement any non pivot report specific logic.
There was a problem hiding this comment.
Alrighty, works for me 👍🏻
| * @return Google_Service_AnalyticsData_Dimension[] An array of AnalyticsData Dimension objects. | ||
| */ | ||
| protected function parse_dimensions( Data_Request $data ) { | ||
|
|
| * @access private | ||
| * @ignore | ||
| */ | ||
| class ReportParsers { |
There was a problem hiding this comment.
Should this be plural? Wouldn't it be a ReportParser? 🤔
There was a problem hiding this comment.
IMO the class contains multiple parsers so this is the report parsers class.
Summary
Addresses issue:
Relevant technical choices
I implemented pivot reporting mostly to the IB with some naming changes where I thought it would be more clear. I also:
pivots.length, in the pivot store,validateParamsfunction becuase this is handled inisValidPivots.I didn't implement multi date range support but it's trivial to add this while I'm here so I might create a follow up ticket with a draft PR in triage for future use if this is needed.
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