kputnam / stupidedi

Ruby API for parsing and generating ASC X12 EDI transactions
BSD 3-Clause "New" or "Revised" License
265 stars 137 forks source link

Inconsistent floating point output in 834 ICM data #267

Closed casconed closed 11 months ago

casconed commented 11 months ago

Noticing that the BigDecimal conversion (I think) is not maintaining the precision of floating-point values passed into ICM segments, even when passed as strings. Minimal example below:

Example Code ```ruby require "stupidedi" b = Stupidedi::Parser::BuilderDsl.build(Stupidedi::Config.hipaa) b.ISA("00", "", "00", "", "30", "SUBMITTER_ID", 30, "RECEIVER_ID", Time.now, Time.now, "=", "00501", "5555", "0", "P", ">") b.GS("BE", "SENDER", "RECEIVER", Time.now, Time.now, "5555", "X", "005010X220A1") b.ST("834", "100000000", "005010X220A1") b.BGN("00", "23000", Time.now, Time.now, nil, "2300", nil, 4) b.N1("P5", "SENDER", "FI", "SENDER_ID") b.N1("IN", "SENDER", "FI", "SENDER_ID") b.N1("TV", "SENDER", "FI", "SENDER_ID") b.INS("Y", "18", "030", "","A") b.DTP("336", "D8", "20231001") b.NM1("IL", "1", "LAST", "FIRST") b.DMG("D8", "20120501", "M") b.ICM("H", "50.00", 40) b.machine.zipper.tap do |z| separators = Stupidedi::Reader::Separators.build(:segment => "~\n", :element=> "*") Stupidedi::Writer::Default.new(z.root, separators).write($stdout) end ```

If you update the values in the ICM segment, you can see the output varies. 50.00 outputs 50, 49.90 outputs 49.9, and 49.95 outputs 49.95.

kputnam commented 11 months ago

Hi @casconed, thanks for the report

I believe this is expected behavior, as whomever reads the file knows the first N digits are the "whole number" portion and the remaining are the decimal. There is no requirement to include trailing zeros. Is your trading partner reporting an error or rejecting the file?

I will take a closer look at it this week. Can you check what happens when you parse this example input, with edi-pp or handwritten code? I expect you will get back the correct decimal value.

If the element was declared with a minimum length, you should see zeros added to meet that length.

casconed commented 11 months ago

The only real issue I'm running into with this is getting adequate test coverage around the output. Because what I'm putting in doesn't necessarily match what is coming out i'm struggling to write a test that won't break when I change the input.

kputnam commented 11 months ago

I see, you're wanting to write a unit test that the data you're emitting is correct? My recommendation is to write the assertion against the parse tree rather than the serialized output.

One relatively simple way to do this is to use the have_structure matcher used in stupidedi's specs. Here is an example of its use. This asserts that the structure of the generated document has a certain structure, and you can narrow it down to elements, eg S(:ICM, "H", 50) would assert that you could find a matching segment starting from the parent element. Here are the definitions from the helper.

The element search constraints documented here can be used, so nil is a wildcard match:

Multiple constraints can be specified and #blank or nil should be used to indicate a wildcard, if needed. For instance to find the NM1*PR "Payer Name" that has a certain organization name in element NM103,

machine.find(:NM1, "PR", nil, "MEDICARE")
casconed commented 11 months ago

ahhh nice yes. Right now I am asserting on the string output, but this feels way cleaner.

kputnam commented 11 months ago

Ok, I'll close this as resolved for now. Please reopen if you find a problem though!

casconed commented 9 months ago

@kputnam digging into this finally. If I want to validate the inclusion of a single segment (say a REF*PID*Mandatory) can i use have_structure with just that? Or do I have to detail the entire document? I'm getting segment REF*PID*Mandatory~ cannot be reached when using isa.find (isa = parser.parent.fetch). have_structure fails similarly

kputnam commented 9 months ago

You don't have to list out the entire document, but do need to specify a path to that segment starting from ISA. So for a hypothetical grammer that has a structure like:

Interchange
ISA
  Functional Group
  GS
    Transaction Set
    ST

      Header Loop
      NM1
      ...

      Detail Loop
      PO1
      ...

          Line Item Loop
          LIN
          REF

...

Then you need to specify GS, ST, PO1, LIN, REF. Another way of thinking about it is that you're basically describing the arguments to StateMachine#sequence when the parser is starting from ISA.

casconed commented 9 months ago

Awesome thanks I will give it a shot!

kputnam commented 9 months ago

My original commit skipped over LIN, which is required to move from the detail loop into the line item loop (I updated my comment for anyone who stumbles across this issue).

This document explains how to traverse the parse tree, and why you get errors like "XYZ can't be reached". Unfortunately GitHub won't render the images because they are relative links -- I'm mentioning this because you won't even see broken image icons, so maybe look at the files locally or just open the images separately.