feat/parser-logic-addition-for-razorpay-ccavenue-pg#1347
Open
RahulGupta0706 wants to merge 2 commits into
Open
feat/parser-logic-addition-for-razorpay-ccavenue-pg#1347RahulGupta0706 wants to merge 2 commits into
RahulGupta0706 wants to merge 2 commits into
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (8)
WalkthroughThis PR adds a complete SAP integration module (API, config, flow, types) and expands settlement processing with new CCAvenue and Razorpay parsers, shared utilities, extended domain types, BillDesk updates, and existing provider migration. ChangesSAP Integration
Settlement Payment Provider Expansion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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. 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/mobility-core/src/Kernel/External/Settlement/CCAvenue/PaymentParser.hs (1)
121-128:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing date-only format with dash separator.
The parser handles date-with-time using both
/and-separators, but date-only parsing only supports/. If CCAvenue sends a date like"31-12-2024", it will fail to parse and returnNothing.🔧 Proposed fix
parseTimeM True defaultTimeLocale "%d/%m/%Y %H:%M:%S" (T.unpack $ T.strip t) <|> parseTimeM True defaultTimeLocale "%d-%m-%Y %H:%M:%S" (T.unpack $ T.strip t) <|> parseTimeM True defaultTimeLocale "%d/%m/%Y" (T.unpack $ T.strip t) + <|> parseTimeM True defaultTimeLocale "%d-%m-%Y" (T.unpack $ T.strip t)🤖 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 `@lib/mobility-core/src/Kernel/External/Settlement/CCAvenue/PaymentParser.hs` around lines 121 - 128, The parseCCAvenueDateTime function is missing a date-only format with dash separators; add an additional alternative using parseTimeM True defaultTimeLocale "%d-%m-%Y" (applied to the same stripped Text via T.unpack) alongside the existing "%d/%m/%Y" branch so date-only strings like "31-12-2024" are parsed; update the parseCCAvenueDateTime alternatives that call parseTimeM to include this new "%d-%m-%Y" format.lib/mobility-core/src/Kernel/External/Settlement/BillDesk/PaymentTypes.hs (1)
72-88:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInclude
payRef2in the JSON serialization for consistency.The
ToJSONinstance serializespayRef1(line 86) but omitspayRef2, even though both fields are parsed from the CSV (lines 69-70) and stored in the record (lines 42-43). This asymmetry could confuse downstream consumers who expect both fields or might cause data loss ifpayRef2is needed for debugging or reconciliation.✨ Proposed fix to add payRef2 to JSON output
instance A.ToJSON BillDeskSettledRow where toJSON row = A.object [ "billerId" A..= row.billerId, "bankId" A..= row.bankId, "bankRefNo" A..= row.bankRefNo, "pgiRefNo" A..= row.pgiRefNo, "ref1" A..= row.ref1, "dateOfTxn" A..= row.dateOfTxn, "settlementDate" A..= row.settlementDate, "grossAmount" A..= row.grossAmount, "charges" A..= row.charges, "gst" A..= row.gst, "netAmount" A..= row.netAmount, "payRef1" A..= row.payRef1, + "payRef2" A..= row.payRef2, "section" A..= ("SETTLED" :: Text) ]🤖 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 `@lib/mobility-core/src/Kernel/External/Settlement/BillDesk/PaymentTypes.hs` around lines 72 - 88, The ToJSON instance for BillDeskSettledRow omits the payRef2 field causing asymmetry with parsing; update the toJSON implementation in the A.ToJSON BillDeskSettledRow (the toJSON function) to include "payRef2" A..= row.payRef2 alongside the existing "payRef1" entry so both payRef1 and payRef2 are serialized into the JSON object (keep the "section" field as-is).
🧹 Nitpick comments (5)
lib/mobility-core/src/Kernel/External/Settlement/Razorpay/PaymentParser.hs (1)
111-117: ⚡ Quick winConsider distinguishing credit vs debit cards using
cardTypefield.Currently
"card"maps unconditionally toCREDIT_CARD, but Razorpay'scard_typecolumn likely contains"credit"/"debit"information. ThePaymentMethodTypeenum supports both variants.This would require passing
cardTypeto the function:parseRazorpayPaymentMethod :: Text -> Text -> Maybe PaymentMethodType parseRazorpayPaymentMethod method cardType = case T.toLower (T.strip method) of "card" -> case T.toLower (T.strip cardType) of "debit" -> Just DEBIT_CARD _ -> Just CREDIT_CARD ...🤖 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 `@lib/mobility-core/src/Kernel/External/Settlement/Razorpay/PaymentParser.hs` around lines 111 - 117, The current parseRazorpayPaymentMethod function always maps "card" to CREDIT_CARD; update its signature to accept the Razorpay card_type (e.g., parseRazorpayPaymentMethod :: Text -> Text -> Maybe PaymentMethodType) and branch on the normalized cardType when method equals "card" so that "debit" returns DEBIT_CARD and other values return CREDIT_CARD; maintain existing mappings for "upi", "netbanking", "wallet" and preserve trimming/lowercasing behavior for both inputs to locate the change in parseRazorpayPaymentMethod.lib/mobility-core/src/Kernel/External/Settlement/CCAvenue/PaymentParser.hs (1)
50-103: 💤 Low valueUnused
idxparameter is dead code.The
idxparameter is passed but explicitly discarded with_ = idx. Other parsers in the codebase use the row index to provide better error messages when row parsing fails. Since this function always returnsRight, the index isn't used for error context either.Consider either removing the parameter if not needed, or using it in potential error paths for better debugging.
🤖 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 `@lib/mobility-core/src/Kernel/External/Settlement/CCAvenue/PaymentParser.hs` around lines 50 - 103, The function parseCCAvenueTxnRow currently takes an unused idx (dead code) which should be removed or used for error context; update either the signature and all callers to drop the Int parameter (remove idx and the `_ = idx` line) OR change parsing helpers inside parseCCAvenueTxnRow to return Either/Text and include idx in Left messages so failures include row index (e.g., use idx when constructing error Text for parseCCAvenueDateTime, parseAmount, nonEmpty' failures) while keeping the PaymentSettlementReport construction on Right.lib/mobility-core/src/Kernel/External/Settlement/Utils/ParserUtils.hs (2)
39-44: 💤 Low valueConsider documenting the datetime formats and timezone conversion.
The function supports two formats, but the behavior when parsing
%Z(timezone names) isn't documented. When a non-UTC timezone is parsed in Format 2,parseTimeMconverts it to UTC, which may not be obvious to callers. Adding a comment would clarify expectations.📝 Suggested documentation
+-- | Parse datetime from text, supporting two formats: +-- 1. "%Y-%m-%d %H:%M:%S" (assumes UTC) +-- 2. "%Y-%m-%d %H:%M:%S %Z" (parses timezone name and converts to UTC) +-- Returns Nothing for blank input or unparseable formats. parseDateTime :: Text -> Maybe UTCTime parseDateTime t | T.null (T.strip t) = Nothing🤖 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 `@lib/mobility-core/src/Kernel/External/Settlement/Utils/ParserUtils.hs` around lines 39 - 44, Document the supported input formats and timezone behavior for parseDateTime: add a comment above parseDateTime stating it accepts "YYYY-MM-DD HH:MM:SS" and "YYYY-MM-DD HH:MM:SS Z" (where Z is a timezone name) and that when the second format is used the underlying parseTimeM will interpret the timezone and convert the resulting UTCTime to UTC; also note that empty or whitespace-only input returns Nothing. Reference the function name parseDateTime and the two format strings "%Y-%m-%d %H:%M:%S" and "%Y-%m-%d %H:%M:%S %Z" in the comment so callers understand expected input and conversion semantics.
28-31: 💤 Low valueConsider optimizing to call
T.striponce.The current implementation calls
T.striptwice: once in the guard and once in the result. Extracting the stripped value improves efficiency.⚡ Proposed optimization
nonEmpty' :: Text -> Maybe Text -nonEmpty' t - | T.null (T.strip t) = Nothing - | otherwise = Just (T.strip t) +nonEmpty' t = + let stripped = T.strip t + in if T.null stripped then Nothing else Just stripped🤖 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 `@lib/mobility-core/src/Kernel/External/Settlement/Utils/ParserUtils.hs` around lines 28 - 31, The function nonEmpty' calls T.strip twice; instead bind the stripped value once (e.g., let s = T.strip t) then check if T.null s and return Nothing or Just s accordingly—update nonEmpty' to compute the trimmed Text once and reuse that binding.lib/mobility-core/src/Kernel/External/Settlement/Types.hs (1)
69-76: ⚡ Quick winConsider documenting the
bankCodefield.The new
bankCodefield lacks inline documentation explaining its purpose, unlike the detailed field comments inPaymentSettlementReport. Adding a comment would improve maintainability.📝 Suggested documentation
parserTypeMap :: Maybe SettlementParserTypeMap, useJuspayOrderStatus :: Maybe Bool, - bankCode :: Maybe Text + bankCode :: Maybe Text -- Optional bank code for provider-specific settlement routing }🤖 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 `@lib/mobility-core/src/Kernel/External/Settlement/Types.hs` around lines 69 - 76, Add an inline comment documenting the bankCode field in the SettlementServiceConfig record: explain what values it holds (e.g., bank identifier or routing code), whether it is optional, its format/constraints, and how it is used relative to payment parsing or reconciliation (similar to existing comments on PaymentSettlementReport). Update the SettlementServiceConfig declaration to place the comment immediately above or inline with the bankCode field so future readers see its purpose and expected values.
🤖 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 `@lib/mobility-core/mobility-core.cabal`:
- Around line 237-240: The four manually-added modules Kernel.External.SAP.API,
Kernel.External.SAP.Config, Kernel.External.SAP.Flow, and
Kernel.External.SAP.Types were removed by hpack regeneration; fix this by
removing these manual additions from the generated .cabal and adding those exact
module names under library.exposed-modules in the package.yaml used to generate
the cabal (ensure the entries are listed as exposed modules so hpack will
include them on regen).
In `@lib/mobility-core/src/Kernel/External/Settlement/BillDesk/PaymentParser.hs`:
- Line 148: The settled-row mapping incorrectly reads paymentMethod from
row.payRef2 (leftover TODO) causing empty values; in
PaymentParser.parseSettledRow change the mapping to use row.ref2 (the same field
used for refunds/chargebacks) or alternatively update BillDeskSettledRow's
Csv.FromNamedRecord to bind Pay Ref2 to the actual CSV header "Ref. 2" — then
remove the inline TODO comment; update references to parseBillDeskPaymentMethod
and BillDeskSettledRow accordingly so settled parsing matches the CSV samples.
In `@lib/mobility-core/src/Kernel/External/Settlement/Razorpay/PaymentParser.hs`:
- Line 72: The inline note "---- see into it" next to vendorId in the
PaymentParser record is an unfinished TODO; either implement the proper vendorId
mapping or remove the ad-hoc comment and replace it with a tracked-task
reference. Update the code where vendorId = Nothing in the PaymentParser (or
related parser function) to derive vendorId from the source payment data (set
the correct MID mapping), or if you cannot implement it now, remove the inline
remark and add a standard TODO comment that references a ticket/issue ID (e.g.,
TODO: track in ISSUE-1234) so the missing work is recorded.
- Line 100: The cardNumber field is incorrectly populated from row.vpa (a UPI
VPA); update PaymentParser so cardNumber is populated only from actual card
fields (e.g., row.cardNumber, row.masked_card, or whichever Razorpay field
contains card data) and stop using nonEmpty' row.vpa for cardNumber; instead map
the VPA to a dedicated field such as paymentMethodSubType or add a new vpa field
(refer to cardNumber, row.vpa, paymentMethodSubType and the PaymentParser
parsing function) so UPI VPAs are preserved without corrupting card data.
In `@lib/mobility-core/src/Kernel/External/Settlement/Utils/ParserUtils.hs`:
- Around line 33-37: The parseAmount function currently returns 0 on any parse
failure which can hide bad financial data; change its type from parseAmount ::
Text -> HighPrecMoney to parseAmount :: Text -> Maybe HighPrecMoney and update
the implementation to return Just v on successful readMaybe and Nothing on
failures (i.e., propagate the parse failure instead of defaulting to 0), then
update all callers of parseAmount to handle the Maybe (use fromMaybe 0 only
where a zero-default is explicitly acceptable) so invalid inputs are surfaced;
alternatively, if you prefer keeping the current behavior, add an inline comment
on parseAmount documenting that non-parseable inputs are intentionally treated
as zero and why.
---
Outside diff comments:
In `@lib/mobility-core/src/Kernel/External/Settlement/BillDesk/PaymentTypes.hs`:
- Around line 72-88: The ToJSON instance for BillDeskSettledRow omits the
payRef2 field causing asymmetry with parsing; update the toJSON implementation
in the A.ToJSON BillDeskSettledRow (the toJSON function) to include "payRef2"
A..= row.payRef2 alongside the existing "payRef1" entry so both payRef1 and
payRef2 are serialized into the JSON object (keep the "section" field as-is).
In `@lib/mobility-core/src/Kernel/External/Settlement/CCAvenue/PaymentParser.hs`:
- Around line 121-128: The parseCCAvenueDateTime function is missing a date-only
format with dash separators; add an additional alternative using parseTimeM True
defaultTimeLocale "%d-%m-%Y" (applied to the same stripped Text via T.unpack)
alongside the existing "%d/%m/%Y" branch so date-only strings like "31-12-2024"
are parsed; update the parseCCAvenueDateTime alternatives that call parseTimeM
to include this new "%d-%m-%Y" format.
---
Nitpick comments:
In `@lib/mobility-core/src/Kernel/External/Settlement/CCAvenue/PaymentParser.hs`:
- Around line 50-103: The function parseCCAvenueTxnRow currently takes an unused
idx (dead code) which should be removed or used for error context; update either
the signature and all callers to drop the Int parameter (remove idx and the `_ =
idx` line) OR change parsing helpers inside parseCCAvenueTxnRow to return
Either/Text and include idx in Left messages so failures include row index
(e.g., use idx when constructing error Text for parseCCAvenueDateTime,
parseAmount, nonEmpty' failures) while keeping the PaymentSettlementReport
construction on Right.
In `@lib/mobility-core/src/Kernel/External/Settlement/Razorpay/PaymentParser.hs`:
- Around line 111-117: The current parseRazorpayPaymentMethod function always
maps "card" to CREDIT_CARD; update its signature to accept the Razorpay
card_type (e.g., parseRazorpayPaymentMethod :: Text -> Text -> Maybe
PaymentMethodType) and branch on the normalized cardType when method equals
"card" so that "debit" returns DEBIT_CARD and other values return CREDIT_CARD;
maintain existing mappings for "upi", "netbanking", "wallet" and preserve
trimming/lowercasing behavior for both inputs to locate the change in
parseRazorpayPaymentMethod.
In `@lib/mobility-core/src/Kernel/External/Settlement/Types.hs`:
- Around line 69-76: Add an inline comment documenting the bankCode field in the
SettlementServiceConfig record: explain what values it holds (e.g., bank
identifier or routing code), whether it is optional, its format/constraints, and
how it is used relative to payment parsing or reconciliation (similar to
existing comments on PaymentSettlementReport). Update the
SettlementServiceConfig declaration to place the comment immediately above or
inline with the bankCode field so future readers see its purpose and expected
values.
In `@lib/mobility-core/src/Kernel/External/Settlement/Utils/ParserUtils.hs`:
- Around line 39-44: Document the supported input formats and timezone behavior
for parseDateTime: add a comment above parseDateTime stating it accepts
"YYYY-MM-DD HH:MM:SS" and "YYYY-MM-DD HH:MM:SS Z" (where Z is a timezone name)
and that when the second format is used the underlying parseTimeM will interpret
the timezone and convert the resulting UTCTime to UTC; also note that empty or
whitespace-only input returns Nothing. Reference the function name parseDateTime
and the two format strings "%Y-%m-%d %H:%M:%S" and "%Y-%m-%d %H:%M:%S %Z" in the
comment so callers understand expected input and conversion semantics.
- Around line 28-31: The function nonEmpty' calls T.strip twice; instead bind
the stripped value once (e.g., let s = T.strip t) then check if T.null s and
return Nothing or Just s accordingly—update nonEmpty' to compute the trimmed
Text once and reuse that binding.
🪄 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
Run ID: 11e03aad-cf16-468b-8807-0febd7ca9d15
📒 Files selected for processing (15)
lib/mobility-core/mobility-core.caballib/mobility-core/src/Kernel/External/Settlement/BillDesk/PaymentParser.hslib/mobility-core/src/Kernel/External/Settlement/BillDesk/PaymentTypes.hslib/mobility-core/src/Kernel/External/Settlement/CCAvenue/PaymentParser.hslib/mobility-core/src/Kernel/External/Settlement/CCAvenue/PaymentTypes.hslib/mobility-core/src/Kernel/External/Settlement/HyperPG/MerchantPaymentParser.hslib/mobility-core/src/Kernel/External/Settlement/HyperPG/PaymentParser.hslib/mobility-core/src/Kernel/External/Settlement/HyperPG/PayoutParser.hslib/mobility-core/src/Kernel/External/Settlement/Interface.hslib/mobility-core/src/Kernel/External/Settlement/Interface/Types.hslib/mobility-core/src/Kernel/External/Settlement/Razorpay/PaymentParser.hslib/mobility-core/src/Kernel/External/Settlement/Razorpay/PaymentTypes.hslib/mobility-core/src/Kernel/External/Settlement/Types.hslib/mobility-core/src/Kernel/External/Settlement/Utils/ParserUtils.hslib/mobility-core/src/Kernel/External/Settlement/YesBiz/PaymentParser.hs
| paymentGateway = Just "BILLDESK", | ||
| paymentMethod = Nothing, | ||
| paymentGateway = Just "BILLDESK", -- PG name → Hardcode as Billdesk | ||
| paymentMethod = parseBillDeskPaymentMethod row.payRef2, -- Paymode → Pay Ref1 ---------mapping correction needed |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for BillDesk payment method mapping documentation or tests
# Look for test files covering BillDesk payment method parsing
rg -n -C5 'parseBillDeskPaymentMethod|payRef2|Pay Ref2' --type=hs --glob '*Test*' --glob '*Spec*'
# Look for sample CSV files or documentation
fd -e csv -e txt -e md -i billdeskRepository: nammayatri/shared-kernel
Length of output: 234
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect the code around the referenced line
FILE="lib/mobility-core/src/Kernel/External/Settlement/BillDesk/PaymentParser.hs"
echo "---- $FILE (lines 130-170) ----"
nl -ba "$FILE" | sed -n '130,170p'
# 2) Find the TODO / inline comment text in the repository
echo
echo "---- Search for 'mapping correction needed' ----"
rg -n "mapping correction needed|Pay Ref2|payRef2|parseBillDeskPaymentMethod" --hidden --no-ignore-vcs
# 3) Check where parseBillDeskPaymentMethod is defined and how payRef2 is used
echo
echo "---- Find definition of parseBillDeskPaymentMethod ----"
rg -n "parseBillDeskPaymentMethod" --type=hs lib/mobility-core/src/Kernel/External/Settlement/BillDesk/PaymentParser.hs
# 4) Inspect the sample CSV and parsed output that were found
echo
echo "---- Show BillDesk sample CSV ----"
CSV="test-settlement-parser/samples/billdesk/billdesk_settlement_report_sample.csv"
nl -ba "$CSV" | head -n 60
echo
echo "---- Show parsed output (contains mapping results) ----"
PARSED_TXT="test-settlement-parser/output/billdesk/billdesk_settlement_report_sample.csv.parsed.txt"
# Show only the first portion to avoid huge output
nl -ba "$PARSED_TXT" | head -n 200Repository: nammayatri/shared-kernel
Length of output: 230
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="lib/mobility-core/src/Kernel/External/Settlement/BillDesk/PaymentParser.hs"
echo "---- $FILE (lines 130-170) ----"
awk -v start=130 -v end=170 '{ if(NR>=start && NR<=end) printf "%5d\t%s\n", NR, $0 }' "$FILE"
echo
echo "---- Search for 'mapping correction needed' / payRef2 / TODO text ----"
rg -n "mapping correction needed|Pay Ref2|payRef2|parseBillDeskPaymentMethod" --hidden --no-ignore-vcs
echo
echo "---- Find definition of parseBillDeskPaymentMethod (file-local) ----"
rg -n "parseBillDeskPaymentMethod" --type=hs "$FILE"
echo
echo "---- Show BillDesk sample CSV (first 60 lines) ----"
CSV="test-settlement-parser/samples/billdesk/billdesk_settlement_report_sample.csv"
awk 'NR<=60 { printf "%5d\t%s\n", NR, $0 }' "$CSV"
echo
echo "---- Show parsed output (first 200 lines) ----"
PARSED_TXT="test-settlement-parser/output/billdesk/billdesk_settlement_report_sample.csv.parsed.txt"
awk 'NR<=200 { printf "%5d\t%s\n", NR, $0 }' "$PARSED_TXT"Repository: nammayatri/shared-kernel
Length of output: 10524
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect parser code around parseBillDeskPaymentMethod and surrounding mappings
FILE="lib/mobility-core/src/Kernel/External/Settlement/BillDesk/PaymentParser.hs"
echo "---- $FILE (lines 180-320) ----"
awk 'NR>=180 && NR<=320 { printf "%5d\t%s\n", NR, $0 }' "$FILE"
# Inspect PaymentTypes for payRef2/ref2 fields and PaymentMethodType mapping
TFILE="lib/mobility-core/src/Kernel/External/Settlement/BillDesk/PaymentTypes.hs"
echo
echo "---- $TFILE (lines 1-220) ----"
awk 'NR>=1 && NR<=220 { printf "%5d\t%s\n", NR, $0 }' "$TFILE"
# Also inspect the CSV-parsing type that defines fields (look for payRef2/ref2 usage)
echo
echo "---- Search for 'payRef2' and 'ref2' field definitions/usages ----"
rg -n "payRef2|\\bref2\\b" --type=hs lib/mobility-core/src/Kernel/External/Settlement/BillDesk
# Dump the entire mapping function if it's short; otherwise show relevant branches
echo
echo "---- Extract parseBillDeskPaymentMethod branches ----"
awk 'NR>=270 && NR<=320 { printf "%5d\t%s\n", NR, $0 }' "$FILE"Repository: nammayatri/shared-kernel
Length of output: 15912
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "Pay Ref1|Pay Ref2|payRef1|payRef2|parseBillDeskPaymentMethod|BillDeskSettledRow" --hidden --no-ignore-vcs .
echo "----"
rg -n "NETBANKING|CREDIT_CARD|DEBIT_CARD|CASH_CARD|UPI|ENACH|CBDC|UPI_CREDIT_LINE|UPI_PREPAID_WALLET" --hidden --no-ignore-vcs lib/mobility-core/src/Kernel/External/Settlement/BillDesk
echo "----"
fd -e md -e txt -e csv -e json -e doc --hidden --no-ignore-vcs . | rg -i "billdesk|payment.?method|settlement.*parser"Repository: nammayatri/shared-kernel
Length of output: 3509
🏁 Script executed:
#!/bin/bash
set -euo pipefail
CSV="test-settlement-parser/samples/billdesk/billdesk_settlement_report_sample.csv"
echo "---- Lines containing 'Pay Ref1' or 'Pay Ref2' in sample CSV ----"
rg -n "Pay Ref1|Pay Ref2|payRef1|payRef2" "$CSV" || true
echo
echo "---- Show the first settled-section header row (line numbers) ----"
# Find the header line after "SETTLED TRANSACTIONS"
# Heuristic: the line immediately following the first header marker contains column names.
awk '
/SETTLED TRANSACTIONS/ {marker=NR; next}
marker && $0 !~ /^,$/ {print "header_line=" NR; print; exit}
' "$CSV"
echo
echo "---- Dump the first settled data row (line after the settled header) ----"
awk '
/SETTLED TRANSACTIONS/ {marker=NR; next}
marker && header_seen==0 {
# current line is header; mark next line as data row
header_seen=1; next
}
marker && header_seen==1 {print "data_line=" NR; print; exit}
' "$CSV"
echo
echo "---- Python: locate Pay Ref2 column and print its value for settled rows ----"
python3 - <<'PY'
import csv, re, sys
path = "test-settlement-parser/samples/billdesk/billdesk_settlement_report_sample.csv"
with open(path, newline='', encoding='utf-8') as f:
lines = f.read().splitlines()
# find settled header
start = None
for i, line in enumerate(lines):
if "SETTLED TRANSACTIONS" in line:
start = i
break
if start is None:
print("No SETTLED TRANSACTIONS marker found")
sys.exit(0)
# header is first non-empty line after marker that contains "Biller Id" (column header)
header_idx = None
for j in range(start+1, len(lines)):
if lines[j].strip() and "Biller Id" in lines[j]:
header_idx = j
break
if header_idx is None:
print("No settled header row found")
sys.exit(0)
header = next(csv.reader([lines[header_idx]]))
col_index = {name: idx for idx, name in enumerate(header)}
print("Settled header columns:", header)
print("Pay Ref2 index:", col_index.get("Pay Ref2"))
# first data row after header
data_idx = header_idx + 1
while data_idx < len(lines) and not lines[data_idx].strip():
data_idx += 1
if data_idx >= len(lines):
print("No settled data row found")
sys.exit(0)
row = next(csv.reader([lines[data_idx]]))
# normalize length
row = row + [""]*(len(header)-len(row))
pay_ref2 = row[col_index["Pay Ref2"]] if "Pay Ref2" in col_index else None
print("First settled row Pay Ref2 value:", pay_ref2)
PYRepository: nammayatri/shared-kernel
Length of output: 1095
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="lib/mobility-core/src/Kernel/External/Settlement/BillDesk/PaymentParser.hs"
echo "---- $FILE (lines 1-190) ----"
awk 'NR>=1 && NR<=190 { printf "%5d\t%s\n", NR, $0 }' "$FILE"
echo
echo "---- Show raw settled section header area from sample CSV (lines 1-10) ----"
CSV="test-settlement-parser/samples/billdesk/billdesk_settlement_report_sample.csv"
awk 'NR<=12 { printf "%5d\t%s\n", NR, $0 }' "$CSV"
echo
echo "---- Check entire sample CSV for any occurrence of 'Pay Ref' ----"
rg -n "Pay Ref" "$CSV" || true
echo
echo "---- Check for decodeNamedRecord / decodeByName usage in PaymentParser.hs ----"
rg -n "decodeByName|decodeByName|decodeNamedRecord|FromNamedRecord|Csv\\.|named" "$FILE"Repository: nammayatri/shared-kernel
Length of output: 11019
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search all BillDesk samples/outputs/docs for Pay Ref1/Pay Ref2
fd -e csv -e txt -e md -i --hidden billdesk test-settlement-parser \
| xargs -r rg -n "Pay Ref1|Pay Ref2|payRef1|payRef2|NETBANKING|UPI_CREDIT|parseBillDeskPaymentMethod" || true
echo
echo "---- Inspect billdesk email-attachment sample header (first ~25 lines) ----"
CSV="test-settlement-parser/samples/billdesk/email-attachment.csv"
if [ -f "$CSV" ]; then
awk 'NR<=25 { printf "%5d\t%s\n", NR, $0 }' "$CSV"
fi
echo
echo "---- Inspect README for any BillDesk mapping notes ----"
rg -n "BillDesk|pay ref|Pay Ref|PaymentMethod|parseBillDeskPaymentMethod" test-settlement-parser/README.md || trueRepository: nammayatri/shared-kernel
Length of output: 2625
🏁 Script executed:
#!/bin/bash
set -euo pipefail
README="test-settlement-parser/README.md"
if [ -f "$README" ]; then
echo "---- $README (lines 1-260) ----"
awk 'NR>=1 && NR<=260 { printf "%5d\t%s\n", NR, $0 }' "$README"
else
echo "README not found at $README"
fi
echo
echo "---- Search README for BillDesk schema/Pay Ref columns ----"
rg -n "BillDesk|Pay Ref1|Pay Ref2|payRef1|payRef2|Ref\\. 2|paymentMethod" "$README" || trueRepository: nammayatri/shared-kernel
Length of output: 9391
Remove/resolve BillDesk mapping TODO: settled paymentMethod reads row.payRef2, but the CSV samples have no Pay Ref2
Line 148 currently uses:
paymentMethod = parseBillDeskPaymentMethod row.payRef2, -- Paymode → Pay Ref1 ---------mapping correction neededPaymentParser.parseSettledRowmaps settledpaymentMethodfromrow.payRef2(TODO), while refund/chargeback userow.ref2.BillDeskSettledRow’sCsv.FromNamedRecordexpects CSV columns namedPay Ref1/Pay Ref2, but the checked-in BillDesk sample CSV headers only includeRef. 1/Ref. 2(noPay Ref2), and the sample parsed output showspaymentMethod: <empty>.- Fix the settled mapping (either use
row.ref2like the other sections, or update the named-record field names to match the actual CSV headers), then remove the inline TODO before merging.
🤖 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 `@lib/mobility-core/src/Kernel/External/Settlement/BillDesk/PaymentParser.hs`
at line 148, The settled-row mapping incorrectly reads paymentMethod from
row.payRef2 (leftover TODO) causing empty values; in
PaymentParser.parseSettledRow change the mapping to use row.ref2 (the same field
used for refunds/chargebacks) or alternatively update BillDeskSettledRow's
Csv.FromNamedRecord to bind Pay Ref2 to the actual CSV header "Ref. 2" — then
remove the inline TODO comment; update references to parseBillDeskPaymentMethod
and BillDeskSettledRow accordingly so settled parsing matches the CSV samples.
| offerCode = Nothing, | ||
| offerId = Nothing, | ||
| actualAmount = Nothing, | ||
| cardNumber = nonEmpty' row.vpa |
There was a problem hiding this comment.
VPA incorrectly mapped to cardNumber field.
row.vpa contains the UPI Virtual Payment Address (e.g., user@upi), which is not a card number. This semantic mismatch corrupts the cardNumber field for UPI transactions and leaves it unpopulated for card transactions.
Consider mapping VPA to a more appropriate field (or a new dedicated field), and extracting actual card number data if available in Razorpay's settlement reports.
🔧 Proposed fix
- cardNumber = nonEmpty' row.vpa
+ cardNumber = Nothing -- Razorpay doesn't provide card number in settlement; VPA is UPI-specificIf VPA needs to be preserved, consider adding it to paymentMethodSubType:
- paymentMethodSubType = Nothing,
+ paymentMethodSubType = nonEmpty' row.vpa, -- UPI VPA when applicable📝 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.
Suggested change
| cardNumber = nonEmpty' row.vpa | |
| cardNumber = Nothing -- Razorpay doesn't provide card number in settlement; VPA is UPI-specific |
🤖 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 `@lib/mobility-core/src/Kernel/External/Settlement/Razorpay/PaymentParser.hs`
at line 100, The cardNumber field is incorrectly populated from row.vpa (a UPI
VPA); update PaymentParser so cardNumber is populated only from actual card
fields (e.g., row.cardNumber, row.masked_card, or whichever Razorpay field
contains card data) and stop using nonEmpty' row.vpa for cardNumber; instead map
the VPA to a dedicated field such as paymentMethodSubType or add a new vpa field
(refer to cardNumber, row.vpa, paymentMethodSubType and the PaymentParser
parsing function) so UPI VPAs are preserved without corrupting card data.
Comment on lines
+33
to
+37
| parseAmount :: Text -> HighPrecMoney | ||
| parseAmount t = | ||
| case readMaybe (T.unpack $ T.strip t) of | ||
| Just v -> v | ||
| Nothing -> 0 |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Consider returning Maybe HighPrecMoney or documenting the zero-default behavior.
The function silently defaults to 0 for any unparseable input (including malformed amounts like "N/A" or "ERROR"). For financial settlement data, silently treating invalid amounts as zero can mask data quality issues or lead to incorrect reconciliation.
Options:
- Return
Maybe HighPrecMoneyand let callers handle parse failures explicitly - Document the zero-default behavior with an inline comment
- Add validation that empty input returns
0but other parse failures raise an error
💡 Option 1: Return Maybe for explicit error handling
-parseAmount :: Text -> HighPrecMoney
-parseAmount t =
- case readMaybe (T.unpack $ T.strip t) of
- Just v -> v
- Nothing -> 0
+parseAmount :: Text -> Maybe HighPrecMoney
+parseAmount t
+ | T.null (T.strip t) = Just 0 -- Empty input defaults to 0
+ | otherwise = readMaybe (T.unpack $ T.strip t)Then callers use fromMaybe 0 (parseAmount x) where zero-default is safe, or handle Nothing explicitly where validation is needed.
🤖 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 `@lib/mobility-core/src/Kernel/External/Settlement/Utils/ParserUtils.hs` around
lines 33 - 37, The parseAmount function currently returns 0 on any parse failure
which can hide bad financial data; change its type from parseAmount :: Text ->
HighPrecMoney to parseAmount :: Text -> Maybe HighPrecMoney and update the
implementation to return Just v on successful readMaybe and Nothing on failures
(i.e., propagate the parse failure instead of defaulting to 0), then update all
callers of parseAmount to handle the Maybe (use fromMaybe 0 only where a
zero-default is explicitly acceptable) so invalid inputs are surfaced;
alternatively, if you prefer keeping the current behavior, add an inline comment
on parseAmount documenting that non-parseable inputs are intentionally treated
as zero and why.
RahulGupta0706
force-pushed
the
feat/parser-logic-addition-for-razorpay-ccavenue-pg
branch
from
4ee5c5a to
1e76876
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
lib/mobility-core/mobility-core.cabal (1)
237-241:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove exposed-module additions to
package.yamlto avoid hpack rollback.Because Line 3 declares this file is hpack-generated, adding modules directly here is non-persistent. On regeneration, these entries can be removed again, which breaks the new settlement/SAP exports and may fail CI. Please add the same module list to
lib/mobility-core/package.yaml(library.exposed-modules) and regenerate the.cabal.Also applies to: 244-245, 254-255, 258-258
🤖 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 `@lib/mobility-core/mobility-core.cabal` around lines 237 - 241, The exposed-module entries are being added directly to the hpack-generated mobility-core.cabal file, but since line 3 declares the file as hpack-generated, these manual additions are non-persistent and will be removed on the next hpack regeneration. Instead of editing the .cabal file directly, add the Kernel.External.SAP.API, Kernel.External.SAP.Config, Kernel.External.SAP.Flow, Kernel.External.SAP.Interface, and Kernel.External.SAP.Types modules to the library.exposed-modules section in lib/mobility-core/package.yaml, then regenerate the .cabal file using hpack to make the changes persistent. The same approach applies to the other module additions mentioned at lines 244-245, 254-255, and 258.
🤖 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 `@lib/mobility-core/src/Kernel/External/Settlement/CCAvenue/PaymentParser.hs`:
- Around line 43-47: Preserve section decode failures before rows are merged: in
PaymentParser’s parse flow, stop letting zipMerge discard Left values from
parseSectionRows for both the payout and transaction sections. Update the
parsing path so any decode error from the section parsers is carried through
into the final ParseResult instead of being dropped, and make sure the affected
sites in
lib/mobility-core/src/Kernel/External/Settlement/CCAvenue/PaymentParser.hs:43-47
and 111-116 are handled consistently.
- Around line 135-156: The authCode is being incorrectly mapped to the rrn field
(which should contain a bank RRN), when authCode is actually an
approval/authorization code. Additionally, ccavenueRefNumber is being duplicated
across both txnId and pgApprovalCode fields. Fix this by: (1) changing the rrn
field assignment to set it as Nothing instead of using authCode, and (2)
changing the pgApprovalCode field assignment to use authCode (via nonEmpty'
row.authCode) instead of ccavenueRefNumber, so each field maps to its correct
source data. This preserves ccavenueRefNumber for txnId and properly maps
authCode to pgApprovalCode where it belongs.
In `@lib/mobility-core/src/Kernel/External/Settlement/Razorpay/PaymentParser.hs`:
- Around line 105-109: The issue spans two transaction type parsers that
silently default unknown transaction types to ORDER, hiding provider format
changes and misclassifying transactions. In
lib/mobility-core/src/Kernel/External/Settlement/Razorpay/PaymentParser.hs lines
105-109, refactor parseRazorpayTxnType to return Either Text TxnType instead of
TxnType, add an explicit case for "chargeback" if supported, and return a Left
error for unknown transaction types instead of defaulting to ORDER. In
lib/mobility-core/src/Kernel/External/Settlement/CCAvenue/PaymentParser.hs lines
197-203, refactor parseCCAvenueTxnType similarly to fail on unknown labels by
removing the default pattern and only handling the supported "payment", "sale",
"refund", and "chargeback" cases, then thread this Either return through
parseMergedRow so that unknown transaction types increment failedRows instead of
producing a credit record.
---
Duplicate comments:
In `@lib/mobility-core/mobility-core.cabal`:
- Around line 237-241: The exposed-module entries are being added directly to
the hpack-generated mobility-core.cabal file, but since line 3 declares the file
as hpack-generated, these manual additions are non-persistent and will be
removed on the next hpack regeneration. Instead of editing the .cabal file
directly, add the Kernel.External.SAP.API, Kernel.External.SAP.Config,
Kernel.External.SAP.Flow, Kernel.External.SAP.Interface, and
Kernel.External.SAP.Types modules to the library.exposed-modules section in
lib/mobility-core/package.yaml, then regenerate the .cabal file using hpack to
make the changes persistent. The same approach applies to the other module
additions mentioned at lines 244-245, 254-255, and 258.
🪄 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
Run ID: 84aacddb-9091-40ec-aa57-a3a448e37648
📒 Files selected for processing (15)
lib/mobility-core/mobility-core.caballib/mobility-core/src/Kernel/External/Settlement/BillDesk/PaymentParser.hslib/mobility-core/src/Kernel/External/Settlement/BillDesk/PaymentTypes.hslib/mobility-core/src/Kernel/External/Settlement/CCAvenue/PaymentParser.hslib/mobility-core/src/Kernel/External/Settlement/CCAvenue/PaymentTypes.hslib/mobility-core/src/Kernel/External/Settlement/HyperPG/MerchantPaymentParser.hslib/mobility-core/src/Kernel/External/Settlement/HyperPG/PaymentParser.hslib/mobility-core/src/Kernel/External/Settlement/HyperPG/PayoutParser.hslib/mobility-core/src/Kernel/External/Settlement/Interface.hslib/mobility-core/src/Kernel/External/Settlement/Interface/Types.hslib/mobility-core/src/Kernel/External/Settlement/Razorpay/PaymentParser.hslib/mobility-core/src/Kernel/External/Settlement/Razorpay/PaymentTypes.hslib/mobility-core/src/Kernel/External/Settlement/Types.hslib/mobility-core/src/Kernel/External/Settlement/Utils/ParserUtils.hslib/mobility-core/src/Kernel/External/Settlement/YesBiz/PaymentParser.hs
🚧 Files skipped from review as they are similar to previous changes (10)
- lib/mobility-core/src/Kernel/External/Settlement/Razorpay/PaymentTypes.hs
- lib/mobility-core/src/Kernel/External/Settlement/Interface.hs
- lib/mobility-core/src/Kernel/External/Settlement/HyperPG/PayoutParser.hs
- lib/mobility-core/src/Kernel/External/Settlement/Interface/Types.hs
- lib/mobility-core/src/Kernel/External/Settlement/BillDesk/PaymentTypes.hs
- lib/mobility-core/src/Kernel/External/Settlement/Types.hs
- lib/mobility-core/src/Kernel/External/Settlement/HyperPG/PaymentParser.hs
- lib/mobility-core/src/Kernel/External/Settlement/Utils/ParserUtils.hs
- lib/mobility-core/src/Kernel/External/Settlement/YesBiz/PaymentParser.hs
- lib/mobility-core/src/Kernel/External/Settlement/BillDesk/PaymentParser.hs
Comment on lines
+43
to
+47
| payoutRows = parseSectionRows @CCAvenuePayoutRow "Payout Summary" sections.payout | ||
| txnRows = parseSectionRows @CCAvenueTxnRow "Transaction Summary" sections.transaction | ||
| mergedRows = zipMerge txnRows payoutRows | ||
| results = zipWith (flip parseMergedRow) [1 ..] mergedRows | ||
| (errs, goods) = partitionEithers results |
There was a problem hiding this comment.
Preserve section decode failures before zipping rows.
parseSectionRows can return Left, but zipMerge strips those entries with [Right r <- ...], so malformed payout/transaction sections disappear from errors. If the transaction section fails to decode, this path currently returns ParseResult [] 0 0 [], which looks like a clean parse to Kernel.External.Settlement.Interface.parsePaymentSettlementCsv.
Suggested fix
parseCCAvenueCsv :: LBS.ByteString -> ParsePaymentSettlementResult
parseCCAvenueCsv csvData =
let textContent = T.pack . map (toEnum . fromEnum) . LBS.unpack $ csvData
allLines = T.lines textContent
sections = splitSections allLines
payoutRows = parseSectionRows `@CCAvenuePayoutRow` "Payout Summary" sections.payout
txnRows = parseSectionRows `@CCAvenueTxnRow` "Transaction Summary" sections.transaction
- mergedRows = zipMerge txnRows payoutRows
+ (payoutErrs, payoutGoods) = partitionEithers payoutRows
+ (txnErrs, txnGoods) = partitionEithers txnRows
+ mergedRows = zipMerge txnGoods payoutGoods
results = zipWith (flip parseMergedRow) [1 ..] mergedRows
- (errs, goods) = partitionEithers results
+ (rowErrs, goods) = partitionEithers results
+ errs = payoutErrs <> txnErrs <> rowErrs
in ParseResult
{ reports = goods,
totalRows = length results,
failedRows = length errs,
errors = errs
}
@@
-zipMerge :: [Either Text CCAvenueTxnRow] -> [Either Text CCAvenuePayoutRow] -> [CCAvenueMergedRow]
-zipMerge txnResults payoutResults =
- let txnRows = [r | Right r <- txnResults]
- payoutRows = [r | Right r <- payoutResults]
- payoutPadded = map Just payoutRows <> repeat Nothing
- in zipWith (\t p -> CCAvenueMergedRow t p) txnRows payoutPadded
+zipMerge :: [CCAvenueTxnRow] -> [CCAvenuePayoutRow] -> [CCAvenueMergedRow]
+zipMerge txnRows payoutRows =
+ let payoutPadded = map Just payoutRows <> repeat Nothing
+ in zipWith CCAvenueMergedRow txnRows payoutPaddedAlso applies to: 111-116
🤖 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 `@lib/mobility-core/src/Kernel/External/Settlement/CCAvenue/PaymentParser.hs`
around lines 43 - 47, Preserve section decode failures before rows are merged:
in PaymentParser’s parse flow, stop letting zipMerge discard Left values from
parseSectionRows for both the payout and transaction sections. Update the
parsing path so any decode error from the section parsers is carried through
into the final ParseResult instead of being dropped, and make sure the affected
sites in
lib/mobility-core/src/Kernel/External/Settlement/CCAvenue/PaymentParser.hs:43-47
and 111-116 are handled consistently.
Comment on lines
+135
to
+156
| txnId = nonEmpty' row.ccavenueRefNumber, -- Transaction ID → CCAvenue Ref Number | ||
| rrn = nonEmpty' row.authCode, -- Bank RRN → Auth Code | ||
| utr = payout >>= nonEmpty' . (.utrNo), -- Settlement UTR → Payout Summary - UTR No | ||
| txnType = txnType', -- Document Type → Transaction Type | ||
| txnStatus = SUCCESS, | ||
| txnDate = parseCCAvenueDateTime row.txnDate, -- Transaction Date → Date | ||
| txnAmount = parseAmount row.amount, -- Charged Amount → Amount | ||
| pgBaseFee = parseAmount row.tdrAmount, -- PG Fee → TDR Amount | ||
| pgTax = parseAmount row.tax, -- GST on PG Fee → Tax | ||
| settlementAmount = parseAmount row.payableAmount, -- Net Settled Amount → Payable Amount | ||
| currency = INR, | ||
| vendorId = nonEmpty' row.merchantId, -- MID → Merchant ID | ||
| uniqueSplitId = Nothing, | ||
| paymentGateway = Just "HDFC-CCAVENUE", -- PG name → Hardcode as HDFC-CCAvenue | ||
| paymentMethod = parseCCAvenuePaymentMethod row.paymentType, -- Paymode → Payment Type | ||
| paymentMethodSubType = Nothing, | ||
| settlementType = settlementType', | ||
| settlementMode = Nothing, | ||
| settlementId = payout >>= nonEmpty' . (.payId), -- Settlement ID → Payout Summary - Pay ID | ||
| settlementDate = parseCCAvenueDateTime row.settlementDate, -- Settlement Date → Settlement Date | ||
| pgApprovalCode = nonEmpty' row.ccavenueRefNumber, -- PG Approval code → CCAvenue Ref Number | ||
| pgRequestId = nonEmpty' row.invoiceRefNo, -- PG Request id → Invoice Reference No |
There was a problem hiding this comment.
authCode and ccavenueRefNumber are populating the wrong report fields.
authCode is an approval/auth code, not a bank RRN, while ccavenueRefNumber is already being used as txnId. Mapping them to rrn and pgApprovalCode this way will poison the identifier fields that downstream reconciliation reads from PaymentSettlementReport.
Suggested fix
- rrn = nonEmpty' row.authCode, -- Bank RRN → Auth Code
+ rrn = Nothing, -- No dedicated RRN field in the CCAvenue row
@@
- pgApprovalCode = nonEmpty' row.ccavenueRefNumber, -- PG Approval code → CCAvenue Ref Number
+ pgApprovalCode = nonEmpty' row.authCode, -- PG Approval code → Auth CodeIf CCAvenue exposes a real RRN in another column later, wire that field into rrn instead of overloading authCode.
📝 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.
Suggested change
| txnId = nonEmpty' row.ccavenueRefNumber, -- Transaction ID → CCAvenue Ref Number | |
| rrn = nonEmpty' row.authCode, -- Bank RRN → Auth Code | |
| utr = payout >>= nonEmpty' . (.utrNo), -- Settlement UTR → Payout Summary - UTR No | |
| txnType = txnType', -- Document Type → Transaction Type | |
| txnStatus = SUCCESS, | |
| txnDate = parseCCAvenueDateTime row.txnDate, -- Transaction Date → Date | |
| txnAmount = parseAmount row.amount, -- Charged Amount → Amount | |
| pgBaseFee = parseAmount row.tdrAmount, -- PG Fee → TDR Amount | |
| pgTax = parseAmount row.tax, -- GST on PG Fee → Tax | |
| settlementAmount = parseAmount row.payableAmount, -- Net Settled Amount → Payable Amount | |
| currency = INR, | |
| vendorId = nonEmpty' row.merchantId, -- MID → Merchant ID | |
| uniqueSplitId = Nothing, | |
| paymentGateway = Just "HDFC-CCAVENUE", -- PG name → Hardcode as HDFC-CCAvenue | |
| paymentMethod = parseCCAvenuePaymentMethod row.paymentType, -- Paymode → Payment Type | |
| paymentMethodSubType = Nothing, | |
| settlementType = settlementType', | |
| settlementMode = Nothing, | |
| settlementId = payout >>= nonEmpty' . (.payId), -- Settlement ID → Payout Summary - Pay ID | |
| settlementDate = parseCCAvenueDateTime row.settlementDate, -- Settlement Date → Settlement Date | |
| pgApprovalCode = nonEmpty' row.ccavenueRefNumber, -- PG Approval code → CCAvenue Ref Number | |
| pgRequestId = nonEmpty' row.invoiceRefNo, -- PG Request id → Invoice Reference No | |
| txnId = nonEmpty' row.ccavenueRefNumber, -- Transaction ID → CCAvenue Ref Number | |
| rrn = Nothing, -- No dedicated RRN field in the CCAvenue row | |
| utr = payout >>= nonEmpty' . (.utrNo), -- Settlement UTR → Payout Summary - UTR No | |
| txnType = txnType', -- Document Type → Transaction Type | |
| txnStatus = SUCCESS, | |
| txnDate = parseCCAvenueDateTime row.txnDate, -- Transaction Date → Date | |
| txnAmount = parseAmount row.amount, -- Charged Amount → Amount | |
| pgBaseFee = parseAmount row.tdrAmount, -- PG Fee → TDR Amount | |
| pgTax = parseAmount row.tax, -- GST on PG Fee → Tax | |
| settlementAmount = parseAmount row.payableAmount, -- Net Settled Amount → Payable Amount | |
| currency = INR, | |
| vendorId = nonEmpty' row.merchantId, -- MID → Merchant ID | |
| uniqueSplitId = Nothing, | |
| paymentGateway = Just "HDFC-CCAVENUE", -- PG name → Hardcode as HDFC-CCAvenue | |
| paymentMethod = parseCCAvenuePaymentMethod row.paymentType, -- Paymode → Payment Type | |
| paymentMethodSubType = Nothing, | |
| settlementType = settlementType', | |
| settlementMode = Nothing, | |
| settlementId = payout >>= nonEmpty' . (.payId), -- Settlement ID → Payout Summary - Pay ID | |
| settlementDate = parseCCAvenueDateTime row.settlementDate, -- Settlement Date → Settlement Date | |
| pgApprovalCode = nonEmpty' row.authCode, -- PG Approval code → Auth Code | |
| pgRequestId = nonEmpty' row.invoiceRefNo, -- PG Request id → Invoice Reference No |
🤖 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 `@lib/mobility-core/src/Kernel/External/Settlement/CCAvenue/PaymentParser.hs`
around lines 135 - 156, The authCode is being incorrectly mapped to the rrn
field (which should contain a bank RRN), when authCode is actually an
approval/authorization code. Additionally, ccavenueRefNumber is being duplicated
across both txnId and pgApprovalCode fields. Fix this by: (1) changing the rrn
field assignment to set it as Nothing instead of using authCode, and (2)
changing the pgApprovalCode field assignment to use authCode (via nonEmpty'
row.authCode) instead of ccavenueRefNumber, so each field maps to its correct
source data. This preserves ccavenueRefNumber for txnId and properly maps
authCode to pgApprovalCode where it belongs.
Comment on lines
+105
to
+109
| parseRazorpayTxnType :: Text -> TxnType | ||
| parseRazorpayTxnType t = case T.toLower (T.strip t) of | ||
| "payment" -> ORDER | ||
| "refund" -> REFUND | ||
| _ -> ORDER |
There was a problem hiding this comment.
Unknown provider transaction labels are being silently coerced to ORDER. Both parsers currently default unrecognized settlement types into the ORDER/CREDIT path instead of failing the row, which hides provider format changes and misclassifies debits. The stricter pattern already exists in lib/mobility-core/src/Kernel/External/Settlement/HyperPG/MerchantPaymentParser.hs#L132-L136.
lib/mobility-core/src/Kernel/External/Settlement/Razorpay/PaymentParser.hs#L105-L109: makeparseRazorpayTxnTypereturnEither Text TxnType, add an explicit"chargeback"branch if Razorpay emits it, and surface unknowntransactionEntityvalues as row errors instead of defaulting toORDER.lib/mobility-core/src/Kernel/External/Settlement/CCAvenue/PaymentParser.hs#L197-L203: makeparseCCAvenueTxnTypefail on unknown labels after the supported"payment"/"sale"/"refund"/"chargeback"cases, then thread that failure throughparseMergedRowsofailedRowsincrements instead of producing a credit record.
📍 Affects 2 files
lib/mobility-core/src/Kernel/External/Settlement/Razorpay/PaymentParser.hs#L105-L109(this comment)lib/mobility-core/src/Kernel/External/Settlement/CCAvenue/PaymentParser.hs#L197-L203
🤖 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 `@lib/mobility-core/src/Kernel/External/Settlement/Razorpay/PaymentParser.hs`
around lines 105 - 109, The issue spans two transaction type parsers that
silently default unknown transaction types to ORDER, hiding provider format
changes and misclassifying transactions. In
lib/mobility-core/src/Kernel/External/Settlement/Razorpay/PaymentParser.hs lines
105-109, refactor parseRazorpayTxnType to return Either Text TxnType instead of
TxnType, add an explicit case for "chargeback" if supported, and return a Left
error for unknown transaction types instead of defaulting to ORDER. In
lib/mobility-core/src/Kernel/External/Settlement/CCAvenue/PaymentParser.hs lines
197-203, refactor parseCCAvenueTxnType similarly to fail on unknown labels by
removing the default pattern and only handling the supported "payment", "sale",
"refund", and "chargeback" cases, then thread this Either return through
parseMergedRow so that unknown transaction types increment failedRows instead of
producing a credit record.
f29eea2 to
52f0f09
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
lib/mobility-core/src/Kernel/External/Settlement/Types.hs (1)
73-74: Add documentation explaining thebankCodefield's purpose.The
bankCodefield inSettlementServiceConfig(line 74) lacks documentation describing its purpose, which settlement providers require it, and how it should be populated. While the field is currently unused in the codebase, clear documentation is essential before relying on it, especially since it extends the shared configuration schema.🤖 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 `@lib/mobility-core/src/Kernel/External/Settlement/Types.hs` around lines 73 - 74, The `bankCode` field in the `SettlementServiceConfig` type definition lacks documentation comments. Add Haskell documentation (using -- | comment syntax) above or inline with the `bankCode` field to explain its purpose, which settlement providers require this field, and how it should be populated. This documentation should clarify the field's role in the settlement service configuration before it becomes actively used in the codebase.
🤖 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 `@lib/mobility-core/src/Kernel/External/SAP/Types.hs`:
- Around line 101-105: The renderXmlElement function directly interpolates the
val parameter into XML text nodes without escaping special characters, which can
produce invalid XML when the value contains &, <, or > characters. Fix this by
applying proper XML character escaping to the val before it is concatenated into
the XML output in the Just val case. The comment indicates similar escaping
issues exist in other XML rendering functions spanning lines 106-173, so apply
the same XML escaping logic throughout all XML rendering functions in that
range.
- Around line 252-258: The lookupField function uses the descendant search
operator `$//` which can incorrectly pull nested values from deeper levels (such
as item-level nodes) and concatenate them together, corrupting the header field
values. Replace the `$//` descendant search operator with a direct child search
operator (such as `$/`) in the cursor query to ensure only immediate child
elements are searched, preventing nested values from being incorrectly included
in the concatenated result.
- Line 158: Remove the duplicate Msgtyp emission from the XML header rendering.
The line containing renderXmlElement "Msgtyp" hdr.msgtyp at line 158 is a
duplicate of an earlier emission at line 136. Delete this duplicate line to
prevent the Msgtyp field from appearing twice in the SAP XML payload, which
could violate payload expectations.
---
Nitpick comments:
In `@lib/mobility-core/src/Kernel/External/Settlement/Types.hs`:
- Around line 73-74: The `bankCode` field in the `SettlementServiceConfig` type
definition lacks documentation comments. Add Haskell documentation (using -- |
comment syntax) above or inline with the `bankCode` field to explain its
purpose, which settlement providers require this field, and how it should be
populated. This documentation should clarify the field's role in the settlement
service configuration before it becomes actively used in the codebase.
🪄 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
Run ID: 75db8543-80bb-4745-9edc-d9dc7d0b9265
📒 Files selected for processing (20)
lib/mobility-core/mobility-core.caballib/mobility-core/src/Kernel/External/SAP/API.hslib/mobility-core/src/Kernel/External/SAP/Config.hslib/mobility-core/src/Kernel/External/SAP/Flow.hslib/mobility-core/src/Kernel/External/SAP/Interface.hslib/mobility-core/src/Kernel/External/SAP/Types.hslib/mobility-core/src/Kernel/External/Settlement/BillDesk/PaymentParser.hslib/mobility-core/src/Kernel/External/Settlement/BillDesk/PaymentTypes.hslib/mobility-core/src/Kernel/External/Settlement/CCAvenue/PaymentParser.hslib/mobility-core/src/Kernel/External/Settlement/CCAvenue/PaymentTypes.hslib/mobility-core/src/Kernel/External/Settlement/HyperPG/MerchantPaymentParser.hslib/mobility-core/src/Kernel/External/Settlement/HyperPG/PaymentParser.hslib/mobility-core/src/Kernel/External/Settlement/HyperPG/PayoutParser.hslib/mobility-core/src/Kernel/External/Settlement/Interface.hslib/mobility-core/src/Kernel/External/Settlement/Interface/Types.hslib/mobility-core/src/Kernel/External/Settlement/Razorpay/PaymentParser.hslib/mobility-core/src/Kernel/External/Settlement/Razorpay/PaymentTypes.hslib/mobility-core/src/Kernel/External/Settlement/Types.hslib/mobility-core/src/Kernel/External/Settlement/Utils/ParserUtils.hslib/mobility-core/src/Kernel/External/Settlement/YesBiz/PaymentParser.hs
🚧 Files skipped from review as they are similar to previous changes (13)
- lib/mobility-core/src/Kernel/External/Settlement/Interface.hs
- lib/mobility-core/mobility-core.cabal
- lib/mobility-core/src/Kernel/External/Settlement/HyperPG/MerchantPaymentParser.hs
- lib/mobility-core/src/Kernel/External/Settlement/Utils/ParserUtils.hs
- lib/mobility-core/src/Kernel/External/Settlement/HyperPG/PaymentParser.hs
- lib/mobility-core/src/Kernel/External/Settlement/BillDesk/PaymentTypes.hs
- lib/mobility-core/src/Kernel/External/Settlement/Interface/Types.hs
- lib/mobility-core/src/Kernel/External/Settlement/HyperPG/PayoutParser.hs
- lib/mobility-core/src/Kernel/External/Settlement/Razorpay/PaymentParser.hs
- lib/mobility-core/src/Kernel/External/Settlement/YesBiz/PaymentParser.hs
- lib/mobility-core/src/Kernel/External/Settlement/Razorpay/PaymentTypes.hs
- lib/mobility-core/src/Kernel/External/Settlement/CCAvenue/PaymentParser.hs
- lib/mobility-core/src/Kernel/External/Settlement/BillDesk/PaymentParser.hs
Comment on lines
+101
to
+105
| renderXmlElement :: Text -> Maybe Text -> Text | ||
| renderXmlElement tag = \case | ||
| Nothing -> "<" <> tag <> "></" <> tag <> ">" | ||
| Just val -> "<" <> tag <> ">" <> val <> "</" <> tag <> ">" | ||
|
|
There was a problem hiding this comment.
Escape XML content before interpolation (current rendering can emit invalid XML).
Values are inserted directly into XML text nodes; special characters in data (&, <, >) can break request parsing and journal posting.
Also applies to: 106-173
🤖 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 `@lib/mobility-core/src/Kernel/External/SAP/Types.hs` around lines 101 - 105,
The renderXmlElement function directly interpolates the val parameter into XML
text nodes without escaping special characters, which can produce invalid XML
when the value contains &, <, or > characters. Fix this by applying proper XML
character escaping to the val before it is concatenated into the XML output in
the Just val case. The comment indicates similar escaping issues exist in other
XML rendering functions spanning lines 106-173, so apply the same XML escaping
logic throughout all XML rendering functions in that range.
| renderXmlElement "Belnr" hdr.belnr, | ||
| renderXmlElement "Gjahr" hdr.gjahr, | ||
| renderXmlElement "Message" hdr.message, | ||
| renderXmlElement "Msgtyp" hdr.msgtyp, |
There was a problem hiding this comment.
Remove duplicate Msgtyp emission in header XML.
Line 158 emits <Msgtyp> a second time even though it is already emitted at Line 136. This can violate SAP payload expectations and cause ambiguous field interpretation.
Suggested fix
renderXmlElement "Belnr" hdr.belnr,
renderXmlElement "Gjahr" hdr.gjahr,
renderXmlElement "Message" hdr.message,
- renderXmlElement "Msgtyp" hdr.msgtyp,
"<ItemSet>",🤖 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 `@lib/mobility-core/src/Kernel/External/SAP/Types.hs` at line 158, Remove the
duplicate Msgtyp emission from the XML header rendering. The line containing
renderXmlElement "Msgtyp" hdr.msgtyp at line 158 is a duplicate of an earlier
emission at line 136. Delete this duplicate line to prevent the Msgtyp field
from appearing twice in the SAP XML payload, which could violate payload
expectations.
Comment on lines
+252
to
+258
| lookupField :: XC.Cursor -> Text -> Maybe Text | ||
| lookupField cursor fieldName = | ||
| case cursor $// laxElement fieldName &/ content of | ||
| [] -> Nothing | ||
| ts -> | ||
| let val = T.strip (T.concat ts) | ||
| in if T.null val then Nothing else Just val |
There was a problem hiding this comment.
lookupField should not use descendant search for header fields.
Using $// from a Header cursor can pull nested values (e.g., from item-level nodes) and then concatenate them, corrupting parsed header fields.
Suggested fix
+import Text.XML.Cursor (content, fromDocument, laxElement, ($/), ($//), (&/))
...
lookupField :: XC.Cursor -> Text -> Maybe Text
lookupField cursor fieldName =
- case cursor $// laxElement fieldName &/ content of
+ case cursor $/ laxElement fieldName &/ content of
[] -> Nothing
ts ->
let val = T.strip (T.concat ts)
in if T.null val then Nothing else Just val📝 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.
Suggested change
| lookupField :: XC.Cursor -> Text -> Maybe Text | |
| lookupField cursor fieldName = | |
| case cursor $// laxElement fieldName &/ content of | |
| [] -> Nothing | |
| ts -> | |
| let val = T.strip (T.concat ts) | |
| in if T.null val then Nothing else Just val | |
| lookupField :: XC.Cursor -> Text -> Maybe Text | |
| lookupField cursor fieldName = | |
| case cursor $/ laxElement fieldName &/ content of | |
| [] -> Nothing | |
| ts -> | |
| let val = T.strip (T.concat ts) | |
| in if T.null val then Nothing else Just val |
🤖 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 `@lib/mobility-core/src/Kernel/External/SAP/Types.hs` around lines 252 - 258,
The lookupField function uses the descendant search operator `$//` which can
incorrectly pull nested values from deeper levels (such as item-level nodes) and
concatenate them together, corrupting the header field values. Replace the `$//`
descendant search operator with a direct child search operator (such as `$/`) in
the cursor query to ensure only immediate child elements are searched,
preventing nested values from being incorrectly included in the concatenated
result.
13ae6d9 to
bde9871
Compare
Contributor
Author
|
@ClaudeWeb please review. |
Contributor
|
Fetching the latest PR state to check for any new commits or fixes.New commits pushed (head SHA changed again). Let me fetch all the key files in parallel.The SAP source files are now present (fixing the CI blocker), but all parser files are unchanged from the previous review. Here's the updated status: PR Review (3rd pass):
|
| # | File | Issue | Status |
|---|---|---|---|
| 1 | SAP source files | Build/CI broken | ✅ Fixed |
| 2 | Razorpay/PaymentParser.hs |
cardNumber = row.vpa |
❌ Open |
| 3 | BillDesk/PaymentParser.hs |
payRef2 wrong field + TODO |
❌ Open |
| 4 | CCAvenue/PaymentParser.hs |
rrn = authCode |
❌ Open |
| 5 | Utils/ParserUtils.hs |
parseAmount zero-default |
❌ Open |
| 6 | CCAvenue/PaymentParser.hs |
zipMerge drops section errors |
❌ Open |
| 7 | Razorpay/PaymentParser.hs |
Chargeback → ORDER |
❌ Open |
| 8 | Razorpay/PaymentParser.hs |
---- see into it TODO |
❌ Open |
RahulGupta0706
force-pushed
the
feat/parser-logic-addition-for-razorpay-ccavenue-pg
branch
from
bde9871 to
eb9b086
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@lib/mobility-core/src/Kernel/External/SAP/API.hs`:
- Around line 37-40: The SAP API Servant contract currently defines grant_type,
token_format, and Authorization as optional parameters using QueryParam and
Header without modifiers. Since the callsites in Flow.hs always provide these
values wrapped in Just, these parameters should be required. Change QueryParam
"grant_type" Text and QueryParam "token_format" Text to QueryParam' '[Required,
Strict] "grant_type" Text and QueryParam' '[Required, Strict] "token_format"
Text respectively, and change Header "Authorization" Text to Header' '[Required,
Strict] "Authorization" Text to enforce that these parameters must always be
provided in all API endpoints that use them.
- Line 47: Remove the explicit `deriving (Typeable)` clause from the
`SAPApplicationXML` data type declaration. Since GHC 7.10 and later
automatically derive Typeable for all types, the explicit derivation is
redundant and can trigger compilation warnings. Simply change the line to end
with just the data type name without the deriving clause to achieve warning-free
compilation under -Wall -Werror.
In `@lib/mobility-core/src/Kernel/External/Settlement/CCAvenue/PaymentParser.hs`:
- Around line 126-131: The `parseCCAvenueTxnType` function contains a fallback
pattern that defaults unknown transaction types to ORDER, which incorrectly
treats them as valid CREDIT settlements. Remove the `_ -> ORDER` catch-all
pattern and instead implement explicit error handling for unknown transaction
types so that rows with unrecognized or misspelled CCAvenue document types fail
validation and can be caught during reconciliation. This change applies to the
function definition as well as any other call sites where unknown transaction
types are similarly defaulted to ORDER (also check lines 196-202).
- Around line 99-105: The base case of parseSectionRows currently returns an
empty list when sectionLines is empty, which silently ignores a missing section
in a non-empty file. Change the pattern match for the empty section lines case
(currently parseSectionRows _ [] = []) to return an error indicating that the
expected section was not found, using the sectionName parameter to identify
which section is missing. This ensures that a malformed or non-matching report
with missing required sections surfaces an error instead of appearing as a
successful empty parse.
- Around line 140-143: The `parseAmount` function silently coerces unparseable
monetary values to zero, allowing invalid CCAvenue data to pass validation.
Create a new helper function `parseRequiredAmount` that returns `Either Text
HighPrecMoney` to properly surface parsing errors. In the `parseMergedRow`
function, use do-notation at the start to sequentially validate the four
required amount fields—`row.amount`, `row.tdrAmount`, `row.tax`, and
`row.payableAmount`—by calling `parseRequiredAmount` on each, then assign the
successful results to variables (e.g., `validatedTxnAmount`,
`validatedPgBaseFee`, `validatedPgTax`, `validatedSettlementAmount`). Finally,
in the record construction, replace the direct `parseAmount row.fieldName` calls
with these validated variables so malformed amounts cause the row to fail rather
than silently become zero.
🪄 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
Run ID: 9f9dd9e4-bcfb-4ac0-a2e0-659edf330700
📒 Files selected for processing (20)
lib/mobility-core/mobility-core.caballib/mobility-core/src/Kernel/External/SAP/API.hslib/mobility-core/src/Kernel/External/SAP/Config.hslib/mobility-core/src/Kernel/External/SAP/Flow.hslib/mobility-core/src/Kernel/External/SAP/Interface.hslib/mobility-core/src/Kernel/External/SAP/Types.hslib/mobility-core/src/Kernel/External/Settlement/BillDesk/PaymentParser.hslib/mobility-core/src/Kernel/External/Settlement/BillDesk/PaymentTypes.hslib/mobility-core/src/Kernel/External/Settlement/CCAvenue/PaymentParser.hslib/mobility-core/src/Kernel/External/Settlement/CCAvenue/PaymentTypes.hslib/mobility-core/src/Kernel/External/Settlement/HyperPG/MerchantPaymentParser.hslib/mobility-core/src/Kernel/External/Settlement/HyperPG/PaymentParser.hslib/mobility-core/src/Kernel/External/Settlement/HyperPG/PayoutParser.hslib/mobility-core/src/Kernel/External/Settlement/Interface.hslib/mobility-core/src/Kernel/External/Settlement/Interface/Types.hslib/mobility-core/src/Kernel/External/Settlement/Razorpay/PaymentParser.hslib/mobility-core/src/Kernel/External/Settlement/Razorpay/PaymentTypes.hslib/mobility-core/src/Kernel/External/Settlement/Types.hslib/mobility-core/src/Kernel/External/Settlement/Utils/ParserUtils.hslib/mobility-core/src/Kernel/External/Settlement/YesBiz/PaymentParser.hs
🚧 Files skipped from review as they are similar to previous changes (16)
- lib/mobility-core/src/Kernel/External/Settlement/Interface.hs
- lib/mobility-core/src/Kernel/External/SAP/Config.hs
- lib/mobility-core/src/Kernel/External/Settlement/Razorpay/PaymentTypes.hs
- lib/mobility-core/src/Kernel/External/Settlement/HyperPG/PayoutParser.hs
- lib/mobility-core/src/Kernel/External/Settlement/CCAvenue/PaymentTypes.hs
- lib/mobility-core/src/Kernel/External/SAP/Flow.hs
- lib/mobility-core/src/Kernel/External/Settlement/Interface/Types.hs
- lib/mobility-core/src/Kernel/External/Settlement/Utils/ParserUtils.hs
- lib/mobility-core/src/Kernel/External/Settlement/YesBiz/PaymentParser.hs
- lib/mobility-core/src/Kernel/External/SAP/Interface.hs
- lib/mobility-core/src/Kernel/External/Settlement/HyperPG/MerchantPaymentParser.hs
- lib/mobility-core/src/Kernel/External/Settlement/BillDesk/PaymentParser.hs
- lib/mobility-core/src/Kernel/External/Settlement/Razorpay/PaymentParser.hs
- lib/mobility-core/src/Kernel/External/SAP/Types.hs
- lib/mobility-core/src/Kernel/External/Settlement/HyperPG/PaymentParser.hs
- lib/mobility-core/src/Kernel/External/Settlement/BillDesk/PaymentTypes.hs
Comment on lines
+37
to
+40
| :> QueryParam "grant_type" Text | ||
| :> QueryParam "token_format" Text | ||
| :> Header "Authorization" Text | ||
| :> Post '[JSON] SAPTokenResp |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all sapTokenAPI/sapJournalPostAPI callsites and whether optional Maybe args are being passed.
set -euo pipefail
echo "== sapTokenAPI callsites =="
rg -nP --type=hs -C3 '\bclient\s+sapTokenAPI\b|\bsapTokenAPI\b'
echo
echo "== sapJournalPostAPI callsites =="
rg -nP --type=hs -C3 '\bclient\s+sapJournalPostAPI\b|\bsapJournalPostAPI\b'Repository: nammayatri/shared-kernel
Length of output: 4244
🏁 Script executed:
#!/bin/bash
# Get the API definitions to verify current parameter types
head -80 lib/mobility-core/src/Kernel/External/SAP/API.hs | tail -50Repository: nammayatri/shared-kernel
Length of output: 1269
🏁 Script executed:
#!/bin/bash
# Get the sapTokenAPI callsite context from Flow.hs
sed -n '35,60p' lib/mobility-core/src/Kernel/External/SAP/Flow.hs | cat -nRepository: nammayatri/shared-kernel
Length of output: 797
🏁 Script executed:
#!/bin/bash
# Get the sapJournalPostAPI callsite context from Flow.hs
sed -n '60,80p' lib/mobility-core/src/Kernel/External/SAP/Flow.hs | cat -nRepository: nammayatri/shared-kernel
Length of output: 690
Make OAuth/query/auth parameters required in the Servant contract.
Lines 37-40 and 66-68 currently model grant_type, token_format, and Authorization as optional using QueryParam and Header without modifiers. The Flow.hs callsites (lines 44-51 and 68-73) wrap all values in Just, confirming these are optional. Weakening the API contract to optional parameters permits invalid outbound requests if a caller passes Nothing. Use QueryParam' '[Required, Strict] and Header' '[Required, Strict] to enforce that these parameters must always be provided.
Proposed fix
type SAPTokenAPI =
"oauth2"
:> "api"
:> "v1"
:> "token"
- :> QueryParam "grant_type" Text
- :> QueryParam "token_format" Text
- :> Header "Authorization" Text
+ :> QueryParam' '[Required, Strict] "grant_type" Text
+ :> QueryParam' '[Required, Strict] "token_format" Text
+ :> Header' '[Required, Strict] "Authorization" Text
:> Post '[JSON] SAPTokenResp
type SAPJournalPostAPI =
"http"
:> "aws"
:> "s4"
:> "odata"
:> "ondc"
- :> Header "Authorization" Text
+ :> Header' '[Required, Strict] "Authorization" Text
:> ReqBody '[SAPApplicationXML] BSL.ByteString
:> Post '[SAPApplicationXML] SAPJournalResponse📝 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.
Suggested change
| :> QueryParam "grant_type" Text | |
| :> QueryParam "token_format" Text | |
| :> Header "Authorization" Text | |
| :> Post '[JSON] SAPTokenResp | |
| :> QueryParam' '[Required, Strict] "grant_type" Text | |
| :> QueryParam' '[Required, Strict] "token_format" Text | |
| :> Header' '[Required, Strict] "Authorization" Text | |
| :> Post '[JSON] SAPTokenResp |
🤖 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 `@lib/mobility-core/src/Kernel/External/SAP/API.hs` around lines 37 - 40, The
SAP API Servant contract currently defines grant_type, token_format, and
Authorization as optional parameters using QueryParam and Header without
modifiers. Since the callsites in Flow.hs always provide these values wrapped in
Just, these parameters should be required. Change QueryParam "grant_type" Text
and QueryParam "token_format" Text to QueryParam' '[Required, Strict]
"grant_type" Text and QueryParam' '[Required, Strict] "token_format" Text
respectively, and change Header "Authorization" Text to Header' '[Required,
Strict] "Authorization" Text to enforce that these parameters must always be
provided in all API endpoints that use them.
|
|
||
| -- XML content type for SAP | ||
|
|
||
| data SAPApplicationXML deriving (Typeable) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify warning risk and flag configuration evidence.
set -euo pipefail
echo "== Searching for explicit deriving(Typeable) in Haskell files =="
rg -nP --type=hs '\bderiving\s*\(\s*Typeable\s*\)'
echo
echo "== Searching for -Werror / -Wall flags in cabal and project configs =="
fd -i 'cabal|package.yaml|stack.yaml|hie.yaml|ghc-options|project' | xargs -r rg -n -- '-Werror|-Wall|default-extensions|ghc-options'Repository: nammayatri/shared-kernel
Length of output: 2571
🏁 Script executed:
cat -n lib/mobility-core/src/Kernel/External/SAP/API.hs | head -60Repository: nammayatri/shared-kernel
Length of output: 2248
🏁 Script executed:
# Check for warning pragmas or suppressions in the file
rg -n '{-#|DEPRECATED|WARNING' lib/mobility-core/src/Kernel/External/SAP/API.hsRepository: nammayatri/shared-kernel
Length of output: 166
🏁 Script executed:
# Check GHC version used and document default extensions
fd -i 'ghc.yaml|cabal.project|stack.yaml' -x cat -n {}Repository: nammayatri/shared-kernel
Length of output: 131
🏁 Script executed:
# Check for LANGUAGE pragmas at the start of the file
head -50 lib/mobility-core/src/Kernel/External/SAP/API.hs | rg -n 'LANGUAGE|OPTIONS'Repository: nammayatri/shared-kernel
Length of output: 50
🏁 Script executed:
# Check cabal file for GHC version constraint and default-extensions
rg -A 20 'library.*' lib/mobility-core/mobility-core.cabal | head -60Repository: nammayatri/shared-kernel
Length of output: 883
🏁 Script executed:
# Search for any GHC configuration that might suppress -Wderiving-typeable
rg -n 'Wno-deriving-typeable|Wwarn.*deriving' lib/mobility-core/Repository: nammayatri/shared-kernel
Length of output: 50
🏁 Script executed:
# Get the exact ghc-options for the mobility-core library stanza
rg -A 10 'name:.*mobility-core' lib/mobility-core/mobility-core.cabal | head -100Repository: nammayatri/shared-kernel
Length of output: 502
🌐 Web query:
GHC -Wderiving-typeable warning when to trigger Typeable derive
💡 Result:
The -Wderiving-typeable warning is triggered when you explicitly list Typeable in a deriving clause or use StandaloneDeriving to derive a Typeable instance [1][2][3]. You should never need to manually derive Typeable in modern Haskell [2]. Since GHC 7.10, all types automatically have Typeable instances derived by the compiler [1][4]. Consequently, explicitly deriving it is redundant and has no effect on your code, other than triggering this warning [2]. If you encounter this warning, the recommended action is to simply remove the deriving Typeable clause from your data declaration [2]. The code will retain its Typeable instance automatically [1][5].
Citations:
- 1: https://downloads.haskell.org/ghc/latest/docs/users_guide/using-warnings.html
- 2: https://errors.haskell.org/messages/GHC-90584/
- 3: https://ghc.gitlab.haskell.org/ghc/doc/users_guide/using-warnings.html?highlight=warning
- 4: https://hackage.haskell.org/package/base/docs/Data-Typeable.html
- 5: https://hackage.haskell.org/package/base-4.19.2.0/docs/Data-Typeable.html
🏁 Script executed:
# Get complete library stanza with ghc-options from cabal
rg -B 5 -A 15 'ghc-options.*-Wall' lib/mobility-core/mobility-core.cabal | head -80Repository: nammayatri/shared-kernel
Length of output: 1642
🏁 Script executed:
# Check if there's a -Wwarn override for deriving-typeable anywhere
rg -n 'Wwarn|Wno-' lib/mobility-core/mobility-core.cabalRepository: nammayatri/shared-kernel
Length of output: 549
Remove deriving (Typeable) — it's redundant and may trigger warnings with newer GHC versions.
Since GHC 7.10, Typeable is automatically derived for all types. Explicitly deriving it is redundant and can trigger the -Wderiving-typeable warning, which becomes an error under -Werror. Although this pattern appears across the codebase, the removal aligns with the coding guideline requiring warning-free compilation under -Wall -Werror.
Proposed fix
-data SAPApplicationXML deriving (Typeable)
+data SAPApplicationXML📝 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.
Suggested change
| data SAPApplicationXML deriving (Typeable) | |
| data SAPApplicationXML |
🤖 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 `@lib/mobility-core/src/Kernel/External/SAP/API.hs` at line 47, Remove the
explicit `deriving (Typeable)` clause from the `SAPApplicationXML` data type
declaration. Since GHC 7.10 and later automatically derive Typeable for all
types, the explicit derivation is redundant and can trigger compilation
warnings. Simply change the line to end with just the data type name without the
deriving clause to achieve warning-free compilation under -Wall -Werror.
Source: Coding guidelines
Comment on lines
+99
to
+105
| parseSectionRows :: (Csv.FromNamedRecord a) => Text -> [Text] -> [Either Text a] | ||
| parseSectionRows _ [] = [] | ||
| parseSectionRows sectionName sectionLines = | ||
| let csvBytes = LBS.fromStrict . encodeUtf8 . T.unlines $ sectionLines | ||
| in case Csv.decodeByName csvBytes of | ||
| Left err -> [Left $ sectionName <> " CSV parse error: " <> T.pack err] | ||
| Right (_, rows) -> map Right (V.toList rows) |
There was a problem hiding this comment.
Report a missing transaction section instead of returning a clean empty parse.
For a non-empty CCAvenue file without Transaction Summary Details, parseSectionRows _ [] = [] flows to ParseResult [] 0 0 []. Upstream already treats only empty input as a clean no-op; a malformed/non-matching report should surface an error.
Suggested fix
- txnRows = parseSectionRows `@CCAvenueTxnRow` "Transaction Summary" sections.transaction
+ txnRows = parseRequiredSectionRows `@CCAvenueTxnRow` "Transaction Summary" sections.transaction
@@
parseSectionRows :: (Csv.FromNamedRecord a) => Text -> [Text] -> [Either Text a]
parseSectionRows _ [] = []
parseSectionRows sectionName sectionLines =
let csvBytes = LBS.fromStrict . encodeUtf8 . T.unlines $ sectionLines
in case Csv.decodeByName csvBytes of
Left err -> [Left $ sectionName <> " CSV parse error: " <> T.pack err]
Right (_, rows) -> map Right (V.toList rows)
+
+parseRequiredSectionRows :: (Csv.FromNamedRecord a) => Text -> [Text] -> [Either Text a]
+parseRequiredSectionRows sectionName [] = [Left $ "Missing " <> sectionName <> " section"]
+parseRequiredSectionRows sectionName sectionLines = parseSectionRows sectionName sectionLines🤖 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 `@lib/mobility-core/src/Kernel/External/Settlement/CCAvenue/PaymentParser.hs`
around lines 99 - 105, The base case of parseSectionRows currently returns an
empty list when sectionLines is empty, which silently ignores a missing section
in a non-empty file. Change the pattern match for the empty section lines case
(currently parseSectionRows _ [] = []) to return an error indicating that the
expected section was not found, using the sectionName parameter to identify
which section is missing. This ensures that a malformed or non-matching report
with missing required sections surfaces an error instead of appearing as a
successful empty parse.
Comment on lines
+126
to
+131
| txnType' = parseCCAvenueTxnType row.txnType | ||
| settlementType' = case txnType' of | ||
| ORDER -> Just CREDIT | ||
| REFUND -> Just DEBIT | ||
| CHARGEBACK -> Just DEBIT | ||
| Right |
There was a problem hiding this comment.
Reject unknown transaction types instead of crediting them as orders.
The _ -> ORDER fallback turns any new/misspelled CCAvenue document type into a CREDIT settlement. Unknown financial transaction types should fail the row so reconciliation can be corrected.
Suggested fix
- txnType' = parseCCAvenueTxnType row.txnType
- settlementType' = case txnType' of
+ txnType' <- parseCCAvenueTxnType idx row.txnType
+ let settlementType' = case txnType' of
ORDER -> Just CREDIT
REFUND -> Just DEBIT
CHARGEBACK -> Just DEBIT
@@
-parseCCAvenueTxnType :: Text -> TxnType
-parseCCAvenueTxnType t = case T.toLower (T.strip t) of
+parseCCAvenueTxnType :: Int -> Text -> Either Text TxnType
+parseCCAvenueTxnType idx t = case T.toLower (T.strip t) of
"payment" -> ORDER
"sale" -> ORDER
"refund" -> REFUND
"chargeback" -> CHARGEBACK
- _ -> ORDER
+ _ -> Left $ "Row " <> T.pack (show idx) <> ": unknown CCAvenue transaction type: " <> tAlso applies to: 196-202
🤖 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 `@lib/mobility-core/src/Kernel/External/Settlement/CCAvenue/PaymentParser.hs`
around lines 126 - 131, The `parseCCAvenueTxnType` function contains a fallback
pattern that defaults unknown transaction types to ORDER, which incorrectly
treats them as valid CREDIT settlements. Remove the `_ -> ORDER` catch-all
pattern and instead implement explicit error handling for unknown transaction
types so that rows with unrecognized or misspelled CCAvenue document types fail
validation and can be caught during reconciliation. This change applies to the
function definition as well as any other call sites where unknown transaction
types are similarly defaulted to ORDER (also check lines 196-202).
Comment on lines
+140
to
+143
| txnAmount = parseAmount row.amount, -- Charged Amount → Amount | ||
| pgBaseFee = parseAmount row.tdrAmount, -- PG Fee → TDR Amount | ||
| pgTax = parseAmount row.tax, -- GST on PG Fee → Tax | ||
| settlementAmount = parseAmount row.payableAmount, -- Net Settled Amount → Payable Amount |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect parseAmount implementation and CCAvenue required amount usage.
# Expected: parseAmount should not silently default invalid required settlement amounts to zero.
rg -n -C4 'parseAmount\s*::|parseAmount\s+' --iglob '*.hs'
rg -n -C2 'txnAmount\s*=\s*parseAmount|pgBaseFee\s*=\s*parseAmount|pgTax\s*=\s*parseAmount|settlementAmount\s*=\s*parseAmount' --iglob '*.hs'Repository: nammayatri/shared-kernel
Length of output: 32540
🏁 Script executed:
# Get the context of parseMergedRow function in CCAvenue PaymentParser
sed -n '120,165p' lib/mobility-core/src/Kernel/External/Settlement/CCAvenue/PaymentParser.hsRepository: nammayatri/shared-kernel
Length of output: 2446
🏁 Script executed:
# Check if parseRequiredAmount exists anywhere
rg -n 'parseRequiredAmount' --iglob '*.hs'Repository: nammayatri/shared-kernel
Length of output: 50
🏁 Script executed:
# Check the full ParserUtils module to understand available helpers
cat -n lib/mobility-core/src/Kernel/External/Settlement/Utils/ParserUtils.hsRepository: nammayatri/shared-kernel
Length of output: 1882
Fail rows with malformed monetary fields.
These required financial fields—txnAmount, pgBaseFee, pgTax, settlementAmount—use parseAmount, which silently coerces unparseable values to zero. This allows bad CCAvenue data to pass through as valid ₹0 reports without marking rows as failed.
To fix this, create a parseRequiredAmount helper (or similar error-handling variant) that returns an Either Text HighPrecMoney and call it in the do-notation at the start of parseMergedRow. Then use the validated values in the record construction instead of calling parseAmount directly on raw fields.
🤖 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 `@lib/mobility-core/src/Kernel/External/Settlement/CCAvenue/PaymentParser.hs`
around lines 140 - 143, The `parseAmount` function silently coerces unparseable
monetary values to zero, allowing invalid CCAvenue data to pass validation.
Create a new helper function `parseRequiredAmount` that returns `Either Text
HighPrecMoney` to properly surface parsing errors. In the `parseMergedRow`
function, use do-notation at the start to sequentially validate the four
required amount fields—`row.amount`, `row.tdrAmount`, `row.tax`, and
`row.payableAmount`—by calling `parseRequiredAmount` on each, then assign the
successful results to variables (e.g., `validatedTxnAmount`,
`validatedPgBaseFee`, `validatedPgTax`, `validatedSettlementAmount`). Finally,
in the record construction, replace the direct `parseAmount row.fieldName` calls
with these validated variables so malformed amounts cause the row to fail rather
than silently become zero.
eb9b086 to
918020d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Type of Change
Description
Additional Changes
Motivation and Context
How did you test it?
billdesk_settlement_report_sample.csv.parsed.txt
Checklist
./dev/format-all-files.shSummary by CodeRabbit