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

[12.0] FIX receive_fe in case of base64 files coming from SDI #2919

Conversation

eLBati
Copy link
Member

@eLBati eLBati commented Aug 27, 2022

Without this change, every SDI file would be doubly encoded in base64.

file_name_content_dict must be base64-encoded, as described in receive_fe documentation.
Next methods must handle the "real" content, in order to process ZIP files also.

Sostituisce #2916

@OCA-git-bot
Copy link
Contributor

Hi @sergiocorato,
some modules you are maintaining are being modified, check this out!

@eLBati eLBati force-pushed the 12.0-fix-l10n_it_fatturapa_pec-base64_attachment-2 branch from 0414010 to e54eec6 Compare August 27, 2022 09:12
@@ -165,7 +165,7 @@ def receive_fe(
for file_name, file_content in file_name_content_dict.items():
attachments_values = self._process_single_fe(
file_name,
file_content,
base64.decodebytes(file_content),
Copy link
Contributor

Choose a reason for hiding this comment

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

Domanda: ha senso fare in modo che sia il chiamante del metodo receive_fe a fare la decode?

In fondo, sdi_channel è generico come modulo, e a priori non tutti i canali necessariamente prevedono base64. L'idea di avere un canale che scarica le fatture e - prima di passarle alla receive_fe - sia costretto a encodarle base64 la trovo convoluta.

La receive_fe() dovrebbe accettare dei file XML, oltre che a degli zip, e immagino che anche i metadati siano testo... - i chiamanti passano dati binari, o zipfile, o bytes adatti al parsing diretto XML (ossia potenzialmente adatti per xml.etree.ElementTree.fromstring() - salvo errori di sintassi ovviamente).

A proposito, poco sopra, riga 115, io invece di testare se il filename termina per .zip, che fa molto PC DOS, farei una try: direttamente prima della with Zipfile.zipfile(...), che fa molto python, o perlomeno userei la zipfile.is_zipfile(), che guarda il magic, se vogliamo mantenere l'if.

@@ -165,7 +165,7 @@ def receive_fe(
for file_name, file_content in file_name_content_dict.items():
attachments_values = self._process_single_fe(
file_name,
file_content,
base64.decodebytes(file_content),
Copy link
Contributor

Choose a reason for hiding this comment

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

Concordo con #2919 (review): questo metodo dovrebbe riceve i file pronti per essere processati, non qualcosa di già codificato.

file_name_content_dict must be base64-encoded, as described in receive_fe documentation.

Allora ci sarebbe da correggere la sua documentazione, l'ho aggiunto in #2916.

eLBati and others added 2 commits September 15, 2022 09:40
…change, every SDI file would be doubly encoded in base64.

file_name_content_dict must be base64-encoded, as described in its documentation.
Next methods must handle the "real" content, in order to process ZIP files also.
@eLBati eLBati force-pushed the 12.0-fix-l10n_it_fatturapa_pec-base64_attachment-2 branch from e54eec6 to b08ab0a Compare September 15, 2022 07:40
@eLBati
Copy link
Member Author

eLBati commented Dec 15, 2022

Quindi questa sarebbe da chiudere in favore di #2916 giusto?

@SirTakobi
Copy link
Contributor

Quindi questa sarebbe da chiudere in favore di #2916 giusto?

Per me sì

@eLBati eLBati closed this Feb 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants