Allow community admins to configure payment methods for order wizard - Issue292#795
Allow community admins to configure payment methods for order wizard - Issue292#795Luquitasjeffrey wants to merge 15 commits intomainfrom
Conversation
…n selected from the payment methods wizzard for communities
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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. WalkthroughAdds community-level payment methods: model field, admin wizard and callback/button to edit methods, order-flow multi-select integration, middleware registration for the wizard, tests, and i18n strings across multiple locales. ChangesCommunity Payment Methods
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 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. Review rate limit: 0/1 reviews remaining, refill in 49 minutes and 4 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
locales/uk.yaml (1)
706-715: 💤 Low valueNew keys are placed outside the established module section boundaries.
The file groups keys with explicit
# START modules/<name>/# END modules/<name>markers. The 10 new keys logically belong to two modules:
- Community-side (
enter_community_payment_methods,current_payment_methods,payment_methods_wizard_commands,payment_methods_saved,payment_methods_reset,community_payment_methods) →# START modules/community…# END modules/community- Order-side (
select_payment_methods,confirm_payment_methods,no_payment_method_selected,custom_payment_method) →# START modules/orders…# END modules/ordersAdding them after the last section's closing marker makes the structure inconsistent and harder to maintain as the file grows, especially since all other locale files added in this PR presumably follow the same pattern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@locales/uk.yaml` around lines 706 - 715, These new translation keys were added outside module boundaries; move the community-related keys (community_payment_methods, enter_community_payment_methods, current_payment_methods, payment_methods_wizard_commands, payment_methods_saved, payment_methods_reset) into the block between the "# START modules/community" and "# END modules/community" markers, and move the order-related keys (select_payment_methods, confirm_payment_methods, no_payment_method_selected, custom_payment_method) into the "# START modules/orders" … "# END modules/orders" block so they follow the established module grouping and file structure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bot/modules/community/scenes.ts`:
- Around line 942-955: The handler currently treats empty or comma-only input as
methods = [] and saves that, clearing community.payment_methods; change it to
detect methods.length === 0 after trimming/splitting and instead send a reply
prompting the user to provide at least one method (e.g., using
ctx.reply(ctx.i18n.t('provide_payment_methods'))), do not set
community.payment_methods or call community.save(), and keep the wizard open
(i.e., return without advancing or exiting the scene) so only an explicit /reset
clears methods.
In `@bot/modules/orders/scenes.ts`:
- Around line 70-73: replaceRegex and paymentMethod are never reassigned so
change their declarations from let to const to satisfy ESLint; update the
declarations for replaceRegex, paymentMethod (which references selectedMethods
and method) to use const so the bindings are immutable while preserving the
existing logic in this block.
- Around line 236-252: When entering the custom payment flow, clear any
previously chosen predefined selections so they don't override the custom text
later; inside the pm_custom branch (the handler assigned to ctx.wizard.state),
reset ctx.wizard.state.selectedMethods = [] (or delete it) before setting
ctx.wizard.state.method = text and calling ctx.wizard.state.updateUI(), and
ensure any serialization logic prefers method when method is set (leave existing
serialization but ensure selectedMethods is emptied here).
---
Nitpick comments:
In `@locales/uk.yaml`:
- Around line 706-715: These new translation keys were added outside module
boundaries; move the community-related keys (community_payment_methods,
enter_community_payment_methods, current_payment_methods,
payment_methods_wizard_commands, payment_methods_saved, payment_methods_reset)
into the block between the "# START modules/community" and "# END
modules/community" markers, and move the order-related keys
(select_payment_methods, confirm_payment_methods, no_payment_method_selected,
custom_payment_method) into the "# START modules/orders" … "# END
modules/orders" block so they follow the established module grouping and file
structure.
🪄 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
Run ID: 9650ec14-ccff-4963-a8e8-01c53055cf36
📒 Files selected for processing (18)
bot/middleware/stage.tsbot/modules/community/commands.tsbot/modules/community/communityContext.tsbot/modules/community/index.tsbot/modules/community/messages.tsbot/modules/community/scenes.tsbot/modules/orders/scenes.tslocales/de.yamllocales/en.yamllocales/es.yamllocales/fa.yamllocales/fr.yamllocales/it.yamllocales/ko.yamllocales/pt.yamllocales/ru.yamllocales/uk.yamlmodels/community.ts
![mostronatorcoder[bot]](https://avatars.githubusercontent.com/in/2922838?s=60&v=4){kind=small}
There was a problem hiding this comment.
I’m requesting changes because the current data model for community payment methods is ambiguous and can corrupt valid values.
The feature stores community payment methods by splitting admin input on commas, and later stores selected methods as a comma-joined string again. That means a legitimate method containing a comma cannot be represented safely: it will be split when configured and flattened again when persisted to the order.
Concretely, the problem is in:
bot/modules/community/scenes.ts(text.split(',')when saving community payment methods)bot/modules/orders/scenes.ts(selected.join(', ')when confirming the selection)
So before merging, the implementation needs one of these two fixes:
- Use a representation that is not ambiguous, or
- Explicitly reject commas in payment method names and validate that constraint at input time with a clear user-facing error
I also expected test coverage for the new wizard flow and selection logic. This PR adds multiple new branches in order creation UX, but I do not see tests covering them. At minimum, please add tests for:
- community with configured payment methods
- multi-select confirmation
- confirmation with no selection
- custom free-text fallback
- community without configured payment methods
Because of the data ambiguity issue, I do not think this is safe to approve yet.
About the splitting of payment methods by a comma, I will dismiss it because:
|
There was a problem hiding this comment.
Thanks for the update. The new test coverage improves this PR substantially, and the intended free-text fallback behavior is now clear.
I initially had concerns about comma ambiguity in payment method names, but after re-reading the admin prompt, the UX explicitly defines commas as the separator for community payment methods:
Enter the payment methods accepted in your community, separated by commas
So within the current product design, commas are effectively reserved as delimiters, which makes the behavior consistent with the prompt.
I still think explicit validation for commas or duplicate entries would make the flow more robust, but at this point I see that as a follow-up improvement rather than a merge blocker.
The added tests cover the main new branches in the order wizard, and the implementation now looks acceptable to me.
Approved.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
bot/modules/community/scenes.ts (1)
948-951: ⚡ Quick winExtract
MAX_COMMUNITY_PAYMENT_METHODSto a module-level constant, consistent withCURRENCIES.Line 17 establishes the pattern:
const CURRENCIES = parseInt(process.env.COMMUNITY_CURRENCIES || '10');— parsed once at module load. The newmaxis re-parsed on every step-2 invocation, diverging from that idiom.♻️ Proposed refactor
const CURRENCIES = parseInt(process.env.COMMUNITY_CURRENCIES || '10'); +const MAX_PAYMENT_METHODS = parseInt( + process.env.MAX_COMMUNITY_PAYMENT_METHODS || '10', +);- const max = parseInt(process.env.MAX_COMMUNITY_PAYMENT_METHODS || '10'); - if (methods.length > max) { - return await ctx.reply(ctx.i18n.t('max_allowed', { max })); + if (methods.length > MAX_PAYMENT_METHODS) { + return await ctx.reply(ctx.i18n.t('max_allowed', { max: MAX_PAYMENT_METHODS })); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bot/modules/community/scenes.ts` around lines 948 - 951, The code parses MAX_COMMUNITY_PAYMENT_METHODS inside the handler (const max = parseInt(process.env.MAX_COMMUNITY_PAYMENT_METHODS || '10')) which duplicates the CURRENCIES pattern; move that parse into a module-level constant (e.g., const MAX_COMMUNITY_PAYMENT_METHODS = parseInt(process.env.MAX_COMMUNITY_PAYMENT_METHODS || '10');) near the existing CURRENCIES declaration and then replace uses of the local variable max with the module constant (refer to the handler code that checks methods.length and the CURRENCIES symbol to align style).tests/bot/modules/orders/scenes.spec.ts (1)
93-105: ⚡ Quick win
calledWithdoesn't verify the inline keyboard markup
ctx.reply.calledWith('select_payment_methods')uses sinon's partial positional matching — it returnstrueeven whenreplyis called asreply('select_payment_methods', { reply_markup: { inline_keyboard: [...] } }). The assertion therefore doesn't validate that the keyboard buttons were actually constructed and passed, so a regression dropping or malforming the keyboard would not be caught.Consider asserting on
ctx.reply.firstCall.args[1]to verify thereply_markupstructure, e.g.:🔍 Suggested assertion addition
expect(ctx.reply.calledWith('select_payment_methods')).to.equal(true); + const replyMarkup = ctx.reply.firstCall.args[1]?.reply_markup; + expect(replyMarkup).to.be.an('object'); + expect(replyMarkup.inline_keyboard).to.be.an('array').with.length.greaterThan(0); expect(ctx.wizard.next.calledOnce).to.equal(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/bot/modules/orders/scenes.spec.ts` around lines 93 - 105, The test currently asserts only that ctx.reply was called with the text 'select_payment_methods' but doesn't validate the inline keyboard; update the test for runStep0 to inspect ctx.reply.firstCall.args[1] and assert that its reply_markup.inline_keyboard matches the expected button layout for the community payment methods (verify button labels and callback_data structure), while keeping the existing assertions for ctx.wizard.next and ctx.wizard.state.selectedMethods intact; reference ctx.reply, ctx.reply.firstCall.args, and runStep0 to locate where to add the assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@bot/modules/community/scenes.ts`:
- Around line 948-951: The code parses MAX_COMMUNITY_PAYMENT_METHODS inside the
handler (const max = parseInt(process.env.MAX_COMMUNITY_PAYMENT_METHODS ||
'10')) which duplicates the CURRENCIES pattern; move that parse into a
module-level constant (e.g., const MAX_COMMUNITY_PAYMENT_METHODS =
parseInt(process.env.MAX_COMMUNITY_PAYMENT_METHODS || '10');) near the existing
CURRENCIES declaration and then replace uses of the local variable max with the
module constant (refer to the handler code that checks methods.length and the
CURRENCIES symbol to align style).
In `@tests/bot/modules/orders/scenes.spec.ts`:
- Around line 93-105: The test currently asserts only that ctx.reply was called
with the text 'select_payment_methods' but doesn't validate the inline keyboard;
update the test for runStep0 to inspect ctx.reply.firstCall.args[1] and assert
that its reply_markup.inline_keyboard matches the expected button layout for the
community payment methods (verify button labels and callback_data structure),
while keeping the existing assertions for ctx.wizard.next and
ctx.wizard.state.selectedMethods intact; reference ctx.reply,
ctx.reply.firstCall.args, and runStep0 to locate where to add the assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1389f81a-ac04-4059-a101-ddce6a9c7ffa
📒 Files selected for processing (3)
.env-samplebot/modules/community/scenes.tstests/bot/modules/orders/scenes.spec.ts
✅ Files skipped from review due to trivial changes (1)
- .env-sample
There was a problem hiding this comment.
Thanks for the follow-up. I re-checked the PR after the latest commit, and the previous inconsistency between the in-code fallback and .env-sample is now resolved.
Making the maximum number of community payment methods configurable is a reasonable improvement, and I do not see any new blocking issue introduced by the latest changes.
The remaining thing I would still consider a future improvement is adding a test for the max-limit branch in the payment methods wizard, but I do not see that as a blocker for this PR.
Approved.
2 participants
Fixes #292
Continues #786
Summary
Adds a payment_methods field (array of strings) to the Community model, analogous to community.currencies
Community admins can configure payment methods via a new wizard scene (UPDATE_PAYMENT_METHODS_COMMUNITY_WIZARD_SCENE_ID) accessible from the community admin panel
When a user creates an order (/buy or /sell) in a community that has payment methods configured, the order wizard replaces the free-text payment method input with an inline keyboard allowing selection of one or more methods; the
final value text in the published order is a comma-joined string of all selected methods
Summary by CodeRabbit
New Features
Localization
Models / Config
Tests