Skip to content

refactor: optimize nickname update logic in Discord actions #2487

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

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from
Draft

refactor: optimize nickname update logic in Discord actions #2487

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
24 changes: 10 additions & 14 deletions controllers/discordactions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -366,32 +366,28 @@ const updateDiscordNicknames = async (req, res) => {
const totalNicknamesNotUpdated = { count: 0, errors: [] };
const nickNameUpdatedUsers = [];
let counter = 0;
for (let i = 0; i < usersToBeEffected.length; i++) {
const { discordId, username, first_name: firstName } = usersToBeEffected[i];
for (const user of usersToBeEffected) {
const { discordId, username, first_name: firstName, id } = user;
try {
if (counter % 10 === 0 && counter !== 0) {
await new Promise((resolve) => setTimeout(resolve, 5500));
}
if (!discordId) {
throw new Error("user not verified");
} else if (!username) {
throw new Error(`does not have a username`);
}
if (!discordId) throw new Error("user not verified");
if (!username) throw new Error("does not have a username");

const response = await setUserDiscordNickname(username.toLowerCase(), discordId);
if (response) {
const message = await response.message;
if (message) {
counter++;
totalNicknamesUpdated.count++;
nickNameUpdatedUsers.push(usersToBeEffected[i].id);
}
if (response?.message) {
counter++;
totalNicknamesUpdated.count++;
nickNameUpdatedUsers.push(id);
}
Comment on lines +379 to 383
Copy link

Choose a reason for hiding this comment

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

Incorrect success condition for nickname updates category Functionality

Tell me more
What is the issue?

The condition response?.message only checks for the existence of a message property, not whether the nickname update was actually successful.

Why this matters

This could lead to counting failed operations as successful updates if the Discord API returns an error message in the response.message field, resulting in inaccurate reporting of update statistics.

Suggested change ∙ Feature Preview

Check for a proper success indicator instead of just the presence of a message. For example:

if (response?.success || (response?.message && !response?.error)) {
  counter++;
  totalNicknamesUpdated.count++;
  nickNameUpdatedUsers.push(id);
}

Or verify the actual structure of the setUserDiscordNickname response to determine the correct success condition.

Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.

} catch (error) {
totalNicknamesNotUpdated.count++;
totalNicknamesNotUpdated.errors.push(`User: ${username ?? firstName}, ${error.message}`);
logger.error(`Error in updating discord Nickname: ${error}`);
}
}

return res.json({
totalNicknamesUpdated,
totalNicknamesNotUpdated,
Expand Down
2 changes: 2 additions & 0 deletions models/progresses.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,8 +100,10 @@ const getRangeProgressData = async (queryParams) => {
async function getProgressByDate(pathParams, queryParams) {
const { type, typeId, date } = pathParams;
const { dev } = queryParams;
/* eslint-disable security/detect-object-injection */
await assertUserOrTaskExists({ [TYPE_MAP[type]]: typeId });
const query = buildQueryToSearchProgressByDay({ [TYPE_MAP[type]]: typeId, date });
/* eslint-enable security/detect-object-injection */
Comment on lines 101 to +106
Copy link

Choose a reason for hiding this comment

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

Unsafe Dynamic Type Mapping category Design

Tell me more
What is the issue?

The code uses dynamic property access with TYPE_MAP[type] and disables security linting, indicating a potential design flaw in type handling.

Why this matters

Using dynamic property access and disabling security checks makes the code more vulnerable and harder to maintain. A more type-safe approach would improve code reliability and security.

Suggested change ∙ Feature Preview
const typeMappers = {
  user: (typeId) => ({ userId: typeId }),
  task: (typeId) => ({ taskId: typeId })
};

if (!typeMappers[type]) {
  throw new Error('Invalid type');
}

const mappedParams = typeMappers[type](typeId);
await assertUserOrTaskExists(mappedParams);
const query = buildQueryToSearchProgressByDay({ ...mappedParams, date });
Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.

const result = await query.get();
if (!result.size) {
throw new NotFound(PROGRESS_DOCUMENT_NOT_FOUND);
Expand Down
Loading