debt(PaymentMethod): surface bitcoin type - #11952
Conversation
Resolve https://open-collective.sentry.io/issues/6174057898 The type is deprecated and not usable, but we have some ledger transactions associated with the legacy Stripe bitcoin implementation from 2018.
📝 WalkthroughWalkthroughThis change marks the BITCOIN payment method type as deprecated. A JSDoc `@deprecated` comment is added to the BITCOIN enum member in the constants file, and a new deprecatedValues constant is introduced in the GraphQL enum builder to conditionally attach a deprecationReason to matching enum values during construction. Estimated code review effort: 1 (Trivial) | ~5 minutes Changes
Sequence Diagram(s)Not applicable; change is a static metadata/annotation update rather than a runtime flow. Related issues: None referenced. Related PRs: None referenced. Suggested labels: graphql, deprecation, low-risk Suggested reviewers: None specified. 🐰 A bitcoin coin, once shiny and bold, 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
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 |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| deprecationReason: deprecatedValues.includes(key) ? 'This value is deprecated' : undefined, | ||
| value: PAYMENT_METHOD_TYPE[key], | ||
| }, |
There was a problem hiding this comment.
Bug: The deprecation check for the BITCOIN payment method fails because it incorrectly compares an enum key ('BITCOIN') with a list of enum values (['bitcoin']).
Severity: LOW
Suggested Fix
Modify the deprecation check to compare enum values instead of keys. Change the condition deprecatedValues.includes(key) to deprecatedValues.includes(PAYMENT_METHOD_TYPE[key]). This will correctly look up the value associated with the key and check for its presence in the deprecatedValues array.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: server/graphql/v2/enum/PaymentMethodType.ts#L18-L20
Potential issue: The logic to deprecate the `BITCOIN` payment method in the GraphQL enum
will not work as intended. The code creates a `deprecatedValues` array containing the
enum's string value (`'bitcoin'`). However, it then iterates through the enum's keys
(e.g., `'BITCOIN'`) and checks for inclusion in the `deprecatedValues` array. The check
`deprecatedValues.includes(key)` will always be false because it compares a key like
`'BITCOIN'` with a value like `'bitcoin'`. Consequently, the `deprecationReason` will
not be applied, and the `BITCOIN` enum value will not be marked as deprecated in the
schema.
Did we get this right? 👍 / 👎 to inform future reviews.
![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
🤖 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 `@server/graphql/v2/enum/PaymentMethodType.ts`:
- Around line 5-6: The deprecation merge in PaymentMethodType is comparing enum
member names from Object.keys(PAYMENT_METHOD_TYPE) against enum values in
deprecatedValues, so the BITCOIN alias never matches. Update the second reduce
in the PAYMENT_METHOD_TYPE handling to compare using PAYMENT_METHOD_TYPE[key]
instead of the raw key, and ensure the deprecated reason from the bitcoin entry
is applied to the uppercase BITCOIN alias as well.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d561956e-3cd1-4600-ae3f-0b2c32684167
📒 Files selected for processing (2)
server/constants/paymentMethods.tsserver/graphql/v2/enum/PaymentMethodType.ts
| const deprecatedValues: readonly PAYMENT_METHOD_TYPE[] = [PAYMENT_METHOD_TYPE.BITCOIN] as const; | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm Object.keys() behavior for string enums compiled by TS, and inspect final generated schema for BITCOIN deprecation.
rg -n "PAYMENT_METHOD_TYPE" server/graphql/schemaV2.graphql -A1 -B1 | rg -i bitcoin -A2 -B2Repository: opencollective/opencollective-api
Length of output: 171
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file and the related enum usage.
git ls-files server/graphql/v2/enum/PaymentMethodType.ts server/graphql/v2/enum
echo '--- outline ---'
ast-grep outline server/graphql/v2/enum/PaymentMethodType.ts --view expanded || true
echo '--- file ---'
wc -l server/graphql/v2/enum/PaymentMethodType.ts
cat -n server/graphql/v2/enum/PaymentMethodType.tsRepository: opencollective/opencollective-api
Length of output: 5203
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for the generated enum/schema references and any tests for payment method deprecation.
rg -n "deprecatedValues|deprecationReason|PAYMENT_METHOD_TYPE|PaymentMethodType" server -g '!**/node_modules/**' -A3 -B3Repository: opencollective/opencollective-api
Length of output: 50390
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Programmatic probe: replicate TS string-enum runtime shape and Object.keys()/includes behavior.
node - <<'JS'
const PAYMENT_METHOD_TYPE = { BITCOIN: 'bitcoin', CREDIT_CARD: 'credit_card' };
const deprecatedValues = ['bitcoin'];
for (const key of Object.keys(PAYMENT_METHOD_TYPE)) {
console.log(key, deprecatedValues.includes(key), deprecatedValues.includes(PAYMENT_METHOD_TYPE[key]));
}
JSRepository: opencollective/opencollective-api
Length of output: 214
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the enum definition and the surrounding GraphQL enum file with line numbers.
cat -n server/constants/paymentMethods.ts | sed -n '1,120p'
echo '---'
cat -n server/graphql/v2/enum/PaymentMethodType.tsRepository: opencollective/opencollective-api
Length of output: 2558
BITCOIN never picks up the deprecation reason in the second reduce.
Object.keys(PAYMENT_METHOD_TYPE) yields member names ('BITCOIN'), while deprecatedValues holds enum values ('bitcoin'), so this check stays false. The lowercase bitcoin enum is already deprecated by the first reduce; compare against PAYMENT_METHOD_TYPE[key] if the uppercase alias should also be deprecated.
🤖 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 `@server/graphql/v2/enum/PaymentMethodType.ts` around lines 5 - 6, The
deprecation merge in PaymentMethodType is comparing enum member names from
Object.keys(PAYMENT_METHOD_TYPE) against enum values in deprecatedValues, so the
BITCOIN alias never matches. Update the second reduce in the PAYMENT_METHOD_TYPE
handling to compare using PAYMENT_METHOD_TYPE[key] instead of the raw key, and
ensure the deprecated reason from the bitcoin entry is applied to the uppercase
BITCOIN alias as well.
1 participant
Resolve https://open-collective.sentry.io/issues/6174057898
The type is deprecated and not usable, but we have some ledger transactions associated with the legacy Stripe bitcoin implementation from 2018.