riesenia / pohoda

XML generator a parser pre Pohodu
35 stars 25 forks source link

Add "extId" child for OrderHeader #35

Closed XeLiatH closed 1 year ago

XeLiatH commented 1 year ago

I would like to add the extId child for orderHeader. Attempted some implementation that I tested locally and that works.

Not sure if my attempt is the "proper" way of doing it.

Adding some documentation for reference.

https://www.stormware.cz/schema/version_2/order.xsd

<xsd:element name="extId" type="typ:extIdType" minOccurs="0">
  <xsd:annotation>
    <xsd:documentation> Odkaz na záznam v externí databázi. V databázi se nachází speciální tabulka obsahující vazbu mezi agendou a externí databází. </xsd:documentation>
  </xsd:annotation>
</xsd:element>

https://www.stormware.cz/schema/version_2/type.xsd

<xsd:complexType name="extIdType">
  <xsd:all>
    <xsd:element name="ids" type="typ:string64">
      <xsd:annotation>
        <xsd:documentation>ID záznamu v externím systému, jedinečný textový identifikátor.</xsd:documentation>
      </xsd:annotation>
    </xsd:element>
    <xsd:element name="exSystemName" type="typ:string64">
      <xsd:annotation>
        <xsd:documentation>Jedinečný název externího systému (např. GUID).</xsd:documentation>
      </xsd:annotation>
    </xsd:element>
    <xsd:element name="exSystemText" type="xsd:string" minOccurs="0">
      <xsd:annotation>
        <xsd:documentation>Textový popis externího systému.</xsd:documentation>
      </xsd:annotation>
    </xsd:element>
  </xsd:all>
</xsd:complexType>
segy commented 1 year ago

I like it, but three things please:

  1. move isset($data['extId']) condition to base Document\Header constructor (it will be used also in Invoice for instance)
  2. extId is already used in Riesenia\Pohoda\Type\Address as _refElement - please switch also this one to your implementation
  3. add simple test (it_can_add_extid?) for this to OrderSpec

Thanks!

segy commented 1 year ago

and also check tests pls - https://github.com/riesenia/pohoda/runs/7917499582

XeLiatH commented 1 year ago

Thank you for the reply. I will get to it right away.

XeLiatH commented 1 year ago

After giving it quite longer thought than wanted, I went back on my implementation and use the $_refElements instead.

The difficulty I was having was that the extId element can have multiple namespaces and I could not figure out how to apply them properly.

For example you have the typ:extId whenever it is withing typ:address, but then you can also have ord:extId or inv:extId when it is in their respective headers.

I was and actually still am unsure of when to use the $_refElements and when to use the custom class for defining the type. However, right now with this approach I feel more comfortable, because it works as expected with less code.

Also I did not see any tests for the extId in Address.php, so I did not add any either. Let me know if I should.

Sorry if this is too long, just sharing my thought process 😅