[ADD] [10.0] hr_employee_operating_unit #97

Matiar · 2017-10-08T12:12:42Z

This module introduces the following features:

Adds the Operating Unit (OU) to the Employee.
The user’s default Operating Unit (OU) is proposed at the time of creating the Employee.
Security rules are defined to ensure that users can only see the Employee of that Operating Units in which they are allowed access to.

Matiar · 2017-10-09T15:02:43Z

@aheficent Could you please review ?

AaronHForgeFlow

Hi @Matiar Thanks for your module. A couple of things:

As there's a field user_id in the employee, if the employee has an user associated, then the employee should have an operating unit from the operating unit of the users.
IMHO an employee may belong to different operating units so the field may be One2many. The problem here is that it may be inconsistencies between the operating unit of the employee and the operating unit of the related user.
Please attend travis and also fix the description in this PR

Matiar · 2017-10-09T15:30:31Z

Hello @aarón Henríquez,

Thanks for your valuable feedback. I'll fix as you suggested and get back to you.

…f the employee will be the same as user account

Matiar · 2017-10-19T18:10:43Z

Hello Aarón Henríquez,

Thanks for your valuable feedback.

As you suggested, when an employee will associate with an user account the operating unit of employee is set with the operating unit of the associated user.

But also you suggest an one2many relationship between employee and operating unit, I didn't yet implement that. I have a question on that as you know the employee has a relationship between department and that is Many2One; Then will it not confusing to maintain One2Many relationship.

Would you please review and let me know your valuable feedback ?

Matiar · 2017-10-22T06:37:44Z

@aheficent Could you please review ?

AaronHForgeFlow · 2017-10-23T08:10:09Z

Hi @Matiar the question is, can an employee belong to different operating units? If the answer it's true then the field have to be Many2many.

AaronHForgeFlow · 2017-10-24T13:47:13Z

I think it's not necessary to have a default_operating_unit field in the employee. I'd leave just the many2many relationship. Don't you agree?

AaronHForgeFlow · 2017-10-24T15:29:49Z

hr_employee_operating_unit/models/hr_employee.py

+
+    @api.onchange('user_id')
+    def _onchange_user(self):
+        super(HREmployee, self)._onchange_user()


Hi @Matiar Thank you for doing the thing but I was wrong. An employee may belong to different operating units but as a user manage different operating units. So probably this is not needed now. I'm sorry for the misunderstanding.

AaronHForgeFlow · 2017-10-24T15:30:18Z

hr_employee_operating_unit/models/hr_employee.py

+
+    @api.model
+    def _get_operating_unit(self):
+        return self.operating_unit_default_get(self._uid)


Is it possible to achieve the same result in a single method?

…n change user method

oca-clabot · 2017-10-24T18:06:47Z

Hey @Matiar, thank you for your Pull Request.

It looks like some users haven't signed our Contributor License Agreement, yet.
You can read and sign our full Contributor License Agreement here: http://odoo-community.org/page/website.cla
Here is a list of the users:

@shamimislam39 (login unknown in OCA database)

Appreciation of efforts,
OCA CLAbot

Matiar · 2017-10-25T06:46:38Z

Hello @aheficent ,

Thanks for your overall feedback and reviewing the PR. I'm not sure how to fix codecov/project & codecov/patch failure issue. Could you please share some source or link to understand the problem and fixing it.

AaronHForgeFlow · 2017-10-25T07:29:21Z

Hi @Matiar,

You need to add some tests in order to turn green codecov. For example you can test that users cannot see the employees the are not allowed to. You can also test the default method.Are you familiarized with unit test?

Matiar · 2017-10-25T15:40:07Z

Hello @aheficent,

Thanks for your valuable feedback and keeping your patience to accept my mistakes. I added the unit tests as you suggested. Could you please review again ?

AaronHForgeFlow

Code looks nice. Only a minor thing

AaronHForgeFlow · 2017-10-25T15:47:09Z

hr_employee_operating_unit/__init__.py

+# © 2017 Genweb2 Limited - Matiar Rahman
+# License LGPL-3.0 or later (https://www.gnu.org/licenses/lgpl.html).
+from . import models
+from . import tests


Importing tests is not needed

Hi @aheficent ,

I fixed that. Thanks.

AaronHForgeFlow

It works fine. Just a little thing. Can you remove the Create and edit from the widget?

BTW, have you signed the CLA?

Matiar · 2017-10-28T15:07:38Z

Hello @aheficent ,

Thanks for your review and valuable feedback, But IMHO, the change you suggested belongs to the feature of HR module, do you think this module (hr_employee_operating_unit) is the right place to
implement your suggested change ?

JordiBForgeFlow · 2017-10-28T15:57:00Z

He meant from the operating unit field.

…

On Oct 28, 2017 5:07 PM, "Matiar Rahman" ***@***.***> wrote: Hello @aheficent <https://github.com/aheficent> , Thanks for your review and valuable feedback, But IMHO, the change you suggested belongs to the feature of HR module, do you think this module (hr_employee_operating_unit) is the right place to implement your suggested change ? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#97 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHU_Vkjn2KmjhRGfoPQ57GSGZVUCoKN7ks5sw0M7gaJpZM4Pxryw> .

oca-clabot · 2017-10-28T16:59:04Z

Hey @Matiar, thank you for your Pull Request.

It looks like some users haven't signed our Contributor License Agreement, yet.
You can read and sign our full Contributor License Agreement here: http://odoo-community.org/page/website.cla
Here is a list of the users:

@Keya-Sultana (login unknown in OCA database)

Appreciation of efforts,
OCA CLAbot

oca-clabot · 2017-10-28T16:59:05Z

Hey @Matiar,
We acknowledge that the following users have signed our Contributor License Agreement:

@shamimislam39

Appreciation of efforts,
OCA CLAbot

Matiar · 2017-10-29T05:55:04Z

Hello @aheficent ,

Thanks for your feedback. I incorporated your suggested change. Could you please review again ?
And yes, I signed the CLA couple of months back.

AaronHForgeFlow

Tested on runbot and working fine. Just some minor comments to the code.

AaronHForgeFlow · 2017-10-30T08:55:51Z

hr_employee_operating_unit/security/security.xml

+<?xml version="1.0" encoding="utf-8"?>
+
+<odoo>
+<data noupdate="0">


data it's not necessary anymore. Use

Sorry. I meant <odoo noupdate="0"> but it does not matter because that it's the same as <odoo>

AaronHForgeFlow · 2017-10-30T08:56:09Z

hr_employee_operating_unit/views/hr_views.xml

@@ -0,0 +1,27 @@
+<?xml version="1.0" encoding="utf-8"?>
+
+<openerp>


AaronHForgeFlow · 2017-10-30T08:56:30Z

hr_employee_operating_unit/views/hr_views.xml

+            </field>
+        </record>
+
+        <!-- Employee -->


Not needed IMO

Matiar · 2017-10-30T09:43:36Z

Hello @aheficent ,

Thanks again for your time and feedback. I've fixed your suggested changes. Take a look at your convenient time.

AaronHForgeFlow

👍 Functional + code. Thanks @Matiar for attending all the comments.

Matiar · 2017-11-01T08:11:34Z

Hello @aheficent ,

Thanks for your time and approve the PR. Could you please inform me what will be my next step to merge the PR with OCA branch or when it will be merged ?

AaronHForgeFlow · 2017-11-02T08:13:26Z

Hi @Matiar we need at least two more approvals. Normally it takes some time but eventually it'll be merged 😄

grindtildeath

@Matiar
Thanks for your contribution. Here are a few remarks/recommandations.

grindtildeath · 2018-02-28T17:46:18Z

hr_employee_operating_unit/models/hr_employee.py

+
+    @api.model
+    def _get_operating_unit(self):
+        user = self.env['res.users'].browse(self._uid)


No need to browse here, you can do return self.env.user.default_operating_unit_id

grindtildeath · 2018-02-28T17:47:11Z

hr_employee_operating_unit/models/hr_employee.py

+        return user.default_operating_unit_id
+
+    @api.model
+    def _get_operating_units(self):


I suppose you can get rid of this method and call directly _get_operating_unit ?

grindtildeath · 2018-02-28T17:47:41Z

hr_employee_operating_unit/models/hr_employee.py

+    operating_unit_ids = fields.Many2many('operating.unit',
+                                          'operating_unit_emp_rel',
+                                          'emp_id', 'ou_id', 'Operating Units',
+                                          default=_get_operating_units)


Please use lambda to proxy the call to _get_operating_unit

grindtildeath · 2018-02-28T17:49:43Z

hr_employee_operating_unit/tests/test_employee_operating_unit.py

+    def _create_user(self, login, groups, company, operating_units):
+        """ Create a user. """
+        group_ids = [group.id for group in groups]
+        user = self.res_users_model.create({


Here would be nice to add with_context('no_reset_password': True, 'mail_create_nosubscribe': True) to avoid sending unnecessary emails during the test.

grindtildeath · 2018-02-28T17:50:24Z

hr_employee_operating_unit/tests/test_employee_operating_unit.py

+
+    def _create_user(self, login, groups, company, operating_units):
+        """ Create a user. """
+        group_ids = [group.id for group in groups]


You can remove this line and do groups.ids to get the same list.

grindtildeath · 2018-02-28T17:52:03Z

hr_employee_operating_unit/tests/test_employee_operating_unit.py

+            'email': 'test@yourcompany.com',
+            'company_id': company.id,
+            'company_ids': [(4, company.id)],
+            'operating_unit_ids': [(4, ou.id) for ou in operating_units],


Shouldn't [6, 0, operating_units.ids] do the same ?

grindtildeath · 2018-02-28T17:54:02Z

hr_employee_operating_unit/tests/test_employee_operating_unit.py

+
+        emp = self.hr_employee_model.sudo(self.user2.id).search(
+            [('id', '=', self.emp1.id),
+             ('operating_unit_ids', '=', self.main_OU.id)])


Not sure this is working as operating_unit_ids is a x2m field. You probably should use in instead of = or remove the line all together as you're already searching with an id.

grindtildeath · 2018-02-28T18:05:21Z

hr_employee_operating_unit/security/security.xml

@@ -0,0 +1,17 @@
+<?xml version="1.0" encoding="utf-8"?>
+
+<odoo>


Missed in the last review but please add noupdate="1" here.

grindtildeath · 2018-02-28T18:09:52Z

hr_employee_operating_unit/security/security.xml

+        <field eval="1" name="perm_unlink"/>
+        <field eval="1" name="perm_write"/>
+        <field eval="1" name="perm_read"/>
+        <field eval="1" name="perm_create"/>


I'm not sure about this but all the other modules only define perm_read to 1 on the record rule. Is there any reason to set all the perms to 1 ?

AaronHForgeFlow · 2018-06-11T09:25:36Z

Hi @Matiar are you still working on this?

github-actions · 2021-11-07T12:21:27Z

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

[ADD] hr_employee_operating_unit to v10.0

d196f7a

AaronHForgeFlow requested changes Oct 9, 2017

View reviewed changes

Matiar added 4 commits October 18, 2017 18:31

[FIX] When an employee tag with an user, the default operating unit o…

85c0b1d

…f the employee will be the same as user account

[FIX] Travis CI Failed Issue

984d57c

[FIX] Travis CI Failed Issue

8e3f4ac

[FIX] Travis CI Failed Issue 3.1

6eee1ed

Matiar changed the title ~~[ADD] hr_employee_operating_unit to v10.0~~ [ADD] [10.0] hr_employee_operating_unit Oct 19, 2017

[FIX] Implement a M2M relationship between Employee and Operating Unit

b05af90

Matiar added 2 commits October 24, 2017 19:47

[FIX] Travis CI Failed Issue 5.1

c46c781

[FIX] Remove default operating unit from employee

8dbe390

AaronHForgeFlow reviewed Oct 24, 2017

View reviewed changes

[FIX] Remove nested method to get default operating unit and remove o…

f6a0a14

…n change user method

Matiar added 3 commits October 25, 2017 20:03

[ADD] Addedd Unit Test

c0f129a

[FIX] Travis CI Error

6800692

[FIX] Travis CI Error

1efeca8

AaronHForgeFlow reviewed Oct 25, 2017

View reviewed changes

[FIX] Remove tests from init file

e7fdf52

AaronHForgeFlow reviewed Oct 26, 2017

View reviewed changes

[FIX] No option to create operating unit from M2M widget

398eeb2

AaronHForgeFlow reviewed Oct 30, 2017

View reviewed changes

[FIX] Fix Suggested Changes

fb26e46

AaronHForgeFlow approved these changes Oct 30, 2017

View reviewed changes

AaronHForgeFlow added the needs review label Nov 10, 2017

grindtildeath requested changes Feb 28, 2018

View reviewed changes

grindtildeath reviewed Feb 28, 2018

View reviewed changes

max3903 added this to the 10.0 milestone Dec 27, 2019

github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Nov 7, 2021

github-actions bot closed this Dec 12, 2021

		@@ -0,0 +1,27 @@
		<?xml version="1.0" encoding="utf-8"?>

		<openerp>

		@@ -0,0 +1,17 @@
		<?xml version="1.0" encoding="utf-8"?>

		<odoo>

[ADD] [10.0] hr_employee_operating_unit #97

[ADD] [10.0] hr_employee_operating_unit #97

Conversation

Matiar commented Oct 8, 2017 • edited

Matiar commented Oct 9, 2017

AaronHForgeFlow left a comment

Choose a reason for hiding this comment

Matiar commented Oct 9, 2017

Matiar commented Oct 19, 2017

Matiar commented Oct 22, 2017

AaronHForgeFlow commented Oct 23, 2017

AaronHForgeFlow commented Oct 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oca-clabot commented Oct 24, 2017

Matiar commented Oct 25, 2017 • edited

AaronHForgeFlow commented Oct 25, 2017

Matiar commented Oct 25, 2017

AaronHForgeFlow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AaronHForgeFlow left a comment

Choose a reason for hiding this comment

Matiar commented Oct 28, 2017

JordiBForgeFlow commented Oct 28, 2017 via email

oca-clabot commented Oct 28, 2017

oca-clabot commented Oct 28, 2017

Matiar commented Oct 29, 2017

AaronHForgeFlow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AaronHForgeFlow Oct 30, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Matiar commented Oct 30, 2017

AaronHForgeFlow left a comment

Choose a reason for hiding this comment

Matiar commented Nov 1, 2017

AaronHForgeFlow commented Nov 2, 2017 • edited

grindtildeath left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grindtildeath Feb 28, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AaronHForgeFlow commented Jun 11, 2018

github-actions bot commented Nov 7, 2021

Matiar commented Oct 8, 2017 •

edited

Matiar commented Oct 25, 2017 •

edited

AaronHForgeFlow Oct 30, 2017 •

edited

AaronHForgeFlow commented Nov 2, 2017 •

edited

grindtildeath Feb 28, 2018 •

edited