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] mgmtsystem_nonconformity_project #190

Closed
wants to merge 2 commits into from
Closed

[9.0][MIG] mgmtsystem_nonconformity_project #190

wants to merge 2 commits into from

Conversation

thinkwelltwd
Copy link

@thinkwelltwd thinkwelltwd commented Jul 3, 2017

#92

@pedrobaeza pedrobaeza mentioned this pull request Jul 3, 2017
30 tasks
@max3903 max3903 added this to the 9.0 milestone Jul 3, 2017
@max3903 max3903 requested review from max3903 and dreispt July 3, 2017 18:55
@max3903
Copy link
Sponsor Member

max3903 commented Jul 3, 2017

Hi @thinkwelltwd

Let us know when you are ready to get reviewed.

@thinkwelltwd
Copy link
Author

I'd say it's ready to review now, although test coverage declined. Is that a deal-breaker for merging?

Copy link
Sponsor Member

@dreispt dreispt left a comment

Choose a reason for hiding this comment

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

LGTM, only a few nitpicks to improve

@@ -0,0 +1,9 @@
# -*- coding: utf-8 -*-

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

@@ -0,0 +1,34 @@
# -*- coding: utf-8 -*-
from openerp import api, models, fields
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Add short authorship/copyright notice

action_type = fields.Selection([
('action', 'Action'),
('project', 'Project'),
],
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Isn't this underindented?

for action in self:
action.complete_name = '%s %s' % action.name_get()[0]

name = fields.Char('Claim Subject', size=128)
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Remove size

project_id = fields.Many2one('project.project', 'Project')
complete_name = fields.Char('Complete Name',
compute='_compute_complete_name',
size=250, store=True)
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Remove size

if o.action_type == 'project' and o.project_id:
r = (o.id, o.project_id.name)
res.append(r)
return res
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I see that this is preexisting code, but I don't see the point to replace the Action name by the corresponding Project name.
@max3903 can we just drop this?

@dreispt
Copy link
Sponsor Member

dreispt commented Aug 16, 2017

@thinkwelltwd Can you address my comments please?

@thinkwelltwd
Copy link
Author

Did you want the complete_name field yanked altogether in the model & views, or just set the complete_name to the value of the "name" field?

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

3 participants