-
Notifications
You must be signed in to change notification settings - Fork 18
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 user mbdscore sync workers and cronjob #1838
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -69,6 +69,7 @@ import { runCheckUserSuperTokenBalancesJob } from '../services/cronJobs/checkUse | |||||
import { runCheckPendingRecurringDonationsCronJob } from '../services/cronJobs/syncRecurringDonationsWithNetwork'; | ||||||
import { runCheckQRTransactionJob } from '../services/cronJobs/checkQRTransactionJob'; | ||||||
import { addClient } from '../services/sse/sse'; | ||||||
import { runCheckPendingUserModelScoreCronjob } from '../services/cronJobs/syncUsersModelScore'; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Inconsistent function naming: 'Cronjob' vs 'CronJob' The function name Apply this diff to correct the function name: -import { runCheckPendingUserModelScoreCronjob } from '../services/cronJobs/syncUsersModelScore';
+import { runCheckPendingUserModelScoreCronJob } from '../services/cronJobs/syncUsersModelScore'; 📝 Committable suggestion
Suggested change
|
||||||
|
||||||
Resource.validate = validate; | ||||||
|
||||||
|
@@ -366,6 +367,11 @@ export async function bootstrap() { | |||||
runCheckProjectVerificationStatus(); | ||||||
} | ||||||
|
||||||
// If we need to deactivate the process use the env var NO MORE | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Incorrect environment variable mentioned in the comment The comment mentions Apply this diff to update the comment: -// If we need to deactivate the process use the env var NO MORE
+// To deactivate the process, set SYNC_USERS_MBD_SCORE_ACTIVE to 'false' 📝 Committable suggestion
Suggested change
|
||||||
if (process.env.SYNC_USERS_MBD_SCORE_ACTIVE === 'true') { | ||||||
runCheckPendingUserModelScoreCronjob(); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Inconsistent function naming: 'Cronjob' vs 'CronJob' The function Apply this diff to correct the function call: - runCheckPendingUserModelScoreCronjob();
+ runCheckPendingUserModelScoreCronJob(); 📝 Committable suggestion
Suggested change
|
||||||
} | ||||||
|
||||||
// If we need to deactivate the process use the env var NO MORE | ||||||
// if (process.env.GIVING_BLOCKS_SERVICE_ACTIVE === 'true') { | ||||||
// runGivingBlocksProjectSynchronization(); | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,85 @@ | ||
import { assert } from 'chai'; | ||
import moment from 'moment'; | ||
import { | ||
createDonationData, | ||
createProjectData, | ||
generateRandomEtheriumAddress, | ||
saveDonationDirectlyToDb, | ||
saveProjectDirectlyToDb, | ||
saveUserDirectlyToDb, | ||
} from '../../../test/testUtils'; | ||
import { QfRound } from '../../entities/qfRound'; | ||
import { updateUsersWithoutMBDScoreInRound } from './syncUsersModelScore'; | ||
import { UserQfRoundModelScore } from '../../entities/userQfRoundModelScore'; | ||
|
||
describe( | ||
'updateUsersWithoutMBDScoreInRound() test cases', | ||
updateUsersWithoutMBDScoreInRoundTestCases, | ||
); | ||
|
||
function updateUsersWithoutMBDScoreInRoundTestCases() { | ||
// for tests it return 1, useful to test cronjob logic and worker | ||
it('should save the score for users that donated in the round', async () => { | ||
await QfRound.update({}, { isActive: false }); | ||
const qfRound = QfRound.create({ | ||
isActive: true, | ||
name: 'test', | ||
allocatedFund: 100, | ||
minimumPassportScore: 8, | ||
slug: new Date().getTime().toString(), | ||
beginDate: new Date(), | ||
endDate: moment().add(10, 'days').toDate(), | ||
}); | ||
await qfRound.save(); | ||
const project = await saveProjectDirectlyToDb(createProjectData()); | ||
project.qfRounds = [qfRound]; | ||
await project.save(); | ||
|
||
const user = await saveUserDirectlyToDb(generateRandomEtheriumAddress()); | ||
const user2 = await saveUserDirectlyToDb(generateRandomEtheriumAddress()); | ||
await saveDonationDirectlyToDb( | ||
{ | ||
...createDonationData(), | ||
segmentNotified: false, | ||
qfRoundId: qfRound.id, | ||
status: 'verified', | ||
}, | ||
user.id, | ||
project.id, | ||
); | ||
|
||
await saveDonationDirectlyToDb( | ||
{ | ||
...createDonationData(), | ||
segmentNotified: false, | ||
qfRoundId: qfRound.id, | ||
status: 'verified', | ||
}, | ||
user2.id, | ||
project.id, | ||
); | ||
|
||
await updateUsersWithoutMBDScoreInRound(); | ||
|
||
const user1ModelScore = await UserQfRoundModelScore.createQueryBuilder( | ||
'score', | ||
) | ||
.where('score."userId" = :userId', { userId: user.id }) | ||
.andWhere('score."qfRoundId" = :qfRoundId', { qfRoundId: qfRound.id }) | ||
.getOne(); | ||
|
||
const user2ModelScore = await UserQfRoundModelScore.createQueryBuilder( | ||
'score', | ||
) | ||
.where('score."userId" = :userId', { userId: user2.id }) | ||
.andWhere('score."qfRoundId" = :qfRoundId', { qfRoundId: qfRound.id }) | ||
.getOne(); | ||
|
||
// base values for mocks | ||
assert.equal(user1ModelScore?.score, 1); | ||
assert.equal(user2ModelScore?.score, 1); | ||
|
||
qfRound.isActive = false; | ||
await qfRound.save(); | ||
}); | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,63 @@ | ||||||||||||||
import { schedule } from 'node-cron'; | ||||||||||||||
import { spawn, Worker, Thread } from 'threads'; | ||||||||||||||
import config from '../../config'; | ||||||||||||||
import { logger } from '../../utils/logger'; | ||||||||||||||
import { | ||||||||||||||
findActiveQfRound, | ||||||||||||||
findUsersWithoutMBDScoreInActiveAround, | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Possible Typo in Function Name 'findUsersWithoutMBDScoreInActiveAround' The function name |
||||||||||||||
} from '../../repositories/qfRoundRepository'; | ||||||||||||||
import { findUserById } from '../../repositories/userRepository'; | ||||||||||||||
import { UserQfRoundModelScore } from '../../entities/userQfRoundModelScore'; | ||||||||||||||
|
||||||||||||||
const cronJobTime = | ||||||||||||||
(config.get('MAKE_UNREVIEWED_PROJECT_LISTED_CRONJOB_EXPRESSION') as string) || | ||||||||||||||
'0 0 * * * *'; | ||||||||||||||
Comment on lines
+12
to
+14
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Incorrect Configuration Variable Used for Cron Job Schedule The cron job schedule is retrieved using the config key Apply this diff to use the correct configuration variable: - const cronJobTime =
- (config.get('MAKE_UNREVIEWED_PROJECT_LISTED_CRONJOB_EXPRESSION') as string) ||
- '0 0 * * * *';
+ const cronJobTime =
+ (config.get('SYNC_USERS_MODEL_SCORE_CRONJOB_EXPRESSION') as string) ||
+ '0 0 * * * *'; 📝 Committable suggestion
Suggested change
|
||||||||||||||
|
||||||||||||||
const qfRoundUsersMissedMBDScore = Number( | ||||||||||||||
process.env.QF_ROUND_USERS_MISSED_SCORE || 0, | ||||||||||||||
); | ||||||||||||||
|
||||||||||||||
export const runCheckPendingUserModelScoreCronjob = () => { | ||||||||||||||
logger.debug( | ||||||||||||||
'runCheckPendingUserModelScoreCronjob() has been called, cronJobTime', | ||||||||||||||
cronJobTime, | ||||||||||||||
); | ||||||||||||||
schedule(cronJobTime, async () => { | ||||||||||||||
await updateUsersWithoutMBDScoreInRound(); | ||||||||||||||
}); | ||||||||||||||
}; | ||||||||||||||
|
||||||||||||||
export const updateUsersWithoutMBDScoreInRound = async () => { | ||||||||||||||
const worker = await spawn( | ||||||||||||||
new Worker('../../workers/userMBDScoreSyncWorker'), | ||||||||||||||
); | ||||||||||||||
Comment on lines
+31
to
+33
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ensure Correct Worker Instantiation and Path Resolution On lines 41-43, the worker is instantiated using |
||||||||||||||
const userIds = await findUsersWithoutMBDScoreInActiveAround(); | ||||||||||||||
const activeQfRoundId = | ||||||||||||||
(await findActiveQfRound())?.id || qfRoundUsersMissedMBDScore; | ||||||||||||||
if (!activeQfRoundId || activeQfRoundId === 0) return; | ||||||||||||||
Comment on lines
+35
to
+37
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Simplify Fallback Logic for 'activeQfRoundId' The fallback for Apply this diff: const activeQfRoundId =
- (await findActiveQfRound())?.id || qfRoundUsersMissedMBDScore;
- if (!activeQfRoundId || activeQfRoundId === 0) return;
+ (await findActiveQfRound())?.id;
+ if (!activeQfRoundId) return; 📝 Committable suggestion
Suggested change
|
||||||||||||||
|
||||||||||||||
if (userIds.length === 0) return; | ||||||||||||||
|
||||||||||||||
for (const userId of userIds) { | ||||||||||||||
try { | ||||||||||||||
const user = await findUserById(userId); | ||||||||||||||
if (!user) continue; | ||||||||||||||
|
||||||||||||||
const userScore = await worker.syncUserScore({ | ||||||||||||||
userWallet: user?.walletAddress, | ||||||||||||||
}); | ||||||||||||||
if (userScore) { | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use Explicit Null Check for 'userScore' On line 57, you check Apply this diff: - if (userScore) {
+ if (userScore != null) { 📝 Committable suggestion
Suggested change
|
||||||||||||||
const userScoreInRound = UserQfRoundModelScore.create({ | ||||||||||||||
userId, | ||||||||||||||
qfRoundId: activeQfRoundId, | ||||||||||||||
score: userScore, | ||||||||||||||
}); | ||||||||||||||
|
||||||||||||||
await userScoreInRound.save(); | ||||||||||||||
} | ||||||||||||||
} catch (e) { | ||||||||||||||
logger.info(`User with Id ${userId} did not sync MBD score this batch`); | ||||||||||||||
} | ||||||||||||||
Comment on lines
+58
to
+60
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Improve Error Logging in Catch Block In the catch block on lines 66-68, you catch an error but only log an info message without including the error details. To aid in debugging, consider logging the error at the 'error' level and include the error message. Apply this diff to improve error logging: } catch (e) {
- logger.info(`User with Id ${userId} did not sync MBD score this batch`);
+ logger.error(`Failed to sync MBD score for user with Id ${userId}: ${e.message}`, e);
} 📝 Committable suggestion
Suggested change
|
||||||||||||||
} | ||||||||||||||
await Thread.terminate(worker); | ||||||||||||||
}; |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,17 @@ | ||||||||||||||||||||||||||||||||
// workers/auth.js | ||||||||||||||||||||||||||||||||
import { expose } from 'threads/worker'; | ||||||||||||||||||||||||||||||||
import { WorkerModule } from 'threads/dist/types/worker'; | ||||||||||||||||||||||||||||||||
import { getGitcoinAdapter } from '../adapters/adaptersFactory'; | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
type UsersMBDScoreSyncWorkerFunctions = 'syncUserScore'; | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
export type UserMBDScoreSyncWorker = | ||||||||||||||||||||||||||||||||
WorkerModule<UsersMBDScoreSyncWorkerFunctions>; | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
const worker: UserMBDScoreSyncWorker = { | ||||||||||||||||||||||||||||||||
async syncUserScore(args: { userWallet: string }) { | ||||||||||||||||||||||||||||||||
return await getGitcoinAdapter().getUserAnalysisScore(args.userWallet); | ||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||
Comment on lines
+11
to
+15
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider adding error handling to the While the implementation is concise and correctly uses async/await, it lacks error handling for potential issues that may occur when calling Consider wrapping the adapter call in a try-catch block to handle potential errors: const worker: UserMBDScoreSyncWorker = {
async syncUserScore(args: { userWallet: string }) {
- return await getGitcoinAdapter().getUserAnalysisScore(args.userWallet);
+ try {
+ return await getGitcoinAdapter().getUserAnalysisScore(args.userWallet);
+ } catch (error) {
+ console.error(`Error syncing score for wallet ${args.userWallet}:`, error);
+ throw error; // Re-throw the error or handle it as appropriate for your use case
+ }
},
}; This change will provide better visibility into any issues that occur during the score synchronization process. 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
expose(worker); |
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.
Improve error handling and security in the new function.
The new
findUsersWithoutMBDScoreInActiveAround
function is a good addition, but there are a few areas for improvement:Consider handling the case where
activeQfRoundId
is 0 more explicitly. The current implementation will return an empty array, which might not be the intended behavior.While using
QfRound.query
likely provides some protection against SQL injection, it's generally safer to use parameterized queries consistently. Consider using the query builder or ORM methods instead of raw SQL for better security and maintainability.Add error handling for potential database query failures.
Here's a suggested refactor to address these points:
This refactored version uses the query builder for better security, includes error handling, and more explicitly handles the case where no active QF round is found.