Skip to content

reduce list of projects for admin resolves #1187 #1238

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.

Sign up for GitHub

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

Open
MontrealSergiy wants to merge 11 commits into
base: dev
Choose a base branch
from
Open

reduce list of projects for admin resolves #1187 #1238

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
b0bc997
reduce list of projects for admin resolves #1187"
MontrealSergiy May 9, 2022
b5e7fc6
add other admin users (mostly for tools setup) fixes #1187
MontrealSergiy May 9, 2022
bc5dac7
add misc public categories reduce list of projects for admin resolve…
MontrealSergiy May 10, 2022
29bcd5d
add back few more projects for admin resolves #1187
MontrealSergiy May 11, 2022
7880426
refactor several pretty name methods into one, admin groups fixes #1187
MontrealSergiy May 30, 2022
816df7d
a refactoring of public group variable of a group table view resolves…
MontrealSergiy Jun 6, 2022
232d7b7
a refactoring of public group variable of a group table view resolves…
MontrealSergiy Jun 6, 2022
5941bee
Merge branch 'dev' into admingroups
MontrealSergiy Aug 31, 2022
809af83
minor fix in group select
MontrealSergiy Jan 23, 2023
69f24cb
add a redundant return to wg pretty categories
MontrealSergiy Apr 17, 2023
f35dd41
put back an old buggy pretty category optimization
MontrealSergiy Apr 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 21 additions & 4 deletions BrainPortal/app/helpers/select_box_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,21 @@ def group_select(parameter_name = "group_id", options = {}, select_tag_options =
selected = selector.to_s
end

# for Admin filter out only public, invisible or non_assignable to reduce the clutter (unless groups passed)
if current_user.has_role?(:admin_user) && !options.has_key?(:groups)
admin_ids = AdminUser.all.pluck(:id) || []
groups = groups.select do |g|
( g.is_a?(SystemGroup) || # everyone group, to make a private tool public
Copy link
Member

@prioux prioux Apr 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Reorder the conditions such that the fast conditions are checked first: g.public, g.invisible, etc
  2. When comparing with selected, the part selected.include(g.id.to_s) will wrongly select some group if for instance we selected is the string "12345" and the current group is ID "34". Use Array(selected).include... instead.

Copy link
Member

@prioux prioux Apr 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't there a better way to generate this list? Isn't this equivalent to

groups = SystemGroup.all.to_a | WorkGroup.where(:creator_id => admin_ids).to_a

(or close to that?)

g.is_a?(UserGroup) && (admin_ids & g.user_ids).present? || # admin usergroups
g.public ||
g.invisible ||
g.not_assignable && (g.user_ids & admin_ids ).present? || # notassingable admin groups
g.not_assignable && admin_ids.include?(g.creator_id) || # notassignable created by admin
(selected == g.id.to_s) || selected.include?(g.id.to_s) # already selected
)
end
end

# Optimize the labels for UserGroups and SiteGroups, by extracting in a hash
group_labels = {}
group_labels.merge!(UserGroup.prepare_pretty_labels(groups))
Expand Down Expand Up @@ -145,15 +160,17 @@ def group_select(parameter_name = "group_id", options = {}, select_tag_options =

# Step 1: My Work Projects first
ordered_category_grouped << [ "My Work Projects", category_grouped_pairs.delete("My Work Projects") ] if category_grouped_pairs["My Work Projects"]

ordered_category_grouped << [ "My Public Work Projects", category_grouped_pairs.delete("My Public Work Projects") ] if category_grouped_pairs["My Public Work Projects"]
# Step 2: All personal work projects first
category_grouped_pairs.keys.select { |proj| proj =~ /Personal Work Projects/ }.sort.each do |proj|
category_grouped_pairs.keys.select { |proj| proj =~ /Personal (Public )?Work Projects/ }.sort.each do |proj|
ordered_category_grouped << [ proj, category_grouped_pairs.delete(proj) ]
end

# Step 3: Other project categories, in that order
[ "Shared Work Projects", "Empty Work Projects", "Site Projects", "User Projects", "System Projects", "Invisible Projects", "Everyone Projects" ].each do |proj|
[ "Shared Work Projects", "Empty Work Projects", "Site Projects", "System Projects", "Invisible Projects", "Everyone Projects", "User Projects"].each do |proj|
ordered_category_grouped << [ proj, category_grouped_pairs.delete(proj) ] if category_grouped_pairs[proj]
proj = proj.sub(" ", " Public ")
ordered_category_grouped << [ proj, category_grouped_pairs.delete(proj) ] if category_grouped_pairs[proj]

end

# Step 4: Other mysterious categories ?!?
Expand Down
32 changes: 20 additions & 12 deletions BrainPortal/app/models/work_group.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,20 +77,28 @@ def self.prepare_pretty_category_names(groups = [], as_user = nil)
wgs
end

def pretty_category_name(as_user) #:nodoc:
def pretty_category_name(as_user = nil) #:nodoc:
return @_pretty_category_name if @_pretty_category_name
if self.invisible?
@_pretty_category_name = 'Invisible Project'
elsif self.public?
@_pretty_category_name = 'Public Project'
elsif self.users.count == 0
@_pretty_category_name = 'Empty Work Project'
elsif self.users.count > 1
@_pretty_category_name = 'Shared Work Project'
elsif as_user.present? && (self.creator_id == as_user.id || self.users.first.id == as_user.id)
@_pretty_category_name = 'My Work Project'
if @_pretty_category_name.blank?
Copy link
Member

@prioux prioux Apr 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the if surrounding the code at line 82. It wasn't there in the original and it's useless.

if self.invisible?
@_pretty_category_name = 'Invisible Project'
elsif self.public?
@_pretty_category_name = 'Public Project'
elsif self.users.count == 0
@_pretty_category_name = 'Empty Work Project'
elsif self.users.count > 1
@_pretty_category_name = 'Shared Work Project'
elsif as_user.present? && (self.creator_id == as_user.id || self.users.first.id == as_user.id)
@_pretty_category_name = 'My Work Project'
else
@_pretty_category_name = "Personal Work Project of #{self.users.first.full_name}"
end
end
# Public is more of quantifier, there could be public shared or public personal projects
Copy link
Member

@prioux prioux Apr 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please adjust the code between line 97 and 102

  1. The @_pretty_category_name.present? is always true
  2. There is no need for a else !!!!

Copy link
Member

@prioux prioux Apr 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, based on the logic, you will never re-sinsert " Public " on anything but Invisible Project. You'll never get for instance 'Share Public Work Project' because of the if/elif structure above.

Copy link
Member

@prioux prioux Apr 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can I just suggest you set up, early, a string variable that has either "" or "Public " and then you just insert it in the original category names, instead of trying to retrofit the word at the end?

Copy link
Contributor Author

@MontrealSergiy MontrealSergiy Apr 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi so would you like

a) adjust code to enable Share Public Work Project or
b) just simplify new lines a bit
c) return old code

at this moment I will be fine with c) to avoid any perceived blame for the existing production bug shown on snapshot ( I factored it out into a separate issue #1323 we can fix it once agree on the best way to do it)

Copy link
Contributor Author

@MontrealSergiy MontrealSergiy Apr 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ignore then the above question I will try add variable .. one min

Copy link
Member

@prioux prioux Apr 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK.

I think honestly that the original code required very little changes. It wasn't buggy.

If you see the filter list not behaving the way you expect, the problem is not in this method. Like I said, look for other places where the instance variable is modified.

if @_pretty_category_name.present? && self.public && ! @_pretty_category_name.include?('Public')
@_pretty_category_name = @_pretty_category_name.sub(" ", " Public ")
else
@_pretty_category_name = "Personal Work Project of #{self.users.first.full_name}"
@_pretty_category_name
end
@_pretty_category_name
end
Expand Down
51 changes: 42 additions & 9 deletions BrainPortal/app/views/shared/_group_tables.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,19 +23,29 @@
-%>

<%
public_groups = []
if current_user.has_role? :admin_user
work_groups = WorkGroup.where(invisible: false).all
invis_groups = WorkGroup.where(invisible: true).all
admin_ids = AdminUser.all.pluck(:id) || []
public_groups = WorkGroup.where( invisible: false, public: true ).all
nonassign_groups = WorkGroup.joins(:users).where( invisible: false, public: false,
not_assignable: true, "users.id" => admin_ids ).all
nonassign_groups |= WorkGroup.where( invisible: false, public: false,
not_assignable: true, creator_id: admin_ids ).all
nonassign_groups = nonassign_groups.uniq
work_groups = model.groups.select { |g| g.is_a?(WorkGroup) && ! g.invisible && ! g.public &&
! nonassign_groups.include?(g) }
invis_groups = WorkGroup.where( invisible: true ).all
elsif current_user.has_role? :site_manager
work_groups = current_user.site.groups.where( :type => "WorkGroup", :invisible => false ).all | current_user.groups.where( :type => "WorkGroup", :invisible => false )
invis_groups = current_user.site.groups.where( :type => "WorkGroup", :invisible => true )
work_groups = current_user.site.groups.where( :type => "WorkGroup", :invisible => false ).all | current_user.groups.where( :type => "WorkGroup", :invisible => false )
invis_groups = current_user.site.groups.where( :type => "WorkGroup", :invisible => true )
else
work_groups = []
invis_groups = []
work_groups = []
invis_groups = []
end
work_groups = work_groups.sort { |a,b| a.name <=> b.name }
invis_groups = invis_groups.sort { |a,b| a.name <=> b.name }
class_name = model.class.sti_root_class.to_s.underscore
work_groups = work_groups.sort { |a,b| a.name <=> b.name }
invis_groups = invis_groups.sort { |a,b| a.name <=> b.name }
public_groups = public_groups.sort { |a,b| a.name <=> b.name }
class_name = model.class.sti_root_class.to_s.underscore
%>

<%
Expand Down Expand Up @@ -75,3 +85,26 @@

<% end %>



<% if public_groups.present? %>

<br>
<label>Public Projects</label>
<%= array_to_table(public_groups, :cols => 4, :td_class => 'left_align no_wrap') do |group,r,c| %>
<%= group_check_box.(group) %>
<% end %>

<% end %>



<% if nonassign_groups.present? %>

<br>
<label>Non-Assignable Projects</label>
<%= array_to_table(nonassign_groups, :cols => 4, :td_class => 'left_align no_wrap') do |group,r,c| %>
<%= group_check_box.(group) %>
<% end %>

<% end %>