evereq
commented
4 years ago OK, so let's go step by step (my opinions below, 🗡️ - I could be wrong haha):
1) The fact that we have Users and Employees tables and not all users will have Employee record is well known already (only users who are employees will have such Employees record table)
2) Agree, that in many places we can figure out if a user is an employee by just looking in the table Employee to see if relevant record exists or not (or we can add a flag to the user's table, but I would prefer NOT to do that). We should do it NOT for permissions/security checks, however. (see number 3 below).
3) Agree, that we should not do any security checks based on Roles, ever (except maybe Super Admin). I.e. all checks should be based on permissions.
4) We absolutely should have a Role called "Employee". Yes, different permissions could be assigned in different organizations to such Employee Role users, but we need to have that Role because it's SO important and easy to use it. For example, if someone setups a new Org, he will have "Super Admin", "Admin" and "Employee" roles pre-configured. So she/he can easily start adding all employees and give them permissions by only assigning them to Employee roles. Basically, every time someone will be added on Employees Management Page, such a record should get automatically Role "Employee" (even before such user can log in later). Note: we could be calling such role something completely different actually and it has nothing to do really with the Employees table in our DB. We probably will also add Role "Contractor" for example and Role "Candidate" etc. Next, for example, users who are "Contractors" will probably have also record in table "Employee" (because Contractors ARE Employees to some extend). However, users of the role "Candidate", will NOT have record in the "Employees" table, etc. This proves that Employee table has NO relations to Role Employee :)
5) Probably agree that it was a bad idea to allow user to be assigned to multiple Roles. That's not needed. What is needed is just that if the user is Admin / Super-Admin roles he automatically gets assigned almost all permissions (at least cover all permissions from Employee Role). That should give such users access to ALL functionality, including access to features available for Employees. I think the problem is that when we create Super-Admin or Admin users, we do NOT create for them Employees record :) So such users can't really use all features of Employees now because many pages failing as no Employee record exists... So we need to add checks on all such places (e.g. page "My Tasks") that users have recorded in the employee table. If no record found, we assume User is NOT employee and hide such page (menu item).
6) At the same time, when user invites from Employee Management Page, we of course will create for such user record in the Employee Table, so such user always will have such Employee record and all will work for her/him
7) More problematic situation is when User created in the Users Management Page (or invited). In such case, we need to figure out what is Role of such a user. If Role is going to be Admin / Super-Admin, we don't necessarily need to create Employee records. However, we need to allow to select some checkbox like "Allow the user to access Employees features" (and allow to do it for existed users) and if such checkbox set, we automatically create a record in the Employees Table for such user, even if his role is not an employee but for example Admin or Super-Admin. I.e. this will allow us to have Users who don't have Employee records and who have Employee record and every time will be possible to change that for each user.
- If some user was set to "allow the user to access Employees features", but later such checkbox was cleared, we need to display a "warning: this user has access to Employees features now. Removing such access will also remove Employee profile from such user".
What do you think?
rmagon
This is from https://github.com/ever-co/gauzy/pull/1565#issuecomment-652830412 Answer to above: Unfortunately, a user can not have multiple roles as of now. But I feel we shouldn't allow users to have multiple roles instead:
I think
Employeeshouldn't really be a role, because Employee is a separate entity altogether and I think we will definitely need to assign roles to Employees too. Whether a user is an employee or not should not be combined with user roles, it can just be a simple check whether the Employee foreign key exists or not, or maybe we can. put a flag.Also, we should definitely not do ANY checks based on
rolethen (they should all be based on permissions). Perhaps just some for Super Admin, but no other.cc: @evereq