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

[9.0][MIG] quality_control #198

Merged
merged 42 commits into from
Nov 23, 2017
Merged

Conversation

LoisRForgeFlow
Copy link
Contributor

Continue the works started in #100.

Copy link
Contributor

@MiquelRForgeFlow MiquelRForgeFlow left a comment

Choose a reason for hiding this comment

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

LGTM 👍
But I think some of the previous commits could be squashed.

@LoisRForgeFlow
Copy link
Contributor Author

Cherry-picked fix done in #205

Copy link
Member

@andhit-r andhit-r 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

# -*- coding: utf-8 -*-
##############################################################################
# For copyright and license notices, see __openerp__.py file in root directory
##############################################################################
Copy link
Member

Choose a reason for hiding this comment

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

Change into short header

# -*- encoding: utf-8 -*-
##############################################################################
# For copyright and license notices, see __openerp__.py file in root directory
##############################################################################
Copy link
Member

Choose a reason for hiding this comment

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

Change into short header

# -*- coding: utf-8 -*-
##############################################################################
# For copyright and license notices, see __openerp__.py file in root directory
##############################################################################
Copy link
Member

Choose a reason for hiding this comment

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

Change into short header

# -*- coding: utf-8 -*-
##############################################################################
# For copyright and license notices, see __openerp__.py file in root directory
##############################################################################
Copy link
Member

Choose a reason for hiding this comment

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

Change into short header

# -*- coding: utf-8 -*-
##############################################################################
# For copyright and license notices, see __openerp__.py file in root directory
##############################################################################
Copy link
Member

Choose a reason for hiding this comment

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

Change into short header

# -*- coding: utf-8 -*-
##############################################################################
# For copyright and license notices, see __openerp__.py file in root directory
##############################################################################
Copy link
Member

Choose a reason for hiding this comment

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

Change into short header

# -*- coding: utf-8 -*-
##############################################################################
# For copyright and license notices, see __openerp__.py file in root directory
##############################################################################
Copy link
Member

Choose a reason for hiding this comment

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

Change into short header

# -*- coding: utf-8 -*-
##############################################################################
# For copyright and license notices, see __openerp__.py file in root directory
##############################################################################
Copy link
Member

Choose a reason for hiding this comment

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

Change into short header

# -*- encoding: utf-8 -*-
##############################################################################
# For copyright and license notices, see __openerp__.py file in root directory
##############################################################################
Copy link
Member

Choose a reason for hiding this comment

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

Change into short header

# -*- coding: utf-8 -*-
##############################################################################
# For copyright and license notices, see __openerp__.py file in root directory
##############################################################################
Copy link
Member

Choose a reason for hiding this comment

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

Change into short header

@LoisRForgeFlow
Copy link
Contributor Author

LoisRForgeFlow commented Jul 3, 2017

@LoisRForgeFlow
Copy link
Contributor Author

@andhit-r Is this ok for you now? could you update your review?

@LoisRForgeFlow
Copy link
Contributor Author

@andhit-r ping

@LoisRForgeFlow
Copy link
Contributor Author

@OCA/manufacturing-maintainers can we get an update on this?

@damendieta
Copy link

Hi, on creation of the Test Categories there there is an error that tries to compute the name but as It's a new record, the value is still false:

File "/mnt/extra-addons/manufacture/quality_control/models/qc_test_category.py", line 23, in _get_complete_name
self.complete_name = " / ".join(reversed(names))
TypeError: sequence item 0: expected string, bool found

Replacing:
names = [self.name]
With:
names = [self.name or '']

Solves the issue.

Daniel.

@damendieta
Copy link

damendieta commented Nov 13, 2017

Hi, I already fixed it and added some minor changes that I needed to test the module.

I made a PR with the changes:
ForgeFlow#2

Also, could you please explain the logic of triggers on the readme, I coudn't find a way to make triggers work.

Thanks a lot.

@LoisRForgeFlow
Copy link
Contributor Author

Hi @damendieta

First of all: thanks for your review!

Regarding the bug you reported, it is already being fixed in v8 and waiting to be ported to this PR after its merge, as noted here: #198 (comment). Please review that PR (#193) in order to move forward.

About the qc triggers, let me clarify that to make use of them you need either quality_control_mrp or quality_control_stock modules. However, they still need to be migrated to v9.

Your help would be appreciated 😉

@damendieta
Copy link

@lreficent ok, I approved the PR #193, what else do we need for it to get merged?

@LoisRForgeFlow
Copy link
Contributor Author

@damendieta fix from #193 added. Now you can review this PR and approve it if everything is ok for you.

@eLBati
Copy link
Member

eLBati commented Nov 20, 2017

@lreficent hi, could you check commits like e6d8961 with no message and unrecognised author?
Also please check if some commits can be squashed

@LoisRForgeFlow
Copy link
Contributor Author

Hi @eLBati

It looks like "alfredo" has changed either his github email or name and that's why the author is unrecognized, however I don't know his current github account (If someone could point it to me I'll change the commit author accordingly). Regarding the squashing, I've already squashed consecutive transifex commits, I don't know if I can squash further since I'm not the author of any of the previous versions commits.

@eLBati
Copy link
Member

eLBati commented Nov 20, 2017

Maybe @oihane or @pedrobaeza know about his account?

@oihane
Copy link
Contributor

oihane commented Nov 20, 2017

Alfredo de la Fuente's github user is alfredoavanzosc

@LoisRForgeFlow
Copy link
Contributor Author

@oihane Thanks!

@eLBati history has been fixed.

Copy link
Member

@eLBati eLBati left a comment

Choose a reason for hiding this comment

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

Just a remark

@@ -0,0 +1,24 @@
<?xml version="1.0" encoding="utf-8"?>
<openerp>
Copy link
Member

Choose a reason for hiding this comment

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

Please use odoo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, Thanks!

Copy link
Member

@SimoRubi SimoRubi left a comment

Choose a reason for hiding this comment

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

Just code review, looks good

@LoisRForgeFlow LoisRForgeFlow removed the request for review from JordiBForgeFlow November 21, 2017 10:40
@LoisRForgeFlow
Copy link
Contributor Author

@SimoRubi Hi, please use the option to approve when submitting the review:
selection_206

@LoisRForgeFlow
Copy link
Contributor Author

@damendieta If you are ok with this we can merge it I think

Copy link
Member

@SimoRubi SimoRubi 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, looks good

@LoisRForgeFlow LoisRForgeFlow merged commit b2faed4 into OCA:9.0 Nov 23, 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.