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][ADD] Migrated business_requirement_deliverable_cost module from v8 to v9 #30

Conversation

sudhir-serpentcs
Copy link
Contributor

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

WIP until #25 and #26 is merged

Migrated business_requirement_deliverable_cost module from v8 to v9.
WIP: Clean up of unit test cases.
@dreispt @pedrobaeza @jbeficent @moylop260 Please review.

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 in separate file

Introduction
============

This module is part of a set of modules (`Business Requirements <https://github.com/OCA/business-requirement/blob/8.0/README.md>`_)
Copy link
Contributor

Choose a reason for hiding this comment

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

9.0

@elicoidal elicoidal mentioned this pull request Sep 30, 2016
8 tasks
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.

👍
Small details
@pedrobaeza @moylop260 @mmalorni

"business_requirement_deliverable",
],
'image': [
'static/description/icon.png',
Copy link
Contributor

Choose a reason for hiding this comment

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

add icon image

mtelahun pushed a commit to Clear-ICT/business-requirement that referenced this pull request Oct 8, 2016
…erable_categ

Cleansed the README+__openerp__.py
…o 9.0-migrate_business_requirement_deliverable_cost
@sudhir-serpentcs
Copy link
Contributor Author

Rebased the PR.
@dreispt @pedrobaeza @jbeficent @moylop260 @elicoidal

br_id = False
if self.business_requirement_deliverable_id:
br_deliverable = self.business_requirement_deliverable_id
if br_deliverable and br_deliverable.business_requirement_id:
Copy link
Member

Choose a reason for hiding this comment

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

Please apply the same fix that was made for V8 in https://github.com/OCA/business-requirement/pull/66/files

Copy link
Contributor

Choose a reason for hiding this comment

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

@sudhir-serpentcs Can you attend @astirpe recommendation so that we can merge cleanly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@elicoidal elicoidal changed the title [ADD] Migrated business_requirement_deliverable_cost module from v8 to v9 [9.0][ADD] Migrated business_requirement_deliverable_cost module from v8 to v9 Feb 25, 2017
@elicoidal elicoidal added this to the 9.0 milestone Feb 25, 2017
@elicoidal
Copy link
Contributor

@astirpe @sudhir-serpentcs can we merge this one?

@sudhir-serpentcs
Copy link
Contributor Author

I think yes. We can merge it.
@elicoidal

@astirpe
Copy link
Member

astirpe commented Mar 8, 2017

With commit 6ebfc4b the method def _get_partner() was replaced with a related field partner_id. Is it possible to apply the same change before to merge? Otherwise we merge now and then open a new PR for that. I think doing now is the quickest way. What do you think?

@elicoidal
Copy link
Contributor

agree with @astirpe indeed.
@sudhir-serpentcs could you do that and then we move forward?

@sudhir-serpentcs
Copy link
Contributor Author

With commit 6ebfc4b the method def _get_partner() was replaced with a related field partner_id. Is it possible to apply the same change before to merge? Otherwise we merge now and then open a new PR for that. I think doing now is the quickest way. What do you think?

@elicoidal @astirpe Replaced the _get_partner() method by related partner_id field.

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.

small details then we can move forward

for resource in self:
resource.sale_price_total = resource.sale_price_unit * resource.qty

# @api.multi
Copy link
Contributor

Choose a reason for hiding this comment

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

please remove dead codes


<record id="group_business_requirement_estimation" model="res.groups">
<field name="name">Business Requirement Estimation</field>
<field name="implied_ids" eval="[(4, ref('business_requirement.group_business_requirement_user'))]"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you improve the format of the lines for readibility?

Copy link
Member

@astirpe astirpe left a comment

Choose a reason for hiding this comment

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

For me is OK to merge, after the removal of dead code ...

@sudhir-serpentcs
Copy link
Contributor Author

@elicoidal @astirpe Removed dead code and improved security xml.

@elicoidal elicoidal merged commit c60efdb into OCA:9.0 Mar 15, 2017
@sudhir-serpentcs sudhir-serpentcs deleted the 9.0-migrate_business_requirement_deliverable_cost branch March 15, 2017 07:43
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.

None yet

3 participants