refactor: incorporate referral points to leaderboard score #506
Conversation
| @@ -70,8 +70,8 @@ describe('crowdsourcer', () => { | |||
|
|
|||
| // assert | |||
| expect(mockResponse.status).toHaveBeenCalledWith(403); | |||
| expect(sendMock).toHaveBeenCalledWith({ | |||
| error: `Users cannot be referred twice. Referral code [${referralCode}] will be ignored`, | |||
| expect(sendMock).toHaveBeenNthCalledWith(1, { | |||
There was a problem hiding this comment.
why change to toHaveBeenNthCalledWith?
There was a problem hiding this comment.
actually my bad! i need to update the controller to return early (i personally don't like doing) so send is only called once if its a 403. Ideally we should throw an error but i dont think express will intercept it and relay the correct message to the client
| @@ -1,5 +1,6 @@ | |||
| import { cloneDeep, times } from 'lodash'; | |||
| import { sortLeaderboards, sortRankings, splitRankings } from '../utils'; | |||
| import * as database from '../../../utils/database'; | |||
There was a problem hiding this comment.
nit: let's use absolute paths since it makes refactoring easier (i believe VSCode should be able to do this automatically if you press tab when the suggested import is shown)
| model: () => ({ | ||
| count: countMock, | ||
| }), | ||
| }) as any, |
There was a problem hiding this comment.
Prevent the typescript compiler from throwing error because i did not provide the MongdoDB connection properties. I am only interested in model and count functions for the feature in progress.
| for (let i = 0; i < rankings.length; i += 1) { | ||
| const ranking = rankings[i]; | ||
| ranking.count += await getReferralPoints(ranking.uid); | ||
| } |
There was a problem hiding this comment.
this can be re-written to use a map loop so that we can encourage a more immutable approach to updating ranking objects
| for (let i = 0; i < rankings.length; i += 1) { | |
| const ranking = rankings[i]; | |
| ranking.count += await getReferralPoints(ranking.uid); | |
| } | |
| const finalRankings = await Promise.all(rankings.map(async (ranking) => ({ | |
| ...ranking, | |
| count: ranking.count + await getReferralPoints(ranking.uid), | |
| }))) |
| @@ -130,18 +130,23 @@ export const getLeaderboard = async ( | |||
| sortLeaderboards(leaderboards); | |||
|
|
|||
| const allRankings = leaderboards.map(({ rankings }) => rankings).flat(); | |||
| const userIndex = allRankings.findIndex(({ uid }) => uid === user.uid); | |||
There was a problem hiding this comment.
i take the approach of using findIndex instead of just find to encourage treating variables as immutable objects - this prevents unexpected situations where data is updated in unexpected places
let's use findIndex so that will encourage an immutable data update pattern
| @@ -90,3 +93,12 @@ export const sortLeaderboards = (leaderboards: Interfaces.Leaderboard[]): void = | |||
| return 0; | |||
| }); | |||
| }; | |||
|
|
|||
| export const getReferralPoints = async (id: string): Promise<number> => { | |||
There was a problem hiding this comment.
nit: add a comment for this function
There was a problem hiding this comment.
the naming conventions for files is camel case, so the file name should be ReferralPoints.ts
There was a problem hiding this comment.
you'll also want to update/create a test for the getLeadboard method
Problem
Include referral points in leaderboard calculation
Solution
Calculated referral points on the fly when leaderboard rankings are requested
QA
Other changes (e.g. bug fixes, UI tweaks, small refactors)
Improved
userRanking(getLeaderboardcontroller) object typing by usingArray.findto return an owner ranking thereby preservingUserRankingtype definition