Add leaderboard by campaign #100
Conversation
Pull Request Test Coverage Report for Build 9155957672Details
💛 - Coveralls |
844d162 to
73594d8
Compare
| ON ("campaigns_period"."campaign_id" = "campaigns_campaign"."id") | ||
| WHERE "campaigns_campaign"."uuid" = %s::uuid | ||
| GROUP BY "campaigns_activity"."address" | ||
| ORDER BY 3 DESC) AS LEADERTABLE where address = %s; |
There was a problem hiding this comment.
ORDER BY 3 is referring to total_campaign_boosted_points right? Can we use the name instead as it seems to be a better reference (also if we shift the select clauses around).
| (SELECT "campaigns_activity"."address", | ||
| SUM("campaigns_activity"."total_points") AS "total_campaign_points", | ||
| SUM("campaigns_activity"."total_boosted_points") AS "total_campaign_boosted_points", | ||
| (SUM("campaigns_activity"."total_boosted_points") / SUM("campaigns_activity"."total_points")) AS "last_boost", |
There was a problem hiding this comment.
This can potentially cause a division by zero right?
There was a problem hiding this comment.
Yes, we can fix it doing it in the serializer:
#100 (comment)
| (SELECT "campaigns_activity"."address", | ||
| SUM("campaigns_activity"."total_points") AS "total_campaign_points", | ||
| SUM("campaigns_activity"."total_boosted_points") AS "total_campaign_boosted_points", | ||
| (SUM("campaigns_activity"."total_boosted_points") / SUM("campaigns_activity"."total_points")) AS "last_boost", |
There was a problem hiding this comment.
nitpick: This is the boost multiplier right? What about renaming last_boost to boost_multiplier
| (SELECT "campaigns_activity"."address", | ||
| SUM("campaigns_activity"."total_points") AS "total_campaign_points", | ||
| SUM("campaigns_activity"."total_boosted_points") AS "total_campaign_boosted_points", | ||
| (SUM("campaigns_activity"."total_boosted_points") / SUM("campaigns_activity"."total_points")) AS "last_boost", |
There was a problem hiding this comment.
I wonder if it's worth to incur on the multiplier boost calculation on the query side (versus client side). What's your opinion on this?
There was a problem hiding this comment.
Good catch, we can remove it from query and do it in the serializer.
| :return: | ||
| """ | ||
|
|
||
| query = """ |
There was a problem hiding this comment.
We should consider the use case where you want to get the data for a single address (which still requires doing all the sum computation on the points and boosted points) but without the ranking.
Maybe we should split this query into two. The "core" query would be:
SELECT
activities."address",
SUM(activities."total_points") AS "total_campaign_points",
SUM(activities."total_boosted_points") AS "total_campaign_boosted_points",
FROM "campaigns_activity" activities
INNER JOIN "campaigns_period" periods ON activities."period_id" = periods."id"
INNER JOIN "campaigns_campaign" campaigns ON periods."campaign_id" = campaigns."id"
WHERE campaigns."uuid" = %s::uuid
GROUP BY activities."address"Then the leaderboard query would use this one to compute the ranks.
There was a problem hiding this comment.
Different endpoint?
We have currently campaigns/{campaignID}/leaderboard/{holder} that returns the mentioned data with the position, because is necessary to share with the client that this address is in that position of the leaderboard.
There was a problem hiding this comment.
Ah ok. I didn't know that the ranking would have to be part of the response for the single "holder" but it makes sense 👍
| @@ -13,4 +13,14 @@ | |||
| views.RetrieveCampaignView.as_view(), | |||
| name="retrieve-campaign", | |||
| ), | |||
| path( | |||
| "campaigns/<str:resource_id>/leaderboard/", | |||
There was a problem hiding this comment.
The campaigns.urls is already prefixed with campaigns/ so we should remove this part of the path.
Closes #83