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.

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

Merged
merged 3 commits into from
Sep 30, 2024

Conversation

CarlosQ96
Copy link
Collaborator

@CarlosQ96 CarlosQ96 commented Sep 25, 2024

Related to: Giveth/giveth-dapps-v2#4602

Summary by CodeRabbit

  • New Features

    • Introduced a new test script for user model score synchronization.
    • Added functionality to identify users without a specific score in the active QF round.
    • Implemented a cron job to periodically update user scores based on donation activity.
    • Created a worker module for concurrent score synchronization.
  • Bug Fixes

    • Enhanced testing coverage for user scoring functionalities.
  • Documentation

    • Updated test cases to ensure clarity and organization within the testing framework.

@CarlosQ96 CarlosQ96 force-pushed the hotfix_automatic_model_score_sync branch from 5ca6f02 to 27bcbbc Compare September 25, 2024 11:16
@CarlosQ96 CarlosQ96 force-pushed the hotfix_automatic_model_score_sync branch from 27bcbbc to 87934df Compare September 26, 2024 18:38
@CarlosQ96 CarlosQ96 marked this pull request as ready for review September 30, 2024 14:18
Copy link
Contributor

coderabbitai bot commented Sep 30, 2024

Warning

Rate limit exceeded

@CarlosQ96 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 53 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Files that changed from the base of the PR and between 38f52bd and e8d3b25.

Walkthrough

The changes introduce new functionalities and tests related to user scoring within the giveth-graphql-api project. A new test script is added to package.json, while several methods are introduced in the repository files to identify users without specific scores in active rounds. Additionally, a cron job is established to periodically update user scores, complemented by a new worker module for asynchronous score synchronization. Tests are implemented to ensure the correctness of these functionalities.

Changes

File(s) Change Summary
package.json Added a new test script test:syncUsersModelScore to run tests for user model score synchronization.
src/repositories/qfRoundRepository.ts Introduced a constant qfRoundUsersMissedMBDScore and a method findUsersWithoutMBDScoreInActiveAround to identify users lacking scores in the active QF round.
src/repositories/qfRoundRepository.test.ts Added a test case findUsersWithoutMBDScoreInActiveAroundTestCases to validate the functionality of findUsersWithoutMBDScoreInActiveAround.
src/services/cronJobs/syncUsersModelScore.test.ts Created a test suite for updateUsersWithoutMBDScoreInRound to verify that the function correctly assigns scores to users based on their donation activity.
src/services/cronJobs/syncUsersModelScore.ts Added the cron job function runCheckPendingUserModelScoreCronjob and the method updateUsersWithoutMBDScoreInRound for periodic user score updates.
src/workers/userMBDScoreSyncWorker.ts Introduced a worker module with a function syncUserScore to retrieve and synchronize user scores asynchronously.

Possibly related PRs

  • Feature new qf scoring model #1634: Introduces new functions in src/repositories/qfRoundRepository.ts that handle user model scores, which may relate to the testing of user scores in the main PR's new test script.
  • Master to develop #1758: Enhances test coverage for the qfRoundRepository, which could be relevant to the new test script added in the main PR.

Suggested reviewers

  • mohammadranjbarz
  • RamRamez
  • kkatusic

Poem

In fields of code, the rabbits play,
With scores and tests, they hop all day.
New scripts and jobs, they bring delight,
Synchronizing scores, oh what a sight!
With every change, the burrow grows,
A joyful dance, as progress flows! 🐇✨


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Collaborator

@mohammadranjbarz mohammadranjbarz left a comment

Choose a reason for hiding this comment

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

@CarlosQ96 Everything seems cool except the thing that I commented

src/services/cronJobs/syncUsersModelScore.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 12

🧹 Outside diff range and nitpick comments (6)
src/workers/userMBDScoreSyncWorker.ts (1)

1-1: Update the file name in the comment.

The comment on the first line refers to "workers/auth.js", which doesn't match the actual file name "src/workers/userMBDScoreSyncWorker.ts".

Please update the comment to reflect the correct file name:

-// workers/auth.js
+// src/workers/userMBDScoreSyncWorker.ts
src/services/cronJobs/syncUsersModelScore.test.ts (1)

20-85: LGTM: Well-structured test case with thorough setup and assertions.

The test case effectively covers the main functionality of updateUsersWithoutMBDScoreInRound. It properly sets up the test environment, executes the function, and verifies the results. The cleanup step at the end is a good practice.

However, there's a minor point that could use clarification:

The comment on line 78 states "base values for mocks". Consider clarifying this comment to explain why the expected score is always 1. For example:

- // base values for mocks
+ // For tests, the score is always 1 to simplify testing of cronjob logic and worker

This will help other developers understand the reasoning behind the fixed score value.

src/repositories/qfRoundRepository.ts (1)

Line range hint 1-203: Provide context for the MBD score-related changes.

The additions to this file seem to be part of a larger feature or bug fix related to handling users without MBD (Model-Based Donation) scores in QF (Quadratic Funding) rounds. It would be helpful to provide more context about the overall feature or issue these changes are addressing. This context could include:

  1. The purpose of MBD scores in the QF round process.
  2. Why it's important to identify users without these scores.
  3. What actions will be taken with the list of users returned by the new function.

This additional information would help reviewers better understand the impact and importance of these changes.

src/repositories/qfRoundRepository.test.ts (1)

50-98: LGTM: Well-structured test case added.

The new test case for findUsersWithoutMBDScoreInActiveAround is comprehensive and well-implemented. It covers the main functionality of finding users without MBD scores in an active round, including proper setup and cleanup.

Consider adding an additional test case to cover the scenario where a user has an MBD score. This would enhance the test coverage and ensure the function correctly excludes users with MBD scores. Here's a suggested addition:

it('should not find users with MBD scores', async () => {
  // ... (setup code similar to the existing test)

  const userWithScore = await saveUserDirectlyToDb(generateRandomEtheriumAddress());
  userWithScore.mbdScore = 10; // Assuming mbdScore is the correct field name
  await userWithScore.save();

  await saveDonationDirectlyToDb(
    {
      ...createDonationData(),
      segmentNotified: false,
      qfRoundId: qfRound.id,
      status: 'verified',
    },
    userWithScore.id,
    project.id,
  );

  const userIds = await findUsersWithoutMBDScoreInActiveAround();
  assert.equal(userIds.length, 2);
  assert.isFalse(userIds.includes(userWithScore.id));

  // ... (cleanup code)
});

This additional test case would ensure that the function correctly excludes users who have an MBD score, even if they have made donations in the active round.

src/services/cronJobs/syncUsersModelScore.ts (2)

21-28: Unused 'workerOptions' Variable

The workerOptions object defined on lines 21-28 is not used in the code. If it's not needed, consider removing it to keep the code clean and maintainable.


53-55: Variable Naming: Rename 'userWallet' to 'user' for Clarity

On line 53, the variable userWallet is assigned the result of findUserById(userId). Since this variable represents the user object, consider renaming userWallet to user for clarity.

Apply this diff:

- const userWallet = await findUserById(userId);
- const userScore = await worker.syncUserScore({
-   userWallet,
- });
+ const user = await findUserById(userId);
+ const userScore = await worker.syncUserScore({
+   user,
+ });
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between ecf4e19 and 22c6e5e.

📒 Files selected for processing (7)
  • package.json (1 hunks)
  • src/repositories/qfRoundRepository.test.ts (3 hunks)
  • src/repositories/qfRoundRepository.ts (2 hunks)
  • src/server/bootstrap.ts (2 hunks)
  • src/services/cronJobs/syncUsersModelScore.test.ts (1 hunks)
  • src/services/cronJobs/syncUsersModelScore.ts (1 hunks)
  • src/workers/userMBDScoreSyncWorker.ts (1 hunks)
🔇 Additional comments (10)
src/workers/userMBDScoreSyncWorker.ts (3)

1-9: LGTM: Imports and type definitions are well-structured.

The imports from the threads library and the custom type definitions provide a solid foundation for the worker implementation. The use of TypeScript ensures type safety for the worker functions.


17-17: LGTM: Worker correctly exposed.

The worker object is properly exposed using the expose function from the threads library, which is the correct approach for making the worker available for use in a multi-threaded environment.


1-17: Overall assessment: Good implementation with minor improvement suggestions.

The userMBDScoreSyncWorker.ts file successfully implements a worker for synchronizing user MBD scores using the threads library. The code is well-structured, uses TypeScript for type safety, and follows good practices for worker implementation.

Key points:

  1. Proper use of the threads library for worker functionality.
  2. Well-defined types for the worker module and functions.
  3. Concise implementation of the syncUserScore function.

Suggestions for improvement:

  1. Add error handling to the syncUserScore function.
  2. Update the file name in the comment on the first line.

These minor enhancements will improve the robustness and accuracy of the code.

src/services/cronJobs/syncUsersModelScore.test.ts (3)

1-18: LGTM: Imports and structure are well-organized.

The imports cover all necessary dependencies for the test, including testing utilities, relevant entities, and the function under test. The overall structure with a describe block and a nested test function follows good testing practices.


1-85: Overall, excellent test implementation with room for minor improvements.

This test file is well-structured and provides comprehensive coverage for the updateUsersWithoutMBDScoreInRound function. It effectively sets up the test environment, executes the function, and verifies the results. The cleanup step at the end is a good practice.

Consider implementing the suggested clarifications to enhance readability and maintainability. Great job on writing thorough tests!


12-12: Verify the function signature of updateUsersWithoutMBDScoreInRound.

The function is called without arguments, which seems correct based on the test setup. However, let's verify that this matches the actual implementation.

Run the following script to check the function signature:

This will help ensure that the function usage in the test matches its actual implementation.

Also applies to: 62-62

src/repositories/qfRoundRepository.ts (1)

18-20: LGTM: New constant for configuration.

The addition of qfRoundUsersMissedMBDScore is a good practice. It allows for flexible configuration through environment variables while providing a sensible default value.

src/repositories/qfRoundRepository.test.ts (2)

20-20: LGTM: Import statement added correctly.

The new import for findUsersWithoutMBDScoreInActiveAround is properly placed and consistent with the existing import style.


31-34: LGTM: New describe block added correctly.

The new describe block for 'findUsersWithoutMBDScoreInActiveAround test cases' is well-placed and follows the existing pattern in the file. The naming is consistent with other test cases.

package.json (1)

138-138: LGTM: New test script added for syncUsersModelScore

The addition of the test:syncUsersModelScore script is consistent with other test scripts in the file. It follows the same pattern of setting the NODE_ENV to test, using mocha, and specifying the pre-test script along with the actual test file.

This new script aligns with the PR objectives of introducing synchronization workers and a cron job for user mbdscore.

To ensure the test file exists and follows the project's testing conventions, let's run the following script:

✅ Verification successful

Verification Successful: syncUsersModelScore Test Script

  • The test file ./src/services/cronJobs/syncUsersModelScore.test.ts exists.
  • It includes necessary imports (chai, etc.) and contains properly structured test cases using describe and it blocks.
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify the existence and basic structure of the syncUsersModelScore test file

# Check if the test file exists
if [ -f "./src/services/cronJobs/syncUsersModelScore.test.ts" ]; then
    echo "Test file exists."
    
    # Check for common testing imports and structure
    grep -E "import.*(describe|it|expect)" "./src/services/cronJobs/syncUsersModelScore.test.ts"
    
    # Check for the presence of test cases
    grep -E "(describe|it)\(" "./src/services/cronJobs/syncUsersModelScore.test.ts"
else
    echo "Test file does not exist at the specified path."
fi

Length of output: 614

Comment on lines +11 to +15
const worker: UserMBDScoreSyncWorker = {
async syncUserScore(args: { userWallet: string }) {
return await getGitcoinAdapter().getUserAnalysisScore(args.userWallet);
},
};
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider adding error handling to the syncUserScore function.

While the implementation is concise and correctly uses async/await, it lacks error handling for potential issues that may occur when calling getGitcoinAdapter().getUserAnalysisScore().

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

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const worker: UserMBDScoreSyncWorker = {
async syncUserScore(args: { userWallet: string }) {
return await getGitcoinAdapter().getUserAnalysisScore(args.userWallet);
},
};
const worker: UserMBDScoreSyncWorker = {
async syncUserScore(args: { userWallet: string }) {
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
}
},
};

Comment on lines +179 to +202
export const findUsersWithoutMBDScoreInActiveAround = async (): Promise<
number[]
> => {
const activeQfRoundId =
(await findActiveQfRound())?.id || qfRoundUsersMissedMBDScore;

if (!activeQfRoundId || activeQfRoundId === 0) return [];

const usersMissingMDBScore = await QfRound.query(
`
SELECT DISTINCT d."userId"
FROM public.donation d
LEFT JOIN user_qf_round_model_score uqrms ON d."userId" = uqrms."userId" AND uqrms."qfRoundId" = $1
WHERE d."qfRoundId" = $1
AND d.status = 'verified'
AND uqrms.id IS NULL
AND d."userId" IS NOT NULL
ORDER BY d."userId";
`,
[activeQfRoundId],
);

return usersMissingMDBScore.map(user => user.userId);
};
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

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:

  1. 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.

  2. 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.

  3. Add error handling for potential database query failures.

Here's a suggested refactor to address these points:

export const findUsersWithoutMBDScoreInActiveAround = async (): Promise<number[]> => {
  try {
    const activeQfRound = await findActiveQfRound();
    const activeQfRoundId = activeQfRound?.id ?? qfRoundUsersMissedMBDScore;

    if (!activeQfRoundId) {
      logger.warn('No active QF round found and no fallback ID provided');
      return [];
    }

    const usersMissingMDBScore = await Donation.createQueryBuilder('d')
      .select('DISTINCT d.userId', 'userId')
      .leftJoin(UserQfRoundModelScore, 'uqrms', 'uqrms.userId = d.userId AND uqrms.qfRoundId = :qfRoundId', { qfRoundId: activeQfRoundId })
      .where('d.qfRoundId = :qfRoundId', { qfRoundId: activeQfRoundId })
      .andWhere('d.status = :status', { status: 'verified' })
      .andWhere('uqrms.id IS NULL')
      .andWhere('d.userId IS NOT NULL')
      .orderBy('d.userId')
      .getRawMany();

    return usersMissingMDBScore.map(user => user.userId);
  } catch (error) {
    logger.error('Error finding users without MBD score', { error });
    throw error;
  }
};

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.

import { logger } from '../../utils/logger';
import {
findActiveQfRound,
findUsersWithoutMBDScoreInActiveAround,
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Possible Typo in Function Name 'findUsersWithoutMBDScoreInActiveAround'

The function name findUsersWithoutMBDScoreInActiveAround on line 7 might contain a typo. Consider renaming it to findUsersWithoutMBDScoreInActiveRound for clarity and consistency.

Comment on lines +41 to +33
const worker = await spawn(
new Worker('../../workers/userMBDScoreSyncWorker'),
);
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure Correct Worker Instantiation and Path Resolution

On lines 41-43, the worker is instantiated using new Worker('../../workers/userMBDScoreSyncWorker'). Since you have imported UserMBDScoreSyncWorker on line 11, consider using it directly or ensure that the path to the worker file is correctly resolved. This will help avoid potential issues with path resolution, especially in different environments.

Comment on lines +13 to +14
const cronJobTime =
(config.get('MAKE_UNREVIEWED_PROJECT_LISTED_CRONJOB_EXPRESSION') as string) ||
'0 0 * * * *';
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Incorrect Configuration Variable Used for Cron Job Schedule

The cron job schedule is retrieved using the config key MAKE_UNREVIEWED_PROJECT_LISTED_CRONJOB_EXPRESSION on lines 14-15, which appears unrelated to this cron job's functionality. It's recommended to create a new configuration key specific to this cron job, such as SYNC_USERS_MODEL_SCORE_CRONJOB_EXPRESSION.

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

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
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 * * * *';

Comment on lines +66 to +60
} catch (e) {
logger.info(`User with Id ${userId} did not sync MBD score this batch`);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

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

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
} catch (e) {
logger.info(`User with Id ${userId} did not sync MBD score this batch`);
}
} catch (e) {
logger.error(`Failed to sync MBD score for user with Id ${userId}: ${e.message}`, e);
}

Comment on lines 41 to 63
const worker = await spawn(
new Worker('../../workers/userMBDScoreSyncWorker'),
);
const userIds = await findUsersWithoutMBDScoreInActiveAround();
const activeQfRoundId =
(await findActiveQfRound())?.id || qfRoundUsersMissedMBDScore;
if (!activeQfRoundId || activeQfRoundId === 0) return;

if (userIds.length === 0) return;

for (const userId of userIds) {
try {
const userWallet = await findUserById(userId);
const userScore = await worker.syncUserScore({
userWallet,
});
if (userScore) {
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`);
}
}
await Thread.terminate(worker);
};
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Ensure Worker is Properly Terminated Even if Errors Occur

To prevent resource leaks, ensure that the worker is properly terminated even if an error occurs during processing. Wrap the worker usage in a try...finally block to guarantee that await Thread.terminate(worker); is called.

Apply this diff:

export const updateUsersWithoutMBDScoreInRound = async () => {
  const worker = await spawn(
    new Worker('../../workers/userMBDScoreSyncWorker'),
  );
+ try {
    const userIds = await findUsersWithoutMBDScoreInActiveAround();
    const activeQfRoundId =
      (await findActiveQfRound())?.id || qfRoundUsersMissedMBDScore;
    if (!activeQfRoundId || activeQfRoundId === 0) return;

    if (userIds.length === 0) return;

    for (const userId of userIds) {
      try {
        const user = await findUserById(userId);
        const userScore = await worker.syncUserScore({
          user,
        });
        if (userScore != null) {
          const userScoreInRound = UserQfRoundModelScore.create({
            userId,
            qfRoundId: activeQfRoundId,
            score: userScore,
          });

          await userScoreInRound.save();
        }
      } catch (e) {
        logger.error(`Failed to sync MBD score for user with Id ${userId}: ${e.message}`, e);
      }
    }
+ } finally {
+   await Thread.terminate(worker);
+ }
};
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const worker = await spawn(
new Worker('../../workers/userMBDScoreSyncWorker'),
);
const userIds = await findUsersWithoutMBDScoreInActiveAround();
const activeQfRoundId =
(await findActiveQfRound())?.id || qfRoundUsersMissedMBDScore;
if (!activeQfRoundId || activeQfRoundId === 0) return;
if (userIds.length === 0) return;
for (const userId of userIds) {
try {
const userWallet = await findUserById(userId);
const userScore = await worker.syncUserScore({
userWallet,
});
if (userScore) {
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`);
}
}
await Thread.terminate(worker);
};
export const updateUsersWithoutMBDScoreInRound = async () => {
const worker = await spawn(
new Worker('../../workers/userMBDScoreSyncWorker'),
);
try {
const userIds = await findUsersWithoutMBDScoreInActiveAround();
const activeQfRoundId =
(await findActiveQfRound())?.id || qfRoundUsersMissedMBDScore;
if (!activeQfRoundId || activeQfRoundId === 0) return;
if (userIds.length === 0) return;
for (const userId of userIds) {
try {
const userWallet = await findUserById(userId);
const userScore = await worker.syncUserScore({
userWallet,
});
if (userScore) {
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`);
}
}
} finally {
await Thread.terminate(worker);
}
};

@@ -366,6 +367,11 @@ export async function bootstrap() {
runCheckProjectVerificationStatus();
}

// If we need to deactivate the process use the env var NO MORE
if (process.env.SYNC_USERS_MBD_SCORE_ACTIVE === 'true') {
runCheckPendingUserModelScoreCronjob();
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Inconsistent function naming: 'Cronjob' vs 'CronJob'

The function runCheckPendingUserModelScoreCronjob() does not follow the naming convention used elsewhere in the codebase, where CronJob is capitalized with a capital 'J'. Consider renaming it to runCheckPendingUserModelScoreCronJob() for consistency.

Apply this diff to correct the function call:

-          runCheckPendingUserModelScoreCronjob();
+          runCheckPendingUserModelScoreCronJob();
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
runCheckPendingUserModelScoreCronjob();
runCheckPendingUserModelScoreCronJob();

@@ -69,6 +69,7 @@
import { runCheckPendingRecurringDonationsCronJob } from '../services/cronJobs/syncRecurringDonationsWithNetwork';
import { runCheckQRTransactionJob } from '../services/cronJobs/checkQRTransactionJob';
import { addClient } from '../services/sse/sse';
import { runCheckPendingUserModelScoreCronjob } from '../services/cronJobs/syncUsersModelScore';
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Inconsistent function naming: 'Cronjob' vs 'CronJob'

The function name runCheckPendingUserModelScoreCronjob does not follow the existing naming convention where CronJob is capitalized with a capital 'J'. For consistency, consider renaming the function to runCheckPendingUserModelScoreCronJob.

Apply this diff to correct the function name:

-import { runCheckPendingUserModelScoreCronjob } from '../services/cronJobs/syncUsersModelScore';
+import { runCheckPendingUserModelScoreCronJob } from '../services/cronJobs/syncUsersModelScore';
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
import { runCheckPendingUserModelScoreCronjob } from '../services/cronJobs/syncUsersModelScore';
import { runCheckPendingUserModelScoreCronJob } from '../services/cronJobs/syncUsersModelScore';

@@ -366,6 +367,11 @@
runCheckProjectVerificationStatus();
}

// If we need to deactivate the process use the env var NO MORE
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Incorrect environment variable mentioned in the comment

The comment mentions use the env var NO MORE, which does not correspond to the environment variable used in the code. Update the comment to accurately reflect the correct environment variable SYNC_USERS_MBD_SCORE_ACTIVE.

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

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// 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'

@CarlosQ96 CarlosQ96 force-pushed the hotfix_automatic_model_score_sync branch from 22c6e5e to 38f52bd Compare September 30, 2024 14:27
@CarlosQ96 CarlosQ96 force-pushed the hotfix_automatic_model_score_sync branch from 38f52bd to e8d3b25 Compare September 30, 2024 14:35
Copy link
Collaborator

@mohammadranjbarz mohammadranjbarz left a comment

Choose a reason for hiding this comment

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

Thanks @CarlosQ96 LGTM

@CarlosQ96 CarlosQ96 merged commit 165f847 into staging Sep 30, 2024
5 checks passed
@CarlosQ96 CarlosQ96 deleted the hotfix_automatic_model_score_sync branch September 30, 2024 16:25
@CarlosQ96 CarlosQ96 restored the hotfix_automatic_model_score_sync branch October 1, 2024 17:10
@CarlosQ96 CarlosQ96 deleted the hotfix_automatic_model_score_sync branch October 1, 2024 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants