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

[14.0][MIG] sale_stock_line_sequence #2607

Merged
merged 6 commits into from
Aug 30, 2023

Conversation

OriolMForgeFlow
Copy link
Contributor

@OriolMForgeFlow OriolMForgeFlow commented Jul 14, 2023

Standard migration
Taking into account the refactor done in !2603

Copy link
Contributor

@AaronHForgeFlow AaronHForgeFlow left a comment

Choose a reason for hiding this comment

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

Edit: nevermind, is the refactor. Can't do functional test in runboat

Copy link
Contributor

@AaronHForgeFlow AaronHForgeFlow 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 LG

@rousseldenis
Copy link
Contributor

/ocabot migration sale_stock_line_sequence

@OCA-git-bot OCA-git-bot added this to the 14.0 milestone Jul 14, 2023
@OCA-git-bot OCA-git-bot mentioned this pull request Jul 14, 2023
88 tasks
@OriolMForgeFlow OriolMForgeFlow force-pushed the 14.0-mig-sale_stock_line_sequence branch from 8ce4ab6 to 5aaf80c Compare July 17, 2023 06:10
@rousseldenis
Copy link
Contributor

/ocabot migration sale_stock_line_sequence

@rousseldenis
Copy link
Contributor

@OriolMForgeFlow Thanks for this. FYI, you can embed your dependency PR here in order to get tests running and runboat for functional testing.

https://github.com/OCA/maintainer-tools/wiki/Use-temporary-reference%28s%29-to-another-pull-request%28s%29

@rousseldenis
Copy link
Contributor

@OriolMForgeFlow @AaronHForgeFlow

I've tested on runboat and if the order of lines change should change the order of invoice lines, that does not work:

Sale Order after change:

image

Invoice after sale order change:

image

@rousseldenis
Copy link
Contributor

@OriolMForgeFlow @AaronHForgeFlow

I've tested on runboat and if the order of lines change should change the order of invoice lines, that does not work:

Sale Order after change:

image

Invoice after sale order change:

image

Oops wrong PR. @OriolMForgeFlow This is for #2603

@OriolMForgeFlow
Copy link
Contributor Author

Thank you for your feedback, @rousseldenis!

The lines in the invoice remain unchanged as we are not utilizing the sequence field, which was causing some issues. Instead, we have introduced a new field called "related_so_sequence," designed as a Char field (SO12345/1 - so_name/sequence). This choice is based on the consideration that an invoice can be generated from multiple Sales Orders. As a result, we cannot use this field to sort the order lines.

Here's an example:

image

@OriolMForgeFlow OriolMForgeFlow force-pushed the 14.0-mig-sale_stock_line_sequence branch 7 times, most recently from 5a252de to bd5b98f Compare July 31, 2023 08:23
@OriolMForgeFlow OriolMForgeFlow force-pushed the 14.0-mig-sale_stock_line_sequence branch 2 times, most recently from e588bf0 to 20eadd8 Compare August 4, 2023 12:06
@OriolMForgeFlow OriolMForgeFlow force-pushed the 14.0-mig-sale_stock_line_sequence branch 2 times, most recently from d419a45 to 53f7621 Compare August 11, 2023 09:41
Copy link
Contributor

@AaronHForgeFlow AaronHForgeFlow left a comment

Choose a reason for hiding this comment

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

Functional review, Works as expected.

@OriolMForgeFlow OriolMForgeFlow force-pushed the 14.0-mig-sale_stock_line_sequence branch 2 times, most recently from e26a12e to aab4536 Compare August 17, 2023 09:13
@OriolMForgeFlow OriolMForgeFlow force-pushed the 14.0-mig-sale_stock_line_sequence branch from aab4536 to d153c7b Compare August 17, 2023 09:14
Copy link
Contributor

@LoisRForgeFlow LoisRForgeFlow left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@rousseldenis rousseldenis 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

@rousseldenis
Copy link
Contributor

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 14.0-ocabot-merge-pr-2607-by-rousseldenis-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 3d39d30 into OCA:14.0 Aug 30, 2023
8 of 10 checks passed
@OCA-git-bot
Copy link
Contributor

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

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.

5 participants