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

[10.0] Module stock_no_negative #275

Merged
merged 3 commits into from
Dec 14, 2017
Merged

Conversation

alexis-via
Copy link
Contributor

This is in fact a port to v10 of my PR #137 but changing the name to stock_no_negative, because there is already an OCA module with that name in older versions.

@JordiBForgeFlow
Copy link
Member

We'll add test cases to this PR, and also backport to 9.0

<field name="inherit_id" ref="stock.view_template_property_form" />
<field name="arch" type="xml">
<field name="categ_id" position="after">
<field name="allow_negative_stock"
Copy link
Member

Choose a reason for hiding this comment

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

I think that this field should only be accessible to a specific security group. You basically prevent negative stocks, but allow any user to override this rule just going to the product and changing it. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only few users should have write access on products... as a lot of parameters on products are sensitive, not only this one !

Copy link
Member

Choose a reason for hiding this comment

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

Agreed!

Copy link
Contributor

@sebalix sebalix left a comment

Choose a reason for hiding this comment

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

👍
One note, the stock_no_negative module from 8.0 has its field named check_no_negative which should be converted to the new allow_negative_stock, but this should be done in the v9.0 migration (PR #288).

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.

👍 Minor comment. Tested on runbot.

# @author Alexis de Lattre <[email protected]>
# License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl).

from odoo import models, api, _
Copy link
Contributor

Choose a reason for hiding this comment

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

Change import order

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why should I change import order ?

Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/OCA/maintainer-tools/blob/master/CONTRIBUTING.md#imports

alphabetically ordered. Details but convention ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Arf !!! Incredible ! Don't count on me to apply these crazy coding conventions !

@sebalix
Copy link
Contributor

sebalix commented Mar 26, 2017

Is this module ready to be merged?

@alexis-via
Copy link
Contributor Author

@sebalix yes, of course. I've been using it in production for several months.

@Cedric-Pigeon
Copy link

@pedrobaeza this one can be merged

@pedrobaeza
Copy link
Member

Maybe you can clean a bit the commits?

@yvaucher
Copy link
Member

Should this module be tested separately or other module's tests should ensure non zero stock level ?

@yvaucher
Copy link
Member

See comment #341 (comment)

@levkar
Copy link

levkar commented Jun 29, 2017

Works like a charm

@rousseldenis
Copy link
Contributor

rousseldenis commented Jul 5, 2017

Setup : akretion#3

@MiquelRForgeFlow
Copy link
Contributor

We have arrived to the conclusion after some analysis, for the good health of the repository, that the better approach to proceed with this topic is the following: In the config.settings of Stock, you have to select an option to activate the stock_no_negative after the installation of the module. In the Readme is put a configuration note that after the installation, you have to select that option if you want to use the module.

We have developed a patch with this approach for v9, and we will open the PR soon. If you like, you may want to cherry-pick it here.

@JordiBForgeFlow
Copy link
Member

To add to the point of @mreficent , the tests of module 'stock_no_negative' will activate this setting, and then proceed to do the tests. The change in config.settings should create an entry in ir_config_parameter with name 'stock_no_negative' and value 'True'.

If the parameter does not exist, or is False, then it is assumed that the negative stocks are allowed, regardless if the products have been set with allow_negative = False.

@pedrobaeza
Copy link
Member

That's not needed: you just need to deactivate the check if in test mode and you are not explicitly testing this module (which is done through a context key). Check https://github.com/OCA/bank-payment/blob/43f8723f8cb4d8defc50d53e83f8b523a7560f00/account_payment_transfer_reconcile_batch/models/bank_payment_line.py#L28 for an example on how to do it

@MiquelRForgeFlow
Copy link
Contributor

@pedrobaeza we will do that way now, thanks!

@MiquelRForgeFlow
Copy link
Contributor

MiquelRForgeFlow commented Aug 30, 2017

Done. @alexis-via please, can you cherry-pick commit 73fa871 from #357?

@pedrobaeza
Copy link
Member

Better wait first to have it finished (although of course it works)

@pedrobaeza
Copy link
Member

Now you can cherry-pick 73fa871

@levkar
Copy link

levkar commented Aug 30, 2017

Actually disallowing negative stock by default makes more sense in othis module. In v8, we had serious problems with customers because of this behavior and finally we wrote another auto-install module which disallows it by default.

There was a PR of a new module named stock_disallow_negative but later author decided to use this module name and cancelled that PR. Same rationale.

I would strongly suggest to disallow by default.

@pedrobaeza
Copy link
Member

But it's already the default behavior in this module AFAIK.

@levkar
Copy link

levkar commented Aug 30, 2017

Sorry, I get it wrong. It's just for tests. No problem.

@pedrobaeza
Copy link
Member

@alexis-via would you be able to clean the commits and cherry-pick 73fa871 for having this module ready to be merged, or should we superseed it?

Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

I have fixed remaining comments (rebase + squash + cherry-pick), so I merge this.

@pedrobaeza pedrobaeza merged commit 9fa19d3 into OCA:10.0 Dec 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants