Revisions #1: State > Users: Add USERS_REQUEST action w/ data-layer support

[bperson]

This is the first chunk coming from my WIP PR to introduce post revisions in calypso: #13367

In the REST API v2, most ressources ( posts, revisions, etc... ) return an author ID, instead of an hydrated ressource with more information ( display name, username ... ). This PR introduces the USERS_REQUEST action, and the necessary code in the data layer to trigger an API v2 request. It then normalises the API response into something similar to what's returned from the API v1 and loads it under the users key in Redux.

In the case of post revisions this gets called after we load a set of revisions, a list of all authors ID is created and used to dispatch a USERS_REQUEST action to request it, before displaying it in the revisions list ( see #13367 for more context )

This code is currently not used by any part of Calypso, to test it you can either run the unit tests, or trigger a USERS_REQUEST manually from the console:

window.dispatch( { type: 'USERS_REQUEST', siteId: <sideID>, ids: [ <userID> ] } );

[seear]

Code looks good and tests well for me.

Question—will this ever need to handle a paging scenario, where the number of users requested is more than received in the first set of results? In my testing I get max 10 results back.

[bperson]

Just a head's up that yesterday's update should work fine but it contains a workaround for pagination (marked with a FIXME): we try to fetch a new page if the one we just received is full (number of items received == number of items per page).

@nylen was kind enough to add the pagination headers to the whitelisted list of headers used in wpcom-proxy-request's iframe on WP.com. Now there are a couple of PRs to bubble up those headers: first in wpcom.js and then in the data-layer.

I don't know whether we should wait for the effort to have proper access to the pagination headers, or whether we should merge this with the current FIXME comment. In the meantime, I will create the other PRs coming out of #13367

[nylen]

wpcom version 5.4.0 has been released with this change (assuming that I did the build process correctly).

I don't know whether we should wait for the effort to have proper access to the pagination headers

Seems OK to me to wait, unless:

1. the PR still works well as currently written (for example, what happens if there are exactly 10 users? there will be an extra network request; will this break anything?)
2. we think it will take a while to get the data-layer changes in

[nylen]

Oh, also, the wpcom.js change that adds headers could break something if there is Calypso code currently expecting headers, which will behave differently when it suddenly receives them. This seems very unlikely, but it's probably worth running the e2e test suite against the PR that upgrades wpcom.js in any case.

[bperson]

wpcom version 5.4.0 has been released with this change (assuming that I did the build process correctly).

hehe, it's been done correctly :) , #14157 bundles the shrinkmap update + the data layer updates.

the PR still works well as currently written (for example, what happens if there are exactly 10 users? there will be an extra network request; will this break anything?)

Not optimal + I don't find the assumption that a pagination system gets us "full" pages to be extremely safe. ( I haven't dug in WP REST API codebase but I've seen pagination systems out there with an extra filtering step after getting paginated data from a database where you could end up with "not full" pages in the middle of the dataset, I hope WP isn't doing it though :D )

Oh, also, the wpcom.js change that adds headers could break something if there is Calypso code currently expecting headers, which will behave differently when it suddenly receives them. This seems very unlikely, but it's probably worth running the e2e test suite against the PR that upgrades wpcom.js in any case.

Yup, I minimised the amount of changes introduced - at least in the data layer - in #14157 so we should be safe in there. But for any code that isn't using the data layer, this is a very real possibility :)

[nylen]

I hope WP isn't doing it though :D

😬

I don't see anything like this in the users endpoint code (#, #) but for posts this is definitely possible: #

[matticbot]

@bperson This PR needs a rebase

[bperson]

Since #14157 got merged and this one rebased on the latest master, this is "free" to be merged. Any last minute review @nylen @seear @youknowriad ?

Merging this one will unblock #14241

[seear]

@bperson when I dispatch a request from the console for a site with a lot of users, I see the first page come back, then no further network requests:

window.dispatch( { type: 'USERS_REQUEST', siteId: 89813773 } );

Should I be seeing the further requests?

This also begs a further question—would we ever really want to make, for example, 48 requests to fetch all these users?

[seear]

I see the first page come back, then no further network requests

My mistake, I didn't have the latest commit. Testing again...

[seear]

Ok, I see the full 48 responses now :)

So, my second question stands—are we comfortable with potentially having this much network traffic triggered automatically?

[bperson]

My mistake, I didn't have the latest commit. Testing again...

yep I ended up push-forcing a lot, sry about that :/

So, my second question stands—are we comfortable with potentially having this much network traffic triggered automatically?

For its current use case - revisions -, I would be confortable with it. We'll be limited by the number of unique authors that edited the same post: https://github.com/Automattic/wp-calypso/pull/14241/files#diff-f6f865948e5e106b927667c32c51bde5R87

Considering the core revisions API doesn't have pagination, I'm pretty sure we will get garbage from the revisions API (timeout?) OR nuke the browser with too much data from that response before we hit a case where the number of unique authors that edited the same post gets unreasonably large.
If we want to be ultra safe, we could only request users from "visible" revisions but it would be even better to introduce that notion of "visible" revisions once we're able to lazy-load revisions from the API imho.

[seear]

Ok, good stuff 👍