woocommerce / google-listings-and-ads

Sync your store with Google to list products for free, run paid ads, and track performance straight from your store dashboard.
https://woo.com/products/google-listings-and-ads/
GNU General Public License v3.0
47 stars 21 forks source link

Handle "next page" requests in report pages #652

Open tomalec opened 3 years ago

tomalec commented 3 years ago

Describe the bug:

Unfortunately, the paging of Google reports API data does not match the "pagination" concept of our woocommerce/components.

As stated in https://github.com/woocommerce/google-listings-and-ads/issues/589#issuecomment-839724571 the fact, we asked for 10 results, doesn't mean we will have full data for 10 products, or 10 intervals. This means, that to populate the data fields in our UI, we would have to keep on requesting for next_pages until we have them all.

We can limit the problem by increasing per_page to quite a large number, so the majority of merchants would be served by a single request and not "chunked" experience. But still, there may be a case that after waiting (already few sec.) for the first chunk of data, a merchant will need to wait for more to come.

We have to adapt our data utils, UI, and UX for that:

  1. Currently, we totally ignore the next_page property and do not perform any subsequent requests. We need to fetch another page if available.
  2. I believe to serve a better UX and reduce time to LargestContentfulPaint and TimeToInteractive, we should render the results of the first request, and gradually load the upcoming data. Not to keep the user waiting for 10s of seconds for everything to load. However, then we would need some indication for the user that the data is still being loaded, and new rows, data points may appear, and the numbers may increase, @j111q WDYT?
  3. Also we would need to keep sorting and aggregating intervals as they come https://github.com/woocommerce/google-listings-and-ads/issues/589

Expected behavior:

I should eventually see the complete report data.

Actual behavior:

If the dataset is big enough, I would see only the first chunk

Steps to reproduce:

Unfortunately, I do not have any sample data, and mocking it with our simplistic mocking hook, is not possible.

@mikkamp, do you have an idea how to mimic that behavior on the server-side, so JS could receive many "pages" for a single query?

Additional details:

@eason9487 , @ecgan do you have some ideas what would be the easiest,/best way to implement that next_page flow with our wp-data stores?

tomalec commented 3 years ago

@jconroy The solution doesn't seem straightforward to me, I'm not sure if we can finish the potentially new UI, and data flow shortly. So, I'm not sure where to assign it to our project boards.

Here are a few fast but not 100% clean workarounds, I can think of:

  1. We can start with increasing per_page to the maximum, to reduce the risk of our early adopters facing the issue.
  2. Implement subsequent calls for all the pages, but keep merchants waiting for quite long before seeing anything. Given our P2 discussion, on Free Listings being even slower, and ureliable, that may be not worth it.
jconroy commented 3 years ago

I'll wait to see what the team thinks on this one

eason9487 commented 3 years ago

Assume the per_size affects both intervals and products/programs, I think we would not have any easy way to deal it with wp-data. To minimize duplicate API requests and to display correct information to users as soon as possible, the approach I have in mind so far is:

But the downside of this approach is that it adds more complexity to the way we use wp-data.


Unfortunately, I do not have any sample data, and mocking it with our simplistic mocking hook, is not possible.

I have the same problem. 😂 There are some difficulties in verifying the feasibility and complete the implementation in the absence of actual data. Is there any chance we can at least have the mock data to simulate it?

mikkamp commented 3 years ago

do you have an idea how to mimic that behavior on the server-side, so JS could receive many "pages" for a single query?

I was hoping to be able to just read the parameters from the request. By adding the second parameter to the filter you get access to the request which can be used to mimic any type of response. For example if in the file mocks/mc/reports/programs.json you add a page token like:

"test_next_page": "pagetwo"

Then add a second file with report data called mocks/mc/reports/programs-pagetwo.json You can then use a snippet like this to make sure it returns the right response file:

add_filter(
    'gla_prepared_response_mc-reports-programs',
    function( $response, $request ) {
        $file = 'programs.json';

        if ( ! empty( $request['test_next_page'] ) ) {
            $file = "programs-{$request['test_next_page']}.json";
        }
        return json_decode( file_get_contents( __DIR__ . '/google-listings-and-ads/tests/mocks/mc/reports/' . $file ), true ) ?: [];
    },
    10,
    2
);

The problem with this technique is that it doesn't prevent the "real" request from happening. So we can't use the actual parameter "next_page" and have to rename it to "test_next_page". I'm going to try and see if there is a better way to mock the data but for initial testing you should manage to get by using this technique.

mikkamp commented 3 years ago

I created a PR for mocked report responses here https://github.com/woocommerce/google-listings-and-ads/pull/659 The free listings program report has an example how it can return multiple pages.

tomalec commented 3 years ago

We discussed it today on Dev call, the general summary was:

tomalec commented 3 years ago

Given the free listings reports, API is flaky it would be nice to have an error solution as in https://github.com/woocommerce/google-listings-and-ads/issues/270 done first, so we could act on errors coming for individual pages.