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

[ADD] Migrated business_requirement_deliverable module from v8 to v9. #26

Conversation

sudhir-serpentcs
Copy link
Contributor

@sudhir-serpentcs sudhir-serpentcs commented Sep 23, 2016

WIP until #25 is merged

Migrated business_requirement_deliverable module from v8 to v9.
@dreispt @pedrobaeza @jbeficent @moylop260 Please review.

@sudhir-serpentcs
Copy link
Contributor Author

Travis failed only because business_requirement module is not found.
business_requirement is already migrated and PR is sent.

@elicoidal elicoidal mentioned this pull request Sep 23, 2016
8 tasks
@elicoidal
Copy link
Contributor

@sudhir-serpentcs you will have to rebase when the other module is merged.

@pedrobaeza
Copy link
Member

You can also cherry-pick/merge the other PR in your branch for this to work, and rebase as said by Eric when the other PR is merged.

@elicoidal
Copy link
Contributor

👍 no test yet / waiting for the first PR to be accepted

_name = "business.requirement.resource"
_description = "Business Requirement Resource"

sequence = fields.Integer('Sequence')
Copy link

@moylop260 moylop260 Sep 24, 2016

Choose a reason for hiding this comment

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

FYI https://github.com/OCA/maintainer-tools/blob/master/CONTRIBUTING.md#field
If the technical name of the field (the variable name) is the same to the string of the label, don't put string parameter for new API fields, because it's automatically taken
(check other similar cases for this PR)

Copy link
Contributor

@elicoidal elicoidal left a comment

Choose a reason for hiding this comment

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

I will update the README file

@@ -0,0 +1,78 @@
language: python
Copy link
Contributor

Choose a reason for hiding this comment

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

probably not need as it is already in the first PR

@@ -1,2 +1,5 @@
# business-requirement
[![Build Status](https://travis-ci.org/OCA/business-requirement.svg?branch=9.0)](https://travis-ci.org/OCA/business-requirement)
Copy link
Contributor

Choose a reason for hiding this comment

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

same here: proably not needed

Resources for your customers',
'version': '9.0.1.0.0',
'website': 'www.elico-corp.com',
"author": "Elico Corp, Odoo Community Association (OCA)",
Copy link
Member

Choose a reason for hiding this comment

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

Change double quotation to single quotation

self.br.write({'partner_id': self.partner.id})
try:
self.br.partner_id_change()
except UserError, e:
Copy link
Member

Choose a reason for hiding this comment

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

flake8 returns this error:
business_requirement_deliverable/tests/test_br.py:247:26: E901 SyntaxError: invalid syntax

Could you check?

@dreispt
Copy link
Sponsor Member

dreispt commented Dec 2, 2016

@sudhir-serpentcs Travis is red. Can you fix it?

@elicoidal
Copy link
Contributor

@sudhir-serpentcs can you recheck travis?

@elicoidal
Copy link
Contributor

@dreispt @gurneyalex
This is strange as the #25 is merged, missing modules should be available.
I am not sure how to fix this actually

@moylop260
Copy link

You could try update this branch from 9.0 to get the merged commits

…o 9.0-migrate_business_requirement_deliverable
@sudhir-serpentcs
Copy link
Contributor Author

Travis is green as #25 is merged and this PR is rebased.
@dreispt @pedrobaeza @jbeficent @moylop260 Please review.

@elicoidal
Copy link
Contributor

👍

@api.onchange('product_id')
def product_id_change(self):
description = ''
uom_id = False

Choose a reason for hiding this comment

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

What about use directly self.uom_id in order to avoid create a dummy variable?

for resource in self:
if resource.resource_type == 'task' and (
resource.uom_id.category_id != (
self.env.ref('product.uom_categ_wtime'))):
Copy link

@moylop260 moylop260 Sep 24, 2016

Choose a reason for hiding this comment

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

If we delete categories we don't want a raise here:

- self.env.ref('product.uom_categ_wtime')
+ self.env.ref('product.uom_categ_wtime', raise_if_not_found=False)

partner_id = brd.business_requirement_id.partner_id
if partner_id and partner_id.property_product_pricelist:
return partner_id.property_product_pricelist
return partner_id

Choose a reason for hiding this comment

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

The method is called _get_pricelist but we are returning a partner_id

def _get_pricelist(self):
for brd in self:
partner_id = False
if brd.business_requirement_id and (

Choose a reason for hiding this comment

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

We don't need parens here

for brd in self:
partner_id = False
if brd.business_requirement_id and (
brd.business_requirement_id.partner_id

Choose a reason for hiding this comment

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

You could use just brd.business_requirement_id.partner_id (without brd.business_requirement_id) because new api supports BrowseNull.BrowseNull

@api.multi
@api.onchange('product_id')
def product_id_change(self):
description = ''

Choose a reason for hiding this comment

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

What about use directly self.variables?

@sudhir-serpentcs
Copy link
Contributor Author

sudhir-serpentcs commented Jan 23, 2017

@moylop260 @astirpe Have made the necessary requested changes.

Please review and approve.

@dreispt
Copy link
Sponsor Member

dreispt commented Feb 5, 2017

Can we merge?

@elicoidal
Copy link
Contributor

I would say so!

@elicoidal elicoidal merged commit 7712cd1 into OCA:9.0 Feb 6, 2017
@elicoidal
Copy link
Contributor

@dreispt @pedrobaeza Any idea why the travis Transfex is failing?

@elicoidal
Copy link
Contributor

@sudhir-serpentcs sudhir-serpentcs deleted the 9.0-migrate_business_requirement_deliverable branch February 6, 2017 04:05
@dreispt
Copy link
Sponsor Member

dreispt commented Feb 8, 2017

The problem is with Transifex. Are all the invoices paid up? 😁

@elicoidal
Copy link
Contributor

obviously no t :p

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

6 participants