Skip to content

[17.0][MIG] purchase_sale_stock_inter_company#605

Closed
cuongnmtm wants to merge 15 commits intoOCA:17.0from
komit-consulting:17.0-mig-purchase_sale_stock_inter_company
Closed

[17.0][MIG] purchase_sale_stock_inter_company#605
cuongnmtm wants to merge 15 commits intoOCA:17.0from
komit-consulting:17.0-mig-purchase_sale_stock_inter_company

Conversation

@cuongnmtm
Copy link
Contributor

@cuongnmtm cuongnmtm commented Mar 27, 2024

@cuongnmtm cuongnmtm mentioned this pull request Mar 27, 2024
17 tasks
@cuongnmtm cuongnmtm force-pushed the 17.0-mig-purchase_sale_stock_inter_company branch from e88c4c7 to 7db17e9 Compare March 28, 2024 09:25
@github-actions
Copy link

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Jul 28, 2024
@pomazanbohdan
Copy link

/ocabot migration purchase_sale_stock_inter_company

@OCA-git-bot
Copy link
Contributor

Sorry @pomazanbohdan you are not allowed to mark the addon tobe migrated.

To do so you must either have push permissions on the repository, or be a declared maintainer of all modified addons.

If you wish to adopt an addon and become it's maintainer, open a pull request to add your GitHub login to the maintainers key of its manifest.

@pomazanbohdan
Copy link

@cuongnmtm Would you be so kind as to repeat the note to the bot?

/ocabot migration purchase_sale_stock_inter_company

@cuongnmtm
Copy link
Contributor Author

/ocabot migration purchase_sale_stock_inter_company

@OCA-git-bot
Copy link
Contributor

Sorry @cuongnmtm you are not allowed to mark the addon tobe migrated.

To do so you must either have push permissions on the repository, or be a declared maintainer of all modified addons.

If you wish to adopt an addon and become it's maintainer, open a pull request to add your GitHub login to the maintainers key of its manifest.

@cuongnmtm
Copy link
Contributor Author

I don't have permission, but I believe the maintainer will somehow receive a notification about this :D

@cuongnmtm cuongnmtm force-pushed the 17.0-mig-purchase_sale_stock_inter_company branch 2 times, most recently from e6ead00 to 2417f1f Compare August 2, 2024 10:20
@github-actions github-actions bot removed the stale PR/Issue without recent activity, it'll be soon closed automatically. label Aug 4, 2024
@cuongnmtm cuongnmtm force-pushed the 17.0-mig-purchase_sale_stock_inter_company branch from 2417f1f to 5f52035 Compare December 20, 2024 10:27
@github-actions
Copy link

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Apr 20, 2025
Copy link
Contributor

@carlos-lopez-tecnativa carlos-lopez-tecnativa left a comment

Choose a reason for hiding this comment

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

Please rebase the branch
TT54931

Comment on lines 8 to 9
from odoo.addons.purchase_sale_inter_company.tests import (
test_inter_company_purchase_sale as test_icps,
)

TestPurchaseSaleInterCompany = test_icps.TestPurchaseSaleInterCompany

Copy link
Contributor

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pre-commit auto fixes I guess

It just refactors the code.

@github-actions github-actions bot removed the stale PR/Issue without recent activity, it'll be soon closed automatically. label May 4, 2025
@cuongnmtm cuongnmtm force-pushed the 17.0-mig-purchase_sale_stock_inter_company branch 2 times, most recently from ad3db76 to 3c75251 Compare May 12, 2025 12:08
Copy link
Contributor

@carlos-lopez-tecnativa carlos-lopez-tecnativa left a comment

Choose a reason for hiding this comment

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

Please include and adapt #733

@carlos-lopez-tecnativa
Copy link
Contributor

carlos-lopez-tecnativa commented May 19, 2025

Please include and adapt #733

ping @cuongnmtm

And remove the last commit, the dependency has already been merged.

@cuongnmtm
Copy link
Contributor Author

Hello @carlos-lopez-tecnativa , I plan to allocate time to work on this later this week. If it's urgent on your end, please feel free to continue from where I left off. Thank you.

@pedrobaeza
Copy link
Member

#804 should be included here.

@cuongnmtm cuongnmtm force-pushed the 17.0-mig-purchase_sale_stock_inter_company branch 2 times, most recently from 36086cb to 9e327cb Compare May 23, 2025 11:10
@cuongnmtm
Copy link
Contributor Author

@cuongnmtm
Copy link
Contributor Author

Something went wrong. I tried to solve conflicts, and the migration changes were moved to the conflicted commit. I will need to redo it.

@cuongnmtm cuongnmtm force-pushed the 17.0-mig-purchase_sale_stock_inter_company branch from 9e327cb to 41ec8f4 Compare June 2, 2025 07:12
@cuongnmtm cuongnmtm force-pushed the 17.0-mig-purchase_sale_stock_inter_company branch from abd8287 to 18331ba Compare June 20, 2025 09:21
@cuongnmtm
Copy link
Contributor Author

There are two issues when process the backorder wizard:

  • test_sync_picking_lot_and_qty_with_move_diff_2: You need to supply a Lot/Serial Number for product: Stockable Product Tracked by Serial
  • test_inter_company_purchase_sale_stock: This lot 111 is incompatible with this product Stockable Product

I am checking it

@cuongnmtm cuongnmtm force-pushed the 17.0-mig-purchase_sale_stock_inter_company branch 3 times, most recently from 5edc793 to a09943f Compare June 20, 2025 12:34
@cuongnmtm
Copy link
Contributor Author

Need to remove the extra moves in case:

# remove the extra move lines in the receipt of lot tracking product
# example: In the receipt, we have 3 move lines for 3 different serials,
# in the delivery we specify 2 serials. When validating the delivery and
# creating back order, Odoo generates 3 move lines in the receipt, so
# we need to remove 1 different move line in the receipt, otherwise it
# will cause an error saying that we need to assign a lot or serial
# for the remaining move line

Copy link
Contributor

@carlos-lopez-tecnativa carlos-lopez-tecnativa left a comment

Choose a reason for hiding this comment

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

When I try to validate a picking from the sale order, I get this error:
image

@cuongnmtm
Copy link
Contributor Author

When I try to validate a picking from the sale order, I get this error: image

This is multi-company rule. I will add sudo to bypass it.

@cuongnmtm
Copy link
Contributor Author

When I try to validate a picking from the sale order, I get this error: image

This is multi-company rule. I will add sudo to bypass it.

actually no need sudo but with_company does the job

@cuongnmtm cuongnmtm force-pushed the 17.0-mig-purchase_sale_stock_inter_company branch from a09943f to d26d93e Compare June 20, 2025 16:19
@cuongnmtm cuongnmtm force-pushed the 17.0-mig-purchase_sale_stock_inter_company branch from d26d93e to 6e4966a Compare June 20, 2025 16:23
@cuongnmtm
Copy link
Contributor Author

Thanks @carlos-lopez-tecnativa for your review.
I updated the code to add a missing with_company to solve your issue, and updated these tests, which hide this issue.
I updated the readme and test as well.

@cuongnmtm cuongnmtm force-pushed the 17.0-mig-purchase_sale_stock_inter_company branch 4 times, most recently from e9147a4 to 455abe0 Compare June 21, 2025 06:40
This commit includes not only the module migration but also additional logic to handle the synchronization of stock.move.line quantities and lots, inspired by this commit OCA@5118408 from the OCA multi-company repository.

Although best practices suggest that a migration commit should focus solely on the migration, separating the sync logic into a different commit before or after the migration was not appropriate in this case, as the migration alone does not address all the necessary scenarios.
@cuongnmtm cuongnmtm force-pushed the 17.0-mig-purchase_sale_stock_inter_company branch from 455abe0 to a9f124e Compare June 21, 2025 08:02
@carlos-lopez-tecnativa
Copy link
Contributor

Please include and adapt #733

From that PR, I noticed that this commit bc478bf which synchronizes stock.move.line when the move is in the done state (and the picking is unlocked), is not included.
Could you please include it or explain why it was discarded?

Additionally, this module has a strange history. In v14, it existed as a single module: purchase_sale_inter_company.
At Tecnativa, we backported it to v13 and split it into two modules: purchase_sale_inter_company and purchase_sale_stock_inter_company, as was done in v15, see this PR: #547. In v14, I found these two PRs: #600 and #656 They were never included in v15, but I think their features could be useful here.
Could you please cherry-pick and adapt those PRs?

Copy link
Contributor

@carlos-lopez-tecnativa carlos-lopez-tecnativa left a comment

Choose a reason for hiding this comment

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

Additionally, I noticed that commit 8e705ae (for pre-commit autofixes) should only contain automatic formatting changes. However, it includes functional/code changes that shouldn't be in that type of commit.

These extra changes should be moved to the migration commit in order to keep a clean and accurate Git history.

image

@cuongnmtm
Copy link
Contributor Author

@carlos-lopez-tecnativa

From that PR, I noticed that this commit bc478bf which synchronizes stock.move.line when the move is in the done state (and the picking is unlocked), is not included.
Could you please include it or explain why it was discarded?

The logic is handled in stock.picking. All tests are passed.

Additionally, this module has a strange history. In v14, it existed as a single module: purchase_sale_inter_company.
At Tecnativa, we backported it to v13 and split it into two modules: purchase_sale_inter_company and purchase_sale_stock_inter_company, as was done in v15, see this PR: #547. In v14, I found these two PRs: #600 and #656 They were never included in v15, but I think their features could be useful here.
Could you please cherry-pick and adapt those PRs?

I will have a look on Friday

@metaminux
Copy link
Contributor

Hello @cuongnmtm and @carlos-lopez-tecnativa

As mentioned in previous comments, the history of the module is quite a mess...

There were only one module purchase_sale_inter_company in 14.0 with maintainers : @aleuffre and @renda-dev

It has been split in purchase_sale_inter_company and purchase_sale_stock_inter_company from 14.0 to 15.0 (see #370 and #371 by @JasminSForgeFlow)

But a lot of improvements were added to 14.0 after that and never ported to 15.0 because it's harder to forward port due to the split.
Some attempts were done, but it's not so easy (see sunflowerit#3 and a lot of comments in PRs when @pedrobaeza asked for porting to newer versions)

Then, original module was split in 13.0 also...

And now, some PRs were merged to 16.0, adding features already present in 14.0, and facing same problems with different ways to fix...

I'm trying to migrate modules purchase_sale_stock_inter_company to 18.0 and purchase_sale_stock_inter_company_mrp to 17.0 and 18.0...

I'd like to use this PR has base for the migrations, but I feel @cuongnmtm is a bit lonely here, right ?

@OCA/intercompany-maintainers Can we gather efforts to clean all that mess ?

I'm trying to get the full history of issues and PRs for each version from 13.0 to 16.0, i'll try to post it here (or in another PR...)
I think 14.0 is the one with all features and tests but only one module, maybe we need to rewrite modules in 16.0 before doing the migration ?

Any thoughts everyone ?

@pedrobaeza
Copy link
Member

I understand your pain, and there has been conflicting interests with some people only working on 14.0 while others working on upper versions. We can try to gather everything together through @carlos-lopez-tecnativa if you agree.

@metaminux
Copy link
Contributor

I understand your pain, and there has been conflicting interests with some people only working on 14.0 while others working on upper versions. We can try to gather everything together through @carlos-lopez-tecnativa if you agree.

Sure, what would be the better way ?

I think porting tests from 14.0 to 16.0 one by one with code making each test pass in each commit would be easier for review...
Maybe opening a new issue with sub issues for each commit ?

@carlos-lopez-tecnativa
Copy link
Contributor

I understand your pain, and there has been conflicting interests with some people only working on 14.0 while others working on upper versions. We can try to gather everything together through @carlos-lopez-tecnativa if you agree.

Sure, what would be the better way ?

I think porting tests from 14.0 to 16.0 one by one with code making each test pass in each commit would be easier for review... Maybe opening a new issue with sub issues for each commit ?

I'm working on this PR to forward-port all features from V13 and V14. Please review this PR and let me know if you have any questions.

Superseded by #856

@pedrobaeza
Copy link
Member

Let's close this one then. Do you agree, @cuongnmtm ?

@cuongnmtm cuongnmtm closed this Jul 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.