Backend : Ench - Adds Yesbiz PG reconcillation report parser#1198
Open
kavya-shree-s wants to merge 1 commit intomainfrom
Open
Backend : Ench - Adds Yesbiz PG reconcillation report parser#1198kavya-shree-s wants to merge 1 commit intomainfrom
kavya-shree-s wants to merge 1 commit intomainfrom
Conversation
WalkthroughYesBiz payment settlement support was introduced through new type definitions, two new CSV parsing modules, routing logic in the settlement interface, SFTP authentication refactoring to support key-based access, and test harness extensions for remote file retrieval and parsing. Changes
Sequence DiagramsequenceDiagram
participant CLI as Test CLI
participant SFTP as SFTP Module
participant Local as Local Filesystem
participant Parser as YesBiz Parser
participant Report as Settlement Report
CLI->>SFTP: fetchSettlementFile(config with privateKeyPath)
alt Key-Based Auth
SFTP->>SFTP: fetchWithKey (sftp -i identity_file)
else Password Auth
SFTP->>SFTP: fetchWithPassword (sshpass + scp)
end
SFTP->>Local: Download remote file to localPath
SFTP->>Local: Read localPath contents
SFTP->>Local: Remove temporary file
SFTP-->>CLI: Return file contents Right
CLI->>Parser: parsePaymentSettlementCsv (YesBiz routing)
Parser->>Parser: parseYesBizCsv (decode CSV, extract rows)
loop For each YesBizRow
Parser->>Parser: parseYesBizRow (normalize types, amounts, dates)
Parser->>Report: Convert to PaymentSettlementReport
end
Parser-->>CLI: Return ParseResult with reports and errors
CLI->>Local: Write parsed output to output/yesbiz/...
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate 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: 3
🧹 Nitpick comments (4)
lib/mobility-core/src/Kernel/External/Settlement/YesBiz/PaymentParser.hs (1)
108-112:parseAmountsilently returns 0 on parse failure, potentially masking data issues.If a CSV contains malformed amount values (e.g., "N/A", currency symbols), they'll be silently converted to 0, which could cause incorrect settlement calculations.
Consider logging a warning or propagating the parse failure to the row-level errors.
♻️ Alternative: Return Either to surface parse failures
parseAmount' :: Text -> Either Text HighPrecMoney parseAmount' t = case readMaybe (T.unpack $ T.strip t) of Just v -> Right v Nothing -> Left $ "Invalid amount: " <> T.strip t🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/mobility-core/src/Kernel/External/Settlement/YesBiz/PaymentParser.hs` around lines 108 - 112, parseAmount currently swallows parse failures by returning 0 which can mask bad CSV data; change parseAmount to surface failures (e.g., return Either Text HighPrecMoney or Maybe HighPrecMoney) instead of defaulting to 0, update callers to handle and propagate the Left/Nothing as a row-level error (or log a warning including the raw Text) so malformed values (like "N/A" or currency symbols) are not silently treated as zero; refer to the parseAmount function and any call sites that consume its result to implement the propagation/logging changes.lib/mobility-core/src/Kernel/External/Settlement/Sources/SFTP.hs (1)
45-50: Minor: Temp file may remain ifreadFilefails.If
LBS.readFile localPaththrows an exception, theremoveFilecleanup won't execute. Consider usingbracketor a finally pattern for cleanup.♻️ Suggested improvement
case result of Right _ -> do - contents <- liftIO $ LBS.readFile localPath - liftIO $ removeFile localPath - pure $ Right contents + liftIO $ do + contents <- LBS.readFile localPath `finally` removeFile localPath + pure $ Right contents Left err -> pure $ Left errThis requires importing
Control.Exception (finally).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/mobility-core/src/Kernel/External/Settlement/Sources/SFTP.hs` around lines 45 - 50, The Right branch of the case on result reads the temp file with LBS.readFile localPath then removes it, but if readFile throws the temp file won't be deleted; change the Right branch to ensure cleanup always runs by wrapping the read+remove sequence with a finally or bracket (import Control.Exception (finally)) so removeFile localPath is executed even on exceptions—i.e., use finally (or bracket) around LBS.readFile localPath to guarantee removeFile localPath is called and return the file contents as Right when successful.lib/mobility-core/src/Kernel/External/Settlement/YesBiz/PaymentTypes.hs (1)
71-95: Consider deriving ToJSON instead of manual implementation.The manual
ToJSONinstance duplicates the field names. Since the record field names already use camelCase (matching JSON conventions), you could derive it:♻️ Suggested simplification
data YesBizRow = YesBizRow { merchantId :: Text, ... } - deriving (Show, Eq, Generic) + deriving (Show, Eq, Generic) + deriving anyclass (A.ToJSON) - -instance A.ToJSON YesBizRow where - toJSON row = - A.object - [ "merchantId" A..= row.merchantId, - ... - ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/mobility-core/src/Kernel/External/Settlement/YesBiz/PaymentTypes.hs` around lines 71 - 95, The manual A.ToJSON instance for YesBizRow duplicates every record field; remove that instance and derive the JSON instance instead by adding Generic to the YesBizRow type (ensure GHC.Generics is imported) and deriving A.ToJSON (or use DeriveAnyClass/DerivingStrategies as your style requires), then delete the explicit instance ToJSON YesBizRow and rely on the derived implementation for toJSON.test-settlement-parser/src/Main.hs (1)
217-222:extractSftpFileListrelies on fragile parsing ofsftp lsoutput.The function uses
last . wordswhich can fail on empty lines or unusual output formats. The filter for entries with.in the name is a reasonable heuristic but may miss edge cases.This is acceptable for a test harness but worth noting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-settlement-parser/src/Main.hs` around lines 217 - 222, The current extractSftpFileList function uses unsafe last . words which can crash or mis-parse unusual/empty lines; replace that with a safe extraction: for each line (excluding those starting with "sftp>" or blank) split into words and take the last element only if the list of words is non-empty (e.g., via a helper safeLast or listToMaybe . reverse and mapMaybe), then keep the existing '.' filter; update the function extractSftpFileList to use this safe parsing approach so it never calls last on an empty list and more robustly ignores malformed lines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/mobility-core/src/Kernel/External/Settlement/YesBiz/PaymentParser.hs`:
- Around line 149-156: parseTxnStatus fails on lowercase values because it only
strips whitespace; update it to normalize case like parseTxnType does (e.g.,
apply T.toUpper to T.strip t) before pattern matching so inputs like "success"
or "failed" are handled; keep the same branches (matching "00", "0", "SUCCESS",
"FAILED") after normalization and return the same Right SUCCESS/FAILED or Left
on unknown values in the parseTxnStatus function.
In `@test-settlement-parser/src/Main.hs`:
- Around line 130-132: The current parsing of userAtHost sets both sftpUser and
sftpHost to the full string when there is no '@', producing misleading config;
update the logic around userAtHost (where sftpUser and sftpHost are derived) to
validate the format and fail fast: if break (== '@') userAtHost does not yield a
proper (user, '@':host) pair then produce a clear IO error (e.g., using error or
fail in IO with a descriptive message) or return an explicit validation failure
instead of silently using the same value for both sftpUser and sftpHost.
- Around line 27-32: The code uses read portStr in the command-line branch
(inside the Main.hs match that calls parsePG and testSftpFetch) which will throw
on non-numeric input; change this to use readMaybe (e.g., import
Text.Read.readMaybe) to parse portStr safely, handle the Nothing case by
returning a clear error message or exiting gracefully, and pass the successfully
parsed Int to testSftpFetch; update both places that call testSftpFetch (the two
patterns that bind portStr) so that testSftpFetch always receives a validated
Int and invalid input is reported to the user instead of crashing.
---
Nitpick comments:
In `@lib/mobility-core/src/Kernel/External/Settlement/Sources/SFTP.hs`:
- Around line 45-50: The Right branch of the case on result reads the temp file
with LBS.readFile localPath then removes it, but if readFile throws the temp
file won't be deleted; change the Right branch to ensure cleanup always runs by
wrapping the read+remove sequence with a finally or bracket (import
Control.Exception (finally)) so removeFile localPath is executed even on
exceptions—i.e., use finally (or bracket) around LBS.readFile localPath to
guarantee removeFile localPath is called and return the file contents as Right
when successful.
In `@lib/mobility-core/src/Kernel/External/Settlement/YesBiz/PaymentParser.hs`:
- Around line 108-112: parseAmount currently swallows parse failures by
returning 0 which can mask bad CSV data; change parseAmount to surface failures
(e.g., return Either Text HighPrecMoney or Maybe HighPrecMoney) instead of
defaulting to 0, update callers to handle and propagate the Left/Nothing as a
row-level error (or log a warning including the raw Text) so malformed values
(like "N/A" or currency symbols) are not silently treated as zero; refer to the
parseAmount function and any call sites that consume its result to implement the
propagation/logging changes.
In `@lib/mobility-core/src/Kernel/External/Settlement/YesBiz/PaymentTypes.hs`:
- Around line 71-95: The manual A.ToJSON instance for YesBizRow duplicates every
record field; remove that instance and derive the JSON instance instead by
adding Generic to the YesBizRow type (ensure GHC.Generics is imported) and
deriving A.ToJSON (or use DeriveAnyClass/DerivingStrategies as your style
requires), then delete the explicit instance ToJSON YesBizRow and rely on the
derived implementation for toJSON.
In `@test-settlement-parser/src/Main.hs`:
- Around line 217-222: The current extractSftpFileList function uses unsafe last
. words which can crash or mis-parse unusual/empty lines; replace that with a
safe extraction: for each line (excluding those starting with "sftp>" or blank)
split into words and take the last element only if the list of words is
non-empty (e.g., via a helper safeLast or listToMaybe . reverse and mapMaybe),
then keep the existing '.' filter; update the function extractSftpFileList to
use this safe parsing approach so it never calls last on an empty list and more
robustly ignores malformed lines.
🪄 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: c1ecb431-e1d5-4853-aa68-29f12aedbb5b
📒 Files selected for processing (8)
lib/mobility-core/mobility-core.caballib/mobility-core/src/Kernel/External/Settlement/Interface.hslib/mobility-core/src/Kernel/External/Settlement/Sources/SFTP.hslib/mobility-core/src/Kernel/External/Settlement/Types.hslib/mobility-core/src/Kernel/External/Settlement/YesBiz/PaymentParser.hslib/mobility-core/src/Kernel/External/Settlement/YesBiz/PaymentTypes.hstest-settlement-parser/src/Main.hstest-settlement-parser/test-settlement-parser.cabal
Comment on lines
+149
to
+156
| -- | Status "00" means success for YesBiz UPI | ||
| parseTxnStatus :: Text -> Either Text TxnStatus | ||
| parseTxnStatus t = case T.strip t of | ||
| "00" -> Right SUCCESS | ||
| "0" -> Right SUCCESS | ||
| "SUCCESS" -> Right SUCCESS | ||
| "FAILED" -> Right FAILED | ||
| other -> Left $ "Unknown transaction status: " <> other |
There was a problem hiding this comment.
Inconsistent case handling: parseTxnStatus doesn't normalize case, but parseTxnType does.
parseTxnType uses T.toUpper before matching, but parseTxnStatus only uses T.strip. If the CSV contains "success" (lowercase), it will fail.
🔧 Suggested fix for consistency
parseTxnStatus :: Text -> Either Text TxnStatus
-parseTxnStatus t = case T.strip t of
+parseTxnStatus t = case T.toUpper (T.strip t) of
"00" -> Right SUCCESS
"0" -> Right SUCCESS
"SUCCESS" -> Right SUCCESS
"FAILED" -> Right FAILED
other -> Left $ "Unknown transaction status: " <> other🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/mobility-core/src/Kernel/External/Settlement/YesBiz/PaymentParser.hs`
around lines 149 - 156, parseTxnStatus fails on lowercase values because it only
strips whitespace; update it to normalize case like parseTxnType does (e.g.,
apply T.toUpper to T.strip t) before pattern matching so inputs like "success"
or "failed" are handled; keep the same branches (matching "00", "0", "SUCCESS",
"FAILED") after normalization and return the same Right SUCCESS/FAILED or Left
on unknown values in the parseTxnStatus function.
Comment on lines
+27
to
+32
| ["sftp", pgStr, keyPath, userAtHost, remotePath, portStr] -> do | ||
| pg <- parsePG pgStr | ||
| testSftpFetch pg keyPath userAtHost (read portStr) remotePath Nothing | ||
| ["sftp", pgStr, keyPath, userAtHost, remotePath, portStr, pattern_] -> do | ||
| pg <- parsePG pgStr | ||
| testSftpFetch pg keyPath userAtHost (read portStr) remotePath (Just pattern_) |
There was a problem hiding this comment.
read portStr can throw a runtime exception on invalid input.
If the user passes a non-numeric port argument, read will throw a parse exception. Consider using readMaybe with a fallback or error message.
🛡️ Suggested fix
+import Text.Read (readMaybe)
+
+parsePort :: String -> IO Int
+parsePort s = case readMaybe s of
+ Just p -> pure p
+ Nothing -> do
+ TIO.putStrLn $ "Invalid port number: " <> T.pack s
+ exitFailure
+
-- Then in main:
- ["sftp", pgStr, keyPath, userAtHost, remotePath, portStr] -> do
- pg <- parsePG pgStr
- testSftpFetch pg keyPath userAtHost (read portStr) remotePath Nothing
+ ["sftp", pgStr, keyPath, userAtHost, remotePath, portStr] -> do
+ pg <- parsePG pgStr
+ port <- parsePort portStr
+ testSftpFetch pg keyPath userAtHost port remotePath Nothing🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test-settlement-parser/src/Main.hs` around lines 27 - 32, The code uses read
portStr in the command-line branch (inside the Main.hs match that calls parsePG
and testSftpFetch) which will throw on non-numeric input; change this to use
readMaybe (e.g., import Text.Read.readMaybe) to parse portStr safely, handle the
Nothing case by returning a clear error message or exiting gracefully, and pass
the successfully parsed Int to testSftpFetch; update both places that call
testSftpFetch (the two patterns that bind portStr) so that testSftpFetch always
receives a validated Int and invalid input is reported to the user instead of
crashing.
Comment on lines
+130
to
+132
| let (sftpUser, sftpHost) = case break (== '@') userAtHost of | ||
| (u, '@' : h) -> (T.pack u, T.pack h) | ||
| _ -> (T.pack userAtHost, T.pack userAtHost) |
There was a problem hiding this comment.
Edge case: Invalid user@host format results in misleading config.
If the input lacks @, both sftpUser and sftpHost are set to the full string, which will cause SFTP to fail with a confusing error. Consider validating the format.
🛡️ Suggested fix
let (sftpUser, sftpHost) = case break (== '@') userAtHost of
(u, '@' : h) -> (T.pack u, T.pack h)
- _ -> (T.pack userAtHost, T.pack userAtHost)
+ _ -> error $ "Invalid user@host format: " <> userAtHost <> ". Expected format: user@hostname"Or better, return an IO error with a clear message.
📝 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
| let (sftpUser, sftpHost) = case break (== '@') userAtHost of | |
| (u, '@' : h) -> (T.pack u, T.pack h) | |
| _ -> (T.pack userAtHost, T.pack userAtHost) | |
| let (sftpUser, sftpHost) = case break (== '@') userAtHost of | |
| (u, '@' : h) -> (T.pack u, T.pack h) | |
| _ -> error $ "Invalid user@host format: " <> userAtHost <> ". Expected format: user@hostname" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test-settlement-parser/src/Main.hs` around lines 130 - 132, The current
parsing of userAtHost sets both sftpUser and sftpHost to the full string when
there is no '@', producing misleading config; update the logic around userAtHost
(where sftpUser and sftpHost are derived) to validate the format and fail fast:
if break (== '@') userAtHost does not yield a proper (user, '@':host) pair then
produce a clear IO error (e.g., using error or fail in IO with a descriptive
message) or return an explicit validation failure instead of silently using the
same value for both sftpUser and sftpHost.
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
1 participant
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?
Checklist
./dev/format-all-files.shSummary by CodeRabbit
New Features
Tests