Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stats: Reduxify fetching post likes #10361

Merged
merged 3 commits into from
Jan 6, 2017
Merged

Conversation

youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Jan 2, 2017

As a first step to resolve #10348 I'm creating a likes state subtree under posts.
This is also part of #5046

Nothing to test for now.

@youknowriad youknowriad added [Feature] Stats Everything related to our analytics product at /stats/ [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jan 2, 2017
@youknowriad youknowriad self-assigned this Jan 2, 2017
@youknowriad youknowriad requested a review from timmyc January 2, 2017 10:42
@matticbot
Copy link
Contributor

Copy link
Contributor

@timmyc timmyc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great so far. I wasn't too familiar with the posts/<id>/likes endpoint so I did a bit of digging into the code behind it. The API response also returns ( as I'm sure you know, but putting here for future me 😆 ) found, i_like, site_ID, and post_ID - the last two items are a bit redundant, but I suppose would be of use in a batch operation. Endpoint documentation here.

Currently, in this state implementation, we are only persisting the likes array and not the other data. The problem being, the code that powers the likes array has a hard limit of 90 records on the query - it also appears that the default logic will only return likees that have gravatars via has_gravatar.

So if we have a post that has > 90 likes, or is liked by users without gravatars, the data we are using in the state tree won't properly reflect the total number of likes. The found attribute in the response though is a raw COUNT of all users who have liked a post, no LIMIT or gravatar requirement.

As such, I think we should probably go ahead and persist the entire response object in the state tree - omitting site_ID and post_ID because they are both comprised in the tree. This would allow us to display the proper number of likes on a given post, and potentially a future component that uses this part of the state tree would also want access to i_like. Do you think that is a good idea too?

} );
} );

it( 'should set site ID to false if request finishes unsuccessfully', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this test description read should set post ID to false if request finishes unsuccessfully?

@youknowriad
Copy link
Contributor Author

Good catch @timmyc I naively supposed all the likes will be returned.
I guess this also answers the question of whether we should limit the avatars displayed since we would get a maximum of 90 in any circumstance.

@youknowriad
Copy link
Contributor Author

updated in 4df3ac2 :)

@youknowriad youknowriad force-pushed the add/stats/reduxify-likes branch from 4df3ac2 to 4735e54 Compare January 5, 2017 12:04
@youknowriad
Copy link
Contributor Author

I rebased the branch, do you think you'll have time to take a look again @timmyc ?

@timmyc
Copy link
Contributor

timmyc commented Jan 5, 2017

Looking good now! Thanks for updating that to accommodate all the data.

@timmyc timmyc added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jan 5, 2017
@youknowriad youknowriad force-pushed the add/stats/reduxify-likes branch from 4735e54 to 989195f Compare January 6, 2017 09:00
@youknowriad youknowriad merged commit 96efd04 into master Jan 6, 2017
@youknowriad youknowriad deleted the add/stats/reduxify-likes branch January 6, 2017 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Stats Everything related to our analytics product at /stats/
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stats: Add Likes Details to Summary Page
4 participants