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

[FIX] [REF] [ADD] [16.0] sale_report_delivered_deposit #289

Merged
merged 3 commits into from
Oct 28, 2024

Conversation

Shide
Copy link

@Shide Shide commented Oct 23, 2024

First of all, sorry for all this stuff in one PR.
It's necessary to make some refactor to make it work.

There are 3 commits:

  1. 47da083 [FIX]: Remove the COALESCE to let the NULL value go to the SELECT (it's in the sub-select) and let use the product_cost of the sale order line
  2. df08f64 [REF]: Refactor to split the conditions in separate functions to allow a better inheritance in the sale_report_delivered_deposit module and other future modules, allowing them to add its own casuistic.
  3. 4694cf7 [ADD]: The new module to show Customer Deposits on the Sale Report Delivered.

MT-7240 @moduon @rafaelbn @yajo @sergio-teruel please review if you want :)

@OCA-git-bot
Copy link
Contributor

Hi @sergio-teruel,
some modules you are maintaining are being modified, check this out!

@Gelojr
Copy link

Gelojr commented Oct 23, 2024

Hi @Shide
When an order is placed to generate a delivery from the customer's deposit, the cost of the product is calculated in the sales order line. The module stock_customer_deposit has already managed that the cost in the sales order in which the delivery will be made is 0, because this cost is already taken into account in the order in which the deposit will be made. In this way we duplicate it and both in the deposit delivery order and in the report there is a negative margin.
SO deposit
image

SO Deposit deliver
image

Report
image

I know this PR affects the deposit slips that show up on the report, but could it be affecting the cost of the line for some reason?

@Shide
Copy link
Author

Shide commented Oct 24, 2024

I know this PR affects the deposit slips that show up on the report, but could it be affecting the cost of the line for some reason?

For that case, you have this module: https://github.com/OCA/stock-logistics-workflow/tree/16.0/stock_customer_deposit_sale_margin

When it's a deposit, marks purchase_price = 0.0, so the margin remains 0.0%

@Shide
Copy link
Author

Shide commented Oct 24, 2024

Also reviewing stock_customer_deposit_sale_margin i found a bug: OCA/stock-logistics-workflow#1749

Copy link

@Gelojr Gelojr left a comment

Choose a reason for hiding this comment

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

@Shide With the clarification of the commentary #289 (comment), I see it fine. Nice Job 👍

@rafaelbn rafaelbn added this to the 16.0 milestone Oct 28, 2024
Copy link
Member

@rafaelbn rafaelbn 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-289-by-rafaelbn-bump-major, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit b74efcd into OCA:16.0 Oct 28, 2024
7 checks passed
@OCA-git-bot
Copy link
Contributor

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

@Shide Shide deleted the 16.0-sale_report_delivered_deposit branch October 28, 2024 11:33
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.

4 participants