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

[16.0][IMP] stock_inventory_discrepancy: add configuration for enabling discrepancy check #1966

Open
wants to merge 1 commit into
base: 16.0
Choose a base branch
from

Conversation

AungKoKoLin1997
Copy link
Contributor

@AungKoKoLin1997 AungKoKoLin1997 commented Mar 20, 2024

This PR aims to resolve the compatibility issue between this module and other inventory adjustment related modules.
Related: #1711

@qrtl QT4850

Comment on lines 56 to 80
# Get the system parameter configuration
param = (
self.env["ir.config_parameter"]
.sudo()
.get_param("stock_inventory_discrepancy.inventory_discrepancy_enable")
)
if self.env.context.get("skip_exceeded_discrepancy", False) or param == "0":
return super().action_apply_inventory()
Copy link
Member

Choose a reason for hiding this comment

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

For early returns.

Suggested change
# Get the system parameter configuration
param = (
self.env["ir.config_parameter"]
.sudo()
.get_param("stock_inventory_discrepancy.inventory_discrepancy_enable")
)
if self.env.context.get("skip_exceeded_discrepancy", False) or param == "0":
return super().action_apply_inventory()
if self.env.context.get("skip_exceeded_discrepancy", False):
return super().action_apply_inventory()
inventory_discrepancy_enable = (
self.env["ir.config_parameter"]
.sudo()
.get_param("stock_inventory_discrepancy.inventory_discrepancy_enable")
)
if inventory_discrepancy_enable == "0":
return super().action_apply_inventory()

@AungKoKoLin1997 AungKoKoLin1997 force-pushed the 16.0-imp-stock_inventory_discrepancy branch from 1519072 to 0a89a0e Compare March 20, 2024 03:23
Copy link
Member

@yostashiro yostashiro 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.

Copy link
Contributor

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

@AungKoKoLin1997
Copy link
Contributor Author

@OCA/stock-logistics-warehouse-maintainers
Can you please review this PR?

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

@JordiBForgeFlow
Copy link
Member

@AungKoKoLin1997 From a usability perspective I think that it would be much better to add a setting in res.config.settings to enable / disable it, and document it in the readme of the module. A checkbox in res.config.settings would then update the parameter that you have proposed.

@LoisRForgeFlow
Copy link
Contributor

@AungKoKoLin1997 From a usability perspective I think that it would be much better to add a setting in res.config.settings to enable / disable it, and document it in the readme of the module. A checkbox in res.config.settings would then update the parameter that you have proposed.

I agree

@yostashiro
Copy link
Member

@AungKoKoLin1997 From a usability perspective I think that it would be much better to add a setting in res.config.settings to enable / disable it, and document it in the readme of the module. A checkbox in res.config.settings would then update the parameter that you have proposed.

@JordiBForgeFlow @LoisRForgeFlow The purpose of this update was more to pass tests from another module rather than providing additional config option to users, and as such I personally thought updating res.config.settings wouldn't be necessary (wasn't sure if there would be some users who'd like to disable the feature after installing the module with config setting when there are already other ways to bypass the check with adjusting location/warehouse settings). Having said that, if the community opts for having a field in res.config.settings, why not!

@rousseldenis
Copy link
Contributor

The purpose of this update was more to pass tests from another module rather than providing additional config option to users

Be careful. Adding code just for tests should trigger 'warnings'. But, that said, adding a configuration that is accessible for users is always a good process to enable/disable a feature.

For several reasons:

  • To be able to disable process in case of bug or... without technical requirements.
  • To be able to deploy a solution without enabling it immediately

@yostashiro
Copy link
Member

Thank you. @AungKoKoLin1997 Can you please follow up to add a configuration?

@AungKoKoLin1997 AungKoKoLin1997 force-pushed the 16.0-imp-stock_inventory_discrepancy branch 2 times, most recently from b0ba48c to 78f5ade Compare June 13, 2024 09:00
@AungKoKoLin1997 AungKoKoLin1997 force-pushed the 16.0-imp-stock_inventory_discrepancy branch from 78f5ade to 726edca Compare June 13, 2024 10:02
@AungKoKoLin1997
Copy link
Contributor Author

AungKoKoLin1997 commented Jun 13, 2024

@LoisRForgeFlow @JordiBForgeFlow I added the configuration from a usability perspective. Can you please review it?

Comment on lines 12 to 13
help="Enable to show line discrepancies in inventory and block validation "
"if the discrepancy exceeds the threshold.",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
help="Enable to show line discrepancies in inventory and block validation "
"if the discrepancy exceeds the threshold.",
help="Block validation of the inventory adjustment if discrepancy exceeds the threshold.",

Since the setting doesn't affect the UI.

<div class="o_setting_right_pane">
<label for="inventory_discrepancy_enable" />
<div class="text-muted">
Enable to show line discrepancies in inventory and block validation if the discrepancy exceeds the threshold.
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

Copy link
Member

@yostashiro yostashiro 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. 👍

@AungKoKoLin1997 AungKoKoLin1997 force-pushed the 16.0-imp-stock_inventory_discrepancy branch from 75165ab to b589a5c Compare July 1, 2024 03:37
@AungKoKoLin1997 AungKoKoLin1997 force-pushed the 16.0-imp-stock_inventory_discrepancy branch from b589a5c to d52a680 Compare September 2, 2024 04:22
@AungKoKoLin1997 AungKoKoLin1997 force-pushed the 16.0-imp-stock_inventory_discrepancy branch from 059f9e8 to d85c615 Compare November 15, 2024 01:36
@AungKoKoLin1997
Copy link
Contributor Author

@rousseldenis @LoisRForgeFlow @JordiBForgeFlow
I have update the PR and I believe it is in good state.
Can you please review?

@AungKoKoLin1997
Copy link
Contributor Author

@kanda999 Could you please review?

@AungKoKoLin1997 AungKoKoLin1997 force-pushed the 16.0-imp-stock_inventory_discrepancy branch from d85c615 to 477c445 Compare November 26, 2024 02:14
@AungKoKoLin1997 AungKoKoLin1997 changed the title [16.0][IMP] stock_inventory_discrepancy: add system parameter configuration for enabling discrepancy check [16.0][IMP] stock_inventory_discrepancy: add configuration for enabling discrepancy check Nov 26, 2024
@AungKoKoLin1997
Copy link
Contributor Author

@rousseldenis I changed the approach like we did for OCA/stock-logistics-workflow#1760.
Can you please review?

Copy link
Member

@yostashiro yostashiro 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.

Comment on lines 6 to 7
from . import res_config_settings
from . import res_company
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest putting them in alphabetical order unless there is a reason not to.

@AungKoKoLin1997 AungKoKoLin1997 force-pushed the 16.0-imp-stock_inventory_discrepancy branch from 477c445 to 6befc0b Compare November 26, 2024 05:21
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.

7 participants