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
[12.0][MIG] stock_account_internal_move #570
[12.0][MIG] stock_account_internal_move #570
Conversation
* Rewrite SQL constraints to Python * Adapt location view to display valuation accounts on internal locations
Treat corresponding valuation account counterparts on internal locations as valuation accounts when a move is `in` or `out`, namely: * treat location's `valuation_in_account` as a valuation on `in` moves, * treat location's `valuation_out_account` as one on `out` moves. Sure, the above takes place only when location has `force_accounting_entries` on it, otherwise vanilla behavior is not to tamper w/.
aae7cb8
to
25877e1
Compare
Migrated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the work 👍
code review: lgtm
just the one question (see below)
self.ensure_one() | ||
res = super()._run_valuation(quantity) | ||
if self._is_internal() and not self.value: | ||
# TODO: recheck if this part respects product valuation method |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
were the todo checked?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, sorry, I am not sure what is the intention of the original author too.
Any hint, @max3903?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also suggested some typo fixes in the strings
if loc.usage != 'internal' and loc.force_accounting_entries: | ||
raise ValidationError(_( | ||
'You cannot force accounting entries' | ||
' on a non-internal locations.')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
' on a non-internal locations.')) | |
' on a non-internal location.')) |
if not loc.valuation_in_account_id \ | ||
or not loc.valuation_out_account_id: | ||
raise ValidationError(_( | ||
'You must provide a valuation in/out accounts' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'You must provide a valuation in/out accounts' | |
'You must provide a valuation in/out account' |
This PR has the |
@kittiu Do you plan to attend @gurneyalex comments ? |
25877e1
to
73c68c2
Compare
Hey @kittiu, thank you for your Pull Request. It looks like some users haven't signed our Contributor License Agreement, yet.
Appreciation of efforts, |
Done :) |
@max3903 Any advice on the TODO ? |
I think we can merge this. |
/ocabot merge |
On my way to merge this fine PR! |
Congratulations, your PR was merged at b986233. Thanks a lot for contributing to OCA. ❤️ |
Migrate to v12