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][FIX] sale_invoice_policy: compute qty to invoice #3102

Merged
merged 1 commit into from
May 2, 2024

Conversation

ioans73
Copy link
Member

@ioans73 ioans73 commented Apr 25, 2024

I have cherry-picked the commit of PR #2763, to try to fix the module

Services should be computed based on the policy established in the product, right? Because it makes no sense to do it based on the quantity delivered, if that is the policy established in the order

@ACheung-FactorLibre @pedrobaeza @rousseldenis

@pedrobaeza
Copy link
Member

pedrobaeza commented Apr 25, 2024

Yes, the override should be in the quantity to invoice. I wrote it correctly at first instance in the other PR, but then put received quantity in the second comment (now also fixed). Please check pre-commit.

@pedrobaeza pedrobaeza added this to the 16.0 milestone Apr 25, 2024
@ioans73 ioans73 force-pushed the 16.0-fix-sale_invoice_policy branch 2 times, most recently from 3aa5173 to d963d90 Compare April 25, 2024 17:48
@ioans73
Copy link
Member Author

ioans73 commented Apr 25, 2024

@pedrobaeza 🍏

@ioans73 ioans73 force-pushed the 16.0-fix-sale_invoice_policy branch from d963d90 to d780b68 Compare April 26, 2024 06:03
@ioans73
Copy link
Member Author

ioans73 commented Apr 26, 2024

@pedrobaeza Could it be merged?

@ioans73
Copy link
Member Author

ioans73 commented Apr 26, 2024

@pedrobaeza Could it be merged?

Don't do it yet, because I want to check if I need to modify anything in this field https://github.com/odoo/odoo/blob/16.0/addons/sale/models/sale_order_line.py#L255

@ioans73 ioans73 force-pushed the 16.0-fix-sale_invoice_policy branch from d780b68 to 683353c Compare April 26, 2024 11:16
@ioans73
Copy link
Member Author

ioans73 commented Apr 26, 2024

Update: compute untaxed amount to invoice

@pedrobaeza
Copy link
Member

Please check CI

Copy link
Contributor

@ACheung-FactorLibre ACheung-FactorLibre left a comment

Choose a reason for hiding this comment

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

Hello @ioans73,

Please make sure you are only modifying the lines whose order has invoice_policy_required set to True. By doing this, all tests will pass without any problem.

Thank you

# were not in so_lines
super(SaleOrderLine, self - done_lines)._compute_qty_to_invoice()
other_lines = self.filtered(
lambda l: l.product_id.type == "service" or not l.order_id.invoice_policy
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi,

I think you need to also check if l.order_id.invoice_policy_required is set on False, because if it is, you don't need to modify the line's qty_to_invoice, since it is not required.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

lambda line: line.product_id.type == "service"
or not line.order_id.invoice_policy
or line.order_id.invoice_policy == line.product_id.invoice_policy
or line.state not in ["sale", "done"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, I think you need to also check if l.order_id.invoice_policy_required is set on False, because if it is, you don't need to modify the line, since it is not required.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@ioans73 ioans73 force-pushed the 16.0-fix-sale_invoice_policy branch 4 times, most recently from 04a44f9 to b09a9d2 Compare April 30, 2024 07:53
@ACheung-FactorLibre
Copy link
Contributor

Hi @ioans73,

Please do a rebase so that tests are executed.

Thank you

@ioans73 ioans73 force-pushed the 16.0-fix-sale_invoice_policy branch from b09a9d2 to 5882f9d Compare April 30, 2024 09:33
@ioans73
Copy link
Member Author

ioans73 commented Apr 30, 2024

@ACheung-FactorLibre I have done it before, but I don't know why the actions are not being executed

@pedrobaeza
Copy link
Member

OCA/oca-addons-repo-template#250

@ioans73 ioans73 force-pushed the 16.0-fix-sale_invoice_policy branch from 5882f9d to ed7fb73 Compare May 1, 2024 19:48
@ioans73
Copy link
Member Author

ioans73 commented May 2, 2024

Hi @ioans73,

Please do a rebase so that tests are executed.

Thank you

The tests are now ok

@@ -36,6 +39,9 @@ def test_sale_order_invoice_order(self):
"invoice_policy": "order",
}
)
so._compute_invoice_policy_required()
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need to manually call the compute method? It shouldn't be needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not really sure, because there is code that comes from the old PR, but I can review it

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed

@ioans73 ioans73 force-pushed the 16.0-fix-sale_invoice_policy branch from ed7fb73 to 129d782 Compare May 2, 2024 06:53
Copy link
Member

@pedrobaeza pedrobaeza 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 major

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 16.0-ocabot-merge-pr-3102-by-pedrobaeza-bump-major, awaiting test results.

OCA-git-bot added a commit that referenced this pull request May 2, 2024
Signed-off-by pedrobaeza
@OCA-git-bot
Copy link
Contributor

It looks like something changed on 16.0 in the meantime.
Let me try again (no action is required from you).
Prepared branch 16.0-ocabot-merge-pr-3102-by-pedrobaeza-bump-major, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 8d72220 into OCA:16.0 May 2, 2024
7 of 9 checks passed
@OCA-git-bot
Copy link
Contributor

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

@ioans73 ioans73 deleted the 16.0-fix-sale_invoice_policy branch May 2, 2024 16:07
@jguenat
Copy link
Member

jguenat commented Jun 19, 2024

Hello @ioans73,

Please make sure you are only modifying the lines whose order has invoice_policy_required set to True. By doing this, all tests will pass without any problem.

Thank you

Hello @ACheung-FactorLibre

By doing this qty_to_invoice won't be recomputed for any sale order line if the config parameter sale_invoice_policy_required is set to false.

Meaning this modules does nothing unless you set sale_invoice_policy_required to True in the settings.

It was not the case in previous versions.

@rousseldenis
Copy link
Contributor

Hello @ioans73,
Please make sure you are only modifying the lines whose order has invoice_policy_required set to True. By doing this, all tests will pass without any problem.
Thank you

Hello @ACheung-FactorLibre

By doing this qty_to_invoice won't be recomputed for any sale order line if the config parameter sale_invoice_policy_required is set to false.

Meaning this modules does nothing unless you set sale_invoice_policy_required to True in the settings.

It was not the case in previous versions.

@ioans73 @jguenat In fact, this is not the expected behavior.

The initial one was :

  • If no invoice policy on SO, we should use the product invoice policy
  • The required parameter should avoid this (but the parameter should stay)
  • If the invoice policy is set on SO level, we should use it

So, the required parameter should not avoid using the SO invoice policy.

I will make a PR to improve it.

@rousseldenis
Copy link
Contributor

Hello @ioans73,
Please make sure you are only modifying the lines whose order has invoice_policy_required set to True. By doing this, all tests will pass without any problem.
Thank you

Hello @ACheung-FactorLibre
By doing this qty_to_invoice won't be recomputed for any sale order line if the config parameter sale_invoice_policy_required is set to false.
Meaning this modules does nothing unless you set sale_invoice_policy_required to True in the settings.
It was not the case in previous versions.

@ioans73 @jguenat In fact, this is not the expected behavior.

The initial one was :

  • If no invoice policy on SO, we should use the product invoice policy
  • The required parameter should avoid this (but the parameter should stay)
  • If the invoice policy is set on SO level, we should use it

So, the required parameter should not avoid using the SO invoice policy.

I will make a PR to improve it.

#3376

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.

7 participants