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

[14.0][REF] l10n_it_delivery_note: refactoring compute invoice status #2327

Merged

Conversation

Borruso
Copy link
Contributor

@Borruso Borruso commented Jun 11, 2021

FIX: create ddt without sale order

--
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

TheMule71 added a commit to odoo-italia/l10n-italy that referenced this pull request Jul 17, 2021
TheMule71 added a commit to odoo-italia/l10n-italy that referenced this pull request Jul 18, 2021
TheMule71 added a commit to odoo-italia/l10n-italy that referenced this pull request Jul 20, 2021
TheMule71 added a commit to odoo-italia/l10n-italy that referenced this pull request Jul 30, 2021
TheMule71 added a commit to odoo-italia/l10n-italy that referenced this pull request Jul 30, 2021
TheMule71 added a commit to odoo-italia/l10n-italy that referenced this pull request Jul 30, 2021
TheMule71 added a commit to odoo-italia/l10n-italy that referenced this pull request Jul 30, 2021
TheMule71 added a commit to odoo-italia/l10n-italy that referenced this pull request Aug 6, 2021
TheMule71 added a commit to odoo-italia/l10n-italy that referenced this pull request Aug 6, 2021
TheMule71 added a commit to odoo-italia/l10n-italy that referenced this pull request Aug 20, 2021
TheMule71 added a commit to odoo-italia/l10n-italy that referenced this pull request Aug 20, 2021
Copy link
Member

@SimoRubi SimoRubi 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!
Puoi aggiungere un test automatico?

Vedo che il codice qui era uguale a com'è attualmente in 12.0:

if lines and all(
line.invoice_status == DOMAIN_INVOICE_STATUSES[2]
for line in lines):
note.state = DOMAIN_DELIVERY_NOTE_STATES[2]
note.invoice_status = DOMAIN_INVOICE_STATUSES[2]
elif any(line.invoice_status == DOMAIN_INVOICE_STATUSES[1]
for line in lines):
note.invoice_status = DOMAIN_INVOICE_STATUSES[1]
else:
note.invoice_status = DOMAIN_INVOICE_STATUSES[0]

se il problema è anche in 12.0 puoi creare una issue di tracciamento come descritto in https://github.com/OCA/l10n-italy/wiki/Team-di-sviluppo#apertura-issue?

Occhio poi che attualmente queste modifiche vanno in conflitto con il codice di 14.0

@Borruso Borruso force-pushed the 14.0-fix-create-ddt-without-sale-order branch from 260eeb3 to baeb924 Compare August 27, 2021 13:20
TheMule71 added a commit to odoo-italia/l10n-italy that referenced this pull request Sep 3, 2021
TheMule71 added a commit to odoo-italia/l10n-italy that referenced this pull request Sep 3, 2021
TheMule71 added a commit to odoo-italia/l10n-italy that referenced this pull request Sep 17, 2021
TheMule71 added a commit to odoo-italia/l10n-italy that referenced this pull request Sep 17, 2021
TheMule71 added a commit to odoo-italia/l10n-italy that referenced this pull request Oct 1, 2021
TheMule71 added a commit to odoo-italia/l10n-italy that referenced this pull request Oct 1, 2021
TheMule71 added a commit to odoo-italia/l10n-italy that referenced this pull request Oct 8, 2021
TheMule71 added a commit to odoo-italia/l10n-italy that referenced this pull request Oct 8, 2021
TheMule71 added a commit to odoo-italia/l10n-italy that referenced this pull request Oct 15, 2021
TheMule71 added a commit to odoo-italia/l10n-italy that referenced this pull request Oct 15, 2021
@TheMule71
Copy link
Contributor

A me sfugge la differenza tra i due codici. In quale caso il comportamento del vecchio codice è diverso dal nuovo. Se lo identifichiamo aggiungere un test diventa + facile.

Copy link
Member

@SimoRubi SimoRubi left a comment

Choose a reason for hiding this comment

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

Se non è chiaro quale sia il problema che viene risolto, declasserei il tutto a un refactoring quindi REF (da https://www.odoo.com/documentation/14.0/developer/misc/other/guidelines.html#tag-and-module-name) quantomeno nel commit.

Vedendolo come un refactoring, il codice mi pare effettivamente più leggibile così come lo vorresti cambiare.

Ho aggiunto un commento ma non è bloccante per la mia approvazione, l'unica cosa secondo me necessaria è cambiare il tipo di commit.

Comment on lines 289 to 299
note.invoice_status = DOMAIN_INVOICE_STATUSES[0]
if lines:
if all(
line.invoice_status == DOMAIN_INVOICE_STATUSES[2] for line in lines
):
note.state = DOMAIN_DELIVERY_NOTE_STATES[2]
note.invoice_status = DOMAIN_INVOICE_STATUSES[2]
elif any(
line.invoice_status == DOMAIN_INVOICE_STATUSES[1] for line in lines
):
note.invoice_status = DOMAIN_INVOICE_STATUSES[1]
Copy link
Member

Choose a reason for hiding this comment

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

Riusciresti a evitare di assegnare più volte note.invoice_status?
Per farlo potresti ad esempio salvare il valore che vuoi in una variabile ad esempio invoice_status e poi assegnarlo subito dopo la conclusione del if lines

@Borruso Borruso changed the title [14.0][FIX] l10n_it_delivery_note: Fix create ddt without sale order [14.0][REF] l10n_it_delivery_note: refactoring compute invoice status Oct 29, 2021
@Borruso Borruso force-pushed the 14.0-fix-create-ddt-without-sale-order branch from baeb924 to fb484bf Compare October 29, 2021 12:23
@Borruso Borruso requested a review from SimoRubi October 29, 2021 12:23
Copy link
Member

@SimoRubi SimoRubi 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!
Revisione del codice, per me è ok

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@tafaRU
Copy link
Member

tafaRU commented Oct 29, 2021

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 14.0-ocabot-merge-pr-2327-by-tafaRU-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit c814e7e into OCA:14.0 Oct 29, 2021
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at dd73db0. Thanks a lot for contributing to OCA. ❤️

TheMule71 added a commit to TheMule71/l10n-italy that referenced this pull request Jan 21, 2022
TheMule71 added a commit to TheMule71/l10n-italy that referenced this pull request Jan 21, 2022
TheMule71 added a commit to TheMule71/l10n-italy that referenced this pull request Jan 21, 2022
TheMule71 added a commit to TheMule71/l10n-italy that referenced this pull request Jan 21, 2022
TheMule71 added a commit to odoo-italia/l10n-italy that referenced this pull request Jun 29, 2022
TheMule71 added a commit to odoo-italia/l10n-italy that referenced this pull request Jun 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants