feat: notify admin to update solvers in the community #618
Conversation
There was a problem hiding this comment.
Hello, did you tested it locally? It's not working properly:
- This message is not being displayed:
Your community ${community.name} doesn't have any solvers. Please add at least one solver to avoid being disabled. - Even though the logs say:
Community: ${community.name} has been disabled due to lack of solvers.the community is not being disabled. You can continue using it normally. - I recommend putting the warning messages to the administrators in the language they have the bot set in, in the yaml of ./locales. You can see as an example at Add cronjob to notify admin about missing solvers (#314) #563
Thank you for the review. I'll work on these feedbacks. |
…sages are localized
|
Hi @Catrya, I've addressed all the feedbacks here. Please have a look and let me know if there're other changes.
|
There was a problem hiding this comment.
Hi @Anyitechs , much better, it already sends messages to the admins and disables the community. However, I think the message for the admins could be improved so that they know how much time they have to put a new admin.
I think it could be something like this:
Your community ${communityName} does not have any solvers. Please add at least one within X days to prevent the community from being disabled.
Actually the number of notifications that the solver receives before disabling the community is MAX_MESSAGES -1 because it disables it in MAX_MESSAGES, I suggest that it shows that number of notifications before disabling it
I will continue testing and I will tell you if I find any bug, good job!
Thank you for the feedback @Catrya. I just pushed a new commit with the changes requested here. Please take a look and let me know your thoughts. Thank you. |
There was a problem hiding this comment.
Hi @Anyitechs, sorry but I remembered that when a community is disabled it should also be deleted from the database, see deleteCommunity
This PR does not delete the community from the database, and I think that is necessary.
Anyway, let's see what @grunch thinks, sorry
Thank you @Catrya. I was thinking we needed a soft delete and that was why I added the Any thoughts? @grunch |
Hi @Anyitechs , I don't mean to disable it first and then delete it, but to delete it. |
That makes sense. Thank you for the clarification. I'll work on that and update the branch. |
Anyitechs
force-pushed
the
update-solvers
branch
from
eb20c72 to
bf0258f
Compare
|
@Catrya I just pushed an update to the branch now. Please do take a look and let me know your thoughts. Thank you. |
There was a problem hiding this comment.
thanks @Anyitechs I already tested it and it works well to me! Let's see @grunch review
Thank you @Catrya. @grunch let me know if there're any changes. Thank you. |
bot/modules/community/commands.js
Outdated
| @@ -143,7 +143,7 @@ exports.updateCommunity = async (ctx, id, field, bot) => { | |||
| if (!(await validateObjectId(ctx, id))) return; | |||
| const community = await Community.findOne({ | |||
| _id: id, | |||
| creator_id: user._id, | |||
There was a problem hiding this comment.
as you can read in our contribution section, we are using the airbnb javascript guide style, please don't remove the comma
https://github.com/airbnb/javascript?tab=readme-ov-file#commas--dangling
bot/modules/community/commands.js
Outdated
| @@ -211,7 +211,7 @@ exports.deleteCommunity = async ctx => { | |||
| if (!(await validateObjectId(ctx, id))) return; | |||
| const community = await Community.findOne({ | |||
| _id: id, | |||
| creator_id: ctx.user._id, | |||
| creator_id: ctx.user._id | |||
There was a problem hiding this comment.
bot/modules/community/commands.js
Outdated
| @@ -233,7 +233,7 @@ exports.changeVisibility = async ctx => { | |||
| if (!(await validateObjectId(ctx, id))) return; | |||
| const community = await Community.findOne({ | |||
| _id: id, | |||
| creator_id: ctx.user._id, | |||
| creator_id: ctx.user._id | |||
There was a problem hiding this comment.
jobs/check_solvers.ts
Outdated
|
|
||
| await bot.telegram.sendMessage( | ||
| admin.tg_id, | ||
| message |
There was a problem hiding this comment.
How about moving to a style that can be checked at CI time? e.g. with prettier |
|
@webwarrior-ws hey, please check CI, it is red. |
Oops wrong contributor! |
yes I have it locally but we need it at CI level |
Anyitechs
force-pushed
the
update-solvers
branch
from
bf0258f to
1772689
Compare
|
Thank you for the review @grunch. I just addressed the feedbacks, rebased and updated the branch. |
I agree on this, we allow users to disable a community for a while, maybe they need to stop it and want to go back later, but we already have a cronjob that deletes a community after N days without a successful order, disabled or not, so I think there is no harm if someone disable a community and forget about it, it will be deleted at some point |
Anyway, @Anyitechs then got the community deleted, which is better IMHO |
I'm sorry I was thinking you were talking about adding a new way of just disable a community from the same user creator I agree on, if the creator execute the command to delete the community it should delete the community immediately |
.env-sample
Outdated
| DISPUTE_START_WINDOW=600 | ||
|
|
||
| # Number of notification messages sent to the admin, informing them of lack of solvers before disabling the community |
There was a problem hiding this comment.
The admin is being sent MAX_MESSAGES -1 notifications, if it says 10 here, the admin will see 9 notifications.
I think that either this description, or MAX_MESSAGES needs to be adjusted to make it clearer for whoever is going to configuring it.
For the rest, everything seems fine to me.
There was a problem hiding this comment.
Thank you @Catrya. What do you recommend?
Adjusting the description to include that the admin receives MAX_MESSAGES -1 notifications seems ok to me.
There was a problem hiding this comment.
Yes, that's fine by me too, go ahead. Thanks @Anyitechs
There was a problem hiding this comment.
Just updated the description and rebased.
Anyitechs
force-pushed
the
update-solvers
branch
from
1772689 to
76e2ba8
Compare
WalkthroughThe changes introduce a new scheduling mechanism that runs the Changes
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
locales/it.yaml (1)
635-636: 🛠️ Refactor suggestionReview Italian Localization for Solver Terminology and Placeholders
The newly added keys for “check_solvers” and “check_solvers_last_warning” include dynamic placeholders (${communityName} and ${remainingDays}). Please verify that the translation aligns with the project’s terminology. In Italian it might be more appropriate to use a term like “risolutori” instead of “solver” if that is consistent with the rest of the localization strings. For example:
- check_solvers: La tua community ${communityName} non ha solver. Aggiungine almeno uno entro ${remainingDays} giorni per evitare che la community venga disabilitata. + check_solvers: La tua community ${communityName} non ha risolutori. Aggiungine almeno uno entro ${remainingDays} giorni per evitare che la community venga disabilitata.Please ensure consistency with other keys and verify that the placeholders match the backend expectations.
jobs/check_solvers.ts (1)
1-57: 💡 Verification agent🧩 Analysis chain
Verify if the MAX_MESSAGES environment variable has appropriate fallback.
The code references
process.env.MAX_MESSAGESwithout providing a fallback value if this environment variable is not set. Consider adding a fallback value.
🏁 Script executed:
#!/bin/bash # Check if MAX_MESSAGES is defined in any environment files grep -r "MAX_MESSAGES" --include="*.env*" .Length of output: 267
Action: Add a Fallback for MAX_MESSAGES
After confirming that
MAX_MESSAGESis defined in the sample environment file (with a value of 10), please ensure that the production code gracefully handles cases where this variable might not be set. Currently,process.env.MAX_MESSAGESis used directly. If this environment variable is missing, usingNumber(process.env.MAX_MESSAGES)will yieldNaN, which can lead to unexpected behavior.Suggested changes:
- Use a fallback value when converting
process.env.MAX_MESSAGESto a number. For example:const maxMessages = Number(process.env.MAX_MESSAGES ?? 10);- Replace occurrences of
Number(process.env.MAX_MESSAGES)with the newmaxMessagesvariable in both the condition that deletes the community and when calculatingremainingDays.Next step: Update the code in
jobs/check_solvers.ts(lines 1-57) accordingly.🧰 Tools
🪛 Biome (1.9.4)
[error] 17-17: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
🧹 Nitpick comments (9)
locales/de.yaml (1)
637-638: Ensure Consistent Terminology for Solvers in German LocalizationThe two new keys use “Solver” (e.g. “Ihre Community ${communityName} hat keine Solver…”), but elsewhere in this file (and likely across the project) the term “Streitschlichter” is used for this role. To maintain consistency, please consider aligning the terminology in these messages. For example:
-check_solvers: Ihre Community ${communityName} hat keine Solver. Bitte fügen Sie innerhalb von ${remainingDays} Tagen mindestens einen hinzu, um zu verhindern, dass die Community deaktiviert wird. +check_solvers: Ihre Community ${communityName} hat keine Streitschlichter. Bitte fügen Sie innerhalb von ${remainingDays} Tagen mindestens einen hinzu, um zu verhindern, dass die Community deaktiviert wird.Similarly, update “check_solvers_last_warning” if needed.
locales/ko.yaml (1)
632-633: Verify Terminology and Placeholder Consistency in Korean LocalizationThe new entries:
check_solvers: ${communityName} 커뮤니티에 해결사가 없습니다. 커뮤니티가 비활성화되는 것을 방지하려면 ${remainingDays}일 이내에 하나 이상 추가하세요. check_solvers_last_warning: ${communityName} 커뮤니티에 해결사가 없습니다. 커뮤니티가 비활성화되는 것을 방지하려면 오늘 하나 이상 추가하세요.correctly use the dynamic placeholders (${communityName} and ${remainingDays}). Please ensure that the term “해결사” is the chosen and consistent translation for “solver” as used in other parts of the localization (or adjust it if another term is preferred).
locales/ru.yaml (1)
637-638: Verify translation accuracy and add a newline at end of file.The new localization entries for notifying admins about missing solvers look good. The message content aligns with the feature described in the PR objectives.
As flagged by YAMLlint, there is no newline at the end of the file. While this is not a functional issue, it's a good practice to include one:
check_solvers_last_warning: В вашем сообществе ${communityName} нет решателей. Пожалуйста, добавьте хотя бы один сегодня, чтобы предотвратить отключение сообщества. +locales/es.yaml (1)
637-638: Verify translation accuracy and add a newline at end of file.The new localization entries for notifying admins about missing solvers have been properly added to the Spanish locale file. The messages align with the feature described in the PR objectives.
As flagged by YAMLlint, there is no newline at the end of the file. While this is not a functional issue, it's a good practice to include one:
check_solvers_last_warning: Tu comunidad ${communityName} no tiene ningún solucionador. Agregue al menos uno hoy para evitar que la comunidad quede inhabilitada. +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 638-638: no new line character at the end of file
(new-line-at-end-of-file)
jobs/check_solvers.ts (5)
16-20: Simplify the conditional logic by using early continue.The current if-else structure with an unnecessary continue statement can be simplified.
- for (const community of communities) { - if (community.solvers.length > 0) { - continue; - } else { - await notifyAdmin(community, bot); - } - } + for (const community of communities) { + if (community.solvers.length === 0) { + await notifyAdmin(community, bot); + } + }🧰 Tools
🪛 Biome (1.9.4)
[error] 17-17: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
40-42: Add error handling for community save operation.The community save operation might fail but there's no error handling. Consider wrapping it in a try-catch block.
- await community.save(); - const admin = await User.findById(community.creator_id); + try { + await community.save(); + const admin = await User.findById(community.creator_id); + } catch (error) { + logger.error(`Failed to save community ${community.name}: ${String(error)}`); + return; + }
43-45: Add logging when admin is not found.If the admin is not found, the function silently returns without any notification. Consider adding a log message for this case.
if (admin) { const i18nCtx: I18nContext = await getUserI18nContext(admin); const remainingDays: number = (Number(process.env.MAX_MESSAGES) - 1) - community.messages_sent_count; + } else { + logger.warn(`Admin not found for community: ${community.name} (ID: ${community.creator_id})`); + return; }
47-47: Improve message selection logic readability.The ternary expression for message selection is a bit hard to read. Consider extracting it to a separate variable or function.
- const message = remainingDays === 0 ? i18nCtx.t('check_solvers_last_warning', { communityName: community.name }) : i18nCtx.t('check_solvers', { communityName: community.name, remainingDays: remainingDays }); + const messageKey = remainingDays === 0 ? 'check_solvers_last_warning' : 'check_solvers'; + const messageParams = remainingDays === 0 + ? { communityName: community.name } + : { communityName: community.name, remainingDays }; + const message = i18nCtx.t(messageKey, messageParams);
49-53: Add error handling for Telegram message sending.The bot.telegram.sendMessage operation might fail, but there's no error handling. Consider wrapping it in a try-catch block.
- await bot.telegram.sendMessage( - admin.tg_id, - message, - ); + try { + await bot.telegram.sendMessage( + admin.tg_id, + message, + ); + logger.info(`Notification sent to admin of community ${community.name}, messages sent: ${community.messages_sent_count}`); + } catch (error) { + logger.error(`Failed to send notification to admin of community ${community.name}: ${String(error)}`); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
-
.env-sample(1 hunks) -
bot/start.ts(2 hunks) -
jobs/check_solvers.ts(1 hunks) -
jobs/index.ts(2 hunks) -
locales/de.yaml(1 hunks) -
locales/en.yaml(1 hunks) -
locales/es.yaml(1 hunks) -
locales/fa.yaml(1 hunks) -
locales/fr.yaml(1 hunks) -
locales/it.yaml(1 hunks) -
locales/ko.yaml(1 hunks) -
locales/pt.yaml(1 hunks) -
locales/ru.yaml(1 hunks) -
locales/uk.yaml(1 hunks) -
models/community.ts(2 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
locales/es.yaml
[error] 638-638: no new line character at the end of file
(new-line-at-end-of-file)
🪛 Biome (1.9.4)
jobs/check_solvers.ts
[error] 17-17: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
🔇 Additional comments (12)
jobs/index.ts (1)
10-10: Implementation looks good!The import and export of the new
checkSolversfunction is clean and consistent with the existing codebase structure.Also applies to: 20-20
.env-sample (1)
85-86: Clear configuration description that addresses previous feedback.The comment clearly explains that admins receive
MAX_MESSAGES - 1notifications before the community is disabled, addressing the previous feedback from the PR discussion.models/community.ts (1)
52-52: Good implementation of the message count tracking.The addition of
messages_sent_countto both the interface and schema with appropriate typing and default value is well-implemented. This will correctly track notification messages sent to community administrators.Also applies to: 85-85
locales/fr.yaml (2)
636-636: Ensure message contains time frame as mentioned in PR comments.Per the PR comments, Catrya suggested that the notification message should include a timeframe for when a solver should be added. This implementation correctly includes the
${remainingDays}variable to provide this information.
636-637:✅ Verification successful
Verify translations exist in all language files.
The localized warning messages have been added to the French locale file, but similar messages should also exist in other language files (en.yaml, de.yaml, etc.) to ensure consistent multilingual support.
🏁 Script executed:
#!/bin/bash # Check if the warning messages exist in all language files echo "Checking for warning messages in all language files..." # Find all locale files locale_files=$(fd -e yaml -e yml . locales) # Check each file for the presence of the warning messages for file in $locale_files; do echo "Checking $file..." has_check_solvers=$(grep -l "check_solvers:" "$file" || echo "") has_last_warning=$(grep -l "check_solvers_last_warning:" "$file" || echo "") if [ -z "$has_check_solvers" ] || [ -z "$has_last_warning" ]; then echo "⚠️ WARNING: Missing translation in $file" if [ -z "$has_check_solvers" ]; then echo " - Missing: check_solvers" fi if [ -z "$has_last_warning" ]; then echo " - Missing: check_solvers_last_warning" fi else echo "✅ $file has both warning messages" fi doneLength of output: 4486
All locale files confirmed for warning message translations
I verified that both translation keys (
check_solversandcheck_solvers_last_warning) exist in every locale file (de.yaml, en.yaml, es.yaml, fa.yaml, fr.yaml, it.yaml, ko.yaml, pt.yaml, ru.yaml, uk.yaml). No further changes are required.bot/start.ts (1)
203-205: Scheduled job for daily solver verification looks good.The implementation correctly schedules the
checkSolversfunction to run at midnight daily using the cron syntax0 0 * * *, which aligns with the PR objectives.locales/uk.yaml (1)
633-634: New Ukrainian Solver Notification Messages Added
The new localization keyscheck_solversandcheck_solvers_last_warninghave been added with proper placeholders (${communityName}and${remainingDays}). The language is clear and directs the user to add at least one solver to prevent community disablement. Make sure that these placeholders match exactly with what the backend expects.locales/fa.yaml (1)
636-637: New Persian Solver Notification Messages Added
The additions forcheck_solversandcheck_solvers_last_warningin Persian are well formatted and use the necessary placeholders (${communityName}and${remainingDays}). The messages clearly instruct users to add a solver and are consistent with the tone used in other locale files. Ensure that the dynamic values render correctly in production.locales/en.yaml (1)
638-639: New English Solver Notification Messages Added
The new entries forcheck_solversandcheck_solvers_last_warningin English are clear and concise. They correctly include the placeholders (${communityName}and${remainingDays}) and deliver a precise call-to-action. Verify that these keys are used consistently in the notification logic (such as in thecheckSolversfunction) and that the placeholders align with the data provided by the service.locales/pt.yaml (1)
634-635: Good addition of localized warning messages for the solver notification system.The new entries for
check_solversandcheck_solvers_last_warninglook good, providing clear and informative messages to community admins about missing solvers and the consequences of not adding them.jobs/check_solvers.ts (2)
4-4: Remove the trailing comma in the import statement.There's an unnecessary trailing comma after
Userin the import statement.-import { Community, User, } from '../models'; +import { Community, User } from '../models';
33-38: Verify community deletion behavior matches requirements.According to the PR objective, communities should be disabled after the fifth notification, but here they're being deleted immediately. Confirm if this is the intended behavior based on the discussion between Anyitechs, Catrya, and Grunch mentioned in the PR comments.
Are you sure you want to delete the community directly instead of first disabling it? The PR comments indicate there was a discussion about this, with Grunch and Catrya supporting immediate deletion, but it's worth confirming this is the final agreed approach.
jobs/check_solvers.ts
Outdated
| /** | ||
| * The community is disabled if the admin has received the maximum notification message (MAX_MESSAGES - 1) to add a solver. | ||
| */ | ||
| if (community.messages_sent_count >= Number(process.env.MAX_MESSAGES)) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix comment to accurately reflect code behavior.
The comment indicates the community is disabled after receiving MAX_MESSAGES - 1 notifications, but the code actually deletes the community when the count reaches MAX_MESSAGES.
/**
- * The community is disabled if the admin has received the maximum notification message (MAX_MESSAGES - 1) to add a solver.
+ * The community is deleted if the admin has received the maximum number of notification messages (MAX_MESSAGES) to add a solver.
*/📝 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.
| /** | |
| * The community is disabled if the admin has received the maximum notification message (MAX_MESSAGES - 1) to add a solver. | |
| */ | |
| if (community.messages_sent_count >= Number(process.env.MAX_MESSAGES)) { | |
| /** | |
| * The community is deleted if the admin has received the maximum number of notification messages (MAX_MESSAGES) to add a solver. | |
| */ | |
| if (community.messages_sent_count >= Number(process.env.MAX_MESSAGES)) { |
|
Hi @Anyitechs Have you tested it? It doesn't work for me with the latest commit |
Hi @Catrya, what issue did you notice? I tested and it works. The latest commit is on documentation, but I tested again and it works. Let me know what issue you noticed so I can try to reproduce it here. |
Hi @Anyitechs sorry, that was a mistake on my part. I tested again and I already approved it, it works fine. Good job. Congratulations |
|
@Anyitechs before merging, it is necessary that no test fails, could you please check that 1 test is failing and fix it? Thanks |
Will check it out and fix it. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/bot/bot.spec.ts (1)
456-456: Correct test update for the new scheduled jobThe updated test correctly verifies that 8 scheduled jobs are now expected instead of 7, which aligns with the addition of the new daily scheduled job for checking communities without solvers.
While this test verifies that the job is scheduled, consider adding specific tests for the
checkSolversfunctionality to ensure it:
- Correctly identifies communities without solvers
- Sends appropriate notifications to admins
- Properly tracks the number of notifications sent
- Deletes communities after reaching the MAX_MESSAGES threshold (10)
@Catrya I just fixed the failing test on CI. It was failing because the PR introduced a new recurring job but the |
|
Hey! I have couple of suggestions: |
.env-sample
Outdated
|
|
||
| # Number of notification messages sent to the admin, informing them of lack of solvers before disabling the community. The admin receives `MAX_MESSAGES - 1` notification messages. | ||
| MAX_MESSAGES=10 |
There was a problem hiding this comment.
@Anyitechs I would recommend renaming this to have a much clearer name; right now "messages" sounds too generic, how about MAX_WARNINGS_TO_ADMIN_BEFORE_COMMUNITY_DEACTIVATION
There was a problem hiding this comment.
Hi @knocte, I think the comment here already explains what "messages" represent in this context. MAX_WARNINGS_TO_ADMIN_BEFORE_COMMUNITY_DEACTIVATION is too long to have as a variable name IMO.
models/community.ts
Outdated
| @@ -81,6 +82,7 @@ const CommunitySchema = new Schema<ICommunity>({ | |||
| }, | |||
| created_at: { type: Date, default: Date.now }, | |||
| nostr_public_key: { type: String }, | |||
| messages_sent_count: { type: Number, default: 0 }, | |||
There was a problem hiding this comment.
@Anyitechs I'd suggest renaming this to warning_messages_count
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
jobs/check_solvers.ts (6)
4-4: Remove trailing comma from import statement.The import statement has a trailing comma after
User, which is inconsistent with the Airbnb JavaScript Style Guide referenced in previous comments.-import { Community, User, } from '../models'; +import { Community, User } from '../models';
15-21: Simplify conditional logic and fix inconsistent indentation.The current code has two issues:
- Unnecessary
elseblock with inconsistent indentation (8 spaces instead of 4)- Unnecessary
continuestatement flagged by static analysisfor (const community of communities) { - if (community.solvers.length > 0) { - continue; - } else { - await notifyAdmin(community, bot); - } + if (community.solvers.length === 0) { + await notifyAdmin(community, bot); + } }🧰 Tools
🪛 Biome (1.9.4)
[error] 17-17: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
33-38: Improve environment variable handling with default fallback.The code directly uses
process.env.MAX_ADMIN_WARNINGS_BEFORE_DEACTIVATIONwithout checking if it exists or has a valid value. This could lead to unexpected behavior if the environment variable is not set.- if (community.warning_messages_count >= Number(process.env.MAX_ADMIN_WARNINGS_BEFORE_DEACTIVATION)) { + const maxWarnings = Number(process.env.MAX_ADMIN_WARNINGS_BEFORE_DEACTIVATION) || 5; // Default to 5 if not set + if (community.warning_messages_count >= maxWarnings) {
45-45: Simplify remaining days calculation for better readability.The current calculation of remaining days is unnecessarily complex and could be simplified.
- const remainingDays: number = (Number(process.env.MAX_ADMIN_WARNINGS_BEFORE_DEACTIVATION) - 1) - community.warning_messages_count; + const maxWarnings = Number(process.env.MAX_ADMIN_WARNINGS_BEFORE_DEACTIVATION) || 5; // Default to 5 if not set + const remainingDays: number = maxWarnings - community.warning_messages_count;
47-47: Break down complex ternary expression for better readability.The long ternary expression makes the code harder to read and understand.
- const message = remainingDays === 0 ? i18nCtx.t('check_solvers_last_warning', { communityName: community.name }) : i18nCtx.t('check_solvers', { communityName: community.name, remainingDays: remainingDays }); + let message; + if (remainingDays === 0) { + message = i18nCtx.t('check_solvers_last_warning', { communityName: community.name }); + } else { + message = i18nCtx.t('check_solvers', { communityName: community.name, remainingDays }); + }
11-26: Add logging for better observability of the job execution.The current implementation doesn't log when it starts checking communities or how many communities it processed. Adding this information would make it easier to monitor and debug the job.
const checkSolvers = async (bot: Telegraf<MainContext>) => { try { + logger.info('Starting to check communities for missing solvers'); const communities = await Community.find(); + logger.info(`Found ${communities.length} communities to check`); for (const community of communities) { if (community.solvers.length === 0) { await notifyAdmin(community, bot); } } + logger.info('Finished checking communities for missing solvers'); } catch (error) { const message = String(error); logger.error(`checkSolvers catch error: ${message}`); } };🧰 Tools
🪛 Biome (1.9.4)
[error] 17-17: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.env-sample(1 hunks)bot/start.ts(2 hunks)jobs/check_solvers.ts(1 hunks)models/community.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- .env-sample
- models/community.ts
- bot/start.ts
🧰 Additional context used
🪛 Biome (1.9.4)
jobs/check_solvers.ts
[error] 17-17: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
🔇 Additional comments (1)
jobs/check_solvers.ts (1)
30-32: Fix comment to accurately reflect code behavior.The comment indicates the community is disabled after receiving
MAX_ADMIN_WARNINGS_BEFORE_DEACTIVATION - 1notifications, but the code actually deletes the community when the count reachesMAX_ADMIN_WARNINGS_BEFORE_DEACTIVATION./** - * The community is disabled if the admin has received the maximum notification message (MAX_ADMIN_WARNINGS_BEFORE_DEACTIVATION - 1) to add a solver. + * The community is deleted if the admin has received the maximum number of notification messages (MAX_ADMIN_WARNINGS_BEFORE_DEACTIVATION) to add a solver. */
| await bot.telegram.sendMessage( | ||
| admin.tg_id, | ||
| message, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for Telegram message sending.
The current implementation doesn't handle potential errors when sending messages via Telegram. This could lead to unhandled exceptions.
- await bot.telegram.sendMessage(
- admin.tg_id,
- message,
- );
+ try {
+ await bot.telegram.sendMessage(
+ admin.tg_id,
+ message,
+ );
+ logger.info(`Notification sent to admin of community: ${community.name}`);
+ } catch (error) {
+ logger.error(`Failed to send notification to admin of community ${community.name}: ${error}`);
+ }📝 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.
| await bot.telegram.sendMessage( | |
| admin.tg_id, | |
| message, | |
| ); | |
| } | |
| try { | |
| await bot.telegram.sendMessage( | |
| admin.tg_id, | |
| message, | |
| ); | |
| logger.info(`Notification sent to admin of community: ${community.name}`); | |
| } catch (error) { | |
| logger.error(`Failed to send notification to admin of community ${community.name}: ${error}`); | |
| } | |
| } |
This PR schedules a daily job at midnight to notify the admin of a community that there are no solvers in the community. The maximum number of notification messages right now is set to 5, afterwards the community is disabled.
Fixes #314
Summary by CodeRabbit
New Features
Bug Fixes
Documentation