moov-io / fincen

Fincen BSA E-Filing forms
https://moov-io.github.io/fincen/
Apache License 2.0
20 stars 7 forks source link

fix: float64 with 7 digits are printing out with scientific notation #90

Closed mimy2001 closed 9 months ago

mimy2001 commented 9 months ago

Two issues that this PR deals with

  1. We had a filing which was $1,252,363.25 and this produced scientific notation looking like 1.252364e+06 which fincen doesn't accept. See image attached and code looking like: <fc2:EFilingBatchXML TotalAmount="1.252364e+06" PartyCount="1"

image (8)

After this change it becomes: <fc2:EFilingBatchXML TotalAmount="1252363" PartyCount="1" ActivityCount="1"

  1. For SAR filings with 0 amounts, the total amount field is omitted because it's omit empty, we used to do something hacky in our code which adds this element with 0 value but taking out the omit empty tag will fix this issue as well

Our workaround looked like this: if totalAmount == 0 { fincenReport.Attrs = append( fincenReport.Attrs, xml.Attr{ Name: xml.Name{Local: "TotalAmount"}, Value: "0", }) }

codecov-commenter commented 9 months ago

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (b631573) 73.74% compared to head (e1c8787) 73.61%.

:exclamation: Current head e1c8787 differs from pull request most recent head f5d4d03. Consider uploading reports for the commit f5d4d03 to get more accurate results

Files Patch % Lines
pkg/batch/batch.go 0.00% 4 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #90 +/- ## ========================================== - Coverage 73.74% 73.61% -0.13% ========================================== Files 17 17 Lines 1699 1702 +3 ========================================== Hits 1253 1253 - Misses 387 390 +3 Partials 59 59 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

adamdecaf commented 9 months ago

Thanks for this fix! It looks like Go will use scientific notation because the XML spec supports it but obviously FinCEN doesn't. Are there other float64 fields to make this fix for too?

mimy2001 commented 9 months ago

@adamdecaf I only noticed it with total amount, but technically this could happen for any other float64 fields. Do you want me to also see if I should make this fix elsewhere as well? And that's right, go uses scientific notation due to XML, if this was json we wouldn't have an issue. Even though XML spec supports this, fincen doesn't, we tried this and it failed with errors.

adamdecaf commented 9 months ago

Yea, this change is fine. I see maybe one other spot to fix this.

@mfdeveloper508 Do you know why we copied in some code from Go's repository? Why do we have a copy of their xml marshaling? https://github.com/moov-io/fincen/blob/3fce8e831a869617e511b94f3d264ba1b03fe60c/marshal.go#L915-L916

Edit: If I remove our copy of MarshalIndent no tests fail, so why was it needed?

mimy2001 commented 9 months ago

@adamdecaf I just realized something, the fix I need is not completely correct, that number needs to round up :( I can submit another PR today when i get a chance or if you have time maybe you can. But I simply made is so it's no precision to print out the integer part of the float but that's not correct, the amount always rounds up to the next whole number no matter what from what my PM just told me. Do you want me to take this on or is that something you want to do quickly?

adamdecaf commented 9 months ago

Is the behavior to round up unique to this field? Adding a roundUpFloat type sounds like the solution to me.

mimy2001 commented 9 months ago

Just thinking this one through, I've been trying to find the CTR/SAR guide where it gives guidance on round up/round down or just truncate and I can't find definitive verbiage on that. So this could also just be dealt with client side thought by prerounding before using this field. I might be able to work around this without a change to the library. And for the record when it was using scientific notation here, it did print out rounding it up to the next dollar. Let me know if you think i should submit any code for this.

adamdecaf commented 9 months ago

If this field always needs to be rounded up we can implement that in the library. Renaming the type added in this PR to add round-up behavior works for me.