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

[16.0][MIG] account_invoice_margin: Migration to v16 #163

Merged
merged 39 commits into from
Jun 2, 2023

Conversation

ljsalvatierra-factorlibre

@legalsylvain
Copy link
Contributor

Hi. could you take a look on CI ?
thanks !

@ljsalvatierra-factorlibre
Copy link
Author

Hi. could you take a look on CI ? thanks !

Hi, yes, i am resolving the issues.

@ljsalvatierra-factorlibre ljsalvatierra-factorlibre changed the title [16.0][MIG] account_invoice_margin: Migration to v16 [WIP][16.0][MIG] account_invoice_margin: Migration to v16 Mar 1, 2023
@ljsalvatierra-factorlibre
Copy link
Author

Hi, can anyone review this PR? Thanks in advance

@pedrobaeza
Copy link
Member

You may find reviewers by chance if someone is interested, but a non selfish way of collaborating is to review other PRs, and ask in exchange that they review yours. When you are a good contributor, there will be other good contributors that will be will to make the reviews for free, but you have to equal the balance.

Anyway, if it's ready, remove the [WIP] label from the title.

@ljsalvatierra-factorlibre ljsalvatierra-factorlibre changed the title [WIP][16.0][MIG] account_invoice_margin: Migration to v16 [16.0][MIG] account_invoice_margin: Migration to v16 Mar 8, 2023
@ljsalvatierra-factorlibre
Copy link
Author

You may find reviewers by chance if someone is interested, but a non selfish way of collaborating is to review other PRs, and ask in exchange that they review yours. When you are a good contributor, there will be other good contributors that will be will to make the reviews for free, but you have to equal the balance.

Anyway, if it's ready, remove the [WIP] label from the title.

Totally agree with you, i'll see what i can do to improve that :)

@legalsylvain
Copy link
Contributor

Pre-commit is still red. Could you take a look ?

@ljsalvatierra-factorlibre ljsalvatierra-factorlibre force-pushed the 16.0-mig-account_invoice_margin branch 2 times, most recently from 9d1ac7b to f6d4953 Compare March 8, 2023 11:51
@ljsalvatierra-factorlibre
Copy link
Author

Pre-commit is still red. Could you take a look ?

Fixed, is it possible to re-run the tests with Odoo?

@legalsylvain
Copy link
Contributor

if you rebase, it will rerun the CI.

@ljsalvatierra-factorlibre
Copy link
Author

if you rebase, it will rerun the CI.

Yes, but it is already up to date.

@ljsalvatierra-factorlibre
Copy link
Author

Fixed one of the README images name.

Copy link
Contributor

@legalsylvain legalsylvain left a comment

Choose a reason for hiding this comment

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

Hi.

since OCA introduced _get_margin_applicable_lines features, margin can be computed correctly removing non relevant invoice lines. (down payment lines for example)

however, the sale_margin odoo module does'nt implement such feature and so the margin in sale.order can be wrong. (same for margin in pos.order) do you face the same issue ?
If yes do you have an idea how to fix it ?

thanks !

margin = 0.0
margin_signed = 0.0
price_subtotal = 0.0
for line in invoice._get_margin_applicable_lines():
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for line in invoice._get_margin_applicable_lines():
for line in invoice.line_ids.filtered(lambda x: x.is_margin_applicable):

What about to have a compute field on account.invoice.line. I think that it's a caracteristic of the line.

Just a proposal, not a blocking point.

Choose a reason for hiding this comment

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

I'll take a look, thank you @legalsylvain for pointing it out.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, in fact, the ideal should be to have a uniq function _is_applicable_margin / _get_margin_applicable_lines or whatever that could be called in the account.move margin module / sale.order margin module / pos.order margin module.

But I'm not sure how to proceed.

@Yadier-Tecnativa
Copy link

Yadier-Tecnativa commented Apr 27, 2023

ping @sergio-teruel and @pedrobaeza TT42417

@pedrobaeza
Copy link
Member

/ocabot migration account_invoice_margin

@OCA-git-bot OCA-git-bot added this to the 16.0 milestone Apr 27, 2023
@OCA-git-bot OCA-git-bot mentioned this pull request Apr 27, 2023
10 tasks
@gerard-vacas
Copy link

Hi.

To the comment that posted @legalsylvain I would comment:

This module is designed to calculate the margin in invoices and although in sale orders the margin could not be correct, we have that:

  • Invoice will always recalculate margin from data in lines
  • Even if using the module account_invoice_margin_sale, wich transfers the cost of the line from SO to invoice, and as far as I know, cost of down payments will be 0 at any case, margin will be calculated correctly in invoices

Based on that, I consider that any fixes on sale order margins would be great but I doubt this module is the one for them.

Having said that, the module LGTM

Thanks

@rafaelbn
Copy link
Member

Runbot not available to test, please @ljsalvatierra-factorlibre could you make a rebase or add an empty commit trying to wake up runboat? 😄

sergio-teruel and others added 5 commits May 26, 2023 07:34
[UPD] README.rst

[UPD] Update account_invoice_margin.pot

Translated using Weblate (Spanish)

Currently translated at 100.0% (7 of 7 strings)

Translation: margin-analysis-11.0/margin-analysis-11.0-account_invoice_margin
Translate-URL: https://translation.odoo-community.org/projects/margin-analysis-11-0/margin-analysis-11-0-account_invoice_margin/es/
@ljsalvatierra-factorlibre
Copy link
Author

Runbot not available to test, please @ljsalvatierra-factorlibre could you make a rebase or add an empty commit trying to wake up runboat? smile

Done, thank you for the reminder! :)

account_invoice_margin/models/account_invoice.py Outdated Show resolved Hide resolved
account_invoice_margin/models/account_invoice.py Outdated Show resolved Hide resolved
account_invoice_margin/models/account_invoice.py Outdated Show resolved Hide resolved
Copy link

@Gelo-fl Gelo-fl left a comment

Choose a reason for hiding this comment

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

Functional revision OK

@ljsalvatierra-factorlibre ljsalvatierra-factorlibre force-pushed the 16.0-mig-account_invoice_margin branch 2 times, most recently from 1b725e2 to 820a3ae Compare June 2, 2023 07:03
@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). 🤖

@RodrigoBM
Copy link

Is it possible to merge this PR? @OCA/accounting-maintainers

@pedrobaeza
Copy link
Member

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 16.0-ocabot-merge-pr-163-by-pedrobaeza-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit d88f8f0 into OCA:16.0 Jun 2, 2023
@OCA-git-bot
Copy link
Contributor

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

@ljsalvatierra-factorlibre ljsalvatierra-factorlibre deleted the 16.0-mig-account_invoice_margin branch June 5, 2023 07:34
@sergio-teruel
Copy link
Contributor

ping @Juangomezzurita

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.