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

[14.0][FIX] l10n_it_ricevute_bancarie: avoid loosing reference to lines partner #2821

Merged
merged 1 commit into from
Jul 19, 2022
Merged

[14.0][FIX] l10n_it_ricevute_bancarie: avoid loosing reference to lines partner #2821

merged 1 commit into from
Jul 19, 2022

Conversation

michelerusti
Copy link
Contributor

@michelerusti michelerusti commented May 30, 2022 •
edited
Loading

Corregge #2829 per 14.0.

@michelerusti michelerusti mentioned this pull request Jun 8, 2022
Closed
3 tasks
@michelerusti michelerusti force-pushed the 14.0-fix-l10n_it_ricevute_bancarie branch from 1cef4f8 to 43f49bb Compare June 8, 2022 15:30
tafaRU
tafaRU reviewed Jun 10, 2022
View reviewed changes
Copy link
Member

@tafaRU tafaRU left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM 👍
Approvo perché i commenti che ho evidenziato sono solo dei nitpick.

Altra nota: potresti sostituire la descrizione della PR con quanto riportato qui sotto?

Corregge #2829 per 14.0.

Così facendo il collegamento con la corrispondente issue di tracciamento è più immediato.
Grazie!

l10n_it_ricevute_bancarie/models/account.py Outdated Show resolved Hide resolved
l10n_it_ricevute_bancarie/models/account.py Outdated Show resolved Hide resolved
l10n_it_ricevute_bancarie/models/account.py Outdated Show resolved Hide resolved
@michelerusti michelerusti force-pushed the 14.0-fix-l10n_it_ricevute_bancarie branch from 43f49bb to 4473038 Compare June 15, 2022 15:26
@tafaRU
Copy link
Member

tafaRU commented Jun 16, 2022

tests / test with Odoo fallisce per #2836

@TheMule71
Copy link
Contributor

Dovrebbe bastare un rebase.

@michelerusti michelerusti force-pushed the 14.0-fix-l10n_it_ricevute_bancarie branch from 4473038 to 763e05e Compare June 20, 2022 07:11
@tafaRU tafaRU requested a review from TheMule71 June 30, 2022 14:17
TheMule71
TheMule71 approved these changes Jul 1, 2022
View reviewed changes
Copy link
Contributor

@TheMule71 TheMule71 left a comment

Choose a reason for hiding this comment

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

LGTM

SirTakobi
SirTakobi approved these changes Jul 1, 2022
View reviewed changes
Copy link
Contributor

@SirTakobi SirTakobi left a comment

Choose a reason for hiding this comment

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

Riporto quanto dedotto in chiamata: il problema in realtà sembra essere di _recompute_tax_lines che se chiamato senza aver prima scollegato le righe non si comporta correttamente e, a quanto vedo dalla issue, scollega i partner.

Ho fatto solo revisione del codice e secondo me va bene; visto che è una fix riesci mica ad aggiungere un test che verifichi il comportamento corretto?
Altrimenti per me si può mergiare anche così

l10n_it_ricevute_bancarie/models/account.py Outdated
return invoice

def get_line_ids_to_remove(self, invoice):
Copy link
Contributor

Choose a reason for hiding this comment

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

Da un metodo get_line_ids_to_remove mi aspetterei che restituisca una lista di ID, così sarebbe anche più riutilizzabile in altri punti.

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

SirTakobi
SirTakobi approved these changes Jul 1, 2022
View reviewed changes
Copy link
Contributor

@SirTakobi SirTakobi left a comment

Choose a reason for hiding this comment

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

Riporto quanto dedotto in chiamata: il problema in realtà sembra essere di _recompute_tax_lines che se chiamato senza aver prima scollegato le righe non si comporta correttamente e, a quanto vedo dalla issue, scollega i partner.

Ho fatto solo revisione del codice e secondo me va bene; visto che è una fix riesci mica ad aggiungere un test che verifichi il comportamento corretto?
Altrimenti per me si può mergiare anche così

@michelerusti michelerusti force-pushed the 14.0-fix-l10n_it_ricevute_bancarie branch from 763e05e to a4602c6 Compare July 19, 2022 10:17
tafaRU
tafaRU requested changes Jul 19, 2022
View reviewed changes
l10n_it_ricevute_bancarie/models/account.py Outdated Show resolved Hide resolved
l10n_it_ricevute_bancarie/models/account.py Outdated Show resolved Hide resolved
@michelerusti michelerusti force-pushed the 14.0-fix-l10n_it_ricevute_bancarie branch from a4602c6 to 089193d Compare July 19, 2022 13:14
@michelerusti
[14.0][FIX] l10n_it_ricevute_bancarie: avoid loosing reference to lin…
cd9d044 
…es partner

Itroduce a check on the array because when no line has a .due_cost_line, the actions modified deleted the partner's reference in all the lines
@michelerusti michelerusti force-pushed the 14.0-fix-l10n_it_ricevute_bancarie branch from 089193d to cd9d044 Compare July 19, 2022 14:57
@tafaRU
Copy link
Member

tafaRU commented Jul 19, 2022

/ocabot merge patch

@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-2821-by-tafaRU-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 97a56f9 into OCA:14.0 Jul 19, 2022
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 8704bd5. 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
Reviewers

@SirTakobi SirTakobi SirTakobi approved these changes

@tafaRU tafaRU tafaRU approved these changes

@TheMule71 TheMule71 TheMule71 approved these changes

Assignees
No one assigned
Labels
approved merged 🎉
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@michelerusti @tafaRU @TheMule71 @OCA-git-bot @SirTakobi