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] [FIX] l10n_it_delivery_note,l10n_it_delivery_note_order_link: delivery note count in sale order #4280

Merged

Conversation

renda-dev
Copy link
Contributor

Sono presenti un paio di problemi riguardo il contatore dei delivery note collegato agli ordini di vendita:

  • Il campo delivery_note_count e delivery_note_ids sul sale.order è duplicato
    • Rimosso le parti di codice inutilizzate dal modulo base l10n_it_delivery_note
  • A parer mio, prendere i delivery note collegati all'ordine, dalle righe dell'ordine, è un giro inusuale che può essere confusionario e sbagliato, mentre prendere i delivery note direttamente dai picking collegati all'ordine mi sembra più diretto ed efficiente

@OCA-git-bot
Copy link
Contributor

Hi @andreampiovesana, @MarcoCalcagni, @aleuffre,
some modules you are maintaining are being modified, check this out!

Since it's a not used anywhere, and it's a duplicate from l10n_it_delivery_note_order_link
In sale orders, instead of getting them from the order lines
Copy link
Contributor

@francesco-ooops francesco-ooops left a comment

Choose a reason for hiding this comment

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

Funzionale ok!

Copy link
Contributor

@aleuffre aleuffre left a comment

Choose a reason for hiding this comment

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

A parte la problematica segnalata, il codice mi sembra ok.

A più alto livello, l10n_it_delivery_note_order_link in alcuni punti sembra essere un modulo che voleva aggiungere il link solo tra PO e Delivery note, in altri casi invece sembra voler aggiungere link sia a SO che a PO. Sarei per adeguare e rendere consistente nome e documentazione del modulo in questo senso.

Inoltre, non so se ci sono persone che utilizzano solo l10n_it_delivery_note e "perdono" in questo modo una funzionalità. Evidentemente la funzionalità era duplicata, ma non so se storicamente era stata mossa nel modulo base per un motivo.

In altre parole, la domanda aperta è: è giusto spostare lo smart button nel modulo separato? Le persone che stanno usando solo l10n_it_delivery_note se ne avranno a male?

@@ -9,11 +9,6 @@
class SaleOrder(models.Model):
_inherit = "sale.order"

delivery_note_ids = fields.Many2many(
Copy link
Contributor

Choose a reason for hiding this comment

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

issue: Sono stati rimossi dei campi e una funzione dal model di l10n_it_delivery_note ma non è stata adeguata la view. Mi aspetto che installando solo il modulo l10n_it_delivery_note senza _order_link dia più di un errore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ho fatto qualche test funzionale e non mi risultano problemi installando solamente il modulo l10n_it_delivery_note.

Pensi che il collegamento da SO a DDT andrebbe spostato tutto nel modulo base? Oppure spostare solamente i field sul model e lasciare la visualizzazione sul modulo _order_link?

Copy link
Contributor

Choose a reason for hiding this comment

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

Per il primo punto: Hai ragione, chiedo scusa, stavo guardando la view di account_move invece che quella del SO e mi sembrava che ci fosse ancora il bottone e i campi.

Visto che ci sono i campi sul model ma non c'è il bottone e non vengono mostrati all'utente, di fatto non servono a molto. Sono d'accordo a tenere tutto su _order_link a questo punto.

Copy link
Contributor

@aleuffre aleuffre Jul 22, 2024

Choose a reason for hiding this comment

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

non bloccante: volendo si può giusto fare un piccolo adeguamento del nome/documentazione del modulo _order_link.

Copy link
Contributor

@aleuffre aleuffre left a comment

Choose a reason for hiding this comment

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

Code review, LGTM

@aleuffre
Copy link
Contributor

/ocabot merge minor

@OCA-git-bot
Copy link
Contributor

Sorry @aleuffre you are not allowed to merge.

To do so you must either have push permissions on the repository, or be a declared maintainer of all modified addons.

If you wish to adopt an addon and become it's maintainer, open a pull request to add your GitHub login to the maintainers key of its manifest.

@francesco-ooops
Copy link
Contributor

/ocabot merge minor

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 14.0-ocabot-merge-pr-4280-by-francesco-ooops-bump-minor, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Jul 22, 2024
Signed-off-by francesco-ooops
@OCA-git-bot
Copy link
Contributor

It looks like something changed on 14.0 in the meantime.
Let me try again (no action is required from you).
Prepared branch 14.0-ocabot-merge-pr-4280-by-francesco-ooops-bump-minor, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 37041d7 into OCA:14.0 Jul 22, 2024
5 of 7 checks passed
@OCA-git-bot
Copy link
Contributor

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

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.

[l10n_it_delivery_note,l10n_it_delivery_note_order_link] - delivery note count in sale order
4 participants