Skip to content

Conversation

@JochenDeBie
Copy link

No description provided.

@rousseldenis
Copy link
Contributor

/ocabot migration sale_order_line_remove

Copy link
Contributor

@BhaveshHeliconia BhaveshHeliconia left a comment

Choose a reason for hiding this comment

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

Funtional test LG.

Copy link

@matteonext matteonext left a comment

Choose a reason for hiding this comment

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

LGTM

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

Copy link
Member

@ivs-cetmix ivs-cetmix left a comment

Choose a reason for hiding this comment

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

Hi @JochenDeBie , please squash administrative commits (if any) with the previous commit for reducing commit noise. Check the migration wiki for details, ensue that you have followed all the steps of that document.
Please prefix your pr with the version tag as well: [18.0][MIG]....

Copy link
Member

@ivs-cetmix ivs-cetmix left a comment

Choose a reason for hiding this comment

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

Thank you!

@JochenDeBie JochenDeBie force-pushed the 18.0-mig-sale_order_line_remove branch from 767adb3 to 82ec9bf Compare July 11, 2025 12:18
@JochenDeBie JochenDeBie requested a review from ivs-cetmix July 11, 2025 12:20
@ivs-cetmix
Copy link
Member

Hi @JochenDeBie, commits are good now, please fix the tests.

@JochenDeBie
Copy link
Author

@ivs-cetmix Not really sure what to do with this one. The test that is failing now was added in another module to specifically check that confirmed SO lines can't be removed, which is the purpose of this module:

https://github.com/OCA/sale-workflow/blame/43f33c42b51eff3b3e3b88f571aec541ce72530f/sale_order_product_recommendation/tests/test_recommendation.py#L207

@ivs-cetmix
Copy link
Member

@ivs-cetmix Not really sure what to do with this one. The test that is failing now was added in another module to specifically check that confirmed SO lines can't be removed, which is the purpose of this module:

https://github.com/OCA/sale-workflow/blame/43f33c42b51eff3b3e3b88f571aec541ce72530f/sale_order_product_recommendation/tests/test_recommendation.py#L207

Looks like we need to use rebel modules here. I'm not very familiar with this flow, so probably we need to ask @dreispt or @pedrobaeza how to do this right.

@ArnauCForgeFlow
Copy link
Contributor

Hello, what's the state of this PR? @ivs-cetmix @JochenDeBie I can help if needed.

Copy link
Member

@ivs-cetmix ivs-cetmix left a comment

Choose a reason for hiding this comment

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

Code review LGTM

@ivs-cetmix
Copy link
Member

ivs-cetmix commented Sep 5, 2025

Hello, what's the state of this PR? @ivs-cetmix @JochenDeBie I can help if needed.

Hi @ArnauCForgeFlow , thank you! Could you please check how we can solve an issue with those tests falling?

@ArnauCForgeFlow
Copy link
Contributor

@ivs-cetmix @JochenDeBie I think rebel modules are needed here. I don’t have much experience with rebel modules, but I think you need to update the copier answers and include this module as a rebel module.

@ivs-cetmix
Copy link
Member

That's the

@ivs-cetmix @JochenDeBie I think rebel modules are needed here. I don’t have much experience with rebel modules, but I think you need to update the copier answers and include this module as a rebel module.

Unfortunately I don't have much experience with them either. Would appreciate if @rousseldenis @sbidoul or @dreispt could give a helper hand here.

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.

8 participants