Have other rights appear in groupless #5
Conversation
| @@ -34,6 +34,9 @@ def by_groups | |||
| rights = [] | |||
| rights += regular_rights_with_group | |||
| rights += restricted_rights_with_group | |||
| other_rights = Right.all - rights | |||
| other_rights.each { |right| right.group = 'Groupless' } | |||
There was a problem hiding this comment.
Can we just append the groupless rights to the end of the resulting array instead of introducing a hardcoded group name?
There was a problem hiding this comment.
Or maybe even clearer if we add a new method that just return the "groupless" rights.
| rights += regular_rights_with_group | ||
| rights += restricted_rights_with_group | ||
| rights = regular_rights_with_group + restricted_rights_with_group | ||
| rights += (Right.all - rights) |
There was a problem hiding this comment.
Maybe I'm reading this wrong, but wouldn't this end up assigning all rights to the rights variable?
It reads like
a = [1,2,3]
b = [4,5,6]
all = [1,2,3,4,5,6,7,8,9]
sum = a + b
sum = sum + all - sum # => allOr are the regular_rights_with_group and restricted_rights_with_group somehow decorated?
There was a problem hiding this comment.
Ahh, I see the decoration happening below, with the assignment of the group. That answers the above question.
There was a problem hiding this comment.
Optional suggestion. Maybe something like...
rights_with_groups = regular_rights_with_group + restricted_rights_with_group
other_rights = Rights.all - rights_with_groups
rights = rights_with_groups + other_rights
It's an extra line but does make it easier to understand the logic.
There was a problem hiding this comment.
@alxberardi Shall I make it like the above and merge?
There was a problem hiding this comment.
I'm happy how it is now. If you want to implement @vgmaster21 's suggestion feel free to. I just didn't immediately notice the assignment of the groups to the rights.
|
👍 (though this gem could use some more nice refactoring) |
No description provided.