Feat: add optional [community] param in order creation command - #881
Feat: add optional [community] param in order creation command#881Luquitasjeffrey wants to merge 6 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughBuy and sell commands accept an optional community identifier after the price margin, resolve explicit communities by handle or public name, preserve chat-based fallback behavior, and update localized help and tests for routing, validation, and missing-community handling. ChangesOrder community routing
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bot/validations.ts (1)
261-267: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftKeep the selected/default community when adding an explicit target.
The current downstream contract and localized instructions treat the explicit community as a replacement, but the PR objective requires publishing to both targets.
bot/validations.ts#L261-L267: update sell routing socommunityNamecreates an additional target.bot/validations.ts#L353-L359: apply the same behavior to buy routing.locales/de.yaml#L74-L76: replace “instead of” wording with additive publication wording.locales/de.yaml#L90-L92: apply the same correction to buy help.locales/en.yaml#L76-L78: state that the explicit community is an additional target.locales/en.yaml#L92-L94: apply the same correction to buy help.locales/es.yaml#L74-L76: state that the explicit community is an additional target.locales/es.yaml#L90-L92: apply the same correction to buy help.locales/fa.yaml#L77-L79: state that the explicit community is an additional target.locales/fa.yaml#L93-L95: apply the same correction to buy help.locales/ru.yaml#L72-L74: state that the explicit community is an additional target.locales/ru.yaml#L88-L90: apply the same correction to buy help.locales/uk.yaml#L73-L75: state that the explicit community is an additional target.locales/uk.yaml#L89-L91: apply the same correction to buy help.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bot/validations.ts` around lines 261 - 267, Update the sell and buy routing logic in bot/validations.ts at lines 261-267 and 353-359 so an explicit communityName is added alongside the selected or default community rather than replacing it. Revise the corresponding sell and buy help text in locales/de.yaml lines 74-76 and 90-92, locales/en.yaml lines 76-78 and 92-94, locales/es.yaml lines 74-76 and 90-92, locales/fa.yaml lines 77-79 and 93-95, locales/ru.yaml lines 72-74 and 88-90, and locales/uk.yaml lines 73-75 and 89-91 to describe the explicit community as an additional publication target.
🧹 Nitpick comments (3)
tests/bot/validation.spec.ts (1)
159-186: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd regression coverage for surplus trailing arguments.
These cases cover the valid one- and two-token forms, but not
priceMargin,communityName, and an unexpected third token. Add one sell and one buy case assertingfalseand the format-message reply.Also applies to: 256-283
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/bot/validation.spec.ts` around lines 159 - 186, Add regression tests for surplus trailing arguments in the sell and buy validation suites, covering inputs with priceMargin, communityName, and an unexpected third trailing token. Assert validateSellOrder and the corresponding buy validator return false and invoke the format-message reply stub.bot/validations.ts (1)
172-174: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid
anyfor the parsed margin.
priceMarginis eitherundefinedor a command string token, and it is only checked withisNaN/passed to downstream numeric parsing. Type it asstring | undefinedand keep the same validation path;anyunnecessarily disables type checking in this TypeScript codebase.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bot/validations.ts` around lines 172 - 174, Update the priceMargin declaration in the validation flow to use the explicit string | undefined type instead of any. Preserve the existing empty-string-to-undefined conversion and keep the current isFloat/parseInt/isNaN validation path unchanged.Source: Coding guidelines
tests/util/communityHelper.spec.ts (1)
17-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
isBanned, the error path, and regex-metacharacter identifiers.Current tests cover the group/name routing logic well, but don't exercise: (1)
isBanned: truewhen the resolved community has the user inbanned_users, (2) thecatchbranch whenCommunity.findOnerejects, and (3) an identifier containing regex metacharacters (e.g.a.b*c) to confirm it's matched literally rather than as a wildcard — directly validating the escaping that the static analyzer flagged.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/util/communityHelper.spec.ts` around lines 17 - 67, Extend the getCommunityByIdentifier routing tests to cover three cases: assert isBanned is true when the resolved community’s banned_users contains the user, verify Community.findOne rejection exercises the catch path and its returned error behavior, and use an identifier such as a.b*c to confirm the group query regex matches it literally rather than treating metacharacters as wildcards.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bot/validations.ts`:
- Around line 164-179: Update parseOptionalOrderParams in bot/validations.ts
(164-179) to reject or signal an invalid parse when optionalArgs contains more
than two trailing tokens, rather than silently ignoring extras; preserve the
existing positional handling for zero, one, or two tokens. Add sell regression
coverage in tests/bot/validation.spec.ts (159-186) and equivalent buy coverage
in tests/bot/validation.spec.ts (256-283), asserting commands with an unexpected
third token are rejected.
In `@locales/it.yaml`:
- Around line 90-92: Update the Italian `/buy` usage text in
`buy_correct_format` and the `/buy` help line to use the localized
`surplus/sconto` terminology consistently with `sell_correct_format` and the
`/sell` help text, replacing the remaining English “discount” and “premium”
labels.
In `@locales/pt.yaml`:
- Around line 249-250: Update the Portuguese /sell and /buy help entries so the
premium/desconto and comunidade arguments are wrapped in the established
optional-argument notation, matching the fr/it/ko locale formatting while
preserving the existing command descriptions.
In `@locales/uk.yaml`:
- Around line 73-75: In the sell_correct_format translation, close the
MarkdownV2 italic span around “метод платежу” by adding the missing underscore
before the escaped closing angle bracket, while preserving the existing command
format and escaping.
---
Outside diff comments:
In `@bot/validations.ts`:
- Around line 261-267: Update the sell and buy routing logic in
bot/validations.ts at lines 261-267 and 353-359 so an explicit communityName is
added alongside the selected or default community rather than replacing it.
Revise the corresponding sell and buy help text in locales/de.yaml lines 74-76
and 90-92, locales/en.yaml lines 76-78 and 92-94, locales/es.yaml lines 74-76
and 90-92, locales/fa.yaml lines 77-79 and 93-95, locales/ru.yaml lines 72-74
and 88-90, and locales/uk.yaml lines 73-75 and 89-91 to describe the explicit
community as an additional publication target.
---
Nitpick comments:
In `@bot/validations.ts`:
- Around line 172-174: Update the priceMargin declaration in the validation flow
to use the explicit string | undefined type instead of any. Preserve the
existing empty-string-to-undefined conversion and keep the current
isFloat/parseInt/isNaN validation path unchanged.
In `@tests/bot/validation.spec.ts`:
- Around line 159-186: Add regression tests for surplus trailing arguments in
the sell and buy validation suites, covering inputs with priceMargin,
communityName, and an unexpected third trailing token. Assert validateSellOrder
and the corresponding buy validator return false and invoke the format-message
reply stub.
In `@tests/util/communityHelper.spec.ts`:
- Around line 17-67: Extend the getCommunityByIdentifier routing tests to cover
three cases: assert isBanned is true when the resolved community’s banned_users
contains the user, verify Community.findOne rejection exercises the catch path
and its returned error behavior, and use an identifier such as a.b*c to confirm
the group query regex matches it literally rather than treating metacharacters
as wildcards.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d5e60cb1-bd0c-42ea-bb20-a3d807201302
📒 Files selected for processing (16)
bot/modules/orders/commands.tsbot/validations.tslocales/de.yamllocales/en.yamllocales/es.yamllocales/fa.yamllocales/fr.yamllocales/it.yamllocales/ko.yamllocales/pt.yamllocales/ru.yamllocales/uk.yamltests/bot/modules/orders/commands.spec.tstests/bot/validation.spec.tstests/util/communityHelper.spec.tsutil/communityHelper.ts
…r and validateBuyOrder
grunch
left a comment
There was a problem hiding this comment.
Strict review — changes requested
I read the complete conversation on issue #877, the full review history from #879, the current #881 conversation and threads, and reviewed current head 064695d4d3e151a192c54ec9c99885780168cf1a.
Blocking
-
The PR still auto-closes an issue it intentionally does not implement. The body says
Closes #877, but #877 requests publishing to the optional community alongside the currently selected one. The issue's own follow-up explicitly says this implementation remains single-destination and should useRefs #877instead. The current handlers callcreateOrderonce and replace the selected/group destination with the explicit one. Either implement the additive workflow, or remove the closing keyword and keep the remaining #877 behavior tracked separately. -
util/communityHelper.ts:90-98keeps the non-selective case-insensitive regex lookup from the previous review.Community.groupis already normalized withlowercase: trueand has a unique index (models/community.ts:76), but every explicit/buyor/selldestination first queries it through/^…$/i. That cannot use the unique index as a selective exact lookup and turns this hot path into a scan. Use an exact normalized query such asgroup: identifier.toLowerCase(); the current helper test should assert case-insensitive behavior rather than requiring aRegExpimplementation.
Verified fixed on the current head
- Unknown explicit communities in group chats now delete the command without replying.
- Surplus positional arguments are rejected for both
/buyand/selland have regression tests. - The Italian, Portuguese, and Ukrainian locale findings are fixed; all locale YAML files parse successfully.
- The focused changed-path suite passes: 90 tests.
Non-blocking follow-ups
tests/bot/modules/orders/commands.spec.ts:43,67mutatesMAX_PENDING_ORDERSwithout restoring it, so the spec can leak process state into later files.docs/FAQ.en.md:74,92anddocs/FAQ.es.md:73,91still show the old command syntax without[community].
CI is green, TypeScript compilation and lint pass, formatting is clean, and git diff --check passes. The two blockers above still prevent approval.
Continues #879
Summary by CodeRabbit
Summary
New Features
/buyand/sellnow accept an optional final community parameter to route orders to a specific community.Bug Fixes
Documentation
/helpand/sell//buyusage text across supported languages.Tests