Add settled_by_admin field in order.ts model and improve getDetailedOrder output.#747
Add settled_by_admin field in order.ts model and improve getDetailedOrder output.#747Luquitasjeffrey wants to merge 10 commits intomainfrom
Conversation
…d_by_admin' field denoting that, and modified the 'getDetailedOrder' function output to show if an order was completed by an admin or not
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaced the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@bot/start.ts`:
- Around line 547-551: The code currently sets order.settled_by_admin = true and
calls await order.save() before calling settleHoldInvoice, which can leave the
flag persisted if settleHoldInvoice throws; change the flow so you first call
await settleHoldInvoice({ secret: order.secret }) and only after it succeeds set
order.settled_by_admin = true and call await order.save(), or wrap both calls in
a try/catch and roll back (reset) order.settled_by_admin and avoid saving if
settleHoldInvoice fails; refer to the order object, its save() method, and the
settleHoldInvoice function when updating the logic.
In `@locales/fa.yaml`:
- Around line 183-184: Replace the English label "Settled by admin:
${settledByAdmin}" in the Persian locale with a Persian translation to keep
localization consistent; update the entry in locales/fa.yaml (the string
"Settled by admin: ${settledByAdmin}") to a Persian equivalent such as "تسویه
شده توسط ادمین: ${settledByAdmin}" ensuring the placeholder ${settledByAdmin}
remains unchanged.
🚨 Request Changes - Critical Issues FoundI need to request changes before this PR can be merged. There are critical security and data integrity issues that must be addressed: 1. BREAKING CHANGE without Data Migration (CRITICAL)Removing "COMPLETED_BY_ADMIN" from the status enum is a breaking change that will break existing orders in production. Problem: Orders currently in "COMPLETED_BY_ADMIN" status will be in an invalid state after deployment. Required: Add a database migration script that:
Without this migration, existing orders will be unreachable and potentially cause runtime errors. 2. Data Integrity Issue in bot/start.ts (SECURITY)Current (WRONG): order.settled_by_admin = true;
await order.save();
await settleHoldInvoice({ secret: order.secret });Problem: If settleHoldInvoice() fails, the order is already marked as settled by admin but the hold invoice was NOT settled. This creates an inconsistent state where the database says "settled by admin" but the Lightning payment did not complete. Required (CORRECT): try {
await settleHoldInvoice({ secret: order.secret });
order.settled_by_admin = true;
await order.save();
} catch (error) {
// Handle error - do NOT mark as settled_by_admin
throw error;
}The settlement must succeed BEFORE marking the order as settled by admin. 3. Schema Issue - Optional FieldIn models/order.ts: settled_by_admin?: boolean; // Optional - BADProblem: Optional fields can be undefined, leading to inconsistent data. Required: settled_by_admin: { type: Boolean, default: false, required: true }4. Missing CodeRabbit ReviewThis PR has been pending CodeRabbit review. I should have waited for that automated analysis before reviewing. Please ensure all automated checks pass. Summary of Required Changes
This is financial software handling real Bitcoin. We cannot merge breaking changes without proper migration and data integrity guarantees. Status: 🔴 Request Changes - DO NOT MERGE until issues are resolved. |
|
Hey @Luquitasjeffrey, friendly reminder — the issues from my previous review are still unresolved. The last two commits are upstream merges, no fixes. Here's a quick recap: 1. 🔴 Missing database migration (CRITICAL)You're removing // Find orders with the old status and migrate them
db.orders.updateMany(
{ status: "COMPLETED_BY_ADMIN" },
{ $set: { status: "PAID_HOLD_INVOICE", settled_by_admin: true } }
)2. 🔴 Order of operations in
|
…correcting order of operations in bot/start.ts due to a potential failure while settling the hold invoice
There was a problem hiding this comment.
♻️ Duplicate comments (1)
bot/start.ts (1)
547-551:⚠️ Potential issue | 🔴 CriticalSet
settled_by_adminbeforesettleHoldInvoiceto avoid a race.At Line 548, settlement can trigger async order-update handlers before Line 549–550 runs, which can leave inconsistent state for admin-settled orders.
Suggested fix
if (order.secret) { - await settleHoldInvoice({ secret: order.secret }); order.settled_by_admin = true; await order.save(); + await settleHoldInvoice({ secret: order.secret }); }Based on learnings: In bot/start.ts /settleorder command,
order.settled_by_admin = truemust be set and saved before callingsettleHoldInvoice({ secret: order.secret })to prevent a race condition.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bot/start.ts` around lines 547 - 551, The order settlement code can race because settleHoldInvoice(...) may trigger async handlers that read order state before we mark it as admin-settled; update the sequence so you set order.settled_by_admin = true and await order.save() before calling settleHoldInvoice({ secret: order.secret }) (refer to the settleorder command, order.settled_by_admin, order.save, and settleHoldInvoice symbols) so the persisted flag exists for any downstream handlers.
🧹 Nitpick comments (1)
scripts/migrate_completed_by_admin_orders.ts (1)
30-35: Preferfinallyfor deterministic connection cleanup.Line 33–35 exits on error without guaranteed close. Consider using
finallyto always close the Mongo connection and setprocess.exitCodeinstead of immediateprocess.exit(...).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/migrate_completed_by_admin_orders.ts` around lines 30 - 35, The migration currently calls mongoose.connection.close() only on success and uses process.exit(...) inside the catch which can leave the DB open; refactor the try/catch to a try/catch/finally where you always await mongoose.connection.close() in the finally block (handling a potentially undefined connection) and replace direct calls to process.exit(0)/process.exit(1) with setting process.exitCode = 0 or 1 so shutdown can occur deterministically; update references in this file to remove immediate process.exit usage and ensure logger.error(`Migration failed: ${error}`) remains in the catch before setting process.exitCode.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@bot/start.ts`:
- Around line 547-551: The order settlement code can race because
settleHoldInvoice(...) may trigger async handlers that read order state before
we mark it as admin-settled; update the sequence so you set
order.settled_by_admin = true and await order.save() before calling
settleHoldInvoice({ secret: order.secret }) (refer to the settleorder command,
order.settled_by_admin, order.save, and settleHoldInvoice symbols) so the
persisted flag exists for any downstream handlers.
---
Nitpick comments:
In `@scripts/migrate_completed_by_admin_orders.ts`:
- Around line 30-35: The migration currently calls mongoose.connection.close()
only on success and uses process.exit(...) inside the catch which can leave the
DB open; refactor the try/catch to a try/catch/finally where you always await
mongoose.connection.close() in the finally block (handling a potentially
undefined connection) and replace direct calls to
process.exit(0)/process.exit(1) with setting process.exitCode = 0 or 1 so
shutdown can occur deterministically; update references in this file to remove
immediate process.exit usage and ensure logger.error(`Migration failed:
${error}`) remains in the catch before setting process.exitCode.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: aae5b76a-95fe-43af-8241-9fac1e0f31ae
📒 Files selected for processing (3)
bot/start.tsscripts/migrate_completed_by_admin_orders.tstsconfig.json
✅ Files skipped from review due to trivial changes (1)
- tsconfig.json
I addressed your review.
|
![mostronatorcoder[bot]](https://avatars.githubusercontent.com/in/2922838?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
This PR removes the COMPLETED_BY_ADMIN status and replaces it with a settled_by_admin boolean field. The design is architecturally correct and includes a critical fix for operation ordering.
✅ Strengths
1. Correct architectural design
- Removing
COMPLETED_BY_ADMINstatus and replacing it with asettled_by_adminboolean is the right approach - Status should reflect workflow state, not who completed it
- Boolean flag captures metadata without polluting the state machine
2. Migration script included
scripts/migrate_completed_by_admin_orders.tsmigrates existing orders- Transforms
status: COMPLETED_BY_ADMIN→status: PAID_HOLD_INVOICE+settled_by_admin: true - Safe logic using
updateMany
3. Complete status cleanup
- All
COMPLETED_BY_ADMINreferences removed except:- Migration script (correct, necessary)
- Locale files (message translations, OK)
4. Critical fix: Operation ordering in bot/start.ts
Before:
if (order.secret) await settleHoldInvoice({ secret: order.secret });
order.status = 'COMPLETED_BY_ADMIN'; // ❌ Status changes even if settle fails
await order.save();Problem: If settleHoldInvoice() throws, order ends up in COMPLETED_BY_ADMIN status but the invoice is NOT settled in LND → critical inconsistency.
Now:
if (order.secret) {
await settleHoldInvoice({ secret: order.secret }); // ✅ If fails, exception exits here
order.settled_by_admin = true; // ✅ Only executes if settle OK
await order.save();
}Analysis: ✅ CORRECT
- If
settleHoldInvoice()fails → exception exits, nothing modified - If settle OK → marks
settled_by_admin = trueand saves - Status remains
PAID_HOLD_INVOICE(correct state after settle)
This is a real bug fix.
5. Display in checkorder
- Now shows "Settled by admin: Yes/No" in
/checkorderoutput - Useful for audit and debugging
⚠️ Issues Found
🔴 CRITICAL: Missing tests
The PR modifies:
- Critical business logic (settle flow in
bot/start.ts) - Data model (
models/order.ts) - Order queries (
bot/commands.ts,bot/modules/block/commands.ts)
But:
git diff cb1838e..7e9a117 -- tests/
(no output)No new or updated tests.
Test cases that SHOULD exist:
1. Migration script
it('should migrate COMPLETED_BY_ADMIN orders to PAID_HOLD_INVOICE + settled_by_admin', async () => {
// Given: order with status COMPLETED_BY_ADMIN
// When: run migration
// Then: status = PAID_HOLD_INVOICE && settled_by_admin = true
});2. Admin settle flow (bot/start.ts)
it('should set settled_by_admin when admin settles with secret', async () => {
// Given: order with secret
// When: admin executes settle
// Then: settled_by_admin = true
});
it('should not modify order if settleHoldInvoice fails', async () => {
// Given: order with invalid secret
// When: admin executes settle (LND fails)
// Then: order status/flags unchanged
});3. Display in checkorder
it('should show "Settled by admin: Yes" when settled_by_admin = true', async () => {
// Given: order with settled_by_admin = true
// When: /checkorder
// Then: output contains "Settled by admin: Yes"
});4. Block queries
it('should exclude settled orders from pending count', async () => {
// Given: orders with settled_by_admin = true
// When: user tries /block
// Then: those orders do NOT count as pending
});Impact: Without tests, any regression in this logic will go unnoticed until production.
🟡 Minor: Unnecessary tsconfig.json change
+ "scripts/**/*"Adding scripts/ to tsconfig include is fine for compiling the migration script, BUT:
- Migration script runs once manually
- Doesn't need to be in production build
- Better approach: Run migration with
ts-nodedirectly, without modifying tsconfig
Not critical, but unnecessary noise in the PR.
🟡 Minor: Unrelated formatting changes
- "lib":["ES2021", "DOM"],
+ "lib": [
+ "ES2021",
+ "DOM"
+ ],This is just formatting. Better to do it in a separate "chore: prettier" commit to avoid contaminating the functional diff.
🔍 Logic Validation
Query in /setinvoice
Before:
const where = {
$and: [
{ buyer_id: user._id },
{
$or: [
{ status: 'PAID_HOLD_INVOICE' },
{ status: 'COMPLETED_BY_ADMIN' }, // ❌ No longer exists
],
},
],
};Now:
const where = {
buyer_id: user._id,
status: 'PAID_HOLD_INVOICE', // ✅ Simplified
};Analysis: ✅ CORRECT
- Previously searched for orders in
PAID_HOLD_INVOICEORCOMPLETED_BY_ADMIN - Now only searches
PAID_HOLD_INVOICE(the only valid status after migration) - Simpler and correct query
📊 Findings Summary
| Severity | Issue | Impact |
|---|---|---|
| 🔴 Critical | No tests | Risk of regression in settle logic and queries |
| 🟡 Minor | tsconfig include scripts/ | Noise in build config |
| 🟡 Minor | Formatting changes | Contaminated diff |
✅ Recommendations
BEFORE MERGE:
- Add tests for migration script
- Add tests for settle flow with
settled_by_admin - Add tests for checkorder/block queries
- Verify in staging that migration script works correctly
NICE TO HAVE:
- Separate formatting changes in separate commit
- Evaluate if tsconfig change is necessary (alternative:
ts-nodefor migration)
POST-MERGE:
- Add grep in CI to prevent accidental use of
COMPLETED_BY_ADMINin the future - Document in README or CHANGELOG the model change (breaking change for external integrations reading the DB)
🎯 Verdict
Status:
Reasons:
- Lack of tests is critical for settle logic and migration
- The code itself is correct and improves the system (fixes operation ordering bug)
- The architectural design is solid
Required action:
- Add minimum tests for migration and settle flow
- Validate migration in staging before merge
Once tests are added: ✅ APPROVE
Additional Notes
I verified all COMPLETED_BY_ADMIN references have been removed from queries:
git grep -n "COMPLETED_BY_ADMIN" | grep -v "locales/"
# Only found in migration script (correct)I also checked for potential broken queries:
git grep -n "status.*:" bot/ util/ models/ | grep -i "success\|paid_hold\|canceled\|expired"
# All queries updated correctlyThe PR is well-structured and the logic is sound. The only blocker is the missing test coverage for critical business logic.
There was a problem hiding this comment.
Issues from previous review addressed ✅
All test cases I requested have been implemented:
-
Migration script test ✅
- Verifies
COMPLETED_BY_ADMIN→PAID_HOLD_INVOICE+settled_by_admin: true - Checks correct query and update structure
- Verifies
-
Admin settle flow test ✅
should set settled_by_admin when admin settles with secret- Verifies
settleHoldInvoiceis called andsettled_by_admin = true
-
Settle failure test ✅
should not modify order if settleHoldInvoice fails- Verifies
order.save()is NOT called when settle throws
-
Block query test ✅
- Verifies query excludes
PAID_HOLD_INVOICEbut NOTCOMPLETED_BY_ADMIN - Ensures status cleanup is correct
- Verifies query excludes
-
getDetailedOrder tests ✅
- Shows "Yes" when
settled_by_admin = true - Shows "No" when
settled_by_admin = false
- Shows "Yes" when
Architecture ✅
- Correct design: status reflects workflow state, boolean captures metadata
- Migration script included for existing data
- Critical bug fix: operation ordering now prevents inconsistent state
CI ✅
All checks passing.
Summary
LGTM. The refactor is clean, properly tested, and includes an important bug fix for the settle operation ordering.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
tests/scripts/migrate_completed_by_admin_orders.spec.ts (2)
1-3: Consider using ES imports for consistency.The file uses CommonJS
require(lines 1-3) but ends withexport {}(line 81). For consistency across a TypeScript codebase, consider using ES imports:import { expect } from 'chai'; import sinon from 'sinon'; import proxyquire from 'proxyquire';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/scripts/migrate_completed_by_admin_orders.spec.ts` around lines 1 - 3, Replace CommonJS requires with ES module imports to match the file's trailing "export {}" and TypeScript style: change the top-level requires for expect, sinon, and proxyquire to ES imports (referencing expect, sinon, proxyquire) so the file consistently uses "import { expect } from 'chai';", "import sinon from 'sinon';" and "import proxyquire from 'proxyquire';" and remove the corresponding require statements.
59-60: Arbitrary timeout is a flaky test pattern.Using
setTimeout(resolve, 10)to wait for the migration's async IIFE is unreliable—the 10ms may not be enough under CI load, or it may be wastefully slow in other cases. If the migration script exports itsmigrate()function (or a promise), the test could await it directly for deterministic behavior.If refactoring the migration script isn't feasible, consider increasing the timeout and adding a comment explaining the trade-off, or poll for
updateManyStub.calledwith a short interval.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/scripts/migrate_completed_by_admin_orders.spec.ts` around lines 59 - 60, The test uses an arbitrary sleep (await new Promise(resolve => setTimeout(resolve, 10))) which is flaky; instead make the migration deterministic by awaiting the migration's exported promise or function (await migrate()) if the migration script can export migrate(), or poll/wait for the spy/stub state (e.g., loop with short intervals until updateManyStub.called is true or timeout) to assert completion; update the test in migrate_completed_by_admin_orders.spec.ts to call the exported migrate() or replace the setTimeout with a short polling loop checking updateManyStub.called to reliably await the async IIFE.tests/bot/modules/block.spec.ts (1)
69-74: Broaden the settled-status assertion to reduce false positives.On Line 72, the test only checks
PAID_HOLD_INVOICE. If another terminal status (likeSUCCESS) is accidentally dropped from$nin, this test would still pass.Suggested assertion hardening
- expect(queryArgs.status.$nin).to.include('PAID_HOLD_INVOICE'); + expect(queryArgs.status.$nin).to.include.members([ + 'PAID_HOLD_INVOICE', + 'SUCCESS', + ]); expect(queryArgs.status.$nin).to.not.include('COMPLETED_BY_ADMIN');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/bot/modules/block.spec.ts` around lines 69 - 74, The test currently only asserts that queryArgs.status.$nin includes 'PAID_HOLD_INVOICE' which can miss other terminal statuses; update the assertion to ensure queryArgs.status.$nin contains the full set of settled/terminal statuses (e.g., PAID_HOLD_INVOICE, SUCCESS, COMPLETED_BY_ADMIN) by either importing the canonical settled-statuses constant from the production code and asserting each element is present, or explicitly asserting inclusion of every known terminal status in queryArgs.status.$nin.tests/bot/bot.spec.ts (1)
627-643: Missingloggerstub in proxyquire setup.Both new tests omit the
'../logger'stub that thebeforeEachblock at lines 437-442 includes. WhensettleHoldInvoicerejects (test 2), the handler's catch block callslogger.error(error). Without a stub, the real logger module is invoked, which could cause console output or unexpected side effects during test runs.Proposed fix
Add the logger stub to both proxyquire setups:
const startModule = proxyquire('../../bot/start', { telegraf: { Telegraf: sinon.stub().returns(botStub) }, '../models': { Order: { findOne: OrderFindOneStub }, User: { findOne: sinon.stub().resolves({ id: 'user' }) }, Dispute: { findOne: sinon.stub().resolves(null) }, }, './validations': { validateParams: sinon.stub().resolves(['orderId']), validateObjectId: sinon.stub().resolves(true), }, '../ln': { settleHoldInvoice: settleHoldInvoiceStub }, './messages': { successCompleteOrderMessage: sinon.stub().resolves(), successCompleteOrderByAdminMessage: sinon.stub().resolves(), }, + '../logger': { + error: sinon.stub(), + notice: sinon.stub(), + debug: sinon.stub(), + info: sinon.stub(), + }, });Also applies to: 682-698
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/bot/bot.spec.ts` around lines 627 - 643, The proxyquire test setups for startModule are missing a stub for the logger module, so when settleHoldInvoiceStub rejects the code path calls logger.error and invokes the real logger; add a logger stub entry to the proxyquire mocks (e.g., include '../logger': { error: sinon.stub(), info: sinon.stub(), warn: sinon.stub() } or the same stub used in beforeEach) for both proxyquire calls that create startModule so the handler's catch block uses the stubbed logger instead of the real module.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/bot/bot.spec.ts`:
- Around line 668-717: The test currently stubs settleHoldInvoice to reject,
which doesn't match the real function (ln/hold_invoice.ts::settleHoldInvoice)
that catches errors and does not reject; update the test in bot.spec.ts to stub
../ln.settleHoldInvoice to resolve (e.g., resolves()), then adjust assertions to
expect orderMock.save to have been called and orderMock.settled_by_admin to be
true to reproduce the real-world "silent failure" false-positive; alternatively,
if you intend to change production behavior to propagate errors, add a TODO in
ln/hold_invoice.ts near settleHoldInvoice noting tests rely on its swallowing
errors and add a new test after refactor to assert rejection behavior.
---
Nitpick comments:
In `@tests/bot/bot.spec.ts`:
- Around line 627-643: The proxyquire test setups for startModule are missing a
stub for the logger module, so when settleHoldInvoiceStub rejects the code path
calls logger.error and invokes the real logger; add a logger stub entry to the
proxyquire mocks (e.g., include '../logger': { error: sinon.stub(), info:
sinon.stub(), warn: sinon.stub() } or the same stub used in beforeEach) for both
proxyquire calls that create startModule so the handler's catch block uses the
stubbed logger instead of the real module.
In `@tests/bot/modules/block.spec.ts`:
- Around line 69-74: The test currently only asserts that queryArgs.status.$nin
includes 'PAID_HOLD_INVOICE' which can miss other terminal statuses; update the
assertion to ensure queryArgs.status.$nin contains the full set of
settled/terminal statuses (e.g., PAID_HOLD_INVOICE, SUCCESS, COMPLETED_BY_ADMIN)
by either importing the canonical settled-statuses constant from the production
code and asserting each element is present, or explicitly asserting inclusion of
every known terminal status in queryArgs.status.$nin.
In `@tests/scripts/migrate_completed_by_admin_orders.spec.ts`:
- Around line 1-3: Replace CommonJS requires with ES module imports to match the
file's trailing "export {}" and TypeScript style: change the top-level requires
for expect, sinon, and proxyquire to ES imports (referencing expect, sinon,
proxyquire) so the file consistently uses "import { expect } from 'chai';",
"import sinon from 'sinon';" and "import proxyquire from 'proxyquire';" and
remove the corresponding require statements.
- Around line 59-60: The test uses an arbitrary sleep (await new Promise(resolve
=> setTimeout(resolve, 10))) which is flaky; instead make the migration
deterministic by awaiting the migration's exported promise or function (await
migrate()) if the migration script can export migrate(), or poll/wait for the
spy/stub state (e.g., loop with short intervals until updateManyStub.called is
true or timeout) to assert completion; update the test in
migrate_completed_by_admin_orders.spec.ts to call the exported migrate() or
replace the setTimeout with a short polling loop checking updateManyStub.called
to reliably await the async IIFE.
🪄 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: 7b2131be-c880-4513-9f03-461c9e098ba8
📒 Files selected for processing (5)
scripts/migrate_completed_by_admin_orders.tstests/bot/bot.spec.tstests/bot/modules/block.spec.tstests/scripts/migrate_completed_by_admin_orders.spec.tstests/util/index.spec.ts
✅ Files skipped from review due to trivial changes (1)
- scripts/migrate_completed_by_admin_orders.ts
| it('should not modify order if settleHoldInvoice fails', async () => { | ||
| const orderMock = { | ||
| _id: 'orderId', | ||
| status: 'DISPUTE', | ||
| secret: 'invalidsecret', | ||
| community_id: null, | ||
| buyer_id: 'buyer', | ||
| seller_id: 'seller', | ||
| save: sinon.stub().resolves(), | ||
| } as any; | ||
|
|
||
| const OrderFindOneStub = sinon.stub().resolves(orderMock); | ||
| const settleHoldInvoiceStub = sinon.stub().rejects(new Error('LND failed')); | ||
|
|
||
| const startModule = proxyquire('../../bot/start', { | ||
| telegraf: { Telegraf: sinon.stub().returns(botStub) }, | ||
| '../models': { | ||
| Order: { findOne: OrderFindOneStub }, | ||
| User: { findOne: sinon.stub().resolves({ id: 'user' }) }, | ||
| Dispute: { findOne: sinon.stub().resolves(null) }, | ||
| }, | ||
| './validations': { | ||
| validateParams: sinon.stub().resolves(['orderId']), | ||
| validateObjectId: sinon.stub().resolves(true), | ||
| }, | ||
| '../ln': { settleHoldInvoice: settleHoldInvoiceStub }, | ||
| './messages': { | ||
| successCompleteOrderMessage: sinon.stub().resolves(), | ||
| successCompleteOrderByAdminMessage: sinon.stub().resolves(), | ||
| }, | ||
| }); | ||
|
|
||
| startModule.initialize('dummy-token', {}); | ||
| const settleOrderCall = botStub.command | ||
| .getCalls() | ||
| .find((c: any) => c.args[0] === 'settleorder'); | ||
| const handler = settleOrderCall.args[2]; | ||
|
|
||
| const ctx = { | ||
| admin: { admin: true }, | ||
| match: ['/settleorder orderId', 'orderId'], | ||
| reply: sinon.stub().resolves(), | ||
| i18n: { t: sinon.stub().returns('Success') }, | ||
| }; | ||
|
|
||
| await handler(ctx); | ||
|
|
||
| expect(orderMock.save.called).to.be.equal(false); | ||
| expect(orderMock.settled_by_admin).to.equal(undefined); | ||
| }); |
There was a problem hiding this comment.
Test stub behavior doesn't match production settleHoldInvoice implementation.
Per the relevant code snippet from ln/hold_invoice.ts:43-49, the real settleHoldInvoice catches errors internally and logs them—it never rejects. This test stubs it to reject, which verifies handler behavior for a scenario that can't occur in production.
In practice, if LN settlement fails:
settleHoldInvoicecatches the error, logs it, and returns normally- Handler continues to set
settled_by_admin = trueand callssave() - Order is incorrectly marked as settled by admin (false positive)
Consider either:
- Adding a test that stubs
settleHoldInvoiceto resolve (mimicking silent failure) and verifying the false-positive scenario, or - If the intent is to refactor
settleHoldInvoiceto propagate errors, add aTODOcomment noting this dependency
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/bot/bot.spec.ts` around lines 668 - 717, The test currently stubs
settleHoldInvoice to reject, which doesn't match the real function
(ln/hold_invoice.ts::settleHoldInvoice) that catches errors and does not reject;
update the test in bot.spec.ts to stub ../ln.settleHoldInvoice to resolve (e.g.,
resolves()), then adjust assertions to expect orderMock.save to have been called
and orderMock.settled_by_admin to be true to reproduce the real-world "silent
failure" false-positive; alternatively, if you intend to change production
behavior to propagate errors, add a TODO in ln/hold_invoice.ts near
settleHoldInvoice noting tests rely on its swallowing errors and add a new test
after refactor to assert rejection behavior.
2 participants
This PR removes the 'COMPLETED_BY_ADMIN' status in an Order and adds a new boolean field 'settled_by_admin' that, as the name suggests, denotes if an order was indeed completed or not by an admin.
Also, now when an admin uses the checkorder command, it will display if the order was completed by an admin
Summary by CodeRabbit
New Features
Updates
Chores