Skip to content
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

data.ref: update XML schemas of "factura electrónica" #200

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

yaselc
Copy link

@yaselc yaselc commented Mar 9, 2021

Changelog:

  • SiiTypes_v10.xsd
    • Replaces CRLF line endings with LF.
    • root: A new simple type Dec14_4-0Type is added for non-negative decimals (admits 0)
    • TipoTransCOMPRA: The base type is changed and adds a restriction for the minimum and the maximum value (1 - 7)
    • TipoTransVENTA: Adds restriction for the minimum and maximum value (1 - 4)
  • DTE_v10.xsd
    • Replaces CRLF line endings with LF.
    • IdDoc: Adds the element TipoFactEsp
    • Receptor.Extranjero: Adds the element TipoDocID
    • IndServicio: Adds a new item to the enumeration
    • MntExeOtrMnda: Type changed to Dec14_4-0Type
    • MntTotOtrMnda: Type changed to Dec14_4-0Type

Source of the previous version of the file:
repository/project "LibreDTE": https://github.com/LibreDTE/libredte-lib/blob/c12f8845/schemas/SiiTypes_v10.xsd

Source of the new version of the file:
cl-sii-extraoficial/archivos-oficiales@c89dec5

@glarrain
Copy link
Contributor

@yaselc please add the referenced files (both the old and the new one) to https://github.com/cl-sii-extraoficial/archivos-oficiales beforehand, so we can reference them here in a more stable way than we did previously

cl-sii-extraoficial/archivos-oficiales#11

@yaselc yaselc force-pushed the feature/data-ref-update-xml-schema-factura-electronica branch from cd8b08f to 220b157 Compare March 10, 2021 15:14
@yaselc
Copy link
Author

yaselc commented Mar 10, 2021

please add the referenced files (both the old and the new one) to https://github.com/cl-sii-extraoficial/archivos-oficiales beforehand,

@glarrain should we use separate directories or separate commits will be enough?

@glarrain glarrain requested a review from jtrh March 11, 2021 12:09
@glarrain
Copy link
Contributor

I'd like the following:

  1. the files that are updated and/or referenced here please upload them to https://github.com/cl-sii-extraoficial/archivos-oficiales
  2. update this PR (description, commits and code) with references to that repo, with permalinks (shorted if you want, but make sure the commit ID is included)

@yaselc
Copy link
Author

yaselc commented Mar 15, 2021

I'd like the following:

  1. the files that are updated and/or referenced here please upload them to https://github.com/cl-sii-extraoficial/archivos-oficiales
  2. update this PR (description, commits and code) with references to that repo, with permalinks (shorted if you want, but make sure the commit ID is included)

@glarrain yes, I realized that the changes had to be added in cl-sii-extraoficial/archivos-oficiales, but I had doubts about the structure. Here a single directory is used for the XML schemas, but in cl-sii-extraoficial/archivos-oficiales, they are separated by domain. Could you please, take a look at cl-sii-extraoficial/archivos-oficiales#12, and when those changes are approved then I update this PR.

@yaselc yaselc force-pushed the feature/data-ref-update-xml-schema-factura-electronica branch 2 times, most recently from f2695c7 to 9620522 Compare March 31, 2021 22:16
@codecov-io
Copy link

codecov-io commented Mar 31, 2021

Codecov Report

Merging #200 (88911a8) into develop (d9e4eab) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #200   +/-   ##
========================================
  Coverage    81.07%   81.07%           
========================================
  Files           32       32           
  Lines         2536     2536           
  Branches       380      380           
========================================
  Hits          2056     2056           
  Misses         306      306           
  Partials       174      174           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d9e4eab...88911a8. Read the comment docs.

@yaselc yaselc force-pushed the feature/data-ref-update-xml-schema-factura-electronica branch from 9620522 to 88911a8 Compare March 31, 2021 22:19
@yaselc
Copy link
Author

yaselc commented Mar 31, 2021

@glarrain @jtrh this PR is ready for your review 🙏

- MD5 checksum: `82d426fc3bd5f3a29e61a1d07ed4d6dd`.
- Retrieval date: 2021-03-31
- MD5 checksum: `33639f61ef3aa0ec785557b1c8778cea`.
- Source: [cl-sii-extraoficial/archivos-oficiales@c89dec5](https://github.com/cl-sii-extraoficial/archivos-oficiales/pull/14/commits/c89dec54f664281721dcb77af327c4f6c58ec4ff)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO the reference should be a permalink to the specific file, not the commit

@glarrain
Copy link
Contributor

glarrain commented Apr 6, 2021

@yaselc since this change might have a significant impact perhaps some validation with lots of real XML files would be appropriate. Also, I'm thinking that perhaps we should record somehow along the validation the "version" of XML schemas set. What do you think @jtrh ?

@glarrain glarrain added task Task or chore component: rtc and removed feature labels Apr 6, 2021
Copy link
Contributor

@jtrh jtrh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestions:

- MD5 checksum: `82d426fc3bd5f3a29e61a1d07ed4d6dd`.
- Retrieval date: 2021-03-31
- MD5 checksum: `33639f61ef3aa0ec785557b1c8778cea`.
- Source: [cl-sii-extraoficial/archivos-oficiales@c89dec5](https://github.com/cl-sii-extraoficial/archivos-oficiales/pull/14/commits/c89dec54f664281721dcb77af327c4f6c58ec4ff)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- Source: [cl-sii-extraoficial/archivos-oficiales@c89dec5](https://github.com/cl-sii-extraoficial/archivos-oficiales/pull/14/commits/c89dec54f664281721dcb77af327c4f6c58ec4ff)
- Source: [cl-sii-extraoficial/archivos-oficiales@c89dec5](https://github.com/cl-sii-extraoficial/archivos-oficiales/blob/c89dec54f664281721dcb77af327c4f6c58ec4ff/src/code/rtc/2019-12-12-schema_cesion/2019-12-12-schema_cesion.zip)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or:

Suggested change
- Source: [cl-sii-extraoficial/archivos-oficiales@c89dec5](https://github.com/cl-sii-extraoficial/archivos-oficiales/pull/14/commits/c89dec54f664281721dcb77af327c4f6c58ec4ff)
- Source: [cl-sii-extraoficial/archivos-oficiales@c89dec5](https://github.com/cl-sii-extraoficial/archivos-oficiales/tree/c89dec54f664281721dcb77af327c4f6c58ec4ff/src/code/rtc/2019-12-12-schema_cesion)

@jtrh
Copy link
Contributor

jtrh commented Apr 6, 2021

Also, I'm thinking that perhaps we should record somehow along the validation the "version" of XML schemas set. What do you think @jtrh ?

@glarrain: What do you think about adding the package version (from setup.py) to the error message of the exception raised in cl_sii.libs.xml_utils.validate_xml_doc? Caveat: The information would only be guaranteed to be correct for Git-tagged releases, as any changes to the XSD files would not be reflected in the package version until the next release; however, the advantage of this idea is that it should be straightforward to implement, and does not require metadata such as the commit IDs or the checksums of the XSD files.

@glarrain
Copy link
Contributor

glarrain commented Apr 6, 2021

Also, I'm thinking that perhaps we should record somehow along the validation the "version" of XML schemas set. What do you think @jtrh ?

@glarrain: What do you think about adding the package version (from setup.py) to the error message of the exception raised in cl_sii.libs.xml_utils.validate_xml_doc?

I like the concept. However, I think it is simpler to use the value cl_sii.__version__.

@jtrh
Copy link
Contributor

jtrh commented Apr 6, 2021

Source of the old (current) DTE_v10.xsd and SiiTypes_v10.xsd is LibreDTE.

commit 8b51350cf0590ab7ba4a1390676f6b307395b950
Author: Germán Larraín <glarrain@users.noreply.github.com>
Date:   Thu Jan 17 16:41:46 2019 -0300

    data.ref: update XML schemas of "factura electrónica"
    
    Update schemas from an unofficial source since the files available on
    SII's website are outdated with respect to the regulations (and even
    to the documentation PDFs published alongside).
    
    Source: repository/project "LibreDTE" at
    https://github.com/LibreDTE/libredte-lib
    
    Specific source per file:
    - 'schema_dte/DTE_v10.xsd':
      https://github.com/LibreDTE/libredte-lib/blob/c12f8845/schemas/DTE_v10.xsd
    - 'schema_dte/EnvioDTE_v10.xsd':
      https://github.com/LibreDTE/libredte-lib/blob/c12f8845/schemas/EnvioDTE_v10.xsd
    - 'schema_dte/SiiTypes_v10.xsd':
      https://github.com/LibreDTE/libredte-lib/blob/c12f8845/schemas/SiiTypes_v10.xsd
    - 'schema_iecv/LceCal_v10.xsd'
      https://github.com/LibreDTE/libredte-lib/blob/c12f8845/schemas/LceCal_v10.xsd
    - 'schema_iecv/LceCoCertif_v10.xsd'

@glarrain
Copy link
Contributor

glarrain commented Apr 8, 2021

I love good commit messages, and it pays back to invest time in crafting them ;)

Update SiiTypes_v10.xsd from the official XML schemas of
AEC (Archivo Electrónico de Cesión)
Changelog:
- Replaces CRLF line endings with LF.
- `root`: A new simple type `Dec14_4-0Type` is added for
  non-negative decimals (admits 0)
- `TipoTransCOMPRA`: The base type is changed and adds a restriction
  for the minimum and the maximum value (1 - 7)
- `TipoTransVENTA`: Adds restriction for the minimum and maximum
   value (1 - 4)

Source of the previous version of the file:
repository/project "LibreDTE": https://github.com/LibreDTE/libredte-lib/blob/c12f8845/schemas/SiiTypes_v10.xsd

Source of the new version of the file:
[cl-sii-extraoficial/archivos-oficiales@c89dec5](https://github.com/cl-sii-extraoficial/archivos-oficiales/tree/c89dec54f664281721dcb77af327c4f6c58ec4ff/src/code/rtc/2019-12-12-schema_cesion)
Update DTE_v10.xsd from the official XML schemas of
AEC (Archivo Electrónico de Cesión)
Changelog:
- Replaces CRLF line endings with LF.
- `IdDoc`: Adds the element `TipoFactEsp`
- `Receptor.Extranjero`: Adds the element `TipoDocID`
- `IndServicio`: Adds a new item to the enumeration
- `MntExeOtrMnda`: Type changed to `Dec14_4-0Type`
- `MntTotOtrMnda`: Type changed to `Dec14_4-0Type`

Source of the previous version of the file:
repository/project "LibreDTE": https://github.com/LibreDTE/libredte-lib/blob/c12f8845/schemas/EnvioDTE_v10.xsd

Source of the new version of the file:
[cl-sii-extraoficial/archivos-oficiales@c89dec5](https://github.com/cl-sii-extraoficial/archivos-oficiales/tree/c89dec54f664281721dcb77af327c4f6c58ec4ff/src/code/rtc/2019-12-12-schema_cesion)
@yaselc yaselc force-pushed the feature/data-ref-update-xml-schema-factura-electronica branch from 88911a8 to 94a2df1 Compare April 9, 2021 00:31
@@ -1,476 +1,477 @@
"""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WARNING! Change of line terminators?

@@ -1,592 +1,599 @@
import difflib
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WARNING! Change of line terminators?

@yaselc yaselc force-pushed the feature/data-ref-update-xml-schema-factura-electronica branch from 795cffc to cc0a347 Compare April 9, 2021 04:06
@yaselc yaselc marked this pull request as draft April 9, 2021 04:07
@@ -31,6 +31,7 @@
import signxml.exceptions
import xml.parsers.expat
import xml.parsers.expat.errors
from cl_sii import __version__
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not seem right. libs modules should not depend on to the source package (cl_sii in this case), but the other way around.

@@ -306,7 +307,7 @@ def validate_xml_doc(xml_schema: XmlSchema, xml_doc: XmlElement) -> None:
# Simplest and safest way to get the error message.
# Error example:
# "Element 'DTE': No matching global declaration available for the validation root., line 2" # noqa: E501
validation_error_msg = str(exc)
validation_error_msg = str(exc) + f". cl-sii Python lib version {__version__}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps add an optional parameter to the function for specifying the xml_schema version (string) (however the caller chooses to define it)

@@ -4,6 +4,7 @@
from datetime import date, datetime

import cl_sii.dte.constants
from cl_sii import __version__
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is confusing to override implicitly the local magic constants. An example of a popular pattern for this:

import cl_sii

LIB_CL_SII_VERSION = cl_sii.__version__

@glarrain
Copy link
Contributor

glarrain commented Apr 9, 2021

@jtrh @yaselc @yanosky I've thinking about this and I have a sense that having a single "version" of the XML schemas at a point in time might not be convenient. For example, some documents might validate correctly in an older version and not the latest.

What if we have a directory for each "version" (it could be a sequential number/letter arbitrarily defined by us, taking care of not conveying the idea that comparing version numbers does not make sense; they are labels/IDs)? It would be up to the caller to decide which version to use ("explicit is better than implicit").

I think that could solve a number of issues that we've been discussing (and other we have not, yet).

@yaselc
Copy link
Author

yaselc commented Apr 9, 2021

For example, some documents might validate correctly in an older version and not the latest.

I think it's the other way around, these schemas are supposed to maintain backward compatibility, preventing old invoices from becoming out of date. The problem is to validate new invoices with old versions of the schema. I think using the last version should be always the recommendation.
It could still happen, but this could be solved using a completely different version of the entire library because most likely even the models need to change, not just the schemas.

@glarrain
Copy link
Contributor

glarrain commented Apr 9, 2021

I think it's the other way around, these schemas are supposed to maintain backward compatibility

I think that SII loves to break the rules and we can not trust them in that regard. Also, it is perfectly possible (and makes sense) for them to introduce a schema for validating all invoices issued from a certain date onwards, and with which older documents wouldn't validate.

@jtrh
Copy link
Contributor

jtrh commented May 5, 2021

CC: @jtrobles-cdd

@yaselc yaselc assigned ycouce-cdd and unassigned yaselc May 17, 2021
@glarrain-cdd glarrain-cdd requested review from glarrain-cdd and jtrobles-cdd and removed request for glarrain-cdd June 1, 2021 22:43
@glarrain-cdd
Copy link
Member

@ycouce-cdd status?

@ycouce-cdd
Copy link
Member

@ycouce-cdd status?

@glarrain-cdd After your last comments, I began to iterate on a new PR to version the XML schemas on #211 and put this on hold until it is decided which way we will go.

@jtrobles-cdd
Copy link
Member

[...] put this on hold until it is decided which way we will go.

@ycouce-cdd I'll remove myself as a reviewer of this PR while it's on hold. Feel free to add me back if anything changes.

@jtrobles-cdd jtrobles-cdd removed their request for review June 4, 2021 16:04
@glarrain glarrain requested review from glarrain and removed request for glarrain June 18, 2021 16:28
@jtrobles-cdd jtrobles-cdd removed their request for review July 9, 2021 17:59
@reviewpad reviewpad bot mentioned this pull request Mar 30, 2023
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants