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

[12.0][MIG] stock_inventory_revaluation #894

Merged
merged 25 commits into from Jun 24, 2020
Merged

[12.0][MIG] stock_inventory_revaluation #894

merged 25 commits into from Jun 24, 2020

Conversation

matt454357
Copy link

Migration from 10.0 to 12.0

@matt454357 matt454357 changed the title 12.0 mig stock inventory revaluation [12.0][MIG] stock_inventory_revaluation Apr 28, 2020
@rousseldenis rousseldenis added this to the 12.0 milestone Apr 28, 2020
@OCA-git-bot OCA-git-bot mentioned this pull request Apr 28, 2020
54 tasks
Copy link

@CasVissers-360ERP CasVissers-360ERP left a comment

Choose a reason for hiding this comment

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

Initial functional review for AVCO costing, will do FiFo later.

sale_stock_info_popup/README.rst Outdated Show resolved Hide resolved
stock_inventory_revaluation/README.rst Outdated Show resolved Hide resolved
@CasVissers-360ERP
Copy link

CasVissers-360ERP commented Apr 29, 2020

I think a nice addition would be a new revaluation type "Based on Template", in which the stock user can choose for example repair or modification as template and stock accounts are automatically loaded. This allows a user without accounting knowledge to do revaluations.

@matt454357
Copy link
Author

@CasVissers,
Adding a template model is a great idea, not only for users without accounting knowledge, but also for automating revaluations from other modules.

Making it a revaluation type doesn't make sense to me. The revaluation types should be mutually exclusive. After choosing a template, we still need to know whether this is a Unit Price Change, or a Total Value Change. I think I would make the revaluation type another field on the template.

@@ -113,7 +113,7 @@
<field name="name"/>
<field name="revaluation_type"/>
<field name="document_date"/>
<field name="user_id"/>
<field name="create_uid"/>
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

@JordiBForgeFlow Do you have an advice on this ? Why did you use a specific field for this ?

Copy link
Author

Choose a reason for hiding this comment

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

@rousseldenis, @JordiBForgeFlow, Can we go ahead, and merge this PR? This is the only unresolved comment.

@matt454357
Copy link
Author

Still getting flake8 FAIL. It passes flake8 tests locally. Other PRs are passing the flake8 test. Can someone help me understand why mine is failing?

@CasVissers-360ERP
Copy link

@matt454357 Nice work on the template functionality!

Added one minor remark about new_value which should be readonly when posted.

For the decimal rounding in the value change I'm not sure yet. I understand the issue, but the current workflow implies that the value will be updated to the entered value. It will be confusing for end-users.

The only solution I have for now is to add a price difference or rounding difference account which is used to book the difference. This gives the user more flexibility and also explains and warns for possible rounding issues. What do you think?

@matt454357
Copy link
Author

@CasVissers thanks for all the feedback.

I made new_value readonly, as you suggested.

For the decimal rounding, I noted the behavior in some field help attributes. I also added some info to the list of known issues. The issue seems to have been present in previous versions, so I don't feel bad about leaving it.

Copy link
Sponsor 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.

Minor remark. Apart of that, seems great

Copy link

@CasVissers-360ERP CasVissers-360ERP left a comment

Choose a reason for hiding this comment

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

LGTM. Even-though I would have liked a better flow for rounding differences when updating the stock value. I think this is still confusing for end-users.

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

@matt454357
Copy link
Author

@rousseldenis, @CasVissers, @pedrobaeza,
Is there something else I need to do, or can this be merged?

@pedrobaeza
Copy link
Member

I let @rousseldenis to merge as I'm not a qualified reviewer for this module and he has merge rights.

Copy link

@HaraldPanten HaraldPanten left a comment

Choose a reason for hiding this comment

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

Seems to be OK

@CasVissers-360ERP
Copy link

@rousseldenis
Think this one can be merged?

@rousseldenis
Copy link
Sponsor Contributor

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 12.0-ocabot-merge-pr-894-by-rousseldenis-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit fe00950 into OCA:12.0 Jun 24, 2020
@OCA-git-bot
Copy link
Contributor

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

@dreispt
Copy link
Sponsor Member

dreispt commented Mar 9, 2021

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 12.0-ocabot-merge-pr-894-by-dreispt-bump-nobump, awaiting test results.

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.

None yet