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.

Sign up for GitHub

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

Jump to bottom

[IMP][14.0] l10n_it_ricevute_bancarie: small QoL improvements #4306

Merged
merged 3 commits into from
Aug 5, 2024
Merged

[IMP][14.0] l10n_it_ricevute_bancarie: small QoL improvements #4306

merged 3 commits into from
Aug 5, 2024

Conversation

aleuffre
Copy link
Contributor

  1. Improve readability between fields "Payment Term" and "Bank"
  2. Stop invoice confirmation if bank is not set. Closes RiBa: conferma fatture senza conto bancario da list view #4304 for 14.0
  3. Fall back on commercial partner's bank if bank is not set for invoice's contact. Closes RiBa: utilizzare banca del partner commerciale in caso di indirizzo di fatturazione #4303 for 14.0

@aleuffre
[IMP] l10n_it_ricevute_bancarie: readability of payment term and bank
78213a2 
Adds a dash "-" between the name of the payment term and the
name of the bank on the invoice, in case the payment term is a RiBa.

Previously, the name of the payment term and the bank were
contiguous, without even a space in between.
This was linked to issues Jul 30, 2024
RiBa: utilizzare banca del partner commerciale in caso di indirizzo di fatturazione #4303
Closed
RiBa: conferma fatture senza conto bancario da list view #4304
Closed
This was referenced Jul 30, 2024
Closed
Closed
francesco-ooops
francesco-ooops approved these changes Jul 30, 2024
View reviewed changes
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!

renda-dev
renda-dev approved these changes Jul 31, 2024
View reviewed changes
Copy link
Contributor

@renda-dev renda-dev 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!

MaurizioPellegrinet
MaurizioPellegrinet approved these changes Jul 31, 2024
View reviewed changes
Copy link

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

@francesco-ooops
Copy link
Contributor

@OCA/local-italy-maintainers si può mergiare? sono fixes abbastanza ovvi. grazie!

@MaurizioPellegrinet
Copy link

Riusciamo poi a passarla anche sulla 16?

@francesco-ooops
Copy link
Contributor

@MaurizioPellegrinet dopo il merge

@francesco-ooops
Copy link
Contributor

@OCA/local-italy-maintainers si può mergiare?

SirAionTech
SirAionTech approved these changes Aug 2, 2024
View reviewed changes
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!

  1. Improve readability between fields "Payment Term" and "Bank"

Per questo manca la issue, la potresti creare? È una modifica piccola ma potrebbe comunque essere utile anche in 16.0.

Per il resto mi sembra essere tutto ok; ho lasciato giusto qualche commento qui sotto ma secondo me nulla di bloccante per il merge.

l10n_it_ricevute_bancarie/models/account.py Outdated Show resolved Hide resolved
l10n_it_ricevute_bancarie/tests/test_riba.py Outdated Show resolved Hide resolved
l10n_it_ricevute_bancarie/models/account.py Outdated Show resolved Hide resolved
@francesco-ooops
Copy link
Contributor

  1. Improve readability between fields "Payment Term" and "Bank"

Per questo manca la issue, la potresti creare? È una modifica piccola ma potrebbe comunque essere utile anche in 16.0.

Sulla 16 mi pare ok:

image

@SirAionTech
Copy link
Contributor

  1. Improve readability between fields "Payment Term" and "Bank"

Per questo manca la issue, la potresti creare? È una modifica piccola ma potrebbe comunque essere utile anche in 16.0.

Sulla 16 mi pare ok:

image

Ah ecco perché non c'è la issue, ok grazie

SirAionTech
SirAionTech requested changes Aug 2, 2024
View reviewed changes
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.

Ora non è chiarissimo quale sia il cambiamento nel onchange, vedi il suggerimento in #4306 (comment)

GSLabIt
GSLabIt reviewed Aug 2, 2024
View reviewed changes
l10n_it_ricevute_bancarie/models/account.py Outdated
or self.riba_partner_bank_id not in self.partner_id.bank_ids
self.partner_id
and self.move_type in ("out_invoice", "out_refund")
and self.invoice_payment_term_id.riba
Copy link
Contributor

Choose a reason for hiding this comment

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

si potrebbe usare is_riba_payment che è relazionato

@aleuffre aleuffre force-pushed the 14.0-riba-imp branch 2 times, most recently from d9fbf20 to e62fac0 Compare August 2, 2024 15:16
@aleuffre
[IMP] l10n_it_ricevute_bancarie: stop invoice confirmation without bank
0aef745 
Cannot confirm an invoice with C/O (RiBa) payment if the bank has not
been set on the invoice.
@aleuffre
[IMP] l10n_it_ricevute_bancarie: fall back on commercial partner's bank
3a6a865 
If creating an invoice with C/O for a contact without a bank,
fall back on the commercial partner's bank.
@MaurizioPellegrinet
Copy link

@francesco-ooops @SirAionTech Sulla 16.0 non è risolto il problema sollevato dalla issue #4303

SirAionTech
SirAionTech approved these changes Aug 5, 2024
View reviewed changes
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.

/ocabot merge patch

l10n_it_ricevute_bancarie/models/account.py Show resolved Hide resolved
l10n_it_ricevute_bancarie/models/account.py Outdated Show resolved Hide resolved
@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 14.0-ocabot-merge-pr-4306-by-SirAionTech-bump-patch, awaiting test results.

@OCA-git-bot
Copy link
Contributor

@SirAionTech The merge process could not start, because command git push origin 14.0-ocabot-merge-pr-4306-by-SirAionTech-bump-patch failed with output:

To https://github.com/OCA/l10n-italy
 ! [remote rejected]     14.0-ocabot-merge-pr-4306-by-SirAionTech-bump-patch -> 14.0-ocabot-merge-pr-4306-by-SirAionTech-bump-patch (cannot lock ref 'refs/heads/14.0-ocabot-merge-pr-4306-by-SirAionTech-bump-patch': reference already exists)
error: failed to push some refs to 'https://github.com/OCA/l10n-italy'

@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). 🤖

1 similar comment
@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). 🤖

@OCA-git-bot OCA-git-bot merged commit 6501163 into OCA:14.0 Aug 5, 2024
7 checks passed
@OCA-git-bot
Copy link
Contributor

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

@SirAionTech SirAionTech mentioned this pull request Aug 21, 2024
Merged
@SirAionTech SirAionTech mentioned this pull request Sep 3, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@francesco-ooops francesco-ooops francesco-ooops approved these changes

@GSLabIt GSLabIt GSLabIt approved these changes

@renda-dev renda-dev renda-dev approved these changes

@MaurizioPellegrinet MaurizioPellegrinet MaurizioPellegrinet approved these changes

@SirAionTech SirAionTech SirAionTech approved these changes

Assignees
No one assigned
Labels
approved merged 🎉 ready to merge
Projects
None yet
Milestone
No milestone
7 participants
@aleuffre @francesco-ooops @MaurizioPellegrinet @SirAionTech @OCA-git-bot @GSLabIt @renda-dev