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 16334 migrate business requirement #25

Merged

Conversation

sudhir-serpentcs
Copy link
Contributor

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

Migrated business_requirement module from v8 to v9.
@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.

some details in README . 👍


.. figure:: https://odoo-community.org/website/image/ir.attachment/5784_f2813bd/datas
:alt: Try me on Runbot
:target: https://runbot.odoo-community.org/runbot/222/8.0
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

.. figure:: ../business_requirement/static/img/bus_req_tags2.png
:width: 600 px

.. figure::business_requirement/static/img/bus_req_tags2.png
Copy link
Contributor

Choose a reason for hiding this comment

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

For all pictures, keep only one remove business_requirement/

@api.model
def _get_default_company(self):
company_id = self.env.user._get_company()
if not company_id:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should never happen as company is mandatory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I do agree with you.
I will remove it.

@elicoidal
Copy link
Contributor

tested in runbot: 👍

company_id = fields.Many2one(
'res.company', string='Company',
required=True, readonly=True, states={'draft': [('readonly', False)]},
default=_get_default_company,

Choose a reason for hiding this comment

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

What about use the following way https://github.com/odoo/odoo/blob/b2804b1d2ae5fd7ec5c0eca79a8db77476badc84/addons/account/models/account.py#L192?

company_id = fields.Many2one('res.company', string='Company', required=True, index=True, default=lambda self: self.env.user.company_id, help="Company related to this journal")

@sudhir-serpentcs
Copy link
Contributor Author

Removed duplicate images from README and added anonymous function to get default company.

@sudhir-serpentcs
Copy link
Contributor Author

Next: Replace old screenshots in README by new ones for v9.

@elicoidal
Copy link
Contributor

True and add some simple demo data as well ( @sudhir-serpentcs )

@elicoidal
Copy link
Contributor

@sudhir-serpentcs demo data can be done in second stage. Can you resubmit with newer screenshot so that we have it reviewed and merged?

@api.multi
@api.onchange('project_id')
def project_id_change(self):
if self.project_id and self.project_id.partner_id:

Choose a reason for hiding this comment

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

What about use:

- if self.project_id and self.project_id.partner_id:
-      self.partner_id = self.project_id.partner_id.id
+ self.partner_id = self.project_id.partner_id.id

Now the new api support BrowseNull.BrowseNull and IMHO this onchange should change in order to avoid use a old value of other onchange.

_name = "business.requirement.category"
_description = "Categories"

name = fields.Char(string='Name', required=True)

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

@sudhir-serpentcs
Copy link
Contributor Author

Replaced old screenshots with new ones, removed string parameters from the fields.

@sudhir-serpentcs
Copy link
Contributor Author

Merged the PR of Astirpe.

More information about business requirements management:

* `Wikipedia <https://en.wikipedia.org/wiki/Business_requirements>`_
* `Six Sigma <(https://www.isixsigma.com/implementation/project-selection-tracking/business-requirements-document-high-level-review/>`_
Copy link
Contributor

Choose a reason for hiding this comment

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

remove "(" in URL

RawEvan and others added 6 commits October 11, 2016 11:53
…case-field

[IMP]business_requirement: add test case field
Porting of latest developments made in V8
Porting of latest developments made in V8.
Updated Contributors list
Restored the period, I removed it by mistake
…ment_patch1

[9.0] business_requirement: Porting of latest developments made in V8
@astirpe
Copy link
Member

astirpe commented Oct 21, 2016

Code 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.

small details. Other than that 👍

('draft', 'Draft'),
('confirmed', 'Confirmed'),
('approved', 'Approved'),
('stakeholder_approval', 'Stakeholder Approval'),
Copy link
Contributor

Choose a reason for hiding this comment

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

stakeholder_approved, Stakeholder Approved

Copy link
Member

Choose a reason for hiding this comment

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

@elicoidal in V8 the states values have stakeholder_approval, we should be careful when changing this item. It also implies to create migration scripts.

Maybe changing only the string 'Stakeholder Approved' would be enough?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree. Let's just change the name

Copy link
Member

Choose a reason for hiding this comment

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

attachments=None, context=None,
content_subtype='html', **kwargs):
subject = None
if context.get(
Copy link
Contributor

Choose a reason for hiding this comment

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

formatting is a little bit weird but non blocking

'Stakeholder Approval' -> 'Stakeholder Approved'

@api.multi
def action_button_stakeholder_approval(self):
self.state = 'stakeholder_approval'
Copy link
Member

Choose a reason for hiding this comment

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

Modification not required anymore, changing the name sudhir-serpentcs#9 is enough

Update business_requirement.py
@dreispt
Copy link
Sponsor Member

dreispt commented Dec 2, 2016

@moylop260 @elicoidal Are there still any blocking points?

@elicoidal
Copy link
Contributor

@dreispt sorry for the delay, just catching up.
I think this is safe to merge.

@elicoidal elicoidal merged commit c3099ac into OCA:9.0 Dec 28, 2016
@sudhir-serpentcs sudhir-serpentcs deleted the 9.0-16334-migrate_business_requirement branch February 9, 2017 06:22
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