-
-
Notifications
You must be signed in to change notification settings - Fork 316
[14.0][IMP] l10n_it_ricevute_bancarie: aggiunta spesa incasso insoluto #4642
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
[14.0][IMP] l10n_it_ricevute_bancarie: aggiunta spesa incasso insoluto #4642
Conversation
07ecf54
to
6e4d841
Compare
56203ac
to
f1439ad
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Funzionale ok, come da issue:
Ogni volta che si registra un insoluto per riba SBF, è necessario inserire manualmente l'importo prelevato e l'eventuale importo delle spese dell'insoluto.
Questi due campi possono essere precompilati a seconda della configurazione RiBa, impostando delle spese per l'insoluto e calcolando l'importo prelevato come importo della riga distinta + le spese insoluto.
Questa funzionalità è propedeutica ad un'implementazione di un'eventuale automazione della registrazione degli insoluti (utile nel caso ci sia il bisogno di registrare decine di insoluti ogni settimana)
@odooNextev vedete problemi con questa implementazione? |
@OCA/local-italy-maintainers si può mergiare? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Grazie della PR!
Sarebbe possibile aggiungere un test per questo nuovo flusso?
Credo siano sufficienti poche righe per impostare un valore nella configurazione per il nuovo campo e verificare che nel wizard dell'insoluto il valore venga calcolato correttamente.
expense_amount = fields.Float( | ||
"Past due fee", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggerimento (non bloccante): Potresti allineare la definizione del campo (e i relativi metodi) con la sua descrizione?
Ad esempio:
expense_amount = fields.Float( | |
"Past due fee", | |
past_due_fee_amount = fields.Float( | |
"Past due fee", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
def get_default_value_unsolved_expense_amount(self): | ||
if not self.env.context.get("active_id", False): | ||
return 0.0 | ||
ribalist_line = self.env["riba.distinta.line"].browse( | ||
self.env.context["active_id"] | ||
) | ||
return ribalist_line.distinta_id.config_id.expense_amount |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Problema: Questo metodo è in buona parte un duplicato del metodo sopra, solo che il metodo sopra è generico e funziona per quasi tutti i campi della configurazione, mentre il metodo aggiunto funziona solo per il nuovo campo.
Penso sia meglio evitare questo duplicato e piuttosto adattare il metodo generico per permettergli di gestire il nuovo campo.
Puoi vedere come è stata fatta un'operazione simile, proprio su questi metodi, in https://github.com/OCA/l10n-italy/pull/4648/files#diff-fe0d1ed5dbb8e1838eedce43d6334a166986efc36d6b35e22d4fb76368ee9f76R115-R120:
res = ribalist_line.slip_id.config_id[field_name]
if res:
# we need to check if the field is string like "config_type" or
# many2one like journals and accounts
if isinstance(res, str):
return res
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Grazie della dritta!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
expense_amount = fields.Float("Fees Amount") | ||
expense_amount = fields.Float("Past due fee", default=_get_unsolved_expense_amount) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Domanda: Prima questo campo era per qualsiasi fee, ora invece si può usare solo per quelle dell'insoluto: come mai? Non ce ne sono effettivamente altre?
Credo che nel dubbio sia meglio lasciarlo generico come era prima.
A seconda di come viene usato il campo si potrebbe allineare il campo creato per il default nella configurazione.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
si in effetti è meglio se rimane generico, l'ho cambiato secondo la tua review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Ora il campo ha cambiato nome, normalmente servirebbe una migrazione ma in questo caso secondo me possiamo non farla perché è un wizard quindi i valori dei record esistenti non servono più.
f1439ad
to
095f3a7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Grazie mille del test! 🙏
Ho fatto solo revisione del codice, per me è ok
expense_amount = fields.Float( | ||
"Past due fee", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
def get_default_value_unsolved_expense_amount(self): | ||
if not self.env.context.get("active_id", False): | ||
return 0.0 | ||
ribalist_line = self.env["riba.distinta.line"].browse( | ||
self.env.context["active_id"] | ||
) | ||
return ribalist_line.distinta_id.config_id.expense_amount |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
expense_amount = fields.Float("Fees Amount") | ||
expense_amount = fields.Float("Past due fee", default=_get_unsolved_expense_amount) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Ora il campo ha cambiato nome, normalmente servirebbe una migrazione ma in questo caso secondo me possiamo non farla perché è un wizard quindi i valori dei record esistenti non servono più.
@@ -309,6 +309,7 @@ def test_riba_incasso_flow(self): | |||
self.assertEqual(self.invoice.payment_state, "paid") | |||
|
|||
def test_unsolved_riba(self): | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
polish (non blocking)
Questa nuova riga vuota si può rimuovere
095f3a7
to
e002561
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TEST FUNZIONALE OK
/ocabot merge minor |
On my way to merge this fine PR! |
Congratulations, your PR was merged at 184d806. Thanks a lot for contributing to OCA. ❤️ |
-Aggiunti i seguenti campi nella sezione "insoluti" nel Configuratore RiBa:
Importo Spese;
-Aggiunto nel Wizard della registrazione insoluti il precompilamento del campo "importo prelevato".