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

[IMP] l10n_it_account_stamp usability #4414

Open
wants to merge 1 commit into
base: 14.0
Choose a base branch
from

Conversation

sergiocorato
Copy link
Contributor

No description provided.

@SirAionTech SirAionTech linked an issue Oct 18, 2024 that may be closed by this pull request
3 tasks
@sergiocorato sergiocorato force-pushed the 14.0-imp-l10n_it_account_stamp-usability branch from f29210b to 68743e1 Compare October 18, 2024 10:25
@SirAionTech SirAionTech added the needs fixing Has conflicts or is failing mandatory CI checks label Oct 18, 2024
@sergiocorato sergiocorato force-pushed the 14.0-imp-l10n_it_account_stamp-usability branch from 68743e1 to bb017a1 Compare October 24, 2024 13:07
@SirAionTech SirAionTech removed the needs fixing Has conflicts or is failing mandatory CI checks label Oct 24, 2024
@SirAionTech SirAionTech added the is porting This pull request is porting a change from another version label Nov 8, 2024
@TheMule71 TheMule71 mentioned this pull request Nov 8, 2024
33 tasks
Copy link
Contributor

@SirAionTech SirAionTech left a comment

Choose a reason for hiding this comment

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

Grazie della PR!
Ho guardato il codice e credo che l'aggiunta di check_move_validity=Falsecausi questo errore

Screencast.from.2024-11-21.09-55-14.webm

puoi verificare?

@@ -135,6 +135,7 @@ Contributors
* Marco Colombo <https://github.com/TheMule71>
* Gianmarco Conte <[email protected]>
* Giovanni Serra <[email protected]>
* `Aion Tech <https://aiontech.company/>`__: Simone Rubino <[email protected]>
Copy link
Contributor

Choose a reason for hiding this comment

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

Non va modificato il README direttamente ma il file readme/CONTRIBUTORS.rst.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fatto

@sergiocorato sergiocorato force-pushed the 14.0-imp-l10n_it_account_stamp-usability branch from bb017a1 to e2ea9e0 Compare December 5, 2024 13:50
If stamp line can be added (invoice in draft): show button to add stamp line.
If stamp line can't be added (invoice not in draft): show message explaining why it can't be added.
If stamp line has already been added: show message saying it has already been added (useful if invoice has many lines).
Button and messages update live while updating the invoice.
Better messages to user for charging stamp to customer

Co-authored-by:  Simone Rubino <[email protected]>
@sergiocorato sergiocorato force-pushed the 14.0-imp-l10n_it_account_stamp-usability branch from e2ea9e0 to d1b8ba1 Compare December 5, 2024 13:54
@primes2h
Copy link
Contributor

primes2h commented Jan 6, 2025

Grazie della PR! Ho guardato il codice e credo che l'aggiunta di check_move_validity=Falsecausi questo errore
Screencast.from.2024-11-21.09-55-14.webm

Senza quell'aggiunta la stessa eccezione compare prima, cliccando su "Reset to Draft". La modifica ne posticipa solo la comparsa alla riconferma della fattura.

In ogni caso noto una problematica di fondo decisamente più rilevante introdotta da queste modifiche (già presenti nella 16.0).

Premessa.
Nella migrazione alla 14.0 il campo is_stamp_line è stato introdotto per consentire di identificare solamente le due righe di movimenti (income e expense) generati, alla conferma della fattura, dall'aggiunta del bollo alla fattura stessa (non mi riferisco all'imputazione del bollo stesso al cliente). Vedi qui e qui.
Quindi, in questo contesto, l'introduzione di move_line_tax_stamp_ids.unlink() permetteva di eliminare le suddette righe reimpostando la fattura a bozza.

Premesso ciò, proseguo nella revisione del codice.

Copy link
Contributor

@primes2h primes2h left a comment

Choose a reason for hiding this comment

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

@@ -76,6 +85,7 @@ def add_tax_stamp_line(self):
invoice_line_vals = {
"move_id": inv.id,
"product_id": stamp_product_id.id,
"is_stamp_line": True,
Copy link
Contributor

Choose a reason for hiding this comment

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

Introdurre il campo qui stravolge il senso originale del campo stesso, cioè quello di identificare univocamente le sole righe contabili income ed expense, per poterle eliminare dopo la reimpostazione della fattura a bozza.

L'effetto collaterale della modifica è che, quando la fattura viene reimpostata bozza, viene eliminata anche la riga fattura di imputazione del bollo al cliente, che non è il comportamento atteso.

)
def _compute_tax_stamp_line_present(self):
for invoice in self:
invoice.tax_stamp_line_present = invoice.is_tax_stamp_line_present()
Copy link
Contributor

Choose a reason for hiding this comment

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

is_tax_stamp_line_present() non identifica univocamente la riga fattura di imputazione del bollo al cliente perché va a prendere il valore da is_stamp_line.

Nel caso di bollo applicato ma non imputato al cliente tax_stamp_line_present è sempre True e fa comparire "Stamp charged to customer" alla conferma della fattura.
Questo è il comportamento attuale della 16.0 dopo il merge della relativa PR e andrà corretto.

Suggested change
invoice.tax_stamp_line_present = invoice.is_tax_stamp_line_present()
invoice.tax_stamp_line_present = invoice.is_tax_stamp_product_present()

Comment on lines -186 to +206
move_line_tax_stamp_ids.unlink()
move_line_tax_stamp_ids.with_context(check_move_validity=False).unlink()
Copy link
Contributor

Choose a reason for hiding this comment

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

Manterrei la riga di codice attuale senza modifiche.

compute="_compute_tax_stamp",
store=True,
)
tax_stamp_line_present = fields.Boolean(
Copy link
Contributor

@primes2h primes2h Jan 16, 2025

Choose a reason for hiding this comment

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

#4414 (comment) e #4414 (comment)

Suggested change
tax_stamp_line_present = fields.Boolean(
tax_stamp_invoice_line_present = fields.Boolean(

)
tax_stamp_line_present = fields.Boolean(
string="Stamp line is present in invoice",
compute="_compute_tax_stamp_line_present",
Copy link
Contributor

@primes2h primes2h Jan 16, 2025

Choose a reason for hiding this comment

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

Vedi sopra

Suggested change
compute="_compute_tax_stamp_line_present",
compute="_compute_tax_stamp_invoice_line_present",

"invoice_line_ids.product_id",
"invoice_line_ids.product_id.is_stamp",
)
def _compute_tax_stamp_line_present(self):
Copy link
Contributor

@primes2h primes2h Jan 16, 2025

Choose a reason for hiding this comment

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

Vedi sopra.

Suggested change
def _compute_tax_stamp_line_present(self):
def _compute_tax_stamp_invoice_line_present(self):

Comment on lines +118 to +120
def _compute_tax_stamp_line_present(self):
for invoice in self:
invoice.tax_stamp_line_present = invoice.is_tax_stamp_line_present()
Copy link
Contributor

Choose a reason for hiding this comment

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

Vedi sopra.

invoice.tax_stamp_line_presentinvoice.tax_stamp_invoice_line_present

@@ -23,20 +27,38 @@
name="manually_apply_tax_stamp"
attrs="{'invisible': [('auto_compute_stamp', '=', True)]}"
/>
<field name="tax_stamp_line_present" invisible="1" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Vedi sopra.

Suggested change
<field name="tax_stamp_line_present" invisible="1" />
<field name="tax_stamp_invoice_line_present" invisible="1" />

alt="Tax stamp"
/>
<span
attrs="{'invisible': [('tax_stamp_line_present', '=', True)]}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Vedi sopra.

Suggested change
attrs="{'invisible': [('tax_stamp_line_present', '=', True)]}"
attrs="{'invisible': [('tax_stamp_invoice_line_present', '=', True)]}"

</span>
</span>
<span
attrs="{'invisible': [('tax_stamp_line_present', '=', False)]}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Vedi sopra.

Suggested change
attrs="{'invisible': [('tax_stamp_line_present', '=', False)]}"
attrs="{'invisible': [('tax_stamp_invoice_line_present', '=', False)]}"

Comment on lines +101 to +124
def test_tax_stamp_line_button(self):
"""Stamp fields show when stamp is added with the button to the invoice."""
# Arrange: Create an invoice eligible for tax stamp but without it
stamp_tax = self.tax_id
invoice = self.init_invoice(
"out_invoice",
taxes=stamp_tax,
amounts=[
100,
],
)
# pre-condition
self.assertTrue(invoice.tax_stamp)
self.assertFalse(invoice.tax_stamp_line_present)

# Act
invoice.add_tax_stamp_line()

# Assert
self.assertTrue(invoice.tax_stamp_line_present)

# Resetting to draft removes the stamp
invoice.button_draft()
self.assertFalse(invoice.tax_stamp_line_present)
Copy link
Contributor

Choose a reason for hiding this comment

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

Questo test va tolto perché controlla che la riga fattura di addebito del bollo venga eliminata quando la fattura viene reimpostata a bozza, che non è il comportamento atteso.
(vedi punto 2. di #4528)

Per la reimpostazione a bozza della fattura andrebbe invece aggiunto il test proposto qui:
5a78dff

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is porting This pull request is porting a change from another version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[IMP] l10n_it_account_stamp: Usabilità per il bollo in fattura
4 participants