Moved the members and groups to single option called members
What does this MR do?
Are there points in the code the reviewer needs to double check?
Why was this MR needed?
Screenshots (if relevant)
Project members and groups
Gear button gif
Does this MR meet the acceptance criteria?
-
Changelog entry added -
Documentation created/updated -
API support added - Tests
-
Added for this feature/bug -
All builds are passing
-
-
Conform by the merge request performance guides -
Conform by the style guides -
Branch has no merge conflicts with master
(if it does - rebase it please) -
Squashed related commits together
What are the relevant issue numbers?
Closes #25985 (closed)
Merge request reports
Activity
@jivanvl : Thanks for starting this! Are you planning to do both FE + BE all together in this one merge request?
@victorwu Yes I'll try to get both ready in this one MR
added 1 commit
- 4d959814 - Fixed rspec tests for the project members also fixed the index
added 1 commit
- a24a2497 - Fixed more rspec tests as well as spinach features
added 1 commit
- bea3afa3 - Added groups to members section, added a members finder
Thanks @jivanvl
added 1 commit
- 89e4fb06 - Removed the "Groups" option from the settings gear
added 1 commit
- 3fba18ee - Removed the "Groups" option from the settings gear
added 1 commit
- 2726e097 - Added tests for the MembersController and corrected some more tests
added 1 commit
- cccdea88 - Added tests for the MembersController and corrected some more tests
assigned to @grzesiek
- Resolved by Jose Ivan Vargas
- Resolved by Grzegorz Bizon
- Resolved by Jose Ivan Vargas
- Resolved by Jose Ivan Vargas
- Resolved by Jose Ivan Vargas
- Resolved by Jose Ivan Vargas
- Resolved by Grzegorz Bizon
11 @project_members = @project_members.non_invite unless can?(current_user, :admin_project, @project) 12 13 group = @project.group 14 15 # group links 16 @group_links = @project.project_group_links.all 17 18 @skip_groups = @group_links.pluck(:group_id) 19 @skip_groups << @project.namespace_id unless @project.personal? 20 21 if group 22 # We need `.where.not(user_id: nil)` here otherwise when a group has an 23 # invitee, it would make the following query return 0 rows since a NULL 24 # user_id would be present in the subquery 25 # See http://stackoverflow.com/questions/129077/not-in-clause-and-null-values 26 group_members = MembersFinder.new(@project_members, group).execute(current_user) The reason I did it was because of the Rubocop rule that prevents the branch assignments count from going to high. So I changed it to a finder to have some of those extra "assignment slots" if you will. But I think you mentioned this in the previous diff about copy-pasting the instance variables. What would be a good way to solve this?
@jivanvl Nice work! It looks exactly how we decided about doing that!
I wonder if we can solve problem with copying & pasting instance variables from old controllers to the new one, which becomes an aggregate for settings. This is a little cumbersome and error prone to do it that way. This was the most boring solution we could think about when talking with @smcgivern, but maybe there is a better way!
I was thinking about introducing
app/presenters/settings/
with settings presenter for each old controller/index action. Let me explain that using an example.In this merge request, we strive for creating a settings aggregate which combines settings we previously had in
ProjectMembersController
andGroupLinksController
. Each controller we had, defined some instance variables inindex
action, which were then passed toindex.html.haml
view to generate forms. Now we want to decouple legacy views and instance variables from controller these elements were associated with.I wonder if having settings presenters would solve this problem. With this approach we can easily move settings forms around, because forms used to edit settings would be decoupled from controllers. Consider having something like below.
app/presenters/settings/project_members_presenter.rb
class Settings::ProjectMembersPresenter include Gitlab::View::Presenter def initialize(project, user, params = {}) # ... end def group_links @project.project_group_links end def project_members if can?(current_user, :admin_project, @project) @project.project_members else @project.project_members.non_invite end end # etc, method for each instance variable we had in old controller # and finally def to_partial_path 'projects/project_members/index' end end
With this approach we can have a can easily move settings around and render partials using:
projects/settings/members_controller.rb
def show @project_members_settings = Settings::ProjectMembersPresenter .new(project, current_user, params) end
projects/settings/members/show.html.haml
.some_css_etc = render partial: @project_members_settings .some_other_selectors_for_subsequent_subsettings_group = render partial: @group_links_settings
How do you like this idea? This actually would make it a little more difficult to finish this MR, from the backend perspective. We still do not have
app/presenters
, but I think that we already decided we need presenters, and this is yet another example why presenters can be useful.What do you think about this solution @smcgivern? /cc @DouweM @rymai
Edited by Grzegorz Bizonassigned to @jivanvl
added 1 commit
- 268d613f - Updated some tests descriptions to represent the correct settings path
@grzesiek I think I get how the presenter logic would work, but something that I'm a little bit of a loss here is. Are we going to add tests, if so how? By doing something similar on how you would test a controller?
cc @tauriedavis :)
@jivanvl Yes, we would need to add tests for both, controllers and presenters. Since presenters are a normal Ruby objects in this case, adding tests should not be too difficult. I think that we should hear feedback from @smcgivern or @rymai before we proceed with presenters here.
@jivanvl Another solution would be continue with copying & pasting instance variables, and submitting a ~"technical debt" issue about moving this to presenters and scheduling this issue for the next release. This may help to deliver this on time.
@grzesiek is anyone helping @jivanvl on the backend for this full-time? If not, I think your solution in https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/8281#note_20604329 is best. It would be nice to not implement presenters for the first time in the case of an MR like this, but I agree that it would make this simpler!
@awhildy @tauriedavis @victorwu Updated the description with screenshots and the gear button gif
Edited by Jose Ivan Vargas@smcgivern There is no backend developer assigned for a full time to help @jivanvl so I agree that this may be a little too much work for now. Let's continue without presenters, but let's make sure we have a proper tech debt issue for that.
There are multiple instances where we use
user
when I think we mean to saymember
. Would we be able to update the copy in this MR and change user to member on this page to keep consistency?Edited by Taurie Davis@tauriedavis There's only one place that I'm confused wether we should
member
oruser
. Personally I'm leaning towardsuser
Edited by Jose Ivan Vargas@tauriedavis @awhildy I have a question, I noticed that there's two ways to delete groups from this single view
What used to be the project members view
What used to be the groups links view
Do we create a separate issue for this or what do you think?
Do you mean in the search box? I think that should also be members, since members is used everywhere else. Why do you lean towards user?
Do we need that bottom section (Groups you share with) of the groups view at all since it is above? I have ideas on how to improve the ux for this page. I think at this point we should make a separate issue so we have the space and time to discuss improvements. What are your thoughts @awhildy ?
Edited by Taurie Davis@tauriedavis I changed most of the
users
tomembers
this is how it looks (didn't change the user select box)@tauriedavis I feel because
user
was because you could invite any GitLab user to be a part of the project. But ifmember
is already being used, I agree we should usemember
for consistencyChanged every match of
user
tomember
@tauriedavisThats a good point @jivanvl - And brings up a weird thing that is happening. It is strange that you can select current members in this dropdown and the button still says "Add to project" even when they are already in the project. There is some UX that can be improved here as well. I will include some improvement to this flow in the new issue as well.
For now I think the search input should say "Search for members to update or invite"
Edited by Taurie DavisThanks @jivanvl + @tauriedavis!
Looking great to me! Yeah, like @awhildy says, let's make any simple copy tweaks and get this merged in, since the goal is navigation migration. I'm sure there's plenty of improvements needed in each settings page.
Thanks @tauriedavis for creating the new issue(s).
New issue for improvements: https://gitlab.com/gitlab-org/gitlab-ce/issues/26167
@tauriedavis Updated the placeholder text for the search box
added 1 commit
- e7335a7a - Updated the "users" to "members" matches in the view
@grzesiek great! Do you mind creating that issue?