Closed nirajkumar999 closed 2 months ago
This is great Niraj - very well documented and easily understandable. Welcome to the open-source world!
You'll just need to run rubocop to make the tests pass bundle exec rubocop -A
This is great Niraj - very well documented and easily understandable. Welcome to the open-source world!
Thanks for accepting my PR.
Now I think at a certain level I have upskilled myself in this field
More PR will come in coming days.
You'll just need to run rubocop to make the tests pass
bundle exec rubocop -A
sir do i need to include schema.rb and Gemfile.lock files also into my commits. I have ran the rubocop test successfully. The main file that was roles.rb have been updated and pushed.
Sir do let me know
Nope - this is all good. Thanks!
Nope - this is all good. Thanks!
Sir I only ran rubocop test.
Do we need to run any other test like eslint or rspec.
If so do let me know along with the required command. This is my first time so I dont know the process.
Thank You Sir.
1. First Improvement
We have "index_roles_on_name_and_provider" UNIQUE, btree (name, provider)" set to our Role Model and to roles db.
So this way admin having ManageRoles permission should not be able to create multiple roles with same role name and provider combination. But current code misses the edge case where we need to ensure that roles names should be unique ignoring their case.
Here is the picture showing this how two roles with same name (ignoring case) with provider 'greenlight' are treated different.
Hence a role with name "Dummy-role" and "dummy-role" with a given provider say 'greenlight' should not be treated different. So i have added case_sensitive: false option to name validation. This way duplicate name, ignoring the case, wont be allowed for roles.
2. Second Improvement
In set_role_color function, we generate a secure random 3 digit hex value to set the role.color property. But this does not handle the case if two roles get assigned same color. Ideally each role should be assigned different color so that at UI we can identify them separately. I know chances of collision will be rare because we will never have requirement for having too many roles. But in the way we handle the case to set a unique friendly_id property and session_id property for a room model, the same I have replicated here.
I hope my both improvements will help.
Thank You.