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

Possible solution for issue #215 #252

Merged
merged 3 commits into from
Sep 8, 2015
Merged

Possible solution for issue #215 #252

merged 3 commits into from
Sep 8, 2015

Conversation

draganjovanovich
Copy link
Contributor

Possible solution for issue #215

@draganjovanovich
Copy link
Contributor Author

Ok, I made one obivious mistake , there is missing ' user.setManager(user.getManager()); in 'saveRoles'

@vitalidze
Copy link
Owner

Generally, looks good. There are two issues:

  1. Formatting, the opening curly brace { should go on the same line of the if statement. You can check the formatting of other if blocks.
  2. I don't like the approach of letting "manager" user change his expiration date and number of devices. He should not be able to change such parameters. I think only admin user can set it up for himself, and maybe even this should be restricted because it looks weird.

@draganjovanovich
Copy link
Contributor Author

Yeah,i think that too,but i thought that is the way you wanted so i leave it there, i will change this....

So admin can change all but expiration date and admin cant set himself to readonly and turn off admin option, and manager cant change expiration date and max number of devices?

@vitalidze
Copy link
Owner

I think this is the way it should be:

  • neither admin nor manager for himself cannot set
    • expiration date
    • blocked
    • read only
    • maximum number of devices
    • admin flag
    • manager flag
  • admin can do anything for all other users
  • manager can do anything to all managed users, except setting up 'admin' flag

@draganjovanovich
Copy link
Contributor Author

Ok, but in that way manager create new account and can set expiration and max device number to an new account and use that instead ?

@vitalidze
Copy link
Owner

Yes, that's right. This should be controlled as well so he cannot set up more devices for the new account that are already distributed along his managed users. This task is more complex from my point of view. How do you suggest to solve it? You cannot restrict 'manager' from changing the maximum number of devices from the underlying user.

@draganjovanovich
Copy link
Contributor Author

I think that if manager is able to set max devices and expiration date to others it should be able to edit it for him self also... this is how i will leave it for my usage of traccar

@vitalidze
Copy link
Owner

Well, by design I think that a 'manager' should be considered as a group of other users (or as a company). Administrator may allow some group to have total N number of devices. Then 'manager' of that group can distribute all these devices among underlying sub-groups (i.e. 'manager's). It don't look fair if any manager can just increase the number of allowed devices. I don't see much difference in allowing to change this for ordinary user, then if anyone can change it - what is the purpose of this setting. What is your use case?

P.S. I like the approach from #214 There is a good picture fully describing this preferred implementation

@draganjovanovich
Copy link
Contributor Author

Ok, i just dont use manager at all,just plain users and andmin, but if you want to manager be able to change max dev number and expiration date to others and not for him self , i think that you shold disable registration of new users and restrict option of adding new users with manager account ...

Becouse i can as manager just logout, registrate new user,go back and login as manager set user as manager ,add more devices change expiration date and so on...

@draganjovanovich
Copy link
Contributor Author

Ok, i just saw picture, so for this solution, i think that is best solution to disable new user registration and just use manager and admin for creation of new users, restrict managers so they can edit only users that they created, and add new sql entry 'maximum managed devices' for managers or something like that ,and that maximum managed device should change only admin...

@vitalidze
Copy link
Owner

  1. There is an option to disable user registration in 'Settings >> Global' menu. It can be disabled anytime.
  2. Why doesn't single setting for 'maximum number of devices' work for managers? I think messing with two settings will be more complex from the user's perspective.
  3. In theory there can be an option to restrict maximum number of devices for all newly created users. Can be set to zero. This will help handling your case when user can register himself as a manager with unlimited number of devices. I think it can be useful somewhere else.

@draganjovanovich
Copy link
Contributor Author

Single 'maximum number of devices' wont work becouse you as manager should get 'maximum managed devices' from admin ,and when you as manager creating new user that 'maximum managed devices' is limit for 'maximum number of device' you can distribute to that and other users ,it wont be complicated for users ,becouse only managers and admin will see that option, and that is just property of managers. And that will solve problem from your 3. answer also. Becouse manager can increase maximum devices for new user, but that way he will decreasing maximum managed number .

@vitalidze
Copy link
Owner

I still don't get the difference. You can name 'maximum number of devices' setting for manager as 'maximum managed devices', but in data model it still will be the same setting with same meaning. Why do you need two equal settings for the manager? Or what will be the difference for manager?

@draganjovanovich
Copy link
Contributor Author

I dont know english that well ,so maybe i wasnt correct in my replies, i just folowing picture of #214

If you want manager to have example "200 devices" than users that he created in sum cant exceed that number, and one setting 'maximum managed devices' <-- that settings only admin can change , but 'maximum devices number' <-- that can change manager for him self and other users...

@vitalidze
Copy link
Owner

I don't understand why manager should bother restricting devices for himself. If administrator limited him to 200 devices why it is not enough? Why should he set up some limit for himself? This is where the mess comes from my point of view.

@draganjovanovich
Copy link
Contributor Author

Ok, he dont need to set for him self,but i understand you in earlier posts that he should be able to,so just becouse of that. for now , i will just make web app to work like its mentioned in picture #214 ,and will create pull request. and you will see than, becouse ,i realy cant explain good enough my self with my english ... sorry :)

@vitalidze
Copy link
Owner

Ok, sorry if I confused you, but I cannot find my answer where I was talking about it. Can you please point me?

Anyway, I will wait for a pull request, you are always welcome implementing it. Then we can discuss the implementation as well.

@draganjovanovich
Copy link
Contributor Author

I think that is best solution ,becouse i want to say something and explain my self ,but i just dont have words... :) i am serbian, and in school i was not learning english...

@vitalidze
Copy link
Owner

No problem, you english is good and understandable.

@draganjovanovich
Copy link
Contributor Author

Ok, i will try one more time.

I will try to implement this:

  1. Manager wont be able able to edit maximum devices, and expiration date for him self.
  2. Manager will be able to create new users and set 'maximum devices number' and 'expiration date' for them.
  3. Manager wont be able to set 'expiration date' for other users that is bigger(longer ?) than him self 'expiration date'.
  4. Managers 'maximum devices number' will decrease for amount of devices he add to his account, and for 'maximum devices number' he reserved for other users.

So if manager 'maximum devices number' is '50' when he create '3' users with 'maximum devices number' of '10' each, than he will have 'maximum devices number' of '20' for him self to use or distribute to new users, i hope my idea is clear now. :)

@vitalidze vitalidze merged commit cd73817 into vitalidze:dev Sep 8, 2015
vitalidze added a commit that referenced this pull request Sep 8, 2015
@vitalidze
Copy link
Owner

Merged and included in 0.8.1rc2

vitalidze added a commit that referenced this pull request Sep 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants