feat: add configurable user requirements for community creation - #841
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (14)
✅ Files skipped from review due to trivial changes (6)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds optional minimum-threshold gating for community creation. Four new ChangesCommunity Creation Minimum Requirements Gating
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![ermeme[bot]](https://avatars.githubusercontent.com/in/3690068?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
Verdict: Approve
Looks Good
- The previous fail-open config issue is fixed: invalid or negative threshold values are now rejected instead of silently disabling enforcement.
- The new eligibility gate is scoped to
/communityand the locale/config updates are complete for the touched surface.
grunch
left a comment
grunch
left a comment
There was a problem hiding this comment.
Review estricto del PR. La funcionalidad es correcta y está bien estructurada (fail-closed en config inválida, i18n completo en los 10 idiomas, campos del modelo User verificados: trades_completed, volume_traded, total_rating —promedio 0–5— y created_at).
El único cambio recomendado antes de merge es el nivel de log (comentario 1). El resto son mejoras opcionales/no bloqueantes. Dejo sugerencias de código inline para que las apliques tú.
grunch
left a comment
grunch
left a comment
There was a problem hiding this comment.
Strict review. The feature is well thought out — fail-closed on bad config, correct use of ?? (so a threshold of 0 is preserved), explicit handling of an invalid created_at, single parse at configure() time, and all 8 placeholders match across the 10 locale files. logger.warning is also correct here since the logger uses syslog levels.
Two things worth resolving before merge (HIGH), plus a few UX/quality notes inline.
HIGH — branch is behind main (2 commits). It doesn't include #839 (the mongoose/grpc-js/bn.js security patch) nor #842. On this branch package.json still pins mongoose ^8.17.1 and the models use Document instead of Document<string>, so tsc --noEmit reports 3 type errors in models/{user,order,community}.ts — main reports 0. It's not a defect introduced by this PR (it doesn't touch those files and main would win the merge), but please rebase onto main so CI is green and the security patch isn't visually reverted in the diff.
| const n = Number(raw); | ||
| if (Number.isFinite(n) && n >= 0) return { value: n, isValid: true }; | ||
| return { value: null, isValid: false }; | ||
| }; |
There was a problem hiding this comment.
HIGH — no tests for non-trivial pure logic. parseOptionalNumber and meetsCommunityCreationRequirements are perfect unit-test targets (empty vs invalid input, negative, threshold = null, invalid date, boundary >=), but there are no specs and both are module-private so they can't be tested as-is. Suggest extracting them to an exported helper (e.g. bot/modules/community/requirements.ts) and adding unit tests — the repo standard is 80% coverage.
| bot.command('community', userMiddleware, async ctx => { | ||
| const { user } = ctx; | ||
|
|
||
| if (invalidEnvVars.length > 0) { |
There was a problem hiding this comment.
MEDIUM — invalid config is only surfaced when a user runs /community. invalidEnvVars is computed once in configure(), but the error is only logged inside the handler on first use. A typo like MIN_REPUTATION=abc silently breaks the feature for everyone (all users get generic_error) until someone hits it. Consider logging the misconfiguration at configure() time so operators catch it at startup.
| return ctx.reply( | ||
| ctx.i18n.t('community_creation_requirements_not_met', { | ||
| userOrders: user.trades_completed, | ||
| requiredOrders: minOrders ?? '-', | ||
| userVolume: user.volume_traded, | ||
| requiredVolume: minVolume ?? '-', | ||
| userDays: daysUsing, | ||
| requiredDays: minDays ?? '-', | ||
| userRep: user.total_rating, | ||
| requiredRep: minReputation ?? '-', | ||
| }), | ||
| ); |
There was a problem hiding this comment.
MEDIUM — the message lists requirements that aren't enforced. All four lines always render, so an unset threshold shows e.g. Orders: 5 / -, advertising a requirement that isn't applied. It's confusing. Consider building the message from only the active (non-null) thresholds.
LOW — userRep (total_rating) is rendered raw (e.g. 4.733333); elsewhere the codebase uses .toFixed(2). Rounding it here would be more consistent.
| # Minimum requirements to create a community (leave unset to disable the restriction) | ||
| # Note: It is recommended to combine MIN_REPUTATION with MIN_ORDERS to avoid gaming the system. | ||
| # COMMUNITY_CREATION_MIN_ORDERS=100 | ||
| # COMMUNITY_CREATION_MIN_VOLUME=10000000 |
There was a problem hiding this comment.
LOW — volume_traded is stored in sats (util/index.ts), so 10000000 = 0.1 BTC. Worth noting the unit in the comment so operators don't read it as fiat or BTC.
| bot.command('mycomm', userMiddleware, commands.communityAdmin); | ||
| bot.command('mycomms', userMiddleware, commands.myComms); | ||
| if (process.env.COMMUNITY_CREATION_ENABLED === 'true') { | ||
| // Parsing of this values only happens once, they are not reparsed every time an user wants to create a new community |
There was a problem hiding this comment.
NOTE (nit) — small grammar in the comment: "Parsing of this values" → "these values", and "an user" → "a user".
…osed configuration checks, improved error logging levels, added explicit user date validation, and updated all locales to provide detailed feedback to the user on unmet requirements.
- Renaming COMMUNITY_CREATION_MIN_VOLUME to COMMUNITY_CREATION_MIN_VOLUME_SATS and documenting the sats unit in .env-sample. - Extracting validation logic to a new module with high test coverage. - Implementing startup logging for misconfigurations. - Dynamically constructing requirement messages to show only active thresholds. - Splitting localization keys for granular control across all locales.
c7d5b1a to
d10222e
Compare
grunch
left a comment
grunch
left a comment
There was a problem hiding this comment.
Strict review — feat: add configurable user requirements for community creation
I reviewed the full diff (14 files, +342) and validated the branch locally: tsc compiles cleanly, ESLint passes on the changed files, and the full test suite is green (144 passing), including the 11 new requirements.spec.ts cases. Nice work — the architecture is clean and the iteration history (unit rename to _SATS, fail-closed handling, helper extraction) addressed the right things.
What's good
- Logic extracted into a pure, well-typed, tested module (
requirements.ts).parseOptionalNumber/getCommunityThresholds/meetsCommunityCreationRequirementsare easy to reason about. - Fail-closed posture is correct: invalid env values and an invalid
created_atboth returngeneric_errorinstead of silently letting the user through. - Unit consistency is right:
volume_tradedis accumulated in satoshis (util/index.ts:142,order.amount"amount in satoshis"), so comparing it againstCOMMUNITY_CREATION_MIN_VOLUME_SATSis correct. The rename removed the earlier ambiguity. - Thresholds parsed once at
configure()time, not per-command — good, and the comment documents it. - Locale parity: all 10 languages get the same 5 split keys; dynamic message assembly only shows active thresholds. Clean.
logger.warningis valid here (the logger uses syslog levels).
Non-blocking findings
1. (Medium) No test coverage for the index.ts command handler. The pure helper is well tested, but the new branching in the command handler — invalidEnvVars early return, the hasInvalidDate path, and the dynamic message builder — has zero tests. That's the part most likely to regress (i18n keys, conditional lines). Consider an integration-style test that drives the handler with a mocked ctx/user, or at least extract the message-building into a testable pure function.
2. (Low) Reputation gating is weak without a minimum review count. total_rating is a running average (bot/commands.ts:283-288) that starts at 0, so a user with a single 5★ review passes MIN_REPUTATION trivially. The .env-sample note already recommends combining it with MIN_ORDERS, which mitigates this, but consider also gating on total_reviews so reputation can't be satisfied by one rating.
3. (Low) parseOptionalNumber accepts non-integers and exotic numerics for count-like thresholds. MIN_ORDERS / MIN_DAYS_USING_BOT are conceptually integers, but "10.5", "1e2", and "0x10" all parse as valid via Number(). Not a security issue (still >= 0, finite), just looser than the intent. An Number.isInteger check for the count thresholds would tighten it.
4. (Note) Double logging on misconfiguration. getCommunityThresholds() logs the invalid config once at startup, and the handler logs error again on every /community invocation while misconfigured. Likely intentional (startup + runtime visibility), but the per-invocation error could be noisy — consider downgrading the runtime one or logging once.
Verdict
No CRITICAL or HIGH issues, no correctness bugs, build/tests/lint all green, and the security-sensitive paths fail closed. Approving. Please consider adding handler-level test coverage (finding #1) in a follow-up.
3 participants
Closes #838
Continues #840
Add environment-based minimum requirements to the /community command so only users who meet configurable thresholds can create a community
If any requirement is not met, the bot replies with a short error message and exits — no scene is entered
If an env var is not set, that restriction is simply not applied
Summary by CodeRabbit
.env-sample) with optional community-creation minimum requirement settings (left unset to disable restrictions).