-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add endpoint to get play stats #340
Conversation
@@ -14,7 +14,7 @@ class ApplicationController < ActionController::API | |||
rescue_from Pundit::NotAuthorizedError, with: :user_not_authorized | |||
rescue_from ActiveRecord::RecordNotFound, with: :model_not_found | |||
|
|||
has_scope :sorted, default: nil, allow_blank: true | |||
# has_scope :sorted, default: nil, allow_blank: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chvp I had to tmp comment this line, since it was causing problems.
When the line was included our query also tried to order per id, which was not present in the GROUP BY
clause. (Even if the request didn't use any scopes). Do we still use/need this somewhere?
ERROR: column "plays.id" must appear in the GROUP BY clause or be used in an aggregate function at character 188
STATEMENT: SELECT COUNT(*), MAX(played_at) as last_played_at, "plays"."track_id", "plays"."user_id" FROM "plays" WHERE "plays"."user_id" = $1 GROUP BY "plays"."track_id", "plays"."user_id" ORDER BY "plays"."id" DESC, "plays"."track_id" DESC LIMIT $2 OFFSET $3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clients implicitly rely on sorting by id to handle the edge case where new data appears while syncing. It needs at least some ordering, otherwise the database might decide to start ordering differently, leaving the clients with some missed data (even if some data disappears). The reverse by id is the order where there is the least chance for older data to disappear.
899ce50
to
8015363
Compare
This PR adds an extra endpoint:
/plays/stats
through which a user is able to get their aggregated play stats.this endpoint should be enough for most use cases we have for plays. It can replace the calculations to show the simple overall play count, and simplify many calculations for the recently played albums, artists or the stats page.
These stats can be filtered by album or artist (for smaller requests) or the played_at date (for detailed stats)
TODO:Update about real life tests
I did some tests in my development env to verify whether this approach can scale.
This test db contains:
This seemed to be no problem - most pages with stats are returned between 200 and 350ms. I don't really expect requests to take considerably longer when there are more plays