OCA / product-kitting

Odoo Kitting Addons
GNU Affero General Public License v3.0
17 stars 45 forks source link

[8.0][PORT] BOM stock #11

Closed SodexisTeam closed 8 years ago

oca-clabot commented 8 years ago

Hey @SodexisTeam, thank you for your Pull Request.

It looks like some users haven't signed our Contributor License Agreement, yet. You can read and sign our full Contributor License Agreement here: http://odoo-community.org/page/website.cla Here is a list of the users:

Appreciation of efforts, OCA CLAbot

elicoidal commented 8 years ago

@SodexisTeam For the CLA, please follow instructions here: https://odoo-community.org/page/cla I did no code inspection, only formal review. You need to follow the guidelines for headers, README, openerp.py and PEP8

stephankeller commented 8 years ago

@elicoidal thanks for helping with this, we are working on the changes. I did sign an Entity CLA in the past. I think all what is missing is the list of github accounts, where should we send that?

Thanks, Stephan.

atchuthan commented 8 years ago

@elicoidal This module was created by CamptoCamp and we have ported this module to the new api of Odoo. So, who should I add as author in openerp.py and in license headers?

elicoidal commented 8 years ago

@stephankeller the CLA in our database is linked to Sodexis but @atchuthan is not declare under this ECLA. Can you send an email at cla at odoo - community dot org with full name and email address so that we add him below Sodexis?

elicoidal commented 8 years ago

@atchuthan yes, you should add in the openerp.py and py file headers Camptocamp (check the copyright of original module to add it here. Authors are companies and contributors individuals. Feel free to add significant contributors from them.

elicoidal commented 8 years ago

small details left. other than that LGTM (no test nor technical review)

stephankeller commented 8 years ago

@elicoidal I sent the email.

For info here is the list of github users that needs to be added: Atchuthan: atchuthan atchuthan@sodexis.com Dhinesh: dvdhinesh dhinesh@sodexis.com John Britto:: johnbritto1 john@sodexis.com SodexisTeam: SodexisTeam dev@sodexis.com Stephan Keller: stephankeller skeller@sodexis.com Suganthi Karunanithi: suganthikarunanithi ksuganthi@sodexis.com Xavier Dass: xavier-dass xavier@sodexis.com

Thanks,

stephankeller commented 8 years ago

@elicoidal I got the email confirmation that the github accounts were received.

max3903 commented 8 years ago

CLA fixed.

stephankeller commented 8 years ago

@max3903 Thanks!

oca-clabot commented 8 years ago

Hey @SodexisTeam, We acknowledge that the following users have signed our Contributor License Agreement:

Appreciation of efforts, OCA CLAbot

SodexisTeam commented 8 years ago

@elicoidal It seems flake8 & pylint test are successful but not with Odoo/OCB test in travis. Also, it seems cl/runbot test failed too... Please clarify what I am missing?

elicoidal commented 8 years ago

cc @pedrobaeza

pedrobaeza commented 8 years ago

Travis fails because this module is depending on stock_available_immediately, and that module is not here. You have to add to the oca_dependencies.txt file in your PR the repo where it's hosted. Is that dependency really needed?

pedrobaeza commented 8 years ago

Module name should be _mrp_bomstock, and I'm not sure that this is related to product-kitting at all.

stephankeller commented 8 years ago

@pedrobaeza do you want us to change the name of the module? I am not sure what the correct approach is here, that module was there in older version, so do we keep the same name or do we change it? We will follow OCA decision on this.

We needed this for one of our customers, so we went ahead with the port. If you don't want this in OCA anymore just let me know. We are not advocating that is the right thing to do or not, we are just trying to contribute what we did. I don't think we changed any of the functionalities. If OCA decide to merge with the other module you mentioned, then we will try to find the time to help.

@sebastienbeau @jgrandguillaume

pedrobaeza commented 8 years ago

I don't have a special preference of one module over the other, because I haven't studied any of them. I was just telling about make any convergence of both to avoid duplicates.

atchuthan commented 8 years ago

Travis test shows an error with lxml package import. ImportError: No module named lxml

pedrobaeza commented 8 years ago

They have blacklisted python-lxml package. I have opened an issue for getting this again whitelisted: https://github.com/travis-ci/apt-package-whitelist/issues/2338

atchuthan commented 8 years ago

@pedrobaeza, Should I squash all the commits to a single commit?

pedrobaeza commented 8 years ago

You have to keep the relevant commits that means anything in your development cycle, not bugfixings nor PEP8 or minor changes that you have forgotten, but that's only my commit criteria. Others may want to be more exhaust on the commits.

atchuthan commented 8 years ago

@pedrobaeza, Label needs to be changed to "Needs review"

gurneyalex commented 8 years ago

:+1:

jgrandguillaume commented 8 years ago

Hi,

I understand may be this module should be in an other repo (to be discussed). But currently he's here, and ported, so go for it. For the name change, I think mrp_* is not appropriate neither (nothing to do with manufacturing order). We just use the bom here...

I've tested it and it worked., so I'm :+1:

Regards,

Joël

gurneyalex commented 8 years ago

How is this different from https://github.com/OCA/stock-logistics-warehouse/tree/8.0/stock_available_mrp ?

OSguard commented 8 years ago

it is the same function

i recommend:

atchuthan commented 8 years ago

We created 2 PRs to remove this module from the repository for both 8.0(https://github.com/OCA/product-kitting/pull/12) and 9.0(https://github.com/OCA/product-kitting/pull/13) versions.