Added challan service provider#1327
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:
WalkthroughAdds a Signzy challan-search integration: service enum and domain types, Signzy config, Servant API client, interface types, Signzy implementation that filters pending challans, a dispatcher, and cabal exports. ChangesChallan Search Service
Sequence Diagram(s)sequenceDiagram
participant Client
participant getPendingChallanCount
participant signzyChallanSearch
participant challanSearchClient
participant SignzyService
Client->>getPendingChallanCount: ChallanSearchServiceConfig, PendingChallanReq
getPendingChallanCount->>signzyChallanSearch: SignzyChallanSearchCfg, PendingChallanReq
signzyChallanSearch->>signzyChallanSearch: Decrypt apiKey
signzyChallanSearch->>challanSearchClient: Build ChallanSearchReq (vehicleNumber, paymentCheck=True)
challanSearchClient->>SignzyService: HTTP POST with Authorization header
SignzyService-->>challanSearchClient: ChallanSearchResp
signzyChallanSearch->>signzyChallanSearch: Filter challanDetails where challanStatus == "pending"
signzyChallanSearch-->>getPendingChallanCount: PendingChallanResp
getPendingChallanCount-->>Client: PendingChallanResp
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
RitoBose
force-pushed
the
feat/challan/signzy
branch
from
cdbae4c to
1299767
Compare
|
@ClaudeWeb review this PR real quick. |
|
Let me fetch the PR diff, description, and comments in parallel.The PR Review: Added Challan Service Provider (#1327)Verdict: Request Changes. The integration structure is clean and follows repo conventions, but there's a likely runtime JSON parsing bug and a few other issues worth addressing before merge. 🔴 Critical1. Snake_case fields will break JSON deserialization at runtimeFile:
data ChallanSearchResult = ChallanSearchResult
{ regNo :: Text, -- camelCase
message :: Text,
status_code :: Int, -- ← snake_case
challanDetails :: [ChallanDetail],
successfulSources :: [Text],
failedSources :: [Text]
}
deriving (Show, Generic, ToJSON, FromJSON)
data ChallanDetail = ChallanDetail
{ ...
court_challan :: Maybe Text, -- ← snake_case
...
}With Fix: Rename to camelCase, or add explicit Aeson options if the API genuinely uses snake_case: -- Option A: rename to match the API's actual casing
data ChallanSearchResult = ChallanSearchResult
{ ...
statusCode :: Int,
...
}
-- Option B: if the API truly mixes cases, use explicit field mapping
instance FromJSON ChallanSearchResult where
parseJSON = withObject "ChallanSearchResult" $ \v ->
ChallanSearchResult
<$> v .: "regNo"
<*> v .: "message"
<*> v .: "status_code"
...🟡 Warning2. Case-sensitive
|
RitoBose
force-pushed
the
feat/challan/signzy
branch
from
1299767 to
60604b4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/mobility-core/src/Kernel/External/ChallanSearch/Interface/Types.hs`:
- Line 10: This interface type is deriving JSON instances (FromJSON, ToJSON)
which couples the interface to a JSON contract; remove FromJSON and ToJSON from
the deriving clause so the type only derives Show, Eq, Generic (i.e., delete the
ToJSON/FromJSON symbols from the deriving list in
lib/mobility-core/src/Kernel/External/ChallanSearch/Interface/Types.hs). Repeat
the same removal for the other interface types flagged (the other deriving
clauses at the same file locations referenced in the comment) to keep interface
types decoupled from JSON serialization.
🪄 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: 9226b87b-aba1-411a-9b10-dc0725100f87
📒 Files selected for processing (8)
lib/mobility-core/mobility-core.caballib/mobility-core/src/Kernel/External/ChallanSearch/Interface.hslib/mobility-core/src/Kernel/External/ChallanSearch/Interface/Signzy.hslib/mobility-core/src/Kernel/External/ChallanSearch/Interface/Types.hslib/mobility-core/src/Kernel/External/ChallanSearch/Signzy/API.hslib/mobility-core/src/Kernel/External/ChallanSearch/Signzy/Config.hslib/mobility-core/src/Kernel/External/ChallanSearch/Signzy/Types.hslib/mobility-core/src/Kernel/External/ChallanSearch/Types.hs
🚧 Files skipped from review as they are similar to previous changes (7)
- lib/mobility-core/src/Kernel/External/ChallanSearch/Interface.hs
- lib/mobility-core/mobility-core.cabal
- lib/mobility-core/src/Kernel/External/ChallanSearch/Types.hs
- lib/mobility-core/src/Kernel/External/ChallanSearch/Signzy/Config.hs
- lib/mobility-core/src/Kernel/External/ChallanSearch/Signzy/API.hs
- lib/mobility-core/src/Kernel/External/ChallanSearch/Signzy/Types.hs
- lib/mobility-core/src/Kernel/External/ChallanSearch/Interface/Signzy.hs
| import Kernel.Prelude | ||
|
|
||
| data ChallanSearchServiceConfig = SignzyChallanSearch Signzy.SignzyChallanSearchCfg | ||
| deriving (Show, Eq, Generic, FromJSON, ToJSON) |
There was a problem hiding this comment.
Avoid JSON instances on interface boundary types.
These interface types should stay decoupled from JSON contracts to avoid accidental UI/API coupling and backward-compatibility lock-in. Please remove ToJSON/FromJSON derivations here.
Suggested change
data ChallanSearchServiceConfig = SignzyChallanSearch Signzy.SignzyChallanSearchCfg
- deriving (Show, Eq, Generic, FromJSON, ToJSON)
+ deriving (Show, Eq, Generic)
newtype PendingChallanReq = PendingChallanReq
{ vehicleNumber :: Text
}
- deriving (Show, Generic, ToJSON, FromJSON)
+ deriving (Show, Generic)
newtype PendingChallanResp = PendingChallanResp
{ pendingChallanCount :: Int
}
- deriving (Show, Generic, ToJSON, FromJSON)
+ deriving (Show, Generic)Based on learnings, in lib/mobility-core/src/Kernel/External/Payout/Interface/Types.hs interface types intentionally do not derive ToJSON/FromJSON to prevent accidental UI usage and compatibility issues.
Also applies to: 15-15, 20-20
🤖 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/ChallanSearch/Interface/Types.hs` at
line 10, This interface type is deriving JSON instances (FromJSON, ToJSON) which
couples the interface to a JSON contract; remove FromJSON and ToJSON from the
deriving clause so the type only derives Show, Eq, Generic (i.e., delete the
ToJSON/FromJSON symbols from the deriving list in
lib/mobility-core/src/Kernel/External/ChallanSearch/Interface/Types.hs). Repeat
the same removal for the other interface types flagged (the other deriving
clauses at the same file locations referenced in the comment) to keep interface
types decoupled from JSON serialization.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
RitoBose
force-pushed
the
feat/challan/signzy
branch
from
60604b4 to
c703e08
Compare
| signzyChallanSearch :: (EncFlow m r, CoreMetrics m, Log m, HasRequestId r, MonadReader r m) => CSC.SignzyChallanSearchCfg -> CIT.PendingChallanReq -> m CIT.PendingChallanResp | ||
| signzyChallanSearch cfg req = do | ||
| apiKey <- decrypt cfg.apiKey | ||
| let signzyReq = CST.ChallanSearchReq {vehicleNumber = req.vehicleNumber, paymentCheck = True} |
There was a problem hiding this comment.
paymentCheck was not required to pass.
There was a problem hiding this comment.
Yes I tried that previously but the signzy API returns an error : {
"error": {
"name": "error",
"message": "Error in getting data",
"status": 409,
"reason": "CONFLICT",
"type": "Conflict",
"statusCode": 409
}
}
when we don't pass paymentCheck=true.
| let pendingChallans = filter isPending signzyResp.result.challanDetails | ||
| pure $ CIT.PendingChallanResp {pendingChallanCount = length pendingChallans} | ||
| where | ||
| isPending :: CST.ChallanDetail -> Bool |
There was a problem hiding this comment.
Can you add toLowerCase and then compare it with "pending" ?
| rto :: Maybe Text, | ||
| accusedName :: Maybe Text, | ||
| amount :: Maybe Text, | ||
| challanStatus :: Maybe Text, |
|
|
||
| data ChallanDetail = ChallanDetail | ||
| { number :: Maybe Int, | ||
| challanNumber :: Maybe Text, |
There was a problem hiding this comment.
Rest of the fields can be removed.
|
@RitoBose Handle the error case properly. |
RitoBose
force-pushed
the
feat/challan/signzy
branch
from
c703e08 to
64597eb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/mobility-core/src/Kernel/External/ChallanSearch/Signzy/Types.hs`:
- Around line 16-24: The type ChallanSearchResult currently derives
ToJSON/FromJSON via Generic which leaks Haskell field names; replace this with
explicit Aeson instances: implement a FromJSON and ToJSON for
ChallanSearchResult (or use Aeson.withObject and object builders) that map each
Haskell field (regNo, message, status_code, challanDetails, successfulSources,
failedSources) to the exact Signzy JSON keys expected by the provider (verify
and use their canonical keys — e.g. "regNo" vs "reg_no" or "regno", "statusCode"
vs "status_code", "challanDetails" spelling), avoid relying on Generic or
default Aeson fieldLabelModifier, and mirror the same key mapping in both
FromJSON and ToJSON to keep round-trip stability.
🪄 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: 802dd7b7-4412-42f5-8bec-d4286c3b5b26
📒 Files selected for processing (8)
lib/mobility-core/mobility-core.caballib/mobility-core/src/Kernel/External/ChallanSearch/Interface.hslib/mobility-core/src/Kernel/External/ChallanSearch/Interface/Signzy.hslib/mobility-core/src/Kernel/External/ChallanSearch/Interface/Types.hslib/mobility-core/src/Kernel/External/ChallanSearch/Signzy/API.hslib/mobility-core/src/Kernel/External/ChallanSearch/Signzy/Config.hslib/mobility-core/src/Kernel/External/ChallanSearch/Signzy/Types.hslib/mobility-core/src/Kernel/External/ChallanSearch/Types.hs
🚧 Files skipped from review as they are similar to previous changes (6)
- lib/mobility-core/src/Kernel/External/ChallanSearch/Interface/Signzy.hs
- lib/mobility-core/src/Kernel/External/ChallanSearch/Interface.hs
- lib/mobility-core/src/Kernel/External/ChallanSearch/Signzy/API.hs
- lib/mobility-core/src/Kernel/External/ChallanSearch/Interface/Types.hs
- lib/mobility-core/mobility-core.cabal
- lib/mobility-core/src/Kernel/External/ChallanSearch/Types.hs
| data ChallanSearchResult = ChallanSearchResult | ||
| { regNo :: Text, | ||
| message :: Text, | ||
| status_code :: Int, | ||
| challanDetails :: [ChallanDetail], | ||
| successfulSources :: [Text], | ||
| failedSources :: [Text] | ||
| } | ||
| deriving (Show, Generic, ToJSON, FromJSON) |
There was a problem hiding this comment.
Make Signzy JSON key mapping explicit (don’t rely on Generic field-label casing).
ChallanSearchResult in lib/mobility-core/src/Kernel/External/ChallanSearch/Signzy/Types.hs derives ToJSON/FromJSON via Generic, so Aeson keys are derived from the Haskell record labels (regNo, status_code, etc.). This is brittle for a third-party provider where casing may differ. Implement explicit FromJSON/ToJSON instances with stable Signzy field names (and avoid relying on the proposed diff’s key casing/typos like chalanDetails unless they match the actual provider payload).
🤖 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/ChallanSearch/Signzy/Types.hs` around
lines 16 - 24, The type ChallanSearchResult currently derives ToJSON/FromJSON
via Generic which leaks Haskell field names; replace this with explicit Aeson
instances: implement a FromJSON and ToJSON for ChallanSearchResult (or use
Aeson.withObject and object builders) that map each Haskell field (regNo,
message, status_code, challanDetails, successfulSources, failedSources) to the
exact Signzy JSON keys expected by the provider (verify and use their canonical
keys — e.g. "regNo" vs "reg_no" or "regno", "statusCode" vs "status_code",
"challanDetails" spelling), avoid relying on Generic or default Aeson
fieldLabelModifier, and mirror the same key mapping in both FromJSON and ToJSON
to keep round-trip stability.
1cdaa34 to
1a8983b
Compare
|
@ClaudeWeb Can you review this PR by making sure, it's implemented as per proper design principles and proper error handling is happening. |
|
Let me pull up existing similar service implementations for comparison.Let me look at comparable existing service implementations to benchmark the design.Let me look at a closely comparable existing service (Verification with Signzy) to benchmark design patterns.Let me read the actual content of the Insurance interface and a comparable API file to benchmark patterns.Let me read the Insurance interface Types and Acko implementation to see the full design pattern.Now let me check the Acko Types to see how they handle snake_case API fields — the key design question.Now I have the full picture. Here's the design-focused review: Design & Error Handling Review — PR #1327 (Challan Service Provider)Verdict: Request Changes. The module structure is correct but deviates from the established codebase pattern in three ways that matter: the adapter layer skips the conversion-function contract, the Types layer has a likely parse bug, and error handling is thin throughout. I benchmarked this against the most comparable service — 🔴 Critical — Runtime Bug1. Mixed snake_case/camelCase in
|
| # | File | Issue | Severity |
|---|---|---|---|
| 1 | Signzy/Types.hs |
status_code/court_challan snake_case breaks JSON parse |
🔴 Critical |
| 2 | Interface/Signzy.hs |
No named conversion functions — deviates from adapter pattern | 🟠 Design |
| 3 | ChallanSearch/Types.hs |
Generic JSON on enum — wrong format, no error messages | 🟠 Design |
| 4 | ChallanSearch/Types.hs |
Missing derivePersistField (check intent) |
🟠 Design |
| 5 | Interface/Signzy.hs |
isPending case-sensitive, no diagnostic logging |
🟡 Error Handling |
| 6 | Interface/Signzy.hs |
failedSources silently ignored |
🟡 Error Handling |
| 7 | Signzy/Types.hs |
No error envelope in response type | 🟡 Error Handling |
|
@ClaudeWeb Can you pull latest code and then review ? Just flag is any critical thing needs to be fixed. |
|
Fetching all the latest file contents from the PR branch in parallel.Got the latest code. Good news — several previous concerns are already addressed. Here's the focused review: Review of Latest Branch (
|
RitikaHanish
left a comment
RitikaHanish
left a comment
There was a problem hiding this comment.
One-line summary: Fix status_code → statusCode to unblock runtime; the rest are polish.
|
@ClaudeWeb the critical issue u mentioned "🔴 Critical — Must Fix data ChallanSearchResult = ChallanSearchResult Fix — rename to match the other fields: statusCode :: Int, is perhaps not applicable as the 3rd party API itself returns this response { |
|
Fair correction — the API genuinely sends However, looking at the actual API response you shared, there's a new concern worth flagging: 🟡 New Suggestion —
|
RitoBose
force-pushed
the
feat/challan/signzy
branch
from
40861bc to
d16b6b4
Compare
3 participants
Type of Change
Description
Added Challan Service Provider
Additional Changes
Motivation and Context
How did you test it?
Checklist
./dev/format-all-files.shSummary by CodeRabbit