Enhance FraudFundsTransferRequest with optional fields and validation…#414
Conversation
… logic - Added optional fields: clabe, bank_code, and tipo_pago. - Implemented validation to ensure either clabe or bank_code is provided, and tipo_pago is required when bank_code is used. - Updated tests to cover new validation scenarios and parameterized input for better coverage.
|
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 (1)
Walkthrough
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #414 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 16 16
Lines 1490 1502 +12
=========================================
+ Hits 1490 1502 +12
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
![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
🧹 Nitpick comments (1)
tests/test_types.py (1)
720-734: ⚡ Quick winAdd invalid cases for blank
bank_codeto lock the new contract.Current invalid coverage misses
bank_code=''/ whitespace-only, which should fail under the “destination required” rule. Adding these cases will prevent regressions once the model validator is tightened.Suggested test additions
`@pytest.mark.parametrize`( 'data, expected_error', [ (dict(user_id='US123'), 'clabe or bank_code required'), + ( + dict(user_id='US123', bank_code=''), + 'clabe or bank_code required', + ), + ( + dict(user_id='US123', bank_code=' '), + 'clabe or bank_code required', + ), ( dict(user_id='US123', bank_code='40012'), 'tipo_pago required when using bank_code', ), ], )🤖 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 `@tests/test_types.py` around lines 720 - 734, Test coverage misses blank/whitespace-only bank_code cases: update the param list in test_fraud_funds_transfer_request_invalid to include entries for dict(user_id='US123', bank_code='') and dict(user_id='US123', bank_code=' ') with expected_error 'clabe or bank_code required' so FraudFundsTransferRequest validation (the model used in test_fraud_funds_transfer_request_invalid) rejects empty/whitespace bank_code 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 `@cuenca_validations/types/requests.py`:
- Around line 330-340: validate_destination currently treats empty or
whitespace-only bank_code as present because it only checks for None; update the
validation to consider bank_code missing when it's None or blank/whitespace by
checking trimmed/string truthiness (e.g., treat bank_code as provided only if
bank_code and bank_code.strip()); use that same presence logic for both the
clabe-or-bank_code requirement and the tipo_pago dependency so that an empty
bank_code does not bypass the "clabe or bank_code required" rule and does not
trigger the tipo_pago-required branch incorrectly.
---
Nitpick comments:
In `@tests/test_types.py`:
- Around line 720-734: Test coverage misses blank/whitespace-only bank_code
cases: update the param list in test_fraud_funds_transfer_request_invalid to
include entries for dict(user_id='US123', bank_code='') and
dict(user_id='US123', bank_code=' ') with expected_error 'clabe or bank_code
required' so FraudFundsTransferRequest validation (the model used in
test_fraud_funds_transfer_request_invalid) rejects empty/whitespace bank_code
values.
🪄 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: 497b4363-fe43-4a81-b457-71dce0dbb1d0
📒 Files selected for processing (3)
cuenca_validations/types/requests.pycuenca_validations/version.pytests/test_types.py
- Bumped version to 2.1.38.dev1. - Introduced FraudFundsTransferTipoPago enum for payment types. - Updated FraudFundsTransferRequest to use FraudFundsTransferTipoPago for tipo_pago field. - Modified tests to validate new tipo_pago values.
- Introduced BankCode as a validated type for bank codes. - Implemented validation logic to ensure bank codes are valid. - Updated FraudFundsTransferRequest to use BankCode for the bank_code field. - Enhanced tests to cover invalid bank code scenarios.
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 `@cuenca_validations/types/general.py`:
- Line 5: The current import of BANK_NAMES and membership check in
validate_bank_code is using a non-public symbol; replace the usage with
clabe.get_bank_name(bank_code) instead: import get_bank_name (or call
clabe.get_bank_name) and call it inside validate_bank_code, treat any
falsy/exception result as invalid and raise the same validation error message
("Not a valid bank code"); ensure you remove the BANK_NAMES import and validate
that bank_code is the expected 3-digit code before calling get_bank_name to
match clabe's API.
🪄 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: a9821e9f-dcb7-40b7-9bf5-e2fd86757deb
📒 Files selected for processing (5)
cuenca_validations/types/__init__.pycuenca_validations/types/general.pycuenca_validations/types/requests.pycuenca_validations/version.pytests/test_types.py
🚧 Files skipped from review as they are similar to previous changes (3)
- cuenca_validations/version.py
- cuenca_validations/types/requests.py
- tests/test_types.py
…um and update related tests to reflect the changes in valid payment types.
| clabe: Clabe | ||
| clabe: Optional[Clabe] = None | ||
| bank_code: Optional[BankCode] = None | ||
| tipo_pago: Optional[FraudFundsTransferTipoPago] = None |
There was a problem hiding this comment.
Quitar el campo tipo_pago y el enum.
No tiene sentido enviarlo desde el cliente
| @@ -1 +1 @@ | |||
| __version__ = '2.1.37' | |||
| __version__ = '2.1.38.dev2' | |||
| @pytest.mark.parametrize( | ||
| 'data, expected_dump', | ||
| [ | ||
| ( | ||
| dict( | ||
| user_id='US123', | ||
| clabe='646180157098510917', | ||
| amount=10000, | ||
| concepto=' Devolución fraude ', | ||
| ), | ||
| dict( | ||
| user_id='US123', | ||
| clabe='646180157098510917', | ||
| amount=10000, | ||
| concepto='Devolución fraude', | ||
| ), | ||
| ), | ||
| ( | ||
| dict(user_id='US123', clabe='646180157098510917'), | ||
| dict(user_id='US123', clabe='646180157098510917'), | ||
| ), | ||
| ( | ||
| dict( | ||
| user_id='US123', | ||
| bank_code='40012', | ||
| tipo_pago=7, | ||
| amount=5000, | ||
| ), | ||
| dict( | ||
| user_id='US123', | ||
| bank_code='40012', | ||
| tipo_pago=7, | ||
| amount=5000, | ||
| ), | ||
| ), | ||
| ], | ||
| ) | ||
| def test_fraud_funds_transfer_request(data, expected_dump): | ||
| request = FraudFundsTransferRequest(**data) | ||
| assert request.model_dump() == expected_dump |
There was a problem hiding this comment.
Este test esta de sobra, estas validando que pydantic sirva
| ), | ||
| ], | ||
| ) | ||
| def test_fraud_funds_transfer_request_invalid(data, expected_error): |
There was a problem hiding this comment.
Deja solo un caso simple, sin bank_code ni clabe y borra el resto
- Bumped version to 2.1.38. - Removed FraudFundsTransferTipoPago enum and its references from the codebase. - Updated tests to reflect changes in the FraudFundsTransferRequest validation logic.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_types.py (1)
678-691: ⚡ Quick winConsider adding test coverage for the
clabefield.While the new
bank_codetests are well-structured and cover the key validation paths, the rewrite has removed all test coverage for theclabefield. Sinceclaberemains a valid way to specify the destination (per the validator inrequests.py:328-339), consider adding at least one test case to verify backward compatibility.For example:
def test_fraud_funds_transfer_request(): # Test with bank_code assert FraudFundsTransferRequest( user_id='US123', bank_code='40012', ).model_dump() == {'user_id': 'US123', 'bank_code': '40012'} # Test with clabe (backward compatibility) assert FraudFundsTransferRequest( user_id='US123', clabe='646180157098510917', ).model_dump() == {'user_id': 'US123', 'clabe': '646180157098510917'} # Test missing both fields with pytest.raises(ValidationError) as exc: FraudFundsTransferRequest(user_id='US123') assert 'clabe or bank_code required' in str(exc.value) # Test invalid bank_code with pytest.raises(ValidationError) as exc: FraudFundsTransferRequest(user_id='US123', bank_code='99999') assert 'Not a valid bank code' in str(exc.value)Note: I see past review comments suggesting simplification, but given that this PR introduces dual-destination support, comprehensive coverage of both paths seems appropriate.
🤖 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 `@tests/test_types.py` around lines 678 - 691, The test_fraud_funds_transfer_request function lacks coverage for the clabe field, which is still a valid destination option according to the validator in requests.py. Add a test case after the initial bank_code test that verifies FraudFundsTransferRequest correctly accepts and preserves the clabe field (e.g., with a valid clabe value like '646180157098510917'), ensuring backward compatibility is maintained alongside the new bank_code functionality.
🤖 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.
Nitpick comments:
In `@tests/test_types.py`:
- Around line 678-691: The test_fraud_funds_transfer_request function lacks
coverage for the clabe field, which is still a valid destination option
according to the validator in requests.py. Add a test case after the initial
bank_code test that verifies FraudFundsTransferRequest correctly accepts and
preserves the clabe field (e.g., with a valid clabe value like
'646180157098510917'), ensuring backward compatibility is maintained alongside
the new bank_code functionality.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9d6b9821-e5e4-43f9-9580-15d64b0548bb
📒 Files selected for processing (4)
cuenca_validations/types/__init__.pycuenca_validations/types/requests.pycuenca_validations/version.pytests/test_types.py
💤 Files with no reviewable changes (2)
- cuenca_validations/types/init.py
- cuenca_validations/types/requests.py
✅ Files skipped from review due to trivial changes (1)
- cuenca_validations/version.py
| user_id: NonEmptyStr | ||
| clabe: Clabe | ||
| clabe: Optional[Clabe] = None | ||
| bank_code: Optional[BankCode] = None |
There was a problem hiding this comment.
Borra el type BankCode
Dejalo así bank_code:str y que tenga el validator este model
- Removed the BankCode type and its validation logic from the codebase. - Updated FraudFundsTransferRequest to use a simple string for bank_code. - Implemented a new field validator for bank_code to ensure it matches valid bank names. - Adjusted related imports and cleaned up unused code in general.py and __init__.py.
2 participants
… logic
Summary by CodeRabbit
Release Notes
New Features
Tests