Merge lp:~savoirfairelinux-openerp/openerp-hr/department_sequence_concatination_NoneType into lp:openerp-hr

Status: Merged
Merged at revision: 79
Proposed branch: lp:~savoirfairelinux-openerp/openerp-hr/department_sequence_concatination_NoneType
Merge into: lp:openerp-hr
Diff against target: 114 lines (+93/-0)
3 files modified
hr_department_sequence/hr_department.py (+2/-0)
hr_department_sequence/tests/__init__.py (+29/-0)
hr_department_sequence/tests/test_hr_department.py (+62/-0)
To merge this branch: bzr merge lp:~savoirfairelinux-openerp/openerp-hr/department_sequence_concatination_NoneType
Reviewer Review Type Date Requested Status
Katja Matthes (community) code review Approve
Guewen Baconnier @ Camptocamp Approve
Daniel Reis Approve
Pedro Manuel Baeza code review Approve
Review via email: mp+212436@code.launchpad.net

Description of the change

Fix a name_search bug and add a test to prevent future regressions:

openerp.addons.hr_department_sequence.hr_department in name_search
TypeError: can only concatenate list (not "NoneType") to list

To post a comment you must log in.
Revision history for this message
Pedro Manuel Baeza (pedro.baeza) wrote :

LGTM. You can also make one liner with:

..., name)] + args or [],

Regards.

review: Approve (code review)
Revision history for this message
Sandy Carter (http://www.savoirfairelinux.com) (sandy-carter) wrote :

Thank you for the review.

I could make it a one liner, but I think it's easier to read this way.
The line is long enough as it is :)

Revision history for this message
Daniel Reis (dreis-pt) wrote :

How about this?
   args = args or []

review: Approve
Revision history for this message
Sandy Carter (http://www.savoirfairelinux.com) (sandy-carter) wrote :

Just trying to have constant style with

if context is None:
    context = context

Could also use args = args or list(), args = [] in the signature.

Revision history for this message
Sandy Carter (http://www.savoirfairelinux.com) (sandy-carter) wrote :

 if context is None:
- context = context
+ context = {}

Revision history for this message
Guewen Baconnier @ Camptocamp (gbaconnier-c2c) wrote :

> How about this?
> args = args or []

With this one, you will build a new list when args is an empty list, which is unnecessary.

review: Approve
Revision history for this message
Katja Matthes (katja-matthes) wrote :

Hello,

looks good. Tests run fine.
I think, since you made a fix you should update the module's version number.

Regards.

review: Approve (code review)
Revision history for this message
Daniel Reis (dreis-pt) wrote :

>> How about this?
>> args = args or []
>
> With this one, you will build a new list when args is an empty list, which is unnecessary.

Indeed, but personally I appreciate it's brevity, compared with a 2-line 'if' statement.

Just occurred to me this one line alternative, but it's still not as neat:
args = [] if args is None else args

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'hr_department_sequence/hr_department.py'
2--- hr_department_sequence/hr_department.py 2013-12-04 13:35:42 +0000
3+++ hr_department_sequence/hr_department.py 2014-03-24 14:52:50 +0000
4@@ -52,6 +52,8 @@
5 for record in self.browse(cr, uid, ids, context=context or {})]
6
7 def name_search(self, cr, uid, name='', args=None, operator='ilike', context=None, limit=100):
8+ if args is None:
9+ args = []
10 ids = self.search(cr, uid, ['|', ('code', 'ilike', name), ('name', 'ilike', name)] + args,
11 limit=limit, context=context)
12 return self.name_get(cr, uid, ids, context=context)
13
14=== added directory 'hr_department_sequence/tests'
15=== added file 'hr_department_sequence/tests/__init__.py'
16--- hr_department_sequence/tests/__init__.py 1970-01-01 00:00:00 +0000
17+++ hr_department_sequence/tests/__init__.py 2014-03-24 14:52:50 +0000
18@@ -0,0 +1,29 @@
19+# -*- encoding: utf-8 -*-
20+##############################################################################
21+#
22+# OpenERP, Open Source Management Solution
23+# This module copyright (C) 2010 - 2014 Savoir-faire Linux
24+# (<http://www.savoirfairelinux.com>).
25+#
26+# This program is free software: you can redistribute it and/or modify
27+# it under the terms of the GNU Affero General Public License as
28+# published by the Free Software Foundation, either version 3 of the
29+# License, or (at your option) any later version.
30+#
31+# This program is distributed in the hope that it will be useful,
32+# but WITHOUT ANY WARRANTY; without even the implied warranty of
33+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
34+# GNU Affero General Public License for more details.
35+#
36+# You should have received a copy of the GNU Affero General Public License
37+# along with this program. If not, see <http://www.gnu.org/licenses/>.
38+#
39+##############################################################################
40+
41+from . import (
42+ test_hr_department,
43+)
44+
45+checks = [
46+ test_hr_department,
47+]
48
49=== added file 'hr_department_sequence/tests/test_hr_department.py'
50--- hr_department_sequence/tests/test_hr_department.py 1970-01-01 00:00:00 +0000
51+++ hr_department_sequence/tests/test_hr_department.py 2014-03-24 14:52:50 +0000
52@@ -0,0 +1,62 @@
53+# -*- encoding: utf-8 -*-
54+##############################################################################
55+#
56+# OpenERP, Open Source Management Solution
57+# This module copyright (C) 2010 - 2014 Savoir-faire Linux
58+# (<http://www.savoirfairelinux.com>).
59+#
60+# This program is free software: you can redistribute it and/or modify
61+# it under the terms of the GNU Affero General Public License as
62+# published by the Free Software Foundation, either version 3 of the
63+# License, or (at your option) any later version.
64+#
65+# This program is distributed in the hope that it will be useful,
66+# but WITHOUT ANY WARRANTY; without even the implied warranty of
67+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
68+# GNU Affero General Public License for more details.
69+#
70+# You should have received a copy of the GNU Affero General Public License
71+# along with this program. If not, see <http://www.gnu.org/licenses/>.
72+#
73+##############################################################################
74+
75+from openerp.tests.common import TransactionCase
76+
77+
78+class test_department(TransactionCase):
79+
80+ def setUp(self):
81+ super(test_department, self).setUp()
82+ # Clean up registries
83+ self.registry('ir.model').clear_caches()
84+ self.registry('ir.model.data').clear_caches()
85+ # Get registries
86+ self.user_model = self.registry("res.users")
87+ self.department_model = self.registry("hr.department")
88+ # Get context
89+ self.context = self.user_model.context_get(self.cr, self.uid)
90+ self.vals = {
91+ 'name': 'test',
92+ 'code': 'TEST',
93+ 'sequence': 1,
94+ }
95+
96+ def test_create_department(self):
97+ cr, uid, context = self.cr, self.uid, self.context
98+ department_id = self.department_model.create(
99+ self.cr, self.uid, self.vals, context=self.context)
100+ department = self.department_model.browse(cr, uid, department_id,
101+ context=context)
102+ self.assertEqual(self.vals['name'], department.name)
103+ self.assertEqual(self.vals['code'], department.code)
104+ self.assertEqual(self.vals['sequence'], department.sequence)
105+
106+ def test_name_search_department(self):
107+ cr, uid, context = self.cr, self.uid, self.context
108+ department_id = self.department_model.create(
109+ self.cr, self.uid, self.vals, context=self.context)
110+
111+ found_id = self.department_model.name_search(
112+ cr, uid, name=self.vals['name'], context=context)[0][0]
113+ self.assertEqual(department_id, found_id,
114+ "Found wrong id for name=%s" % self.vals['name'])