rtc: Update AEC XML document validations #199

yaselc · 2021-03-06T00:44:56Z

Add new validators to rtc.data_models described in the document "Instructivo Técnico Registro Público Electrónico de Transferencia de Crédito" (retrieved on 2021-03-03)
Source: Instructivo Técnico Registro Público Electrónico de Transferencia de Crédito

rtc.data_models_aec: The cedente_rut should be the DTE's emisor_rut or the last cesionario_rut when the sequence is greater than 1
rtc.data_models: The fecha_cesion_dt should be before or equal to the current day
rtc.data_models: The fecha_ultimo_vencimiento should be after or equal to the fecha_emision of the DTE

glarrain · 2021-03-10T14:55:26Z

cl_sii/rtc/data_models.py

+
+    tz_utils.validate_dt_tz(value, tz)
+
+    current_date_in_tz = tz_utils.get_now_tz_aware().astimezone(tz)


AFAIK it is not necessary to .astimezone(tz) when comparing TZ-aware datetime values, because the "real" underlying datetime is not affected by the TZ used for representation. Am I right @jtrh ?

I guess I was wrong (see my latest review)

glarrain · 2021-03-10T14:56:10Z

@yaselc please update PR regarding the referenced documentation

glarrain · 2021-03-23T13:10:13Z

cl_sii/rtc/data_models.py

        if isinstance(v, datetime):
-            tz_utils.validate_dt_tz(v, cls.DATETIME_FIELDS_TZ)
+            validate_cesion_fecha(v, cls.DATETIME_FIELDS_TZ)


inconsistent naming

@glarrain that is the naming convention adopted for the validators of the cesion's fields e.g:
https://github.com/fyntex/lib-cl-sii-python/blob/058bf1704eb5ca403763541c5f731a126a0933c8/cl_sii/rtc/data_models.py#L37
https://github.com/fyntex/lib-cl-sii-python/blob/058bf1704eb5ca403763541c5f731a126a0933c8/cl_sii/rtc/data_models.py#L50

glarrain · 2021-03-23T13:12:35Z

cl_sii/rtc/data_models.py

+
+    now_tz_aware = tz_utils.get_now_tz_aware()
+
+    if not (value.date() <= now_tz_aware.date()):


I think this is not correct because the output of now_tz_aware.date() depends on the TZ of now_tz_aware, which has not been set.

I agree. Let's bring back current_date_in_tz = tz_utils.get_now_tz_aware().astimezone(tz).

glarrain · 2021-03-23T13:15:09Z

cl_sii/rtc/data_models.py

        if isinstance(v, datetime):
-            tz_utils.validate_dt_tz(v, cls.DATETIME_FIELDS_TZ)


Does it make sense to nesting tz_utils.validate_dt_tz() into validate_cesion_fecha()?

glarrain · 2021-03-23T13:16:32Z

cl_sii/rtc/data_models.py

+
+    if not (value.date() <= now_tz_aware.date()):
+        raise ValueError(
+            'Value of "fecha_cesion_dt" must be before or equal to the current day.', value


The value of "current day" depends on the timezone

glarrain · 2021-03-23T13:20:49Z

cl_sii/rtc/data_models.py

+    cesion_value: date, dte_value: date
+) -> None:
+    """
+    Validate 'fecha_ultimo_vencimiento' of the "cesión" is after or equal


Is there any documentation for this rule?

glarrain · 2021-03-23T13:22:10Z

cl_sii/rtc/data_models.py

@@ -321,6 +351,9 @@ class CesionL0:
        - Same timestamp as the "Registro AoR DTE" event ``DTE Cedido``.
        - The above statements were empirically verified for
          ``CesionNaturalKey(dte_key=DteNaturalKey(Rut('99***140-4'), 33, 3105), seq=2)``.
+        - When receiving an XML AEC document, the SII validates this date is before or
+          equal to the current day.
+          Source: (https://www.sii.cl/factura_electronica/ins_tecnico.pdf)


Reference a permalink please, and indicate exactly where in that document the rule is (and perhaps even quote some text, if possible)

glarrain · 2021-03-23T13:41:04Z

Also, please update the PR description and all the commits messages so we have stable references (SII's hosted static files can not be trusted)

glarrain · 2021-03-23T13:42:18Z

Also, since this PR involves a critical part of validation, please run it against many many many real cases (you will need to code something for that I suppose).

I'd also like to know the difference (timedelta) between fecha_firma_dt and fecha_cesion_dt for all those cases. I've got a feeling that difference must be very little (for valid "cesiones" I mean).

What if we add a warning (for more than X seconds of difference) instead of the "validation" that we are removing?

@jtrh your thoughts on this please

jtrh · 2021-03-24T05:26:19Z

I'd also like to know the difference (timedelta) between fecha_firma_dt and fecha_cesion_dt for all those cases. I've got a feeling that difference must be very little (for valid "cesiones" I mean).

What if we add a warning (for more than X seconds of difference) instead of the "validation" that we are removing?

@jtrh your thoughts on this please

I would keep the validation of fecha_firma_dt and fecha_cesion_dt, but change it from an equality comparison to the following condition: the absolute value of the difference between the two datetimes must be ≤ 60 seconds.

jtrh · 2021-03-24T05:31:43Z

cl_sii/rtc/data_models.py

+
+    if not (value.date() <= now_tz_aware.date()):
+        raise ValueError(
+            'Value of "fecha_cesion_dt" must be before or equal to the current day.', value


In addition to the value of fecha_cesion_dt (value), I would include the value of current day in the exception message so that it's clear to the user which date was used for the comparison, which is especially useful when debugging time zone--related issues.

jtrh · 2021-03-24T05:37:27Z

I would keep the validation of fecha_firma_dt and fecha_cesion_dt, but change it from an equality comparison to the following condition: the absolute value of the difference between the two datetimes must be ≤ 60 seconds.

Also, I would do the above change in a separate pull request. The other validations are nice-to-haves, but I think we should prioritize this one so we can speed up the fix of https://github.com/fyntex/fd-cl-data/issues/671.

glarrain · 2021-03-30T00:26:49Z

LGTM. @jtrh your call.

yaselc · 2021-03-31T19:21:24Z

I would keep the validation of fecha_firma_dt and fecha_cesion_dt, but change it from an equality comparison to the following condition: the absolute value of the difference between the two datetimes must be ≤ 60 seconds.

Also, I would do the above change in a separate pull request. The other validations are nice-to-haves, but I think we should prioritize this one so we can speed up the fix of fyntex/fd-cl-data#671.

@jtrh I moved this specific validation to PR #208, with an analysis of a representative set of AEC, because I still have doubts about it.

jtrh · 2021-05-05T23:17:35Z

CC: @jtrobles-cdd

jtrobles-cdd · 2021-05-06T06:14:04Z

Is this PR ready for a new review?

The 'cedente_rut' should be the DTE's 'emisor_rut' or the last 'cesionario_rut' when the sequence is greater than 2 Condition sourced from document "Instructivo Técnico Registro Público Electrónico de Transferencia de Crédito" Source: (https://github.com/cl-sii-extraoficial/archivos-oficiales/blob/master/src/docs/rtc/2013-02-11-instructivo-tecnico.pdf)

…e current day

…th dte Validate 'fecha_ultimo_vencimiento' of the "cesión" is after or equal to 'fecha_emision' of the DTE. Source: (https://github.com/cl-sii-extraoficial/archivos-oficiales/blob/master/src/docs/rtc/2013-02-11-instructivo-tecnico.pdf)

codecov-commenter · 2021-05-14T01:31:49Z

Codecov Report

Merging #199 (b1ca3c7) into develop (b97bb11) will increase coverage by 0.13%.
The diff coverage is 91.48%.

@@             Coverage Diff             @@
##           develop     #199      +/-   ##
===========================================
+ Coverage    81.02%   81.16%   +0.13%     
===========================================
  Files           32       32              
  Lines         2525     2565      +40     
  Branches       375      384       +9     
===========================================
+ Hits          2046     2082      +36     
+ Misses         306      305       -1     
- Partials       173      178       +5

Impacted Files	Coverage Δ
cl_sii/rtc/data_models.py	`91.03% <90.62%> (-0.32%)`	⬇️
cl_sii/rtc/data_models_aec.py	`90.09% <93.33%> (-0.36%)`	⬇️
cl_sii/libs/tz_utils.py	`67.64% <0.00%> (+2.94%)`	⬆️

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 b97bb11...b1ca3c7. Read the comment docs.

ycouce-cdd · 2021-05-14T02:26:43Z

Is this PR ready for a new review?

It's now... thanks

jtrobles-cdd

Taking into account what @ycouce-cdd and me discussed yesterday (2021-05-24), are there any validations that should be removed because they conflict with the SII's de facto rules?

ycouce-cdd · 2022-04-04T22:02:27Z

Taking into account what @ycouce-cdd and me discussed yesterday (2021-05-24), are there any validations that should be removed because they conflict with the SII's de facto rules?

It took me a long time to answer, sorry for that, but practice showed us that one thing is what the rules say and another is how they are applied. Obviously, it is not wise to implement validations that probably the SII's own system doesn't apply, it would only make sense if we wanted to generate our own AECs, but I would not use it to restrict the ones we accept.
The smartest thing to do for now is to keep this PR on hold.

yaselc added feature component: rtc labels Mar 6, 2021

yaselc requested review from glarrain and jtrh March 6, 2021 00:44

yaselc self-assigned this Mar 6, 2021

glarrain reviewed Mar 10, 2021

View reviewed changes

yaselc force-pushed the feature/update-aec-xml-document-validations branch from 490fc71 to 7c6d1ea Compare March 15, 2021 12:51

glarrain suggested changes Mar 23, 2021

View reviewed changes

glarrain added enhancement New feature or request and removed feature labels Mar 23, 2021

glarrain added the bugfix label Mar 23, 2021

jtrh reviewed Mar 24, 2021

View reviewed changes

yaselc force-pushed the feature/update-aec-xml-document-validations branch from 7c6d1ea to e0b1809 Compare March 25, 2021 15:23

yaselc force-pushed the feature/update-aec-xml-document-validations branch 3 times, most recently from f132946 to 656b66e Compare April 8, 2021 04:59

ycouce-cdd added 3 commits May 13, 2021 20:21

rtc.data_models: validates 'fecha_cesion_dt' is before or equal to th…

8f2d3f1

…e current day

ycouce-cdd force-pushed the feature/update-aec-xml-document-validations branch from 656b66e to b1ca3c7 Compare May 14, 2021 01:29

ycouce-cdd requested a review from jtrobles-cdd May 14, 2021 02:26

yaselc assigned ycouce-cdd and unassigned yaselc May 17, 2021

jtrobles-cdd reviewed May 25, 2021

View reviewed changes

jtrobles-cdd added bug Something isn't working and removed bugfix labels Feb 3, 2022

jtrobles-cdd marked this pull request as draft September 28, 2022 22:57

reviewpad bot mentioned this pull request Mar 30, 2023

feat: reviewpad onboarding #472

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rtc: Update AEC XML document validations #199

rtc: Update AEC XML document validations #199

yaselc commented Mar 6, 2021 •

edited by ycouce-cdd

glarrain Mar 10, 2021

glarrain Mar 23, 2021

glarrain commented Mar 10, 2021

glarrain Mar 23, 2021

yaselc Mar 25, 2021 •

edited

glarrain Mar 23, 2021

jtrh Mar 24, 2021

glarrain Mar 23, 2021

glarrain Mar 23, 2021

glarrain Mar 23, 2021

glarrain Mar 23, 2021

glarrain commented Mar 23, 2021

glarrain commented Mar 23, 2021 •

edited

jtrh commented Mar 24, 2021

jtrh Mar 24, 2021

jtrh commented Mar 24, 2021

glarrain commented Mar 30, 2021

yaselc commented Mar 31, 2021

jtrh commented May 5, 2021

jtrobles-cdd commented May 6, 2021

codecov-commenter commented May 14, 2021 •

edited

ycouce-cdd commented May 14, 2021

jtrobles-cdd left a comment

ycouce-cdd commented Apr 4, 2022


		tz_utils.validate_dt_tz(value, tz)

		current_date_in_tz = tz_utils.get_now_tz_aware().astimezone(tz)


		now_tz_aware = tz_utils.get_now_tz_aware()

		if not (value.date() <= now_tz_aware.date()):

		if isinstance(v, datetime):
		tz_utils.validate_dt_tz(v, cls.DATETIME_FIELDS_TZ)

rtc: Update AEC XML document validations #199

Are you sure you want to change the base?

rtc: Update AEC XML document validations #199

Conversation

yaselc commented Mar 6, 2021 • edited by ycouce-cdd

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glarrain commented Mar 10, 2021

Choose a reason for hiding this comment

yaselc Mar 25, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glarrain commented Mar 23, 2021

glarrain commented Mar 23, 2021 • edited

jtrh commented Mar 24, 2021

Choose a reason for hiding this comment

jtrh commented Mar 24, 2021

glarrain commented Mar 30, 2021

yaselc commented Mar 31, 2021

jtrh commented May 5, 2021

jtrobles-cdd commented May 6, 2021

codecov-commenter commented May 14, 2021 • edited

Codecov Report

ycouce-cdd commented May 14, 2021

jtrobles-cdd left a comment

Choose a reason for hiding this comment

ycouce-cdd commented Apr 4, 2022

yaselc commented Mar 6, 2021 •

edited by ycouce-cdd

yaselc Mar 25, 2021 •

edited

glarrain commented Mar 23, 2021 •

edited

codecov-commenter commented May 14, 2021 •

edited