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.

Sign up for GitHub

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

Jump to bottom

Leaderboard: Add limit/offset params #256

Closed
wants to merge 1 commit into from
Closed

Leaderboard: Add limit/offset params #256

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 14 additions & 4 deletions botCommands/__snapshots__/leaderboard.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,9 @@ Object {
"fields": Array [
Object {
"name": "In Odin We Trust",
"value": "Be the first to earn a point!",
"value": "1 - user100 [1000 points] :tada:
2 - user101 [999 points]
",
},
],
"title": "Leaderboard",
Expand All @@ -125,7 +127,10 @@ Object {
"fields": Array [
Object {
"name": "In Odin We Trust",
"value": "Be the first to earn a point!",
"value": "3 - user102 [998 points]
4 - user103 [997 points]
5 - user104 [996 points]
",
},
],
"title": "Leaderboard",
Expand Down Expand Up @@ -259,7 +264,9 @@ Object {
"fields": Array [
Object {
"name": "In Odin We Trust",
"value": "Be the first to earn a point!",
"value": "1 - user100 [1000 points] :tada:
2 - user101 [999 points]
",
},
],
"title": "Leaderboard",
Expand All @@ -276,7 +283,10 @@ Object {
"fields": Array [
Object {
"name": "In Odin We Trust",
"value": "Be the first to earn a point!",
"value": "3 - user102 [998 points]
4 - user103 [997 points]
5 - user104 [996 points]
",
},
],
"title": "Leaderboard",
Expand Down
19 changes: 10 additions & 9 deletions botCommands/leaderboard.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,30 +10,31 @@ const leaderboard = {
.split(' ')
.find((word) => word.includes('start='));
let start = sEquals ? sEquals.replace('start=', '') : 1;
start = Math.max(start, 1);
start = Number.isNaN(Number(start)) ? 1 : Math.max(start, 1);

const nEquals = content.split(' ').find((word) => word.includes('n='));
let length = nEquals ? nEquals.replace('n=', '') : 5;
length = Number.isNaN(Number(length)) ? 5 : length;
arinze19 marked this conversation as resolved.
Show resolved Hide resolved
length = Math.min(length, 25);
length = Math.max(length, 1);

const users = await axios.get(
'https://www.theodinproject.com/api/points',
const { data: users } = await axios.get(
`https://www.theodinproject.com/api/points?limit=${length}&offset=${start - 1}`,
);
const data = users.data.filter((user) => guild.members.cache.get(user.discord_id));
Copy link
Member

Choose a reason for hiding this comment

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

I get why you've removed this line where we validate if the user exists in the guild. But without this filtering, we'll end up displaying "UNDEFINED" in our leaderboard. This is what I mean:

image

Even if we keep this filter, it won't have a huge effect because we're always fetching a subset of the leaderboard. One way to tackle this would be to still display the points and rank on the leaderboard with "User left the server" instead of username. Or maybe not do anything at all? I'll also discuss with the team. Do share your thoughts, thanks!

Copy link
Author

Choose a reason for hiding this comment

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

Yeah that was very much the purpose of this comment earlier.
Perhaps we could trigger an API call that removes the user details from the API once they leave or are banned from the guild?

let usersList = '';
for (let i = start - 1; i < length + start - 1; i += 1) {
const user = data[i];
for (let i = 0; i < length; i += 1) {
const user = users[i];
if (user) {
// on user leave guild: remove user from server?
Copy link
Member

Choose a reason for hiding this comment

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

Not sure what the purpose of this comment is.

Copy link
Author

Choose a reason for hiding this comment

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

See below

const member = guild.members.cache.get(user.discord_id);
const username = member
? member.displayName.replace(/!/g, '!')
: undefined;
if (username) {
if (i === 0) {
usersList += `${i + 1} - ${username} [${user.points} points] :tada: \n`;
if (i === 0 && start - 1 === 0) {
usersList += `${start + i} - ${username} [${user.points} points] :tada: \n`;
} else {
usersList += `${i + 1} - ${username} [${user.points} points] \n`;
usersList += `${start + i} - ${username} [${user.points} points] \n`;
}
} else {
usersList += 'UNDEFINED \n';
Expand Down
159 changes: 132 additions & 27 deletions botCommands/leaderboard.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ const { generateLeaderData } = require('./mockData');
/* eslint-disable */
/* eslint max-classes-per-file: ["error", 2] */


class GuildMembersMock {
members;

Expand Down Expand Up @@ -80,66 +79,119 @@ describe('!leaderboard', () => {
"'%s' - command should be its own word!group - no leading or trailing characters",
(string) => {
expect(commands.leaderboard.regex.test(string)).toBeFalsy();
},
}
);
});

describe('callback', () => {
it('returns correct output', async () => {
const members = generateLeaderData(5);
const members = generateLeaderData(5);

axios.get = jest.fn();
axios.get.mockResolvedValue({
data: members,
});

axios.get = jest.fn();
it('returns correct output', async () => {
axios.get.mockResolvedValue({
data: members,
data: members.slice(0, 5),
});

expect(
await commands.leaderboard.cb({
guild: new GuildMock(members),
content: '!leaderboard n=5 start=1',
}),
})
).toMatchSnapshot();
});

it('returns correct output', async () => {
axios.get.mockResolvedValue({
data: members.slice(0, 3),
});

expect(
await commands.leaderboard.cb({
guild: new GuildMock(members),
content: '!leaderboard n=3 start=1',
}),
})
).toMatchSnapshot();
});

it('returns correct output', async () => {
axios.get.mockResolvedValue({
data: members.slice(2, 4),
});

expect(
await commands.leaderboard.cb({
guild: new GuildMock(members),
content: '!leaderboard n=2 start=3',
}),
})
).toMatchSnapshot();
});

it('returns correct output', async () => {
axios.get.mockResolvedValue({
data: members.slice(2),
});

expect(
await commands.leaderboard.cb({
guild: new GuildMock(members),
content: '!leaderboard start=3',
}),
})
).toMatchSnapshot();
});

it('returns correct output', async () => {
axios.get.mockResolvedValue({
data: members.slice(0, 2),
});

expect(
await commands.leaderboard.cb({
guild: new GuildMock(members),
content: '!leaderboard n=2',
}),
})
).toMatchSnapshot();
});

it('returns correct output', async () => {
axios.get.mockResolvedValue({
data: members.slice(0, 2),
});

expect(
await commands.leaderboard.cb({
guild: new GuildMock(members),
content: '!leaderboard n=2 start=wtf',
}),
})
).toMatchSnapshot();
});

it('returns correct output', async () => {
axios.get.mockResolvedValue({
data: members.slice(2),
});

expect(
await commands.leaderboard.cb({
guild: new GuildMock(members),
content: '!leaderboard n=wtf start=3',
}),
})
).toMatchSnapshot();
});

it('returns correct output', async () => {
axios.get.mockResolvedValue({
data: members.slice(9998, 25),
});

expect(
await commands.leaderboard.cb({
guild: new GuildMock(members),
content: '!leaderboard n=25 start=9999',
}),
})
).toMatchSnapshot();
});
});
Expand Down Expand Up @@ -195,66 +247,119 @@ describe('!leaderboard', () => {
"'%s' - command should be its own word/group - no leading or trailing characters",
(string) => {
expect(commands.leaderboard.regex.test(string)).toBeFalsy();
},
}
);
});

describe('callback', () => {
it('returns correct output', async () => {
const members = generateLeaderData(5);
const members = generateLeaderData(5);

axios.get = jest.fn();
axios.get.mockResolvedValue({
data: members,
});

axios.get = jest.fn();
it('returns correct output', async () => {
axios.get.mockResolvedValue({
data: members,
data: members.slice(0, 5),
});

expect(
await commands.leaderboard.cb({
guild: new GuildMock(members),
content: '!leaderboard n=5 start=1',
}),
})
).toMatchSnapshot();
});

it('returns correct output', async () => {
axios.get.mockResolvedValue({
data: members.slice(0, 3),
});

expect(
await commands.leaderboard.cb({
guild: new GuildMock(members),
content: '!leaderboard n=3 start=1',
}),
})
).toMatchSnapshot();
});

it('returns correct output', async () => {
axios.get.mockResolvedValue({
data: members.slice(2, 4),
});

expect(
await commands.leaderboard.cb({
guild: new GuildMock(members),
content: '!leaderboard n=2 start=3',
}),
})
).toMatchSnapshot();
});

it('returns correct output', async () => {
axios.get.mockResolvedValue({
data: members.slice(2),
});

expect(
await commands.leaderboard.cb({
guild: new GuildMock(members),
content: '!leaderboard start=3',
}),
})
).toMatchSnapshot();
});

it('returns correct output', async () => {
axios.get.mockResolvedValue({
data: members.slice(0, 2),
});

expect(
await commands.leaderboard.cb({
guild: new GuildMock(members),
content: '!leaderboard n=2',
}),
})
).toMatchSnapshot();
});

it('returns correct output', async () => {
axios.get.mockResolvedValue({
data: members.slice(0, 2),
});

expect(
await commands.leaderboard.cb({
guild: new GuildMock(members),
content: '!leaderboard n=2 start=wtf',
}),
})
).toMatchSnapshot();
});

it('returns correct output', async () => {
axios.get.mockResolvedValue({
data: members.slice(2),
});

expect(
await commands.leaderboard.cb({
guild: new GuildMock(members),
content: '!leaderboard n=wtf start=3',
}),
})
).toMatchSnapshot();
});

it('returns correct output', async () => {
axios.get.mockResolvedValue({
data: members.slice(9998, 25),
});

expect(
await commands.leaderboard.cb({
guild: new GuildMock(members),
content: '!leaderboard n=25 start=9999',
}),
})
).toMatchSnapshot();
});
});
Expand Down