This repository has been archived by the owner on Jan 4, 2021. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 2
Have other rights appear in groupless #5
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
921c670
Have other rights appear in groupless
mnoack 41c1fb9
Don't define group, leave nil and let view display how it wants
mnoack c78b9a7
Collapse existing array building
mnoack 998299c
Simplify code
mnoack cc26744
Break up complex methods
mnoack 15fbfd3
Rails 5 support
mnoack 0da1a97
Test rails 5 in travis
mnoack File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
source 'https://rubygems.org' | ||
gemspec :path => '../' | ||
|
||
group :development, :test do | ||
gem 'rails', '~> 5.0' | ||
end |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I'm reading this wrong, but wouldn't this end up assigning all rights to the
rights
variable?It reads like
Or are the
regular_rights_with_group
andrestricted_rights_with_group
somehow decorated?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alxberardi Shall I make it like the above and merge?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.