-
Notifications
You must be signed in to change notification settings - Fork 71
Don't check for truthy amount values when creating ChargesRecord objects #170
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the conditionals when creating ChargesRecord objects to allow zero-value amounts to be processed correctly. Key changes include:
- Removing explicit truthiness checks on amount values in XML parsing.
- Updating conditionals in both Record.php and EntryTransactionDetail.php to always process zero amounts.
- Adding test data files to verify the correct handling of zero charges.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
File | Description |
---|---|
test/data/camt052.v6.Amt-zero.xml | Added XML test data with zero amounts for charges |
test/data/camt052.v6.Amt-zero.json | Added JSON test data with zero amounts for charges |
src/Decoder/Record.php | Removed truthy checks to correctly handle zero amount values |
src/Decoder/EntryTransactionDetail.php | Removed truthy checks to correctly handle zero amount values |
Comments suppressed due to low confidence (4)
src/Decoder/Record.php:201
- Removing the explicit truthy check ensures that zero values are processed correctly. Please confirm that the money factory handles zero amounts as expected.
if (isset($xmlEntry->Chrgs->TtlChrgsAndTaxAmt)) {
src/Decoder/Record.php:212
- The removal of the truthy check for the Amt element correctly captures cases when the amount is zero. Verify that this change does not introduce unintended side effects in amount processing.
if (isset($chargesRecord->Amt)) {
src/Decoder/EntryTransactionDetail.php:311
- Adjusting the condition to remove the truthy check ensures that zero charges are handled without omission. Please ensure the money creation logic supports zero values.
if (isset($xmlDetail->Chrgs->TtlChrgsAndTaxAmt)) {
src/Decoder/EntryTransactionDetail.php:323
- The update to the conditional removes the false exclusion of zero amounts, improving correctness. Confirm that this change is aligned with the intended business logic for charge records.
if (isset($chargesRecord->Amt)) {
When parsing an XML-File which contains an amount of a ChargesRecord that has "0" as its value, the amount was not being set on the ChargesRecord object. Example: ```xml <Chrgs> <Rcrd> <Amt Ccy="CHF">0</Amt> ... </Rcd> </Crgs> ``` Now, if the amount is available in XML, then it will be set on the ChargesRecord object, even with a value of 0. Correct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request updates the logic for creating ChargesRecord objects by removing truthy checks on amount values so that zero amounts are now handled properly. In addition, the tests and assertions have been updated accordingly.
- Updated XML and JSON test fixtures to include zero amount values
- Modified conditions in the Decoder to process zero amounts correctly
- Refactored assertions in RegressionTest for improved clarity
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
File | Description |
---|---|
test/data/camt052.v6.Amt-zero.xml | New XML test fixture for zero amounts |
test/data/camt052.v6.Amt-zero.json | New JSON test fixture for zero amounts |
test/Unit/RegressionTest.php | Updated assertion method and logging in tests |
src/Decoder/Record.php | Removed truthy checks in amount processing for Charges and ChargesRecord |
src/Decoder/EntryTransactionDetail.php | Removed truthy checks in amount processing for Charges and ChargesRecord |
Thank for your patience. Next time consider adding unit test with you change, so it can be processed faster. |
Resolves #169