feat: add Pasargad/Wepod bank parser and tests#591
Conversation
| if (pureNumeric.length >= 4) { | ||
| return pureNumeric.takeLast(4) | ||
| } | ||
| return fullAccount |
There was a problem hiding this comment.
Inconsistent fallback in
extractAccountLast4: when pureNumeric.length < 4 the method returns fullAccount — a dot-separated string like "1.2.3" — rather than null. The method contract is "last 4 digits", so returning a non-numeric string with dots breaks callers that expect either digits-only or null.
| if (pureNumeric.length >= 4) { | |
| return pureNumeric.takeLast(4) | |
| } | |
| return fullAccount | |
| if (pureNumeric.length >= 4) { | |
| return pureNumeric.takeLast(4) | |
| } | |
| return null |
| override fun isTransactionMessage(message: String): Boolean { | ||
| return pattern.containsMatchIn(message.trim()) | ||
| } |
There was a problem hiding this comment.
OTP/promo filter not inherited
BaseIranianBankParser.isTransactionMessage explicitly rejects messages containing OTP keywords ("otp", رمز یکبار مصرف, کد تایید) and promotional phrases before checking transaction keywords. The Pasargad override bypasses all of those guards. The pattern is specific enough that a false positive today is unlikely, but if Pasargad ever bundles OTP context alongside the numeric balance line (some banks do), the parser would treat it as a transaction. Consider running the base OTP/promo rejections first via a super call before the pattern check.
|
|
||
| return ignoredMessages.map { msg -> | ||
| DynamicTest.dynamicTest("Should ignore: ${msg.take(30)}...") { | ||
| org.junit.jupiter.api.Assertions.assertNull(parser.parse(msg, "B.Pasargad", System.currentTimeMillis())) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @TestFactory | ||
| fun `factory resolves pasargad bank`(): List<DynamicTest> { | ||
| val cases = listOf( | ||
| SimpleTestCase( | ||
| bankName = "Pasargad Bank", | ||
| sender = "B.Pasargad", |
There was a problem hiding this comment.
Ignore-test messages trivially non-matching
Both messages in the ignore test ("OTP verification" and "Your OTP code is 12345") don't contain مانده: or any signed numeric amount, so they would never match the parser's regex regardless. The test isn't actually verifying the OTP-rejection path — it only confirms that random non-Pasargad strings don't parse. A more meaningful case would be an OTP-style SMS from "B.PASARGAD" that happens to include a balance line, to confirm the parser handles that edge case correctly.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
Thanks for this, @rafi-ghanbari — parser and tests look solid on the happy path. 🙏 Greptile flagged three small hardening points I'd like tightened before merge (all minor, none blocking the core logic):
Once those are addressed it's good to go. Appreciate the contribution! |
|
Thanks for the parser + tests! The automated review flagged a few things to tighten up before merge:
Could you address these, especially the OTP/promo filtering + a real negative test? |
Summary
Adds support for parsing SMS transaction notifications from Pasargad / Wepod Bank (Iran).
Type of change
How to test
Run the Pasargad Bank parser unit tests to verify parsing rules:
./gradlew :parser-core:jvmTest --tests "com.pennywiseai.parser.core.bank.PasargadBankParserTest"