Apply auto_groups_filter to Project Manager groups (#5300)#5420
Conversation
|
|
||
| group_names.grep(/#{filter}/) | ||
| rescue RegexpError => e | ||
| if defined?(Rails) && Rails.respond_to?(:logger) && Rails.logger |
There was a problem hiding this comment.
Did you encounter a case where Rails.logger is undefined here? Not that we can't be overly cautious here (as it is important that something is logged) just curious if this was something you actually ran into
There was a problem hiding this comment.
No, I didn’t actually run into it. VS Code kind of autofilled that pattern while I was writing the method and seeing that suggestion, I figured being a bit defensive made sense at the time. But you’re right, in this context it’s probably unnecessary so I've cleaned it up
Bubballoo3
left a comment
Bubballoo3
left a comment
There was a problem hiding this comment.
This looks pretty good, the only part I am unsure of is the abundance of caution within the filtered_group_names method, where things that are universally relied upon within the dashboard like Configuration and Rails are treated as though they could be absent. It seems to me that if either are undefined there are bigger problems going on, and we would want to raise an error (though other code would almost certainly raise first).
But overall that constitutes a few extraneous lines that should never be needed, so nothing major but we should still try to avoid it if possible.
| return group_names unless defined?(Configuration) | ||
|
|
||
| filter = Configuration.auto_groups_filter | ||
| return group_names if filter.nil? || filter.to_s.empty? |
There was a problem hiding this comment.
nil.to_s.empty? evaluates to true every time, so we can remove the separate nil check
There was a problem hiding this comment.
That's a good point. Now that I look back at it, I think that filter.blank? would work even better here so it handles nil/empty cleanly in one check. This would mean that auto_groups_filter: " " would now be treated as empty, which I think is the behavior we want
github-project-automation
Bot
moved this from Awaiting Review
to Changes Requested
in PR Review Pipeline
Bubballoo3
left a comment
Bubballoo3
left a comment
There was a problem hiding this comment.
I tested this out and it works well, but I think we can just use the filtering approach from auto_groups without modification, ensuring that the behavior in app forms does not change at all.
|
|
||
| def filtered_group_names | ||
| filter = Configuration.auto_groups_filter | ||
| return group_names if filter.blank? |
There was a problem hiding this comment.
After playing around with this I'm pretty sure this extra check is not needed at all.
When the filter is a blank string like ' ' then the filter will reject everything and return an empty array (since group names cannot contain spaces). This is a good signal to anyone setting this configuration that they might have made a mistake, as well as being the current behavior of auto_groups_filter in app forms.
When the filter is nil, the regex // filters nothing, so you get the full group_names that you are returning here.
Basically when in doubt, we should stick to the existing behavior, and this check slightly changes that behavior with no tangible benefit. By all means correct me if there is something I am missing
| require 'etc' | ||
| require 'singleton' | ||
| require 'active_support/core_ext/module/delegation' | ||
| require 'active_support/core_ext/object/blank' |
There was a problem hiding this comment.
I removed this and blank? was still defined, but it looks like we may not even need the blank? check in the first place
github-project-automation
Bot
moved this from Changes Requested
to Merged/Closed
in PR Review Pipeline
3 participants
Fixes #5300
Project Manager’s group selection dropdown was showing every group the user belongs to (including broad/noisy ones like domain users).