fix: prevent None values in Razorpay amount conversion#2
Conversation
📝 WalkthroughWalkthroughFormatting and readability edits across multiple modules; numeric fields in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
razorpay_frappe/razorpay_integration/doctype/razorpay_order/razorpay_order.py
🧰 Additional context used
🪛 Ruff (0.14.10)
razorpay_frappe/razorpay_integration/doctype/razorpay_order/razorpay_order.py
44-44: Undefined name Pending
(F821)
44-44: Undefined name Failed
(F821)
44-44: Undefined name Paid
(F821)
44-44: Undefined name Refund
(F821)
44-44: Undefined name Progress
(F821)
44-44: Undefined name Refunded
(F821)
🔇 Additional comments (3)
razorpay_frappe/razorpay_integration/doctype/razorpay_order/razorpay_order.py (3)
43-45: Formatting improvement is good; static analysis warnings are false positives.The multi-line format improves readability. The Ruff warnings about undefined names are false positives—these are string literals within a
DF.Literaltype annotation, not variable references.
182-184: LGTM! Readability improvement.Breaking the complex conditional across multiple lines with explicit parentheses improves code clarity without changing the logic.
138-147: Defensive None handling is necessary and correctly implemented.The
.get(key, 0) or 0pattern is essential here becauseconvert_from_razorpay_moneydoes not handle None inputs—it performs integer division and will crash if None is passed. The fee, tax, and base_amount fields from the Razorpay API may return None, making this defensive pattern a required safeguard.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
razorpay_frappe/razorpay_integration/doctype/razorpay_order/razorpay_order.py (2)
132-133: Consider removing or using conditional logging.These debug print statements will output to the error log on every payment detail fetch. If these are temporary debug traces, consider removing them before merging. If logging is needed for production, use conditional logging based on a debug flag or proper log levels.
🔎 Alternative: Conditional debug logging
- frappe.errprint("Payment Entity: ") - frappe.errprint(payment_entity) + if frappe.conf.get("developer_mode"): + frappe.log("Payment Entity: " + str(payment_entity))
136-136: Optional: Remove extra blank line.This extra blank line appears to be a leftover from the duplicate debug print removal mentioned in the previous review.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
razorpay_frappe/razorpay_integration/doctype/razorpay_order/razorpay_order.py
🧰 Additional context used
🧬 Code graph analysis (1)
razorpay_frappe/razorpay_integration/doctype/razorpay_order/razorpay_order.py (1)
razorpay_frappe/utils.py (1)
convert_from_razorpay_money(48-49)
🪛 Ruff (0.14.10)
razorpay_frappe/razorpay_integration/doctype/razorpay_order/razorpay_order.py
44-44: Undefined name Pending
(F821)
44-44: Undefined name Failed
(F821)
44-44: Undefined name Paid
(F821)
44-44: Undefined name Refund
(F821)
44-44: Undefined name Progress
(F821)
44-44: Undefined name Refunded
(F821)
🔇 Additional comments (4)
razorpay_frappe/razorpay_integration/doctype/razorpay_order/razorpay_order.py (4)
43-45: LGTM: Formatting improvement.The multi-line formatting of the status literal improves readability without changing functionality.
Note: The Ruff errors flagging "Pending", "Failed", etc. as undefined names are false positives—these are string literals within a
DF.Literaltype annotation, not variable references.
144-146: LGTM: Consistent None-guard for base_amount.The same defensive pattern is correctly applied to
base_amount, consistent with thefeeandtaxfields above.
181-183: LGTM: Improved readability.The extra parentheses make the operator precedence clearer without changing the logic.
137-142: Good defensive pattern for handling None/missing values.The
.get("fee", 0) or 0and.get("tax", 0) or 0guards correctly prevent None from reachingconvert_from_razorpay_money, which expects an int. The pattern handles both missing keys and explicit None values appropriately.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci.yml
🧰 Additional context used
🪛 actionlint (1.7.9)
.github/workflows/ci.yml
57-57: the runner of "actions/setup-node@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Server
🔇 Additional comments (2)
.github/workflows/ci.yml (2)
59-59: Node.js 24 (Krypton) is available and stable as of December 2025. It was released on May 6, 2025, and entered Active LTS status on October 28, 2025, with end-of-life planned for April 30, 2028. Using Node.js 24 in the CI pipeline is appropriate and will not cause failures due to version unavailability.Likely an incorrect or invalid review comment.
54-54: No action required. Python 3.14 was released on October 7, 2025, and is fully supported by actions/setup-python and GitHub-hosted runners as of December 2025. The specified version will work correctly in the CI pipeline.Likely an incorrect or invalid review comment.
| uses: actions/setup-node@v3 | ||
| with: | ||
| node-version: 18 | ||
| node-version: 24 | ||
| check-latest: true |
There was a problem hiding this comment.
Update actions/setup-node to v4.
The actions/setup-node@v3 action version is outdated. According to the static analysis tool, the runner is too old to run on GitHub Actions.
🔎 Proposed fix
- name: Setup Node
- uses: actions/setup-node@v3
+ uses: actions/setup-node@v4
with:
node-version: 24
check-latest: trueBased on static analysis hints.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uses: actions/setup-node@v3 | |
| with: | |
| node-version: 18 | |
| node-version: 24 | |
| check-latest: true | |
| uses: actions/setup-node@v4 | |
| with: | |
| node-version: 24 | |
| check-latest: true |
🧰 Tools
🪛 actionlint (1.7.9)
57-57: the runner of "actions/setup-node@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
.github/workflows/ci.yml around lines 57 to 60: the workflow uses
actions/setup-node@v3 which is outdated; update the action reference to
actions/setup-node@v4 by replacing the uses line accordingly (keep the existing
with: keys like node-version: 24 and check-latest: true intact) so the runner
uses the current setup-node major version.
|
@vishwajeet-13 is it still relevant? |
2 participants
handled none values
Summary by CodeRabbit
Bug Fixes
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.