google / site-kit-wp

Site Kit is a one-stop solution for WordPress users to use everything Google has to offer to make them successful on the web.
https://sitekit.withgoogle.com
Apache License 2.0
1.22k stars 278 forks source link

Add Google Analytics page views count to WP Admin's Posts list view. #5418

Open jamesozzie opened 2 years ago

jamesozzie commented 2 years ago

Feature Description

Considering adding a total Google Analytics view count on both the pages and posts tab, as per the screenshot below, once Site Kit is set up with Analytics connected. This could be a sortable column, as some of the other columns are within the same list views.

image

This is a feature request on behalf of one user in the support forums.


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

Acceptance criteria

Implementation Brief

For overview and clarity

The approach will use transient data to store post IDs, which maps to associative array holding it’s permalinks and views. ID will be used to match the data. Transient should be stored for 24 hours to minimize calls to generate the report only when necessary - otherwise it will be updated earlier in case when new post is added, or usage of pagination/filter. Visible posts permalinks on admin list view will be used with inListFilter dimension filter to make 1 batch request to pull the data. Transient will be allways replaced with new data, so at most it will hold 100 posts if user set 100 rows per table in the view which in most cases will be default 20. This data will be matched with $post_id in new post column to display views.

New class to handle transient data and post columns

Manage columns

Analytics_4 class

Register method updates

Test Coverage

QA Brief

Changelog entry

iamkingsleyf commented 2 years ago

following

shah-ahmadyusof commented 2 years ago

Thank you and following!

0xWTC commented 2 years ago

please implement this. This feature would be very helpful both for bloggers and SEOs.

EmilioCodes commented 2 years ago

This would be a VERY useful thing for WordPress.

AveCeasar commented 2 years ago

That's exactly on point! I hope Google implements it soon, it would make our lives so much easier ;)

JiveDig commented 2 years ago

+1

An option/setting/filter to save these values as post_meta would be equally as important as showing those values in the admin.

taronavagyan commented 2 years ago

I'd love to see this implemented as well. Keenly following!

Explorergt92 commented 2 years ago

+1 Please!

bscreative commented 2 years ago

+1 This would be incredibly helpful.

candland commented 1 year ago

This would be really nice

jimmymadon commented 1 year ago

@aaemnnosttv @marrrmarrr Have added some preliminary ACs. It makes sense to only implement this for GA4. Just need to add a bit more clarity on the "position" of this column.

I have also split this issue and created #6503 to implement the same column for the Pages list view separately.

tofumatt commented 1 year ago

The above will require Site Kit's assets to be loaded on the Posts wp-admin page similar to how we load them for the WP Dashboard.

In this case, wouldn't it make more sense to load the data in PHP and then serve it directly in the UI? I suppose that means a request on every page load since we don't cache data in PHP, but it seems very excessive to load an entire React app for every table row and then make a getReport call for each.

I know this is venturing into implementation, but I wonder why this needs to be part of the ACs here, as it's more a "how" than a "what" for this issue. πŸ€”

jimmymadon commented 1 year ago

@tofumatt I completely agree - we can discuss this at the IB level and happy to remove this point from the ACs.

tofumatt commented 1 year ago

Cool, in that case ACs here are good; moving to IB πŸ‘πŸ»

derweili commented 1 year ago

@jimmymadon althought I think I'm not able to write the IB, as I'm not sure about how the PHP classes should be structured, some thoughts from my side:

I don't think the sorting functionality should be included in this issue. It's one thing load the data for the shown posts on every pageload, but its something completely different to add this sorting.

The sorting is done by the database query. The post lists are paginated to 10 posts / page (can be increased to 100 posts/page), so client side sorting won't work. To implement sorting, we would need to have the post views already in the database. Therefore we would need to somehow continuously sync the page view data from Analytics to our database and save the page views as a postmeta.

We currently don't store/sync analytics data to the WordPress database. If we want to have this sorting function, we should think in depth about how such a sync could look like and create a separate issue for it.

Just for the sake of completeness: instead of keeping the postview data in the WordPress database and synchronising it continuously, we could solve the pageview sorting "on the fly" with the help of "ORDER BY CASE". But that would still mean that we would have to load the pageviews of all posts. For WordPress installations with a large number of posts, this would mean quite a performance loss. Also, this would probably lead to compatibility problems with other plugins as we would have to edit the sql query directly instead of going via WP_Query. So this approach does not make sense.

jimmymadon commented 1 year ago

@aaemnnosttv @tofumatt Any thoughts on how to implement sorting here based on what @derweili has raised?

tofumatt commented 1 year ago

Oh, actually, that's a great point and sometimes I didn't fully consider πŸ˜…

The filed issue says:

This could be a sortable column, as some of the other columns are within the same list views.

But really only "Title" and "Date" are sortable. Sorting by page views might be useful but adds a lot of complexity and overhead for minimal value, especially as you can view top-performing pages elsewhere in Analytics views.

It's not the primary purpose of this screen to view pages by page views, so I've updated the ACs and omitted the sortable requirement from it.

iamkingsleyf commented 1 year ago

Can one make an input? I think it should display the pageviews (all time or if there is a way change it between days like the sitekit page) then the sorting should be HIGHEST and LOWEST.

Currently our theme has this, views shows all time pageviews to that particular URL.

jimmymadon commented 1 year ago

@nfmohit As @derweili pointed out above, sorting will be hard here unless you figure out something completely different. If it is too cumbersome like we predict, I can remove sorting by page views from the AC and we can continue without it for now.

jimmymadon commented 1 year ago

Apologies, @tofumatt already made that change! πŸ˜„

JiveDig commented 1 year ago

Putting in a vote for some API to (optionally?) use CRUD functions via PHP to get/set view counts in post_meta and/or options so we can use WP_Query to get the most popular or currently trending posts according to GA.

techanvil commented 1 year ago

Hi @nfmohit, thanks for drafting this IB. I have a minor point, and a more substantial aspect that we need to consider.

The minor point is that we need to ensure the column is placed in the right order as per the AC, and can refer to this comment on the linked blog post for an example on how this can be achieved.

More substantially though, while the IB as is stands would work in terms of functionality, I see a problem with the approach.

By following this approach we'd end up with a synchronous series of GA4 report requests being made on the backend before the page is even rendered. This could be any number of requests - the default number of posts per page is 20.

This could introduce an unacceptably high delay to the page loading, which I am sure our users would not appreciate.

We need to batch the Analytics metric retrieval so that only one GA4 report request is made. However, having spent some time looking into it, I don't see this being realistically achievable on the PHP side, due to the way the code is structured for the post list page (for reference, here's the entry point to the loop where the table rows are rendered). Plus - even if we were to find a way, just adding one GA4 report request can introduce a significant delay, with a variety of factors in play.

I think that a better approach here would be to add the column with a placeholder for the data in PHP, and then use JS to make the GA4 report request and populate the data on the client-side. I'm not sure exactly how we'd do this without looking into it, but I'm sure it's achievable. What do you think?

iamkingsleyf commented 1 year ago

Hi

Nice work guys, but how is this coming up? its been over a year now.

nfmohit commented 1 year ago

Nice work guys, but how is this coming up? its been over a year now.

Hi, @iamkingsleyf. Thank you for checking in. Unfortunately, this had to fall back a little since we had some other priorities that came up. We acknowledge this feature has received quite a good number of +1s, and we will work on putting this back in the pipeline again.

iamkingsleyf commented 1 year ago

Nice work guys, but how is this coming up? its been over a year now.

Hi, @iamkingsleyf. Thank you for checking in. Unfortunately, this had to fall back a little since we had some other priorities that came up. We acknowledge this feature has received quite a good number of +1s, and we will work on putting this back in the pipeline again.

Alright thanks.

nfmohit commented 1 year ago

Hi @nfmohit, thanks for drafting this IB. I have a minor point, and a more substantial aspect that we need to consider.

Thank you for the kind IB review, @techanvil! Unfortunately, I had to leave this one hanging and unassigned myself as I had to adhere to other priorities. Apologies for not leaving a comment earlier, I remember to have left one but don't see it now, so it might have not gone through.

Since the approach needs some more thought, I'll unassign myself from here so that someone who has the capacity can pick up the IB review feedback. Thank you!

CC: @bethanylang

eugene-manuilov commented 10 months ago

Thanks, @zutigrm. This is a good start, but we need to make some improvements to IB:

  • Create new class, e.g. Post_List_View_Analytics_Column that will handle the report data and analytics column modification

Where should it be created? Which namespace should it use? I believe it should live in the Google\Site_Kit\Modules\Analytics_4 namespace, right?

  • The class constructor will accept a $modules instance. This will be injected to provide access to the analytics module using it’s get_module( $module_slug ) method
  • Then using module instance check if analytics 4 module is available, e.q $this->modules->get_module(Analytics_4::MODULE_SLUG), and if not, return early
  • ... and use it as param in $this->modules->get_module(Analytics_4::MODULE_SLUG)->generate_posts_report() method to get report data
  • In includes/Plugin.php on init hook, instantiate Post_List_View_Analytics_Column and pass $modules instance
    • Call register method

This is not how it should work. The new class is part of the Analytics_4 functionality. Thus it should be instantiated and registered in the Analytics_4 class. In this case, we don't need to check whether the Analytics 4 module is available. The only thing that we will need to check is that the module is connected.

Also this will let us pass anything that we need from the module class to the column class. For example, we will need to pass the reference to the transients class to be able to read and manage transients. Use the Custom_Dimensions_Data_Available class as an example.

zutigrm commented 10 months ago

@eugene-manuilov Thanks for the feedback. Originally I though it might be more handy to make this class more decoupled for flexibility as it would have access to the $modules which would be easier to extend in the future for some other module, hence the proposal to instantiate it that way. But on the second thought, it does make more sense to group it with Analytics module for instantiation as you suggested, as this will only rely on analytics data.

Updated IB per suggestions. I also included reseting of transient upon property change (something I missed originally), and showing N/A as a value if there is no report data, and check for gathering data state.

mxbclang commented 8 months ago

@eugene-manuilov Reminder on this one please. :)

eugene-manuilov commented 8 months ago

@bethanylang this one i want to discuss on our round table call that is scheduled for the next week. Planned to discuss it in Dec on the same call but we skipped those calls.

eugene-manuilov commented 7 months ago

@ivonac4 i see you have just added this ticket to the Sprint 120, but I don't think we are going to work on it in this sprint. I still wait for our engineering sync call to discuss whether it makes sense to work on this ticket or we should better close it. So, probably better to remove it from the current sprint.

linokilo commented 6 months ago

i would like to see this feature too, very helful one

ivonac4 commented 6 months ago

@eugene-manuilov , @bethanylang latest comment by Eugene is that we are still considering if this ticket should be worked on. Who will decide on that? Should I move it back to Backlog then?

mxbclang commented 6 months ago

@ivonac4 Unfortunately we haven't had the roundtable meeting that @eugene-manuilov mentioned wanting to discuss this in in several months because of other priorities.

@eugene-manuilov Maybe you can discuss this with @aaemnnosttv in your next 1:1 sync? If we're not able to discuss it soon, we should probably move it to Backlog to avoid confusion about when/if it can be added to sprints. Thanks!

ivonac4 commented 5 months ago

@aaemnnosttv I moved this back to Backlog as we haven't discussed it for 26 days now and latest comment by Eugene is that you need to check if it still makes sense to work on this ticket.

mxbclang commented 5 months ago

@ivonac4 There is an active thread on this issue in Slack right now, so it looks like this may be actually moving sooner than later.

@eugene-manuilov In the future, can you please make sure to leave updates on these issues if there is a discussion in Slack or elsewhere? That way we can avoid crossed wires like this. Thank you! cc @eclarke1

folletst commented 5 months ago

Perfect idea! Also, I need to use an eye icon near my post. It will be good if I can use it in the block to place in any area of my blog. Thank you.