Skip to content
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.

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

reduce list of projects for admin resolves #1187 #1238

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

Conversation

MontrealSergiy
Copy link
Contributor

this reduces the project lists for admin, which became unwieldy.

@natacha-beck the feature complicates task of setting the prototype tools to a private group, as well as creation of private data providers. Please confirm that you ok with it or need some workaround (e.g. adding admin workgroups, perhaps prioriterization instead of removal)

@MontrealSergiy MontrealSergiy added the Admin Features or bugs related to administrative features label May 11, 2022
@MontrealSergiy MontrealSergiy self-assigned this May 11, 2022
end
end

def pretty_category_name_no_pub(as_user) #:nodoc:
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not really see the point to have an additional method here, I think we can only keep pretty_category_name and add extra logic in it.

Copy link
Contributor Author

@MontrealSergiy MontrealSergiy May 14, 2022

Choose a reason for hiding this comment

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

I wanted to have something like Public Shared group, etc that is use 'public' a quantifier to existing group, In this way it is not just adding one Public Group category so adding a new method is justified imho.

BTW it also it's possible to use instead just Public group category then only small corrections to the existing method will be required (probably due to bug in the existing code)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the additional method is removed

@@ -118,6 +118,22 @@ 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
if current_user.has_role? :admin_user
Copy link
Contributor

Choose a reason for hiding this comment

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

When reading the Issue:

Those lists should be trimmed down to remove all private work projects

I think to keep the old groups and then remove only the private work projects will do what we want.
Instead of a long selection with a lot of conditions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's was @prioux suggestion keep unassignable groups

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as a compromise maybe keep all the nonassignable group, rather than admin related

BrainPortal/app/views/shared/_group_tables.html.erb Outdated Show resolved Hide resolved
@prioux
Copy link
Member

prioux commented Aug 30, 2022

Also rebase this one.

@MontrealSergiy
Copy link
Contributor Author

done

@natacha-beck
Copy link
Contributor

@MontrealSergiy this PR will close issue number #1187 no ? If it is the case can you update the Development session on this page.

@MontrealSergiy MontrealSergiy linked an issue Sep 27, 2022 that may be closed by this pull request
@MontrealSergiy
Copy link
Contributor Author

@MontrealSergiy this PR will close issue number #1187 no ? If it is the case can you update the Development session on this page.

done

@prioux
Copy link
Member

prioux commented Jan 12, 2023

The new helper group_select does not behave properly.

If a programmer invokes it in a page with an explicit list of groups passed in options, it is not supposed to replace that list with its own list. It's supposed to respect it.

The behavior we want is for the de list of groups to be reduced ONLY when the default list is generated internally.

Copy link
Member

@prioux prioux left a comment

Choose a reason for hiding this comment

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

See today's comment (2022-01-12)

Copy link
Member

@prioux prioux left a comment

Choose a reason for hiding this comment

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

I made these comments two weeks ago and forgot to 'commit' them

elsif as_user.present? && (self.creator_id == as_user.id || self.users.first.id == as_user.id)
@_pretty_category_name = 'My Work Project'
def pretty_category_name(as_user = nil) #:nodoc:

Copy link
Member

Choose a reason for hiding this comment

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

Why are you removing the quick 'return cached value' code? It's designed to make the code quicker! It's a standard patern:

def something
  return @cached_value if @cached_value
  @cached_value = compute it
  ...
  @cached_value
end

Copy link
Member

Choose a reason for hiding this comment

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

You removed the final returned value and replaced it with a ugly if else (line 95 in the original code, line 98-102 in your code)

Copy link
Contributor Author

@MontrealSergiy MontrealSergiy Apr 13, 2023

Choose a reason for hiding this comment

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

hi, I guess redundant final value is a coding style thing, so I should not touch it.

Early return seems like a bug resulting e.g. in absence of Public type in filter dropdown (see snapshot attached), but I can factor out bugfix into a separate pull request to make think moving, what do you think @prioux
Screenshot from 2023-04-13 15-45-13

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand. Why would caching the computed value of pretty_category_name cause a bug? It's not something that changes over time, ever.

Copy link
Member

Choose a reason for hiding this comment

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

Just leave the code as it was! Caching at beginning, final returned value 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.

did you see screen
shot? Plenty of "private" in the table but none in the filter dropdown is strange at the least.

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.

Yes, but that's not related to the pretty_category_name() method. That method computes a value and caches it as an instance variable of the object. That's totally outside the realm of the table filter generator. I suggest you look at other places where @_pretty_category_name might be overriden. As far as pretty_category_name() is involved, it should do exactly like it has always done, + your modifications (but with the caching and final returned value re-added).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually you are right, the filter is not related, it just uses attribute as is. But for other instances it is related to pretty_category_name() (@_pretty_category_name is being overwritten by self.prepare_pretty_category_names()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created separate bug issues as it is not directly related to the subject of reducing project list

Copy link
Member

@prioux prioux left a comment

Choose a reason for hiding this comment

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

See comments

elsif as_user.present? && (self.creator_id == as_user.id || self.users.first.id == as_user.id)
@_pretty_category_name = 'My Work Project'
def pretty_category_name(as_user = nil) #:nodoc:

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.

Yes, but that's not related to the pretty_category_name() method. That method computes a value and caches it as an instance variable of the object. That's totally outside the realm of the table filter generator. I suggest you look at other places where @_pretty_category_name might be overriden. As far as pretty_category_name() is involved, it should do exactly like it has always done, + your modifications (but with the caching and final returned value re-added).

@MontrealSergiy
Copy link
Contributor Author

MontrealSergiy commented Apr 20, 2023

in my opinion it is directly related. The method got updated while the 'pre-optimization' have not.

But if you are still in doubt I can factor out bug fix into separate issue, as several different solutions are possible. Removal of short return is simple fix but not necessarily the best

@_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

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

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

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

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

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

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.

@_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

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.

Copy link
Member

@prioux prioux left a comment

Choose a reason for hiding this comment

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

See comments

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

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

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?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Admin Features or bugs related to administrative features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove full list of work projects in admin interfaces
3 participants