[14.0][FIX] l10n_it_fatturapa_out: use parent codice_destinatario #4595
Conversation
SylweKra
changed the title
SylweKra
force-pushed
the
14.0-FIX-l10n_it_fatturapa_out
branch
3 times, most recently
from
c561e6e to
f474a52
Compare
Uses the parent codice_destinatario for each child if the parent has set the flag l10n_it_use_parent_codice_destinatario.
SylweKra
force-pushed
the
14.0-FIX-l10n_it_fatturapa_out
branch
from
f474a52 to
6d74e1b
Compare
SylweKra
changed the title
|
La modifica dei test è stata fatta perché abbiamo implementato una funzionalità che prevede che se il contatto padre ha il flag "soggetto obbligato" abilitato e il contatto figlio non ce l'ha, quando si fattura al contatto figlio va utilizzato il codice destinatario del padre. Questo molto probabilmente ha portato ad un errore nei test perché nel modulo l10n_it_fatturapa_out i test non passavano a causa di un errore di confronto delle stringhe XML. L'errore riguardava la differenza nel CAP e nella Provincia nel confronto tra le stringhe xml della fattura di demo e la fattura generata. Per risolvere il problema, è stato necessario modificare i dati di test, in particolare il file XML di prova CHE114993395IVA_00007.xml. Questo file conteneva un CAP errato (era "00100" invece di "00000") e mancava il tag Provincia. Secondo le linee guida per la fatturazione elettronica estera (fonte: https://www.agenziaentrate.gov.it/portale/schede/comunicazioni/fatture-e-corrispettivi/faq-fe/risposte-alle-domande-piu-frequenti-categoria/fatture-verso-e-da-soggetti-stranieri-transfrontaliere#:~:text=Infine%2C%20per%20indicare%20in%20fattura,per%20indicare%20il%20CAP%20straniero), tutte le fatture per clienti esteri (sia UE che Extra UE) devono avere il CAP "00000" e il Codice Destinatario "XXXXXXX". Nel file di test, è stato quindi corretto il CAP e aggiunta la Provincia. Sebbene la Provincia non sia necessaria per le fatture estere, è stata inserita con la sigla "EE" per rispettare le convenzioni usate nei test, che richiedono la provincia per il confronto tra il file XML generato e quello di prova. Dopo queste modifiche, i test passavano correttamente in locale, ma non nella PR. Questo accadeva perché anche i dati di demo dei test nel modulo l10n_it_fatturapa_out_rc presentavano lo stesso problema: il CAP errato e la provincia assente per le aziende estere. È probabile che ci sia una dipendenza tra i due moduli, e che i dati di test nel modulo secondario non fossero stati aggiornati in modo coerente con la funzionalità implementata. |
There was a problem hiding this comment.
Un paio di considerazioni
- realisticamente, immagino che si setterà il codice destinatario sul commercial partner, e poi si erediterà sui figli. Si potrebbe pensare di semplificare il funzionamento del modulo utilizzando direttamente il commercial partner, senza ricorsività?
- C'è già un meccanismo per sincronizzare dei campi tra contatti genitori e figli. Vedi la funzione
_fields_syncdires.partnerin odoo base. È un attimo complessa da comprendere la prima volta che la si vede ma un'implementazione alternativa potrebbe essere di inserirsi su quella funzione o sulle altre correlate (ad esempio_children_sync)
| @@ -6,7 +6,7 @@ | |||
|
|
|||
| { | |||
| "name": "ITA - Fattura elettronica - Base", | |||
| "version": "14.0.2.3.4", | |||
| "version": "14.0.2.4.2", | |||
There was a problem hiding this comment.
chore: revert this change. Only change version number manually if needed by e.g. a migration script. Otherwise let odoobot change it on merge
| for child in child_records: | ||
| child.codice_destinatario = record.codice_destinatario | ||
|
|
||
| def _recursive_parent_codice_destinatario(self, parent): |
There was a problem hiding this comment.
chore: this seems like an @api.model
| help="The code, 7 characters long, assigned by ES to subjects with an " | ||
| "accredited channel; if the addressee didn't accredit a channel " | ||
| "to ES and invoices are received by PEC, the field must be " | ||
| "the standard value ('%s')." % STANDARD_ADDRESSEE_CODE, | ||
| default=STANDARD_ADDRESSEE_CODE, | ||
| ) | ||
| l10n_it_use_parent_codice_destinatario = fields.Boolean( |
There was a problem hiding this comment.
question: this boolean is supposed to be set on the parent, correct?
In that case, what do you think about renaming the variable to something like l10n_it_use_codice_destinatario_for_children? (similar to the current string value)
With the current name, I'd expect it to be set on the child.
| "electronic_invoice_subjected", | ||
| "electronic_invoice_obliged_subject", |
There was a problem hiding this comment.
issue: these two fields are set here, but they're not used in the function. Are they used indirectly? Should they be checked explicitly?
On the other hand, some fields are missing:
electronic_invoice_use_this_addressparent_id.l10n_it_use_parent_codice_destinatarioparent_id.codice_destinatario
| if ( | ||
| not partner.is_company | ||
| and not partner.electronic_invoice_use_this_address | ||
| and partner.parent_id.l10n_it_use_parent_codice_destinatario |
There was a problem hiding this comment.
issue: what if you have a 3-tier structure with Partners A -> B -> C
A has l10n_it_use_parent_codice_destinatario set, and B and C should inherit the code.
With the check as it is now, it seems to me that C would not inherit the codice_destinatario because l10n_it_use_parent_codice_destinatario is False for B
| [ | ||
| ("parent_id", "=", record.id), | ||
| ("l10n_it_use_parent_codice_destinatario", "=", False), | ||
| ] |
There was a problem hiding this comment.
issue: shouldn't this also check for is_company and electronic_invoice_use_this_address? You don't want to overwrite the code for a child that has that option set. Basically, this check should be almost the same as lines 191-195 above.
I'll add to this comment also this:
Consider an alternative solution with no inverse method at all, and only a compute with the correct fields set in the depends.
|
|
||
| @api.onchange("electronic_invoice_subjected") | ||
| def onchange_electronic_invoice_subjected(self): | ||
| if not self.electronic_invoice_subjected: | ||
| self.electronic_invoice_obliged_subject = False | ||
| else: | ||
| if self.supplier_rank > 0: | ||
| self.onchange_country_id_e_inv() | ||
| self._compute_codice_destinatario() |
There was a problem hiding this comment.
issue: The fields electronic_invoice_subjected and electronic_invoice_obliged_subject are already part of the depends of _compute_codice_destinatario, so there is no need to call the function explicitly IMO. Cf. the comment for _compute_codice_destinatario
| self.electronic_invoice_obliged_subject = True | ||
|
|
||
| @api.onchange("electronic_invoice_obliged_subject") | ||
| def onchange_e_inv_obliged_subject(self): | ||
| if not self.electronic_invoice_obliged_subject: | ||
| self.onchange_country_id_e_inv() | ||
| self._compute_codice_destinatario() |
There was a problem hiding this comment.
Issue: See other comments
|
scusate, ma non e' un po too much questa modifica? non si potrebbe semplicemente fare una modifica che se il commercial partner_id ha un codice destinatario diverso o meglio diverso dai dai 7 zeri, allora si prende dal commerciale partner_id? cioe' mi sembra che si aggiunga della complessita' all'export della fattura |
Fixed issue with the tests.
Superseeds #4436