[MIG] crm_action module from v8 to v9 #119
Conversation
|
Hey @atularvind, thank you for your Pull Request. It looks like some users haven't signed our Contributor License Agreement, yet.
Appreciation of efforts, |
|
@oca-clabot I have already signed the CLA and sent on the CLA email address. Thanks! |
| code = fields.Char( | ||
| string='Lead Number', required=True, default="/", readonly=True) | ||
| code = fields.Char(string='Lead Number', required=True, default="/", | ||
| readonly=True) |
There was a problem hiding this comment.
It' just my personal opinion, but I prefer the former formatting.
In general, either we like or dislike the formatting style, we should avoid editing lines just to change to another also valid formatting choice.
|
It would have been best to have a PR per module. |
There was a problem hiding this comment.
@atularvind Can you check the lint log: https://travis-ci.org/OCA/crm/jobs/169148076#L328
You can ignore the wrong-tabs-instead-of-spaces on XML files if you like, but there at least two messages that need fixing.
|
@dreispt Yes, I have reverted the migrated code for the crm_lead_code from this branch, will submit it in another PR. Now the travis-ci & runbot should go well. |
|
I don't understand: why did you revert those changes? |
|
I wanted to submit one module per PR, and crm_lead_code was failing on the runbot. It needed changes in post_init_hook. |
|
IMO no problem to submit both now. I just added that to keep in mind for future PRs. |
|
Already submitted #120 . |
There was a problem hiding this comment.
One small comment, plus can you check the Lint warnings seen here?: https://travis-ci.org/OCA/crm/jobs/169159500#L330
At least the duplicate-xml-record-id and file-not-used warnings.
| 'version': '8.0.1.1.0', | ||
| 'author': 'Savoir-faire Linux,Odoo Community Association (OCA)', | ||
| 'version': '9.0.1.1.0', | ||
| 'author': 'Savoir-faire Linux,Odoo Community Association (OCA),AtulArvind', |
There was a problem hiding this comment.
These are the original authors. Keep the original ones unless significant original changes have been made.
| self.actions_count = len(self.action_ids) | ||
|
|
||
| actions_count = fields.Integer(compute='count_actions') | ||
| actions_count = fields.Integer(compute='_compute_count_actions') |
There was a problem hiding this comment.
The standard name, now that you take the time to rename the method, would be _compute_actions_count.
| @@ -26,10 +26,10 @@ | |||
| class CrmLead(models.Model): | |||
| _inherit = 'crm.lead' | |||
|
|
|||
| def count_actions(self): | |||
| def _compute_count_actions(self): | |||
| self.actions_count = len(self.action_ids) | |||
There was a problem hiding this comment.
This lacks @api.multi and a loop over self.
| @@ -32,14 +32,15 @@ Known issues / Roadmap | |||
| Credits | |||
| ======= | |||
|
|
|||
| Module developed and tested with Odoo version 8.0 | |||
| Module developed and tested with Odoo version 9.0 | |||
| !python {model: crm.lead} : | | ||
| lead_rec = self.browse(cr, uid, ref('test_crm_lead')) | ||
| lead_rec._compute_count_actions() | ||
| lead_rec.button_actions() |
There was a problem hiding this comment.
Thanks for providing tests!
However these should be written directly with Python. Sooner or later the YAML support will be dropped.
I will not block this however, because better YAML tests than nothing, but change it if you can please.
| 'Date', required=True, default=fields.Date.context_today) | ||
| user_id = fields.Many2one( | ||
| 'res.users', string='User', required=True, | ||
| default=lambda self: self.env.user) |
There was a problem hiding this comment.
Put field definitions above methods.
If you need a method defined, then use a lambda as you do here, or the method name in places where the ORM allows it.
| @@ -1,4 +1,4 @@ | |||
| # -*- encoding: utf-8 -*- | |||
| # -*- coding: utf-8 -*- | |||
| ############################################################################## | |||
| # | |||
| # OpenERP, Open Source Management Solution | |||
There was a problem hiding this comment.
Please update these awful license headers and put the shorter ones if you don't mind.
|
Hi @atularvind , thanks for this migration. Are you going to answer comments from @dreispt and @yajo please? Let us know. Thanks! |
|
Superseeded by #140 |
Migrated code of crm_action from v8 to v9