nerdocs / pydifact

A python EDIFACT library.
MIT License
153 stars 45 forks source link

UNT element correctly implemented? #44

Closed julian-r closed 3 years ago

julian-r commented 3 years ago

It seems like the UNT element outputs the wrong order here:

https://www.truugo.com/edifact/d96a/unt/

The lib outputs: UNT+1+272'

For a message with 273 elements. Actually it should be: UNT+273+1'

Am I wrong? Is D96A a different standard?

nerdoc commented 3 years ago

Ok, I'll have a look on that. Could you provide me a demo EDIFACT file where the error occurs? UNT+1+272' is definitely wrong. So pydifact makes an error here, I suppose.

nerdoc commented 3 years ago

@julian-r: Would it be possible that you upload a file where I can test this behaviour?

nerdoc commented 3 years ago

Because when I test reading in a message:

>>> a=SegmentCollection.from_str("... ... ... UNT+24+1'UNZ+1+PAYO0012101221'")
>>> a.get_segment("UNT")
UNT segment: ['24', '1']
>>> a.serialize()
"UNB... ... ... +7+VAT'UNT+24+1'UNZ+1+PAYO0012101221'"

So UNT+24+1' is serialized correctly.

I need more info from you please.

julian-r commented 3 years ago

Thank you for looking into it. I was using the message class so:

>>> from pydifact.segmentcollection import Message, Interchange
>>> from pydifact import Segment
>>> interchange = Interchange(
...     sender=["bob"],
...     recipient=["alice"],
...     control_reference="1",
...     syntax_identifier=["UNOC", "3"],
... )
>>> message = Message('1', '1')
>>> message.add_segment(Segment("ABC", "1", "2"))
>>> message.add_segment(Segment("DAA", "2", "3"))
>>> interchange.add_message(message)
>>> interchange.serialize(True)
>>> print(interchange.serialize(True))
UNB+UNOC:3+bob+alice+211026:0935+1'
UNH+1+1'
ABC+1+2'
DAA+2+3'
UNT+1+2'
UNZ+1+4'

I was just unsure if I was correctly interpreting the standard. The problem is in https://github.com/nerdocs/pydifact/blob/7847722d296c5e3642e6bda89449f43d70550db0/pydifact/segmentcollection.py#L331-L335 . I can also create a pull request for this.

I like this library :) Greetings to Salzburg.

fabriziogozzi commented 3 years ago

The right code shoud be: (+2 because "including UNH and UNT")

Row 331 --

    def get_footer_segment(self) -> Segment:
        return Segment(
            "UNT",
            str(len(self.segments)+2),
            self.reference_number,
        )

Thi is the officia documentation: Segment: UNT, Message Trailer

Function: To end and check the completeness of a Message

 Ref.   Repr.       Name                       Remarks

 0074   n..6    M   NUMBER OF SEGMENTS IN THE  Control count
                    MESSAGE                    including UNH and UNT
 ___________________________________________________________________

 0062   an..14  M   MESSAGE REFERENCE NUMBER   Shall be identical to
                                               0062 in UNH
nerdoc commented 3 years ago

Hmmmm. yes. TBH, Segments in production should not use Segmentdirectly. The docstring says:

This class is used internally. read-world implementations of specialized should subclass 
Segment and provide the `tag` and `validate` attributes.

That's my (our, pydifact's) job in the end, I know. And ATM there is no other way to create segments, as using Segment directly.

I plan to have a segment like UNTSegment(..., ..., ...) as subclsas of Segment. But let's see what I can do here...

nerdoc commented 3 years ago

Ah, sorry, I saw (after formatting your posting @fabriziogozzi) that you use Message correctly...

nerdoc commented 3 years ago

Thanks @fabriziogozzi - this was the right hint. I had to fix a bunch of tests too, as we (me and @JocelynDelalande) repeated this error even into the tests. So pydifact always had clean test results with bad test data... ;-)

Thanks to all! I'll provide a new hotfix version later today.

JocelynDelalande commented 3 years ago

Thanks to you two :)