Update Analytics setup to display all available accounts and properties. #9440
Conversation
|
Build files for a721e81 have been deleted. |
|
Size Change: +954 B (+0.05%) Total Size: 1.86 MB
ℹ️ View Unchanged
|
…ts-pagination-fetch.
…ts-pagination-fetch.
Make action as simple function rather than generator.
There was a problem hiding this comment.
Thanks @ankitrox ! This is pretty solid and looks nice overall. I ran into a few problems in my testing which should be relatively quick to address and suggested a few other changes and feedback for your consideration. LMK if you have any questions 👍
|
Thank you @aaemnnosttv for your valuable feedback. I've addressed the feedback provided and tested it at my end. Over to you for another review. Thanks again. |
There was a problem hiding this comment.
Thanks @ankitrox – this is nearly there, just a few things left to address.
| { | ||
| pageToken: nextPageToken, | ||
| } |
There was a problem hiding this comment.
This doesn't work as expected since the fetch action expects a positional argument of just the token rather than this params-like object. It works for the initial request, but won't for subsequent pages.
See the second (highlighted request) invoked manually with a page token here:
It should be as simple as this to fix (may need formatting)
| { | |
| pageToken: nextPageToken, | |
| } | |
| nextPageToken |
There was a problem hiding this comment.
@aaemnnosttv If we pass nextPageToken instead of {pageToken: nextPageToken}, the create-fetch-store throws the error
TypeError: Cannot convert undefined or null to object
fetchGetAccountSummaries will always be called with {pageToken: nextPageToken} and we won't be passing string to it.
| return $this->get_service( 'analyticsadmin' )->accountSummaries->listAccountSummaries( array( 'pageSize' => 200 ) ); | ||
| return $this->get_service( 'analyticsadmin' )->accountSummaries->listAccountSummaries( | ||
| array( | ||
| 'pageSize' => 200, |
There was a problem hiding this comment.
You can test this PR by temporarily setting the pageSize to something small like 5 to verify multiple pages are requested (assuming you have a Google account with more than 5 GA accounts :)
| return API.get( | ||
| 'modules', | ||
| 'analytics-4', | ||
| 'account-summaries', | ||
| {}, | ||
| pageToken, |
There was a problem hiding this comment.
This also needs to be an object – it's currently just passing through the string, which is why this is being split into an object of letters in my last comment.
| pageToken, | |
| { pageToken }, |
|
@aaemnnosttv I've addressed all the feedback points except for the last one which suggests to change This is creating error in fetch store because fetch store is trying to serialise the object with |
@ankitrox this requires the additional comment I left in #9440 (comment) which seems to have been missed :) I'll see if I can address it quickly as the rest seems good to go here. |
|
Looks good now, there was a weird problem with passing Tested with a |
aaemnnosttv
deleted the
enhancement/9348-analytics-display-accounts-pagination-fetch
branch
Summary
Addresses issue:
Relevant technical choices
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