[14.0][IMP] l10n_it_fatturapa_in: intermediary contact creation depending by vat #4621

Chionne27 · 2025-02-17T14:42:39Z

No description provided.

francesco-ooops

Errata approvazione

francesco-ooops

Functional ok

l10n_it_fatturapa_in/security/ir.model.access.csv

l10n_it_fatturapa_in/wizard/wizard_check_intermediary_vat.py

l10n_it_fatturapa_in/wizard/wizard_check_intermediary_vat.xml

l10n_it_fatturapa_in/wizard/wizard_import_fatturapa.py

l10n_it_fatturapa_in/wizard/wizard_check_intermediary_vat.py

l10n_it_fatturapa_in/wizard/wizard_check_intermediary_vat.xml

l10n_it_fatturapa_in/wizard/wizard_import_fatturapa.py

SirPyTech

Mi pare che questi 3 commit siano un po' mischiati, se possibile sarebbe meglio fare:

1 commit [REF] di refactoring prima dove estrai tutti i metodi che ti serve estrarre, senza cambiare il comportamento che c'era prima della PR.
Se possibile, come scrivevo in #4621 (comment), sulla linea di #3100 visto che prima o poi dovremo allinearci
1 commit [IMP] dove aggiungi la nuova funzionalità

l10n_it_fatturapa_in/tests/fatturapa_common.py

l10n_it_fatturapa_in/wizard/wizard_check_intermediary_vat.xml

SirPyTech · 2025-07-22T14:34:09Z

l10n_it_fatturapa_in/wizard/wizard_import_fatturapa.py

-            # XXX maybe San Marino needs special formatting too?
-            else:
-                vat = id_codice
+        vat = self.extract_vat(DatiAnagrafici)


Questo commit ha lo scopo di

[IMP] l10n_it_fatturapa_in: intermediary contact creation depending b…
…y vat

(riesci a riassumere in modo che non vada a capo?)

Però ci sono dentro diversi refactoring, sicuramente utili, che non fanno capire cosa sia davvero necessario allo scopo del commit e cosa no.

Puoi riunire i refactoring in un commit dedicato, precedente agli altri?
Nota che il refactoring per definizione non dovrebbe cambiare il comportamento attuale.

Ora è tutto in un commit 👍

SirPyTech · 2025-07-22T15:26:08Z

l10n_it_fatturapa_in/wizard/wizard_check_intermediary_vat.py

-                    "name": line.name,
+                    "name": line.lastname,
+                    "firstname": line.firstname or False,
+                    "lastname": line.lastname or line.name,


Questa modifica non mi pare abbia a che fare con lo scopo del commit

[IMP] l10n_it_fatturapa_in: better partner search

Forse andrebbe nel commit in cui è stato definito il wizard?

Ora è tutto in un commit 👍

SirPyTech · 2025-07-22T15:27:49Z

l10n_it_fatturapa_in/wizard/wizard_import_fatturapa.py

            )

-    def getPartnerBase(self, DatiAnagrafici):  # noqa: C901
+    def getPartnerBase(self, DatiAnagrafici, partnerCreate=True):  # noqa: C901


Ora che il metodo è stato semplificato forse si può riattivare il linting

Suggested change

def getPartnerBase(self, DatiAnagrafici, partnerCreate=True): # noqa: C901

def getPartnerBase(self, DatiAnagrafici, partnerCreate=True):

SirPyTech · 2025-07-22T15:31:52Z

l10n_it_fatturapa_in/wizard/wizard_import_fatturapa.py

+
+            return vals
+
+    def _find_partners_by_cf_vat(self, DatiAnagrafici):


In che senso questa ricerca è meglio di com'era prima?
A me sembra che venga cercato esattamente allo stesso modo, sono solo stati estratti dei metodi, oppure mi son perso qualche pezzo?
Se è solo un'estrazione di metodi puoi metterla in un commit di refactoring [REF]?

Ora è tutto in un commit 👍

SirPyTech · 2025-07-22T15:45:33Z

l10n_it_fatturapa_in/wizard/wizard_import_fatturapa.py

-                                    "name": name,
-                                    "vat": vat,
+                                    "name": partner_vals.get("name"),
+                                    "firstname": partner_vals.get("firstname"),
+                                    "lastname": partner_vals.get("lastname"),
+                                    "vat": partner_vals["vat"],


Ci sono diversi valori in partner_vals che sono stati faticosamente trovati da _prepare_partner_values, e altri override potrebbero includerne altri.
Sicuramente aggiungerei il codice fiscale perché insieme alla partita IVA identifica il partner (come hai visto nelle diverse search).

Dopo questa PR l'intermediario avrà meno campi valorizzati, che è una regressione, puoi correggere?
Per riuscire a passarli tutti al wizard si potrebbero mettere in una stringa con json.dump e salvarli in un campo del wizard che poi riotterrà il dizionario con json.loads, cosa ne pensi?

Questo non è risolto

SirPyTech · 2025-07-22T15:47:17Z

l10n_it_fatturapa_in/wizard/wizard_check_intermediary_vat.py

                {
-                    "name": line.lastname,
                    "firstname": line.firstname or False,


Questo come ha a che fare con

[IMP] l10n_it_fatturapa_in: commercial partner search improvement

?

Ora è tutto in un commit 👍

SirPyTech · 2025-07-22T15:49:20Z

l10n_it_fatturapa_in/wizard/wizard_import_fatturapa.py

+
+            return vals
+
+    def _find_commercial_partner(self, DatiAnagrafici, partners):


In che senso questa ricerca è meglio di com'era prima?
A me sembra che venga cercato esattamente allo stesso modo, sono solo stati estratti dei metodi, oppure mi son perso qualche pezzo?
Se è solo un'estrazione di metodi puoi metterla in un commit di refactoring [REF]?

Non avevamo detto si semplificare il metodo get_partner_base? per questo ho pensato fosse una buona idea cercare i partner associati in base al dominio in una funzione e poi il commercial partner in un'altra

Ora è tutto in un commit 👍

SirPyTech · 2025-07-22T15:51:00Z

l10n_it_fatturapa_in/wizard/wizard_import_fatturapa.py

-        vat = self.extract_vat(DatiAnagrafici)
+        vat = False
+        if DatiAnagrafici.IdFiscaleIVA:
+            id_paese = DatiAnagrafici.IdFiscaleIVA.IdPaese.upper()
+            id_codice = re.sub(r"\W+", "", DatiAnagrafici.IdFiscaleIVA.IdCodice).upper()
+            # Format Italian VAT ID to always have 11 char
+            # to avoid validation error when creating the given partner
+            if id_paese == "IT" and not id_codice.startswith("IT"):
+                vat = "IT{}".format(id_codice.rjust(11, "0")[:11])
+            # XXX maybe San Marino needs special formatting too?
+            else:
+                vat = id_codice


Perché ora non va più bene extract_vat?

A me sembra meglio la nuova funzione. Anche perché così riusciamo ad evitare una funzione che deve restituire il cf (estrarre ripetutamente il cf dai dati anagrafici in giro non è una ripetizione del codice?). Prima extract_vat andava a restituire solo la partita iva quindi sostituire quella funzione con una che dia anche il cf mi sembra più pratico da usare quando serve. Poi la funzione l'ho spostata con le altre nuove prima era in fondo

estrarre ripetutamente il cf dai dati anagrafici in giro non è una ripetizione del codice?

Secondo me no perché si parla semplicemente di recuperare il valore di un campo:

cf = DatiAnagrafici.CodiceFiscale

è come quando devi recuperare il partner di una fattura: partner = invoice.partner_id è scritto mille volte, ma non è codice ripetuto.

Ora che c'è la nuova _define_vat_fiscalcode va bene, quando scrissi il commento era stato rimosso extract_vat e c'era semplicemente scritto tutto l'if:

👍

Chionne27 · 2025-08-18T07:31:07Z

Mi pare che questi 3 commit siano un po' mischiati, se possibile sarebbe meglio fare:

1 commit [REF] di refactoring prima dove estrai tutti i metodi che ti serve estrarre, senza cambiare il comportamento che c'era prima della PR.
Se possibile, come scrivevo in #4621 (comment), sulla linea di [12.0] [FIX] l10n_it_fatturapa_in - Gruppi IVA #3100 visto che prima o poi dovremo allinearci

1 commit [IMP] dove aggiungi la nuova funzionalità

Quando ho finito di fare le modifiche la mia intenzione sarà quella di fare squash dei commit perché ovviamente le modifiche fatte fino ad ora erano dati da tentativi di implementazione. Nella descrizione del commit ho messo che è stato fatto anche un refactoring iniziale. Fare un commit REF risulterebbe ora troppo complesso perché significherebbe ripristinare tutto quello che ho fatto e ripeterlo da capo ed è secondo me un lavoro evitabile

l10n_it_fatturapa_in/tests/fatturapa_common.py

SirPyTech · 2025-08-19T13:03:35Z

l10n_it_fatturapa_in/wizard/wizard_check_intermediary_vat.py

+            data = json.loads(line.intermediary_data_json)
+            if isinstance(data, str):
+                data = json.loads(data)


Perché serve ripetere loads?

SirPyTech · 2025-08-19T13:19:17Z

l10n_it_fatturapa_in/wizard/wizard_import_fatturapa.py

+                        partner = self.getPartnerBase(
+                            Intermediary.DatiAnagrafici, partnerCreate=False
+                        )
+
+                        if isinstance(partner, int):
+                            partner_id = self.env["res.partner"].browse(partner)
+                            invoice.write({"intermediary": partner_id})


Qui mi pare ci sia un po' di confusione tra l'ID del partner (un intero) e un record del partner perché non si sa bene cosa potrà uscire dalla nuova implementazione di getPartnerBase.

In generale penso sia più chiaro usare il suffisso _id quando si parla di un ID (es partner_id), e nessun suffisso quando si parla di un record (es partner).

Quindi suggerirei

Suggested change

partner = self.getPartnerBase(

Intermediary.DatiAnagrafici, partnerCreate=False

)

if isinstance(partner, int):

partner_id = self.env["res.partner"].browse(partner)

invoice.write({"intermediary": partner_id})

partner_id = self.getPartnerBase(

Intermediary.DatiAnagrafici, partnerCreate=False

)

if isinstance(partner_id, int):

invoice.write({"intermediary": partner_id})

che è anche più simile al codice prima di questa PR (era intermediary_id).

Però in questo caso getPartnerBase può restituire o in intero (se trova il partner) oppure un dizionario che verra sfruttato nella funzione di import. Per questo avevo pensato di generalizzare e scrivere partner. Infatti c'è anche la seconda condizione sotto che valuta se viene restituito un dizionario

Però in questo caso getPartnerBase può restituire o in intero (se trova il partner) oppure un dizionario che verra sfruttato nella funzione di import. Per questo avevo pensato di generalizzare e scrivere partner. Infatti c'è anche la seconda condizione sotto che valuta se viene restituito un dizionario

Ok, però dentro partner di solito c'è un record; se non è così si crea confusione nel resto del codice.
Ad esempio

partner_id = self.env["res.partner"].browse(partner)

è il contrario di quello che si farebbe normalmente.
Se non si sa di che tipo sia la variabile puoi chiamarla tipo partner_id_or_vals.

Ho come la sensazione che questo metodo che può restituire un dizionario o un intero ci stia complicando la vita, forse sarebbe più semplice qualcosa del tipo:

partner_id = self._find_partner(...) if partner_id: invoice.write(...) else: partner_values = self._prepare_partner_values(...)

Cosa ne pensi?

SirPyTech · 2025-08-19T13:20:31Z

l10n_it_fatturapa_in/wizard/wizard_import_fatturapa.py

+                        elif isinstance(partner, bool):
+                            invoice.write({"intermediary": False})


Qui non si entra mai perché il primo if controlla se partner è un intero e

>>> isinstance(False, int) True

SirPyTech · 2025-08-19T13:35:39Z

l10n_it_fatturapa_in/wizard/wizard_import_fatturapa.py

+                    if Intermediary.DatiAnagrafici.IdFiscaleIVA:
+                        partner = self.getPartnerBase(
+                            Intermediary.DatiAnagrafici, partnerCreate=False
+                        )


Perché ora questo viene fatto solo quando c'è il nodo Intermediary.DatiAnagrafici.IdFiscaleIVA?
Può essere utile per recuperare i dati dai nodi del XML anche se non troverà la partita IVA, come veniva fatto prima di questa PR.

Era già stato segnalato in #4621 (comment).

SirPyTech · 2025-08-19T14:46:15Z

l10n_it_fatturapa_in/wizard/wizard_import_fatturapa.py

+
+            return vals
+
+    def _find_commercial_partner(self, DatiAnagrafici, partners):


Quando c'è solo un record in partners questo metodo ritorna False, dovrebbe invece restituire il commercial partner dell'unico record in partners.

SirPyTech · 2025-08-19T14:49:47Z

l10n_it_fatturapa_in/wizard/wizard_import_fatturapa.py

+
+            return vals
+
+    def _find_commercial_partner(self, DatiAnagrafici, partners):


In questo metodo cf e vat vengono usati solo per loggare l'inconsistenza, si può evitare di calcolarli sempre?

SirPyTech · 2025-08-19T14:57:21Z

l10n_it_fatturapa_in/wizard/wizard_import_fatturapa.py

+            "vat": vat if vat else False,
+            "fiscalcode": cf if cf else False,


Se vat (o cf) sono vuoti, cosa cambia scriverci invece False?
Se non cambia nulla puoi rimettere com'era prima?

Suggested change

"vat": vat if vat else False,

"fiscalcode": cf if cf else False,

"vat": vat,

"fiscalcode": cf,

SirPyTech · 2025-08-19T15:08:52Z

l10n_it_fatturapa_in/wizard/wizard_import_fatturapa.py

+                                "name": denominazione,
+                                "firstname": nome,
+                                "lastname": cognome,
+                                "vat": vat,
+                                "invoice_id": invoice.id,
+                                "intermediary_data_json": {
+                                    "name": denominazione,
+                                    "firstname": nome,
+                                    "lastname": cognome,
+                                    "vat": vat,
+                                },


Suggested change

"name": denominazione,

"firstname": nome,

"lastname": cognome,

"vat": vat,

"invoice_id": invoice.id,

"intermediary_data_json": {

"name": denominazione,

"firstname": nome,

"lastname": cognome,

"vat": vat,

},

"invoice_id": invoice.id,

"intermediary_data_json": {

"name": denominazione,

"firstname": nome,

"lastname": cognome,

"vat": vat,

},

I valori dei primi campi potrebbero essere calcolati in base a quello che c'è in intermediary_data_json invece di ripeterli.

È necessario allora implementare una funzione in WizardCheckIntermediaryVatLine che estragga i dati da intermediary_data_json

SirPyTech · 2025-08-19T15:11:11Z

l10n_it_fatturapa_in/wizard/wizard_import_fatturapa.py

+                                "lastname": new_data.get("lastname")
+                                or new_data.get("name"),


Come mai non mettiamo invece un name dedicato? C'era nelle vecchie versioni della PR. Sarebbe da valorizzare solo quando c'è Denominazione.

Ci può stare

francesco-ooops approved these changes Feb 17, 2025

View reviewed changes

francesco-ooops self-requested a review February 17, 2025 15:34

francesco-ooops mentioned this pull request Feb 17, 2025

l10n_it_fatturapa_in - scelta creazione contatto intermediario #4622

Open

2 tasks

francesco-ooops linked an issue Feb 17, 2025 that may be closed by this pull request

l10n_it_fatturapa_in - scelta creazione contatto intermediario #4622

Open

2 tasks

Chionne27 force-pushed the 14.0-IMP-l10n_it_fatturapa_in branch 4 times, most recently from ed30f19 to c28b517 Compare February 19, 2025 08:15

francesco-ooops approved these changes Feb 19, 2025

View reviewed changes

sergiocorato requested changes Feb 19, 2025

View reviewed changes

l10n_it_fatturapa_in/security/ir.model.access.csv Show resolved Hide resolved

l10n_it_fatturapa_in/wizard/wizard_check_intermediary_vat.py Outdated Show resolved Hide resolved

Chionne27 force-pushed the 14.0-IMP-l10n_it_fatturapa_in branch 3 times, most recently from 37f7831 to f6b31d2 Compare February 20, 2025 08:31

GSLabIt reviewed Feb 20, 2025

View reviewed changes

l10n_it_fatturapa_in/wizard/wizard_check_intermediary_vat.xml Outdated Show resolved Hide resolved

l10n_it_fatturapa_in/wizard/wizard_check_intermediary_vat.xml Show resolved Hide resolved

l10n_it_fatturapa_in/wizard/wizard_import_fatturapa.py Outdated Show resolved Hide resolved

primes2h requested changes Feb 20, 2025

View reviewed changes

l10n_it_fatturapa_in/wizard/wizard_check_intermediary_vat.py Outdated Show resolved Hide resolved

l10n_it_fatturapa_in/wizard/wizard_check_intermediary_vat.xml Outdated Show resolved Hide resolved

l10n_it_fatturapa_in/wizard/wizard_import_fatturapa.py Outdated Show resolved Hide resolved

Chionne27 force-pushed the 14.0-IMP-l10n_it_fatturapa_in branch 2 times, most recently from 1e7be0b to 4c3b6da Compare February 25, 2025 13:32

Chionne27 force-pushed the 14.0-IMP-l10n_it_fatturapa_in branch 4 times, most recently from 2f2ce53 to 9b1bc90 Compare March 13, 2025 13:19

Chionne27 force-pushed the 14.0-IMP-l10n_it_fatturapa_in branch 3 times, most recently from 3a52536 to 67261be Compare March 18, 2025 11:54

TheMule71 reviewed Mar 18, 2025

View reviewed changes

l10n_it_fatturapa_in/wizard/wizard_import_fatturapa.py Outdated Show resolved Hide resolved

Chionne27 force-pushed the 14.0-IMP-l10n_it_fatturapa_in branch from 67261be to 7c229f6 Compare March 19, 2025 08:43

GSLabIt reviewed Mar 25, 2025

View reviewed changes

l10n_it_fatturapa_in/wizard/wizard_import_fatturapa.py Outdated Show resolved Hide resolved

l10n_it_fatturapa_in/wizard/wizard_import_fatturapa.py Outdated Show resolved Hide resolved

Chionne27 force-pushed the 14.0-IMP-l10n_it_fatturapa_in branch from 7c229f6 to 3095bbc Compare March 25, 2025 11:04

francesco-ooops requested review from primes2h and sergiocorato March 26, 2025 09:05

Chionne27 force-pushed the 14.0-IMP-l10n_it_fatturapa_in branch 4 times, most recently from 0fe8bf8 to 06f1a50 Compare July 8, 2025 12:19

Chionne27 force-pushed the 14.0-IMP-l10n_it_fatturapa_in branch 2 times, most recently from e917c43 to 2f752bb Compare July 16, 2025 12:00

SirPyTech suggested changes Jul 22, 2025

View reviewed changes

Chionne27 force-pushed the 14.0-IMP-l10n_it_fatturapa_in branch 8 times, most recently from aa8db53 to 1ad5242 Compare August 18, 2025 12:33

SirPyTech suggested changes Aug 19, 2025

View reviewed changes

Chionne27 mentioned this pull request Aug 20, 2025

[14.0][IMP] l10n_it_fatturapa_in: intermediary contact creation depending by vat #4876

Open

[REF] l10n_it_fatturapa_in: refactoring

a880695

Chionne27 force-pushed the 14.0-IMP-l10n_it_fatturapa_in branch 9 times, most recently from 21ffd97 to 1da39c1 Compare August 22, 2025 07:23

[IMP] l10n_it_fatturapa_in: Create intermediary contact by VAT

f319210

Chionne27 force-pushed the 14.0-IMP-l10n_it_fatturapa_in branch from 1da39c1 to f319210 Compare August 22, 2025 07:52

	def getPartnerBase(self, DatiAnagrafici, partnerCreate=True): # noqa: C901
	def getPartnerBase(self, DatiAnagrafici, partnerCreate=True):


		return vals

		def _find_partners_by_cf_vat(self, DatiAnagrafici):


		return vals

		def _find_commercial_partner(self, DatiAnagrafici, partners):

		elif isinstance(partner, bool):
		invoice.write({"intermediary": False})

		"vat": vat if vat else False,
		"fiscalcode": cf if cf else False,

		"lastname": new_data.get("lastname")
		or new_data.get("name"),

Uh oh!

[14.0][IMP] l10n_it_fatturapa_in: intermediary contact creation depending by vat #4621

Are you sure you want to change the base?

[14.0][IMP] l10n_it_fatturapa_in: intermediary contact creation depending by vat #4621

Uh oh!

Conversation

Chionne27 commented Feb 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

francesco-ooops left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

francesco-ooops left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SirPyTech left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Chionne27 Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Chionne27 commented Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Chionne27 commented Feb 17, 2025 •

edited

Loading

francesco-ooops left a comment •

edited

Loading

Chionne27 Aug 18, 2025 •

edited

Loading

Chionne27 commented Aug 18, 2025 •

edited

Loading

Chionne27 Aug 21, 2025 •

edited

Loading