[14.0] Migration: l10n_it_split_payment#1959
Conversation
|
Bisogna riscrivere i test. C'è qualcuno che mi può aiutare ?? |
9d4377d to
fba9faf
Compare
SimoRubi
left a comment
There was a problem hiding this comment.
Grazie della PR!
So che hai problemi con i test, ma runbot ha un altro problema che secondo me si può correggere con quanto ti ho segnalato qui sotto.
Inoltre, Travis si ferma su black ancora prima di eseguirli, puoi correggere così vediamo che problemi riporta Travis per i test?
| "name": "ITA - Split Payment", | ||
| "version": "14.0.1.0.0", | ||
| "category": "Localization/Italy", | ||
| "summary": "Split Payment", |
There was a problem hiding this comment.
Puoi tradurre in italiano? Rif. https://www.odoo-italia.org/documentazione/12.0/sviluppo/guidelines.html#riepilogo-del-modulo
|
|
||
| amount_sp = fields.Float( | ||
| string="Split Payment", | ||
| digits=dp.get_precision("Account"), |
There was a problem hiding this comment.
Riporto il warning di Runbot:
2020-12-04 12:14:55,969 160 WARNING openerp_test odoo.addons.base.models.decimal_precision: Deprecated call to decimal_precision.get_precision(<application>), use digits=<application> instead
Puoi correggere?
Ho fatto Borruso#3 che mette a posto parte dei problemi, ora non ho tempo per fare altre modifiche ma penso non manchi molto |
5ea081c to
0ee0be9
Compare
Aggiunto le modifiche ai test che mancavano |
SimoRubi
left a comment
There was a problem hiding this comment.
Grazie della PR!
Ho fatto review del codice: ci sono un paio di modifiche che non mi sono del tutto chiare e qualche suggerimento.
| "date": self.date_invoice, | ||
| "debit": self.amount_sp, | ||
| "credit": 0, | ||
| "date": self.invoice_date or self.invoice_date_due, |
There was a problem hiding this comment.
Come mai qui e sotto hai aggiunto invoice_date_due?
There was a problem hiding this comment.
modificato e tenuto solo self.invoice_date
| {"credit": receivable_line_amount} | ||
| ) | ||
| write_off_line_vals = self._build_debit_line() | ||
| write_off_line_vals["move_id"] = self.id |
There was a problem hiding this comment.
Perché non includerlo direttamente in _build_debit_line?
Mi pare che qui la logica sia cambiata: a quanto vedo prima veniva modificata la riga receivable e veniva creata la riga di write off, ora invece crei solo la riga di write off, come mai?
There was a problem hiding this comment.
non esiste il conto su account.move
There was a problem hiding this comment.
Se ho capito bene le nuove modifiche, ora modifichi la riga receivable (line_client, trovata grazie al conto configurato sul partner) se c'è già la riga di write off, mentre la crei quando manca.
Prima invece la riga di write off veniva creata sempre, in action_move_create; come mai hai dovuto modificarlo? Forse era sbagliato prima?
There was a problem hiding this comment.
dato che ora il modulo account.invoice e account.move sono la medesima tabella, le registrazioni contabili vengono create da subito mentre sulla 12 la fattura veniva creata prima e dopo confermata venivano create le registrazioni
| def action_post(self): | ||
| for move in self: | ||
| if move.split_payment: | ||
| if move.move_type in ["in_invoice", "in_refund"]: |
There was a problem hiding this comment.
Puoi usare invece move.is_purchase_document()?
There was a problem hiding this comment.
modificato e utilizzato move.is_purchase_document()
| check = False | ||
| for line in self.line_ids: | ||
| if line.name == _("Split Payment Write Off"): | ||
| check = True |
There was a problem hiding this comment.
Puoi usare anche for..else, che è fatto proprio per questo caso d'uso: https://docs.python.org/3.6/tutorial/controlflow.html#break-and-continue-statements-and-else-clauses-on-loops.
Se ho capito bene stai cercando se esiste già una debit_line, non sarebbe più sicuro cercarla magari tramite un flag aggiunto alla account.move.line invece che per un confronto di stringhe?
Ad esempio potresti creare un campo account.move.line.is_split_writeoff e settarlo a True solo in _build_debit_line, poi nel for cerchi in base a quel flag, che ne pensi?
There was a problem hiding this comment.
modificato logica dovuto all'unione di accoun.invoice con account.move
b544d68 to
74d915c
Compare
|
nel fare i test ho trovato alcune cose che non vanno. |
83b16c0 to
eaa300b
Compare
MarcoCalcagni
left a comment
There was a problem hiding this comment.
Tested for me is OK it can be merged.
@eLBati go for merge please
|
@Borruso many thanks for the job done ! :-) 👍 |
|
Ci sono dei commenti di @SimoRubi senza risposta apparentemente |
SimoRubi
left a comment
There was a problem hiding this comment.
Ci sono un paio di punti in cui va controllato quello che trovi con filtered, vedi sotto
| "debit": self.amount_sp, | ||
| "credit": 0, | ||
| "date": self.invoice_date, | ||
| "date_maturity": self.invoice_date, |
There was a problem hiding this comment.
Come mai serve aggiungere questo campo?
There was a problem hiding this comment.
posso anche togliere il date_maturity
There was a problem hiding this comment.
Se non serve toglilo grazie, ma come mai l'avevi aggiunto?
| write_off_line_vals = self._build_debit_line() | ||
| line_sp = self.line_ids.filtered(lambda l: l.is_split_payment) | ||
| if line_sp: | ||
| dif_price = write_off_line_vals["price_unit"] - line_sp.price_unit |
There was a problem hiding this comment.
Se qui line_sp contenesse più record, verrebbe sollevata un'eccezione Expected singleton, puoi aggiungere un controllo? O forse queste operazioni andrebbero fatte per tutte le line_sp?
There was a problem hiding this comment.
Solo la riga che creo con la funzione _build_debit_line avrà il campo is_split_payment a True, quindi sarà sempre una sola riga.
There was a problem hiding this comment.
filtered può tornare più record, se deve tornarne solo uno va controllato sollevando un'eccezione o almeno aggiungendo un line_sp[0] da qualche parte per prevenire l'errore che ti ho indicato sopra
| == self.partner_id.property_account_receivable_id.id | ||
| ) | ||
| if line_client: | ||
| line_client.write({"price_unit": line_client.price_unit - dif_price}) |
There was a problem hiding this comment.
Anche qui, va gestito il caso in cui ci siano più record in line_client, attualmente modificherebbe tutte le righe trovate ma non penso sia corretto.
There was a problem hiding this comment.
qui dovrei fare un controllo
There was a problem hiding this comment.
Sei riuscito a fare questo controllo?
| if line_client: | ||
| line_client.write({"price_unit": line_client.price_unit - dif_price}) | ||
| else: | ||
| if self.id: |
There was a problem hiding this comment.
Come mai hai aggiunto questo controllo? Non ho trovato nulla di simile nel codice pre-migrazione.
There was a problem hiding this comment.
Se la fattura è in creazione ma non salvata va in errore perché come id ha idNew
There was a problem hiding this comment.
Credo sia rischioso distinguere così il comportamento tra quando la fattura è in creazione rispetto a quando è già creata perché ci si aspetta che il comportamento sia lo stesso.
Potresti invece togliere il campo move_id dalla riga di write off e assegnarla direttamente al campo self.invoice_line_ids, facendo ad esempio
| if self.id: | |
| self.invoice_line_ids = [(0, 0, write_off_line_vals)] |
o qualcosa di simile, che ne pensi?
There was a problem hiding this comment.
grazie per il suggerimento
MarcoCalcagni
left a comment
There was a problem hiding this comment.
Testato e funzione come dovrebbe
eaa300b to
f34ac27
Compare
|
Nell'installare un nuovo db, ho trovato questo problema: non credo sia un problema mio locale... Edit: ok ho visto adesso il log di runbot. Stesso problema. |
f34ac27 to
8b58f61
Compare
Aggiunto controllo se fattura presenta split payment allora esegue le funzioni di split. |
| "company_id", | ||
| "invoice_date", | ||
| "move_type", | ||
| ) |
There was a problem hiding this comment.
Decorando il metodo con api.depends rimuovi tutti i campi impostati dal api.depends del metodo nel super, puoi verificarlo durante l'esecuzione guardando self._compute_amount._depends.
Se qui non ti serve aggiungere nuovi campi, puoi evitare del tutto l'aggiunta del decoratore per mantenere i campi del metodo originale, altrimenti temo tu debba riportare quelli del metodo originale (forse si può usare qualche barbatrucco richiamando self._compute_amount._depends) e aggiungere i nuovi campi che ti servono
There was a problem hiding this comment.
eliminato api.depends
There was a problem hiding this comment.
Mi pare ci sia ancora, ma direi che non è una modifica strettamente necessaria per il merge perché ho scoperto che non è necessario ripetere tutti i campi nel depends del metodo nel super grazie a https://github.com/odoo/odoo/blob/b2235e2749504e6eee9e6f8090d1df54cf5971b7/odoo/fields.py#L433
| def set_receivable_line_ids(self): | ||
| self._recompute_dynamic_lines() | ||
| if self.move_type == "out_invoice": | ||
| line_client_ids = self.line_ids.filtered( |
There was a problem hiding this comment.
Qui e nel metodo sotto ripeti due volte praticamente lo stesso codice, riesci a evitare la duplicazione?
Nota che per accedere ad esempio al campo line_client.debit puoi anche usare line_client['debit']
There was a problem hiding this comment.
evitato la duplicazione
There was a problem hiding this comment.
Non mi pare, ma direi che non è una modifica strettamente necessaria per il merge
| return vals | ||
|
|
||
| def set_receivable_line_ids(self): | ||
| self._recompute_dynamic_lines() |
There was a problem hiding this comment.
Puoi aggiungere qualche riga di documentazione ai nuovi metodi che spieghi cosa fanno?
| ) / inv_total | ||
| else: | ||
| receivable_line_amount = 0 | ||
| line_client.with_context(check_move_validity=False).write( |
There was a problem hiding this comment.
Questi metodi vengono spesso chiamati anche durante gli onchange, quindi è meglio evitare di chiamare i metodi CRUD come spiegato in https://github.com/odoo/odoo/blob/9b7a4e1815de580575bed6657f9f8915907ce534/odoo/api.py#L162-L171.
Puoi sostituire i vari write con update o semplicemente assegnare il valore ai campi?
There was a problem hiding this comment.
sostituito write con update o assegnando il valore
| if line_sp: | ||
| if ( | ||
| self.move_type == "out_invoice" | ||
| and line_sp.debit != write_off_line_vals["debit"] |
There was a problem hiding this comment.
aggiunto [0] al line_sp
|
Sostituita da #2234 |
Porting del modulo l10n_it_split_payment dalla versione 12.0 alla 14.0
--
Confermo di aver firmato il CLA https://odoo-community.org/page/cla e di aver letto le linee guida su https://odoo-community.org/page/contributing